RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert-san,

Thank you for the reply again!

> Hi Shimoda-san,
> 
> On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > According to the gadget.h, a "complete" function will always be called
> > with interrupts disabled. However, sometimes usbhsg_queue_pop() function
> > is called with interrupts enabled. So, this function should calls
> > local_irq_save() before this calls the usb_gadget_giveback_request().
> > Otherwise, there is possible to cause a spinlock suspected in a gadget
> > complete function.
> 
> I still feel uneasy about adding the call to local_irq_save(), as the need for
> this is usually an indicator of another locking problem.

I also think that I would like to avoid using local_irq_save().
But, I have no idea to resolve this issue for now.

> Unfortunately I know next to nothing about the USB gadget subsystem, so some
> help from the USB gadget experts would be appreciated.
> 
> I had a look at other drivers, and it seems most drivers actually release
> and reacquire a spinlock around the call to usb_gadget_giveback_request(),
> i.e. they do:
> 
>     spin_unlock(...);
>     usb_gadget_giveback_request(...);
>     spin_lock();
> 
> This means they had already acquired the spinlock (and disabled interrupts!)
> before, which looks much healthier to me.
> 
> There's only one driver that uses local_irq_save() (pxa27x_udc).

I had a look at the musb driver. It uses a dmaengine driver.
And, in the callback routine of the musb driver, it calls spin_lock_irqsave() at first.

cppi41_dma_callback
--> spin_lock_irqsave(&musb->lock, flags);
--> cppi41_trans_done
 --> musb_dma_completion
  --> musb_g_tx or musb_g_rx
   --> musb_g_giveback
    --> spin_unlock(&musb->lock);   # This means unlocked the spin, but disabled interrupts.
    --> usb_gadget_giveback_request
    --> spin_lock(&musb->lock);
 < snip >
--> spin_unlock_irqrestore(&musb->lock, flags);

So, about disabled interrupts before usb_gadget_giveback_request(),
it is similar with this patch.

Best regards,
Yoshihiro Shimoda

��.n��������+%������w��{.n�����{���)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux