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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 23 Jul 2015 22:01:06 +0530
From:	Jassi Brar <jassisinghbrar@...il.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	kernel@...inux.com, Devicetree List <devicetree@...r.kernel.org>
Subject: Re: [PATCH 3/6] mailbox: Add support for ST's Mailbox IP

On Thu, Jul 23, 2015 at 1:59 PM, Lee Jones <lee.jones@...aro.org> wrote:
>> On Fri, Jul 17, 2015 at 5:34 PM, Lee Jones <lee.jones@...aro.org> 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);


>> > +       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()?
Anyways my point was about set/clr type regs. And you may look at if
channel really needs disabling while interrupts are handled? I think
it shouldn't because either it is going to be a to-fro communication
or random broadcasts without any guarantee of reception. But of course
you would know better your platform.
BTW  sti_mbox_channel_is_enabled() also needs to have locking around
enabled[] if you want to keep it.  And maybe embed sti_mbox_chan_lock
inside sti_mbox_device.

>> However, you need some mechanism to check if you succeeded 'owning'
>> the channel by reading back what you write to own the channel (not
>> sure which is that register here). Usually we need that action and
>> verification when we assign a channel to some user.
>
> I don't think we need to do that with this driver.  All of the
> allocation is controlled from within this code base.  The channels are
> pre-allocated and written into the co-proc's Firmware.
>
OK

>> > +       }
>> > +
>> Doesn't mbox->chans[i].con_priv need some locking here?
>
> Not that I can see.  Would you mind explaining further please?
>
Not anymore, after the clarification that we don't need a 'break' statement.

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