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

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3103 bytes --]

Hi,

On Fri, Mar 13, 2015 at 01:14:01AM +0000, yoshihiro shimoda wrote:
> > > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > index e0384af77e56..e9d75d85be59 100644
> > > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > @@ -119,7 +119,7 @@ struct usbhsg_recip_handle {
> > > >  /*
> > > >   *		queue push/pop
> > > >   */
> > > > -static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > +static void __usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > >  			     struct usbhsg_request *ureq,
> > > >  			     int status)
> > > >  {
> > > > @@ -133,6 +133,15 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > >  	usb_gadget_giveback_request(&uep->ep, &ureq->req);
> > > >  }
> > > >
> > > > +static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> > > > +			     struct usbhsg_request *ureq,
> > > > +			     int status)
> > > > +{
> > > > +	usbhs_lock(priv, flags);
> > > > +	__usbhsg_queue_pop(uep, ureq, status);
> > > > +	usbhs_unlock(priv, flags);
> > > > +}
> > > > +
> > > >  static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
> > > >  {
> > > >  	struct usbhs_pipe *pipe = pkt->pipe;
> > > >
> > > >
> > > > then, for cases where lock is already held you call __usbhsg_queue_pop()
> > > > and for all other cases, call usbhsg_queue_pop().
> > >
> > > Thank you for the suggestion. However, we cannot use this
> > > usbhsg_queue_pop() because a gadget driver might call usb_ep_queue()
> > > in the "complete" function and this driver calls usbhs_lock() in the
> > > usb_ep_queue().
> > 
> > right, in that case just call __usbhs_queue_pop() directly.
> 
> Yes. So, my understanding is that this driver always calls __usbhs_queue_pop()
> because this driver cannot know that a gadget driver calls usb_ep_queue() or not
> in the "complete" function.

but you don't need to know that. All you have to do is spin_unlock()
before calling usb_gadget_giveback_request(), look at
drivers/usb/dwc3/gadget.c::dwc3_gadget_giveback()

> > > Perhaps my explanation was bad, but this issue was caused by the
> > > following conditions:
> > >  - I use the renesas_usbhs driver as udc.
> > >  - I use an old usb-dmac driver that the callback runs on a kthread.
> > >  - I use the ncm driver. In this environment, the ncm driver might
> > >	cause a spinlock suspected.
> > >
> > > As an old solution, I fixed the renesas_usbhs driver by this patch.
> > > However, if I use a new usb-dmac driver that the callback runs on a
> > > tasklet, I don't need this patch. (This is a new solution.)
> > 
> > then differentiate based on some revision register or something like
> > that ?
> 
> Sorry for the lack information. I am submitting this usb-dmac driver that
> the callback runs on a tasklet now. This means that the driver is not
> merged yet. So, I think that we don't need to differentiate.

But as soon as your driver gets merged, you will need to differentiate,
right ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]