linux-kernel - Re: [PATCH 08/18] soc: qcom: ipa: the generic software interface

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]

Message-ID: <CAK8P3a3RCHF0UyKunaoCNvEd0X9EiD62PPfnfGxKG1L95Jz7dA@mail.gmail.com>
Date:   Wed, 15 May 2019 14:40:16 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Alex Elder <elder@...aro.org>
Cc:     David Miller <davem@...emloft.net>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        syadagir@...eaurora.org, mjavid@...eaurora.org,
        evgreen@...omium.org, Ben Chan <benchan@...gle.com>,
        Eric Caruso <ejcaruso@...gle.com>, abhishek.esse@...il.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 08/18] soc: qcom: ipa: the generic software interface

On Wed, May 15, 2019 at 2:13 PM Alex Elder <elder@...aro.org> wrote:
> On 5/15/19 2:21 AM, Arnd Bergmann wrote:


> >> +/* Wait for all transaction activity on a channel to complete */
> >> +void gsi_channel_trans_quiesce(struct gsi *gsi, u32 channel_id)
> >> +{
> >> +       struct gsi_channel *channel = &gsi->channel[channel_id];
> >> +       struct gsi_trans_info *trans_info;
> >> +       struct gsi_trans *trans = NULL;
> >> +       struct gsi_evt_ring *evt_ring;
> >> +       struct list_head *list;
> >> +       unsigned long flags;
> >> +
> >> +       trans_info = &channel->trans_info;
> >> +       evt_ring = &channel->gsi->evt_ring[channel->evt_ring_id];
> >> +
> >> +       spin_lock_irqsave(&evt_ring->ring.spinlock, flags);
> >> +
> >> +       /* Find the last list to which a transaction was added */
> >> +       if (!list_empty(&trans_info->alloc))
> >> +               list = &trans_info->alloc;
> >> +       else if (!list_empty(&trans_info->pending))
> >> +               list = &trans_info->pending;
> >> +       else if (!list_empty(&trans_info->complete))
> >> +               list = &trans_info->complete;
> >> +       else if (!list_empty(&trans_info->polled))
> >> +               list = &trans_info->polled;
> >> +       else
> >> +               list = NULL;
> >> +
> >> +       if (list) {
> >> +               struct gsi_trans *trans;
> >> +
> >> +               /* The last entry on this list is the last one allocated.
> >> +                * Grab a reference so we can wait for it.
> >> +                */
> >> +               trans = list_last_entry(list, struct gsi_trans, links);
> >> +               refcount_inc(&trans->refcount);
> >> +       }
> >> +
> >> +       spin_lock_irqsave(&evt_ring->ring.spinlock, flags);
> >> +
> >> +       /* If there is one, wait for it to complete */
> >> +       if (trans) {
> >> +               wait_for_completion(&trans->completion);
> >
> > Since you are waiting here, you clearly can't be called
> > from interrupt context, or with interrupts disabled, so it's
> > clearer to use spin_lock_irq() instead of spin_lock_irqsave().
> >
> > I generally try to avoid the _irqsave versions altogether, unless
> > it is really needed for a function that is called both from
> > irq-disabled and irq-enabled context.
>
> OK.  And I appreciate what your saying here because I do prefer
> code that communicates more about the context in ways like
> you describe.

Right, also reading the status of the irq-enable flag can be
expensive on some CPUs, so spin_lock_irqsave() ends up
much more slower than spin_lock() or spin_lock_irq(). Not sure
if it makes a huge difference on this particular platform, but
it's better not to have to worry about it.

     Arnd