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