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

[Jassi Brar]

On Fri, Jul 24, 2015 at 3:06 PM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> On Thu, 23 Jul 2015, Jassi Brar wrote:
>
>> >> > +static void sti_mbox_enable_channel(struct mbox_chan *chan)
>> >> > +{
>> >> > +       struct sti_channel *chan_info = chan->con_priv;
>> >> > +       struct sti_mbox_device *mdev = chan_info->mdev;
>> >> > +       struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev);
>> >> > +       unsigned int instance = chan_info->instance;
>> >> > +       unsigned int channel = chan_info->channel;
>> >> > +       unsigned long flags;
>> >> > +       void __iomem *base;
>> >> > +
>> >> > +       base = mdev->base + (instance * sizeof(u32));
>> >> > +
>> >> Maybe have something simpler like MBOX_BASE(instance)? Or some inline
>> >> function to avoid this 5-lines ritual?
>> >
>> > I've checked and we can't do this, as the we need most (all?) of the
>> > intermediary variables too.  No ritual just to get the final variable
>> > for instance.
>> >
>> OK. How about ?
>>   #define MBOX_BASE(m, n)   ((m)->base + (n) * 4)
>>   void __iomem *base = MBOX_BASE(mdev, instance);
>
> Oh, those 5 lines.  I thought you meant:
>
>        struct sti_channel *chan_info = chan->con_priv;
>        struct sti_mbox_device *mdev = chan_info->mdev;
>        unsigned int instance = chan_info->instance;
>        base = mdev->base + (instance * sizeof(u32));
>
> ... which is why I said that the intermediary variables are required.
>
> Well, I 'can' do that, but it seems to be unnecessarily obfuscating
> what's going on and doesn't actually save any lines.
>
> It's not a point that I consider arguing over though, so if you want
> me to do it, I will.  You have the final say here.
>
The macro seems tidier. Just a nit.

>> >> > +       spin_lock_irqsave(&sti_mbox_chan_lock, flags);
>> >> > +       mdev->enabled[instance] |= BIT(channel);
>> >> > +       writel_relaxed(BIT(channel), base + pdata->ena_set);
>> >> > +       spin_unlock_irqrestore(&sti_mbox_chan_lock, flags);
>> >> >
>> >> You don't need locking for SET/CLR type registers which are meant for
>> >> when they could be accessed by processors that can not share a lock.
>> >> So maybe drop the lock here and elsewhere.
>> >
>> > From what I can gather, I think we need this locking.  What happens if
>> > we get scheduled between setting the enabled bit in our cache and
>> > actually setting the ena_set bit?  We would be out of sync.
>> >
>> IIU what you mean... can't that still happen because of the  _relaxed()?
>
> Not sure what you mean.  The _relaxed variant merely omit some IO
> barriers.
>
By the time you exit the spinlock the write may still haven't been
effected. Maybe use writel() there.

>> And maybe embed sti_mbox_chan_lock inside sti_mbox_device.
>
> Not sure this is required.  I can find >600 instances of others using
> spinlocks as static globals.
>
And there should be >600 instances of *static* globals that are
protected by some static spinlock  ;)

Here the static sti_mbox_chan_lock protects sti_mbox_device which is
allocated during probe. I hope you agree that the standard practice is
to make the lock a member of the same structure that it protects.
Otherwise it gives the wrong impression that the same lock will be
used for any number of allocated mailbox instances.

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