Messages in this thread | | | From | Arnd Bergmann <> | Date | Wed, 15 May 2019 14:40:16 +0200 | Subject | Re: [PATCH 08/18] soc: qcom: ipa: the generic software interface |
| |
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
| |