Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices

[Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On Wed, Oct 08, 2014 at 03:33:28PM +0300, Octavian Purdila wrote:
> On Wed, Oct 8, 2014 at 3:04 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, Oct 08, 2014 at 01:54:07PM +0300, Octavian Purdila wrote:
> >> On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >> > On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
> >> >
> >> >> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> >> >> +                          u16 handle, u16 rx_slot)
> >> >> +{
> >> >> +     struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> >> >> +     struct dln2_rx_context *rxc;
> >> >> +     struct device *dev = &dln2->interface->dev;
> >> >> +
> >> >> +     spin_lock(&rxs->lock);
> >> >
> >> > You must use spin_lock_irqsave here as you call it from the completion
> >> > handler.
> >>
> >> Why? AFAICS the completion handler gets called from the HCD irq handler:
> >
> > The completion handler is currently called with local interrupts
> > disabled but that is about to change once all drivers have been updated:
> >
> >         http://marc.info/?l=linux-usb&m=137353360511003&w=2
> >
> > In this case you could probably get away with not disabling interrupts
> > anyway, but using the irqsave versions would make it obvious.
> >
> 
> I was not assuming that interrupts are disabled while running the
> completion handler. Since that spinlock is not touched by any other
> interrupt context code I don't think irqsave is necessary.

No, it isn't necessary so leave it as it is.

But you are exporting interfaces to other drivers and it may appear to
someone that those could possibly end up indirectly calling a function
taking that lock in IRQ context. We know that isn't the case now, but I
bet someone will post conversion patch for that spinlock at some point.
;)

Johan