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

[Arnd Bergmann <arnd@arndb.de>]

On Wed, May 15, 2019 at 2:13 PM Alex Elder <elder@linaro.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