Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1206929 > unrolled thread

Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP Jassi Brar <jassisinghbrar@gmail.com> - 2015-08-13 17:50 +0200
    Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP Lee Jones <lee.jones@linaro.org> - 2015-08-14 08:40 +0200
      Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP Jassi Brar <jassisinghbrar@gmail.com> - 2015-08-14 09:40 +0200
        Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP Lee Jones <lee.jones@linaro.org> - 2015-08-14 12:50 +0200

#1206929 — Re: [PATCH v2 3/6] mailbox: Add support for ST's Mailbox IP

On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:

> +
> +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
> +{
> +       struct sti_channel *chan_info = chan->con_priv;
> +       struct sti_mbox_device *mdev = chan_info->mdev;
> +       unsigned int instance = chan_info->instance;
> +       unsigned int channel = chan_info->channel;
> +       void __iomem *base = MBOX_BASE(mdev, instance);
> +
> +       if (!(chan_info->direction & MBOX_TX))
> +               return false;
>
Here the 'direction' is gotten via DT node of the client i.e, you
expect consumer drivers to tell the provider what its limitations are?

IMO if some physical channel can't do TX then that should be either
hardcoded inside the controller driver or learnt via DT node of the
_controller_.


> +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox,
> +                                       const struct of_phandle_args *spec)
> +{
> +       struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev);
> +       struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> +       struct sti_channel *chan_info;
> +       struct mbox_chan *chan = NULL;
> +       unsigned int instance  = spec->args[0];
> +       unsigned int channel   = spec->args[1];
> +       unsigned int direction = spec->args[2];
> +       int i;
> +
> +       /* Bounds checking */
> +       if (instance >= pdata->num_inst || channel  >= pdata->num_chan) {
> +               dev_err(mbox->dev,
> +                       "Invalid channel requested instance: %d channel: %d\n",
> +                       instance, channel);
> +               return NULL;
return ERR_PTR(-EINVAL)

> +       }
> +
> +       for (i = 0; i < mbox->num_chans; i++) {
> +               chan_info = mbox->chans[i].con_priv;
> +
> +               /* Is requested channel free? */
> +               if (direction != MBOX_LOOPBACK &&
>
Consider this example when 2 clients ask for same physical channel but
in different modes.
           mboxes = <&mboxA 0 1 MBOX_TX>;
           mboxes = <&mboxA 0 1 MBOX_LOOPBACK>;

You happily assign 2 virtual channels backed by one physical channel
{mboxA, 0, 1}. The 2 clients think they can freely do startup(),
shutdown() and send_data() on the channels. But obviously we are
screwed with races like
   client1.startup()
    -> client2.startup()
        -> client2.send_data()
            -> client2.shutdown()
                -> client1.send_data()  XXXX

 Now you can shove in some more checks to 'fix' the race OR you can
simply expose only physical channels. Practically no client would ever
ask it to do what it can't, and for the hypothetical possibility that
some does, just return error.

> +                   chan_info &&
> +                   mbox->dev == chan_info->mdev->dev &&
> +                   instance == chan_info->instance &&
> +                   channel == chan_info->channel) {
> +                       dev_err(mbox->dev, "Channel in use\n");
> +                       return NULL;
return ERR_PTR(-EBUSY)

> +               }
> +
> +               /*
> +                * Find the first free slot, then continue checking
> +                * to see if requested channel is in use
> +                */
> +               if (!chan && !chan_info)
> +                       chan = &mbox->chans[i];
> +       }
> +
> +       if (!chan) {
> +               dev_err(mbox->dev, "No free channels left\n");
> +               return NULL;
return ERR_PTR(-EBUSY)

> +       }
> +
> +       chan_info = devm_kzalloc(mbox->dev, sizeof(*chan_info), GFP_KERNEL);
> +       if (!chan_info)
> +               return NULL;
return ERR_PTR(-ENOMEM)

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1207350

On Thu, 13 Aug 2015, Jassi Brar wrote:
> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > +
> > +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
> > +{
> > +       struct sti_channel *chan_info = chan->con_priv;
> > +       struct sti_mbox_device *mdev = chan_info->mdev;
> > +       unsigned int instance = chan_info->instance;
> > +       unsigned int channel = chan_info->channel;
> > +       void __iomem *base = MBOX_BASE(mdev, instance);
> > +
> > +       if (!(chan_info->direction & MBOX_TX))
> > +               return false;
> >
> Here the 'direction' is gotten via DT node of the client i.e, you
> expect consumer drivers to tell the provider what its limitations are?
> 
> IMO if some physical channel can't do TX then that should be either
> hardcoded inside the controller driver or learnt via DT node of the
> _controller_.

That's a fair point.  I need to create a new property similar to the
already existing 'read-only'.  I guess 'tx-only' is equivalent.

> > +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox,
> > +                                       const struct of_phandle_args *spec)
> > +{
> > +       struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev);
> > +       struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
> > +       struct sti_channel *chan_info;
> > +       struct mbox_chan *chan = NULL;
> > +       unsigned int instance  = spec->args[0];
> > +       unsigned int channel   = spec->args[1];
> > +       unsigned int direction = spec->args[2];
> > +       int i;
> > +
> > +       /* Bounds checking */
> > +       if (instance >= pdata->num_inst || channel  >= pdata->num_chan) {
> > +               dev_err(mbox->dev,
> > +                       "Invalid channel requested instance: %d channel: %d\n",
> > +                       instance, channel);
> > +               return NULL;
> return ERR_PTR(-EINVAL)

I can handle all these, no problem.

> > +       }
> > +
> > +       for (i = 0; i < mbox->num_chans; i++) {
> > +               chan_info = mbox->chans[i].con_priv;
> > +
> > +               /* Is requested channel free? */
> > +               if (direction != MBOX_LOOPBACK &&
> >
> Consider this example when 2 clients ask for same physical channel but
> in different modes.
>            mboxes = <&mboxA 0 1 MBOX_TX>;
>            mboxes = <&mboxA 0 1 MBOX_LOOPBACK>;
> 
> You happily assign 2 virtual channels backed by one physical channel
> {mboxA, 0, 1}. The 2 clients think they can freely do startup(),
> shutdown() and send_data() on the channels. But obviously we are
> screwed with races like
>    client1.startup()
>     -> client2.startup()
>         -> client2.send_data()
>             -> client2.shutdown()
>                 -> client1.send_data()  XXXX

Good catch and a fair point.  As you say, it's unlikely to happen, but
I would like to prevent it in any case.

>  Now you can shove in some more checks to 'fix' the race OR you can
> simply expose only physical channels.

We can't expose all of the channels.  There are too many and would
take up too much *unused* memory.

I don't want to have an endless stream of checks either, but we should
try to cover the bases.  I think smarter (rather than more) checks is
the answer.  I'll have a think about it.

> Practically no client would ever
> ask it to do what it can't, and for the hypothetical possibility that
> some does, just return error.

Right.  Smarter error checking here and returning an error on a bad
config is what I plan to do.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1207378

On Fri, Aug 14, 2015 at 12:03 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 13 Aug 2015, Jassi Brar wrote:
>> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
>>
>> > +
>> > +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
>> > +{
>> > +       struct sti_channel *chan_info = chan->con_priv;
>> > +       struct sti_mbox_device *mdev = chan_info->mdev;
>> > +       unsigned int instance = chan_info->instance;
>> > +       unsigned int channel = chan_info->channel;
>> > +       void __iomem *base = MBOX_BASE(mdev, instance);
>> > +
>> > +       if (!(chan_info->direction & MBOX_TX))
>> > +               return false;
>> >
>> Here the 'direction' is gotten via DT node of the client i.e, you
>> expect consumer drivers to tell the provider what its limitations are?
>>
>> IMO if some physical channel can't do TX then that should be either
>> hardcoded inside the controller driver or learnt via DT node of the
>> _controller_.
>
> That's a fair point.
.

>  I need to create a new property similar to the
> already existing 'read-only'.  I guess 'tx-only' is equivalent.
>
Just to be clear, if you must have such a property it should come from
the _controller_ node.

However at one point you said,  "Only the A9 (Mbox 0) can Rx."
  Which sounds like the 'simplex' constraint is not coming from the
mailbox controller but from the remote endpoints that don't RX+TX
except for one of them. That makes more sense than a controller with
differently capable physical channels. If that is indeed the
situation, then the controller is actually 'duplex' and there should
be no tx-only/rx-only property anywhere. Everything automatically
falls into place because client drivers are written for specific
targets and, unless you write some code, there can be no TX call to a
remote that doesn't listen.

>> > +
>> > +       for (i = 0; i < mbox->num_chans; i++) {
>> > +               chan_info = mbox->chans[i].con_priv;
>> > +
>> > +               /* Is requested channel free? */
>> > +               if (direction != MBOX_LOOPBACK &&
>> >
>> Consider this example when 2 clients ask for same physical channel but
>> in different modes.
>>            mboxes = <&mboxA 0 1 MBOX_TX>;
>>            mboxes = <&mboxA 0 1 MBOX_LOOPBACK>;
>>
>> You happily assign 2 virtual channels backed by one physical channel
>> {mboxA, 0, 1}. The 2 clients think they can freely do startup(),
>> shutdown() and send_data() on the channels. But obviously we are
>> screwed with races like
>>    client1.startup()
>>     -> client2.startup()
>>         -> client2.send_data()
>>             -> client2.shutdown()
>>                 -> client1.send_data()  XXXX
>
> Good catch and a fair point.  As you say, it's unlikely to happen, but
> I would like to prevent it in any case.
>
No, such races are a practical problem. We must own them.

I was talking about problems that arise because someone wrote bad
DT... those are not 'practical' problems because there are too many
ways to screw up with bad DT properties that if we try to check for
them we'll go insane.

>>  Now you can shove in some more checks to 'fix' the race OR you can
>> simply expose only physical channels.
>
> We can't expose all of the channels.  There are too many and would
> take up too much *unused* memory.
>
I am aware of that. I said expose _only_ physical channels, not _all_ :)

Cheers!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1207486

On Fri, 14 Aug 2015, Jassi Brar wrote:

> On Fri, Aug 14, 2015 at 12:03 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Thu, 13 Aug 2015, Jassi Brar wrote:
> >> On Mon, Jul 27, 2015 at 3:14 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >>
> >> > +
> >> > +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan)
> >> > +{
> >> > +       struct sti_channel *chan_info = chan->con_priv;
> >> > +       struct sti_mbox_device *mdev = chan_info->mdev;
> >> > +       unsigned int instance = chan_info->instance;
> >> > +       unsigned int channel = chan_info->channel;
> >> > +       void __iomem *base = MBOX_BASE(mdev, instance);
> >> > +
> >> > +       if (!(chan_info->direction & MBOX_TX))
> >> > +               return false;
> >> >
> >> Here the 'direction' is gotten via DT node of the client i.e, you
> >> expect consumer drivers to tell the provider what its limitations are?
> >>
> >> IMO if some physical channel can't do TX then that should be either
> >> hardcoded inside the controller driver or learnt via DT node of the
> >> _controller_.
> >
> > That's a fair point.
> .
> 
> >  I need to create a new property similar to the
> > already existing 'read-only'.  I guess 'tx-only' is equivalent.
> >
> Just to be clear, if you must have such a property it should come from
> the _controller_ node.

Correct.  That's the plan.

> However at one point you said,  "Only the A9 (Mbox 0) can Rx."
>   Which sounds like the 'simplex' constraint is not coming from the
> mailbox controller but from the remote endpoints that don't RX+TX
> except for one of them. That makes more sense than a controller with
> differently capable physical channels. If that is indeed the
> situation, then the controller is actually 'duplex' and there should
> be no tx-only/rx-only property anywhere. Everything automatically
> falls into place because client drivers are written for specific
> targets and, unless you write some code, there can be no TX call to a
> remote that doesn't listen.

Unfortunately it's a restriction of the hardware (or the controller as
you call it, although it's not really a controller).  There is only
one IRQ for Rx'ing and that's wired up to the A9's mailbox (Mailbox
0).  If one of the remote processors attempted to send a message
through any of the other mailboxes (other than the Mailbox 0), then no
one would hear the doorbell ring and the message would go unserviced.

> >> > +
> >> > +       for (i = 0; i < mbox->num_chans; i++) {
> >> > +               chan_info = mbox->chans[i].con_priv;
> >> > +
> >> > +               /* Is requested channel free? */
> >> > +               if (direction != MBOX_LOOPBACK &&
> >> >
> >> Consider this example when 2 clients ask for same physical channel but
> >> in different modes.
> >>            mboxes = <&mboxA 0 1 MBOX_TX>;
> >>            mboxes = <&mboxA 0 1 MBOX_LOOPBACK>;
> >>
> >> You happily assign 2 virtual channels backed by one physical channel
> >> {mboxA, 0, 1}. The 2 clients think they can freely do startup(),
> >> shutdown() and send_data() on the channels. But obviously we are
> >> screwed with races like
> >>    client1.startup()
> >>     -> client2.startup()
> >>         -> client2.send_data()
> >>             -> client2.shutdown()
> >>                 -> client1.send_data()  XXXX
> >
> > Good catch and a fair point.  As you say, it's unlikely to happen, but
> > I would like to prevent it in any case.
> >
> No, such races are a practical problem. We must own them.
> 
> I was talking about problems that arise because someone wrote bad
> DT... those are not 'practical' problems because there are too many
> ways to screw up with bad DT properties that if we try to check for
> them we'll go insane.

The only thing I'm having trouble with protecting at the moment is
other clients _also_ requesting a LOOPBACK channel.  I would like to
check which clients have already requested one/them, however that
information is not available until _after_ xlate() has been called,
which is pretty frustrating.  Perhaps I'll put a comment in instead.

> >>  Now you can shove in some more checks to 'fix' the race OR you can
> >> simply expose only physical channels.
> >
> > We can't expose all of the channels.  There are too many and would
> > take up too much *unused* memory.
> >
> I am aware of that. I said expose _only_ physical channels, not _all_ :)

It's impossible to know which physical channels will be used by
clients and subsequently which physical channels to expose.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web