lkml.org 
[lkml]   [2019]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2019-05-15 14:41    [W:2.244 / U:0.252 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site