On Wed, May 15, 2019 at 2:26 PM Alex Elder <el...@linaro.org> wrote: > On 5/15/19 2:34 AM, Arnd Bergmann wrote: > >> +/* Cancel a channel's pending transactions */ > >> +void gsi_channel_trans_cancel_pending(struct gsi_channel *channel) > >> +{ > >> + struct gsi_trans_info *trans_info = &channel->trans_info; > >> + u32 evt_ring_id = channel->evt_ring_id; > >> + struct gsi *gsi = channel->gsi; > >> + struct gsi_evt_ring *evt_ring; > >> + struct gsi_trans *trans; > >> + unsigned long flags; > >> + > >> + evt_ring = &gsi->evt_ring[evt_ring_id]; > >> + > >> + spin_lock_irqsave(&evt_ring->ring.spinlock, flags); > >> + > >> + list_for_each_entry(trans, &trans_info->pending, links) > >> + trans->result = -ECANCELED; > >> + > >> + list_splice_tail_init(&trans_info->pending, &trans_info->complete); > >> + > >> + spin_unlock_irqrestore(&evt_ring->ring.spinlock, flags); > >> + > >> + spin_lock_irqsave(&gsi->spinlock, flags); > >> + > >> + if (gsi->event_enable_bitmap & BIT(evt_ring_id)) > >> + gsi_event_handle(gsi, evt_ring_id); > >> + > >> + spin_unlock_irqrestore(&gsi->spinlock, flags); > >> +} > > > > That is a lot of irqsave()/irqrestore() operations. Do you actually call > > all of these functions from hardirq context? > > The transaction list is definitely updated in IRQ context, > but I think it is no longer updated in hardirq context (the > softirq was a recent change). This particular function is > definitely not called in a hardirq context, so I can remove > the irqsave/irqrestore.
If you want to protect against concurrent softirqs, you still need spin_lock_bh(), which is cheaper than spin_lock_irqsave() but still requires writing to the shared cache line for the atomic update of the lock. > I'll survey my spinlock use throughout the driver and will > remove any irqsave/irqrestore used in non-hardirq contexts. Ok. I actually hope that most of the spinlocks can be removed from the data path entirely. I just replied on the ring.spinlock, which I think can go away and be replaced either with two atomic_t values (rp_local and wp_local only; 'wp' appears to be unused), or even just an smp_rmb()/ smp_wmb() pair for each access. The gsi register spinlock can probably be avoided as well if we stop disabling and renabling the interrupts as I suggested. gsi_trans_info->spinlock is harder to get rid of unfortunately, as that would require changing the way you do the doubly linked lists. Arnd