RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function
Hi,
> Hi,
>
> On Fri, Mar 13, 2015 at 01:14:01AM +, 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()
Thank you for the suggestion. I understood it.
> > > > 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 ?
Right. So, I will submit v3 patch soon.
Best regards,
Yoshihiro Shimoda
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function
Hi,
On Fri, Mar 13, 2015 at 01:14:01AM +, 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
signature.asc
Description: Digital signature
RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function
Hi,
> Hi,
>
> On Thu, Mar 12, 2015 at 05:40:56AM +, yoshihiro shimoda wrote:
> > Hi,
> >
< snip >
> > >
> > > try something like below:
> > >
> > > 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.
> > 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.
Best regards,
Yoshihiro Shimoda
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function
Hi,
On Thu, Mar 12, 2015 at 05:40:56AM +, yoshihiro shimoda wrote:
> Hi,
>
> > On Thu, Mar 12, 2015 at 04:33:41AM +, yoshihiro shimoda wrote:
> > > Hi Geert-san again,
> > >
> > > > Hi Geert-san,
> > > >
> > > > Thank you for the reply again!
> > > >
> > > > > Hi Shimoda-san,
> > > > >
> > > > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > > > 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.
> > >
> > > After I modified usb-dmac driver to use a tasklet instead of a kthread,
> > > this issue disappeared.
> > >
> > > My understanding is the followings:
> > > - According to the backtrace below, during usbhsf_dma_complete() was
> > > running,
> > > a softirq happened. After ncm_tx_tasklet() was called, the issue
> > > happened.
> > > http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> > >
> > > - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver
> > > is able
> > > to do the ncm_tx_tasklet().
> > >
> > > - After the current usb-dmac driver, since usbhsf_dma_complete() runs on
> > > a tasklet,
> > > ncm driver is not able to do the ncm_tx_tasklet during
> > > usbhsf_dma_complete() was running.
> > >
> > >
> > > So, I would like to recall this patch and I will resubmit this patch
> > > series as v3.
> >
> > try something like below:
> >
> > 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.
> 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 ?
--
balbi
signature.asc
Description: Digital signature
RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function
Hi,
> On Thu, Mar 12, 2015 at 04:33:41AM +, yoshihiro shimoda wrote:
> > Hi Geert-san again,
> >
> > > Hi Geert-san,
> > >
> > > Thank you for the reply again!
> > >
> > > > Hi Shimoda-san,
> > > >
> > > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > > 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.
> >
> > After I modified usb-dmac driver to use a tasklet instead of a kthread,
> > this issue disappeared.
> >
> > My understanding is the followings:
> > - According to the backtrace below, during usbhsf_dma_complete() was
> > running,
> > a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> > http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
> >
> > - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is
> > able
> > to do the ncm_tx_tasklet().
> >
> > - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a
> > tasklet,
> > ncm driver is not able to do the ncm_tx_tasklet during
> > usbhsf_dma_complete() was running.
> >
> >
> > So, I would like to recall this patch and I will resubmit this patch series
> > as v3.
>
> try something like below:
>
> 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().
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.)
Best regards,
Yoshihiro Shimoda
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function
Hi,
On Thu, Mar 12, 2015 at 04:33:41AM +, yoshihiro shimoda wrote:
> Hi Geert-san again,
>
> > Hi Geert-san,
> >
> > Thank you for the reply again!
> >
> > > Hi Shimoda-san,
> > >
> > > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > > 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.
>
> After I modified usb-dmac driver to use a tasklet instead of a kthread,
> this issue disappeared.
>
> My understanding is the followings:
> - According to the backtrace below, during usbhsf_dma_complete() was running,
> a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
> http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
>
> - This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is
> able
> to do the ncm_tx_tasklet().
>
> - After the current usb-dmac driver, since usbhsf_dma_complete() runs on a
> tasklet,
> ncm driver is not able to do the ncm_tx_tasklet during
> usbhsf_dma_complete() was running.
>
>
> So, I would like to recall this patch and I will resubmit this patch series
> as v3.
try something like below:
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().
--
balbi
signature.asc
Description: Digital signature
RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function
Hi Geert-san again,
> Hi Geert-san,
>
> Thank you for the reply again!
>
> > Hi Shimoda-san,
> >
> > On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> > 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.
After I modified usb-dmac driver to use a tasklet instead of a kthread,
this issue disappeared.
My understanding is the followings:
- According to the backtrace below, during usbhsf_dma_complete() was running,
a softirq happened. After ncm_tx_tasklet() was called, the issue happened.
http://thread.gmane.org/gmane.linux.usb.general/122023/focus=43729
- This means that usbhsf_dma_complete() ran on a kthread. So, ncm driver is able
to do the ncm_tx_tasklet().
- After the current usb-dmac driver, since usbhsf_dma_complete() runs on a
tasklet,
ncm driver is not able to do the ncm_tx_tasklet during usbhsf_dma_complete()
was running.
So, I would like to recall this patch and I will resubmit this patch series as
v3.
Best regards,
Yoshihiro Shimoda
N�r��yb�X��ǧv�^�){.n�+{��^n�r���z���h�&�� �G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
RE: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function
Hi Geert-san,
Thank you for the reply again!
> Hi Shimoda-san,
>
> On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
> 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�r��yb�X��ǧv�^�){.n�+{��^n�r���z���h�&�� �G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
Re: [PATCH v2 1/4] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function
Hi Shimoda-san,
On Mon, Feb 16, 2015 at 2:52 AM, Yoshihiro Shimoda
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.
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).
> Signed-off-by: Yoshihiro Shimoda
> ---
> drivers/usb/renesas_usbhs/mod_gadget.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> b/drivers/usb/renesas_usbhs/mod_gadget.c
> index e0384af..104bddf 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -126,11 +126,22 @@ static void usbhsg_queue_pop(struct usbhsg_uep *uep,
> struct usbhsg_gpriv *gpriv = usbhsg_uep_to_gpriv(uep);
> struct usbhs_pipe *pipe = usbhsg_uep_to_pipe(uep);
> struct device *dev = usbhsg_gpriv_to_dev(gpriv);
> + unsigned long flags;
>
> dev_dbg(dev, "pipe %d : queue pop\n", usbhs_pipe_number(pipe));
>
> ureq->req.status = status;
> + /*
> +* According to the gadget.h, a "complete" function will always be
> +* called with interrupts disabled. However, sometimes this function
> +* is called with interrupts enabled. (e.g. complete a DMAC transfer
> or
> +* write data and done is set immediately when PIO.) So, this function
> +* should calls local_irq_save() before this calls the
> +* usb_gadget_giveback_request().
> +*/
> + local_irq_save(flags);
> usb_gadget_giveback_request(&uep->ep, &ureq->req);
> + local_irq_restore(flags);
> }
>
> static void usbhsg_queue_done(struct usbhs_priv *priv, struct usbhs_pkt *pkt)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
