RE: [PATCH 1/4] usb: renesas_usbhs: fix spinlock recursion by usbhsf_dma_complete()
Hi Geert-san, > > Still, that would need some better protection, as local_irq_save() disables > > interrupts only on the CPU it's running on, not on other CPUs in a > > multiprocessor system. > > I see. I will investigate this issue more. I tried this issue on v3.19 with dmac enabled, it also caused an oops. However, the log is useful to investigate this issue. This issue is caused by tx_complete() and ncm_tx_tasklet() because the renesas_usbhs driver called usb_gadget_giveback_request with interrupts enabled. So, spin_lock_irqsave(&dev->req_lock, flags) in u_ether.c is held. So, I will submit a patch v2 as the followings. What do you think? -- Subject: [PATCH] usb: renesas_usbhs: fix spinlock suspected in a gadget complete function 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. 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) === oops log on v3.19 + dmac === BUG: spinlock lockup suspected on CPU#0, irq/102-e65a000/547 lock: 0xeea2dd5c, .magic: dead4ead, .owner: /-1, .owner_cpu: -1 CPU: 0 PID: 547 Comm: irq/102-e65a000 Tainted: GW 3.19.0-05634-g11371d7-dirty #55 Hardware name: Generic R8A7791 (Flattened Device Tree) Backtrace: [] (dump_backtrace) from [] (show_stack+0x18/0x1c) r6:00989680 r5:eea2dd5c r4: r3:00208040 [] (show_stack) from [] (dump_stack+0x7c/0x98) [] (dump_stack) from [] (spin_dump+0x80/0x94) r4: r3:c0671334 [] (spin_dump) from [] (do_raw_spin_lock+0x13c/0x190) r5: r4:00989680 [] (do_raw_spin_lock) from [] (_raw_spin_lock_irqsave+0x18/0x20) r9:eea2dd5c r8:eea2dd40 r7:eebaa034 r6: r5:000f r4:6113 [] (_raw_spin_lock_irqsave) from [] (eth_start_xmit+0xd0/0x37c [u_ether]) r4:eea2d800 r3: [] (eth_start_xmit [u_ether]) from [] (ncm_tx_tasklet+0x44/0x4c [usb_f_ncm]) r10:eeb87d00 r9:c06b6b00 r8: r7: r6:c0671274 r5: r4:ee0f8e00 [] (ncm_tx_tasklet [usb_f_ncm]) from [] (tasklet_action+0x94/0xf4) r5:ee0f8ee0 r4:ee0f8edc [] (tasklet_action) from [] (__do_softirq+0xec/0x220) r8:c0676098 r7:0100 r6:c0676088 r5:0030 r4:eeb86000 r3:4004 [] (__do_softirq) from [] (irq_exit+0x8c/0xfc) r10:0002 r9:6013 r8:0001 r7:ee806000 r6: r5:c0671ac8 r4: [] (irq_exit) from [] (__handle_domain_irq+0x94/0xb8) r4: r3:018e [] (__handle_domain_irq) from [] (gic_handle_irq+0x40/0x64) r8:eea2dd5c r7:eeb87de4 r6:c067c964 r5:eeb87db0 r4:f0002000 r3:eeb87db0 [] (gic_handle_irq) from [] (__irq_svc+0x40/0x54) Exception stack(0xeeb87db0 to 0xeeb87df8) 7da0: eea2dd5c aa05aa04 7dc0: eea2dd40 ee0be1c0 ee180e80 eea2dd5c eea2dd5c 6013 0002 eeb87e1c 7de0: eeb87e20 eeb87df8 c04751a4 c005586c 6013 r6: r5:6013 r4:c005586c r3:c04751a4 [] (do_raw_spin_lock) from [] (_raw_spin_lock+0x10/0x14) r9:6013 r8:ee1d3ab4 r7:eea2dd5c r6:ee180e80 r5:ee0be1c0 r4:eea2dd40 [] (_raw_spin_lock) from [] (tx_complete+0x70/0xd8 [u_ether]) [] (tx_complete [u_ether]) from [] (usb_gadget_giveback_request+0x14/0x18) r7:ee1d3a10 r6:eebaa86c r5: r4:ee0be1f4 [] (usb_gadget_giveback_request) from [] (usbhsg_queue_done+0x2c/0x30) [] (usbhsg_queue_done) from [] (usbhsf_pkt_handler+0xfc/0x114) [] (usbhsf_pkt_handler) from [] (usbhsf_dma_complete+0x20/0x58) r10: r9:0
RE: [PATCH 1/4] usb: renesas_usbhs: fix spinlock recursion by usbhsf_dma_complete()
Hi Geert-san, Thank you for the reply! > Hi Shimoda-san, > > On Mon, Feb 9, 2015 at 9:16 AM, Yoshihiro Shimoda > wrote: > > The usbhsf_pkt_handler(pipe, USBHSF_PKT_DMA_DONE) in usbhsf_dma_complete() > > will call the complete function of a usb gadget driver finally. > > According to the gadget.h, "The function will always be called with > > interrupts disabled". > > > > So, this patch adds a local_irq_save/local_irq_restore in the > > usbhsf_dma_complete() because a dmaengine driver may call this > > callback function when interrupts enabled (e.g. in tasklet). > > > > Signed-off-by: Yoshihiro Shimoda > > --- > > drivers/usb/renesas_usbhs/fifo.c |3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/usb/renesas_usbhs/fifo.c > > b/drivers/usb/renesas_usbhs/fifo.c > > index d891bff..b1440d0 100644 > > --- a/drivers/usb/renesas_usbhs/fifo.c > > +++ b/drivers/usb/renesas_usbhs/fifo.c > > @@ -1165,11 +1165,14 @@ static void usbhsf_dma_complete(void *arg) > > struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe); > > struct device *dev = usbhs_priv_to_dev(priv); > > int ret; > > + unsigned long flags; > > > > + local_irq_save(flags); > > Adding "local_irq_save()" without a spinlock is usually not correct. > I'm a bit confused here. usbhsf_pkt_handler() itself calls > > usbhs_lock(priv, flags); > > which is actually > > spin_lock_irqsave(usbhs_priv_to_lock(priv), flags) > > so it does disable interrupts internally? Yes, it does. But.. > Or is this about protecting the call to > > pkt->done(priv, pkt); > > at the end of usbhsf_pkt_handler(), which is done after releasing the > spinlock? yes, I would like to protect pkt->done(priv, pkt) because it will call usb_gadget_giveback_request() in mod_gadget.c. Otherwise, an oops happens if I enabled CONFIG_DEBUG_SPINLOCK and used g_ncm. I copied the oops log at the end of this email. (After I loocked the log again, I should have described a commit log that this issue is caused from a gadget driver.) Also, this driver cannot protect pkt->done(priv, pkt) using usbhs_lock() because a gadget driver may call usb_ep_queue() in the complete function from usb_gadget_giveback_request(). So, a spinlock recursion will happen. > Still, that would need some better protection, as local_irq_save() disables > interrupts only on the CPU it's running on, not on other CPUs in a > multiprocessor system. I see. I will investigate this issue more. === oops log === BUG: spinlock recursion on CPU#0, irq/102-e65a000/546 lock: 0xee1f5d5c, .magic: dead4ead, .owner: irq/102-e65a000/546, .owner_cpu: 0 CPU: 0 PID: 546 Comm: irq/102-e65a000 Tainted: GW 3.19.0-rc4-01754-gfd5b84e-dirty #181 Hardware name: Generic R8A7791 (Flattened Device Tree) Backtrace: [] (dump_backtrace) from [] (show_stack+0x18/0x1c) r6: r5:ee1f5d5c r4: r3:00208040 [] (show_stack) from [] (dump_stack+0x7c/0x98) [] (dump_stack) from [] (spin_dump+0x80/0x94) r4:ee967240 r3:c0653334 [] (spin_dump) from [] (spin_bug+0x2c/0x30) r5:c059b229 r4:ee1f5d5c [] (spin_bug) from [] (do_raw_spin_lock+0x50/0x190) r5:000f r4:6113 [] (do_raw_spin_lock) from [] (_raw_spin_lock_irqsave+0x18/0x20) r9:ee1f5d5c r8:ee1f5d40 r7:eeaea434 r6: r5:000f r4:6113 [] (_raw_spin_lock_irqsave) from [] (eth_start_xmit+0xd0/0x37c [u_ether]) r4:ee1f5800 r3: [] (eth_start_xmit [u_ether]) from [] (ncm_tx_tasklet+0x44/0x4c [usb_f_ncm]) r10:eea5fd38 r9:c06987c0 r8: r7: r6:c0653274 r5: r4:ee115600 [] (ncm_tx_tasklet [usb_f_ncm]) from [] (tasklet_action+0x94/0xf4) r5:ee1156e0 r4:ee1156dc [] (tasklet_action) from [] (__do_softirq+0xec/0x220) r8:c0658098 r7:0100 r6:c0658088 r5:0030 r4:eea5e000 r3:4004 [] (__do_softirq) from [] (irq_exit+0x8c/0xfc) r10:0002 r9:6013 r8:0001 r7:ee806000 r6: r5:c0653ac8 r4: [] (irq_exit) from [] (__handle_domain_irq+0x94/0xb8) r4: r3:00a2 [] (__handle_domain_irq) from [] (gic_handle_irq+0x40/0x64) r8:eea4f8b0 r7:eea5fe1c r6:c065e95c r5:eea5fde8 r4:f0002000 r3:eea5fde8 [] (gic_handle_irq) from [] (__irq_svc+0x40/0x54) Exception stack(0xeea5fde8 to 0xeea5fe30) fde0: ee1f5d5c ac4aac49 ee0b65e4 ee1f5d40 ee1f5d40 ee0b65c0 fe00: eeb5b280 ee1f5d5c eea4f8b0 6013 0002 eea5fe4c eea5fe20 eea5fe30 fe20: c0470254 bf0005b8 6013 r6: r5:6013 r4:bf0005b8 r3:c0470254 [] (tx_complete [u_ether]) from [] (usb_gadget_giveback_request+0x14/0x18) r7:eea4f810 r6:ee8e206c r5: r4:ee0b65f4 [] (usb_gadget_giveback_request) from [] (usbhsg_queue_done+0x2c/0x30) [] (usbhsg_queue_done) from [] (usbhsf_pkt_handler+0xfc/0x114) [] (usbhsf_pkt_handler) from [] (usbhsf_dma_complete+0x20/0x58) r10: r9:0001 r8:00200200 r7:00100100 r6:eeb118c8 r5:ee9a5a00 r4:ee8e206c [] (usbhsf_dma_complete) from [] (usb_dmac_isr_channel_thread+0x8c/0xcc) r5:eeb118
Re: [PATCH 1/4] usb: renesas_usbhs: fix spinlock recursion by usbhsf_dma_complete()
Hi Shimoda-san,
On Mon, Feb 9, 2015 at 9:16 AM, Yoshihiro Shimoda
wrote:
> The usbhsf_pkt_handler(pipe, USBHSF_PKT_DMA_DONE) in usbhsf_dma_complete()
> will call the complete function of a usb gadget driver finally.
> According to the gadget.h, "The function will always be called with
> interrupts disabled".
>
> So, this patch adds a local_irq_save/local_irq_restore in the
> usbhsf_dma_complete() because a dmaengine driver may call this
> callback function when interrupts enabled (e.g. in tasklet).
>
> Signed-off-by: Yoshihiro Shimoda
> ---
> drivers/usb/renesas_usbhs/fifo.c |3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/renesas_usbhs/fifo.c
> b/drivers/usb/renesas_usbhs/fifo.c
> index d891bff..b1440d0 100644
> --- a/drivers/usb/renesas_usbhs/fifo.c
> +++ b/drivers/usb/renesas_usbhs/fifo.c
> @@ -1165,11 +1165,14 @@ static void usbhsf_dma_complete(void *arg)
> struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
> struct device *dev = usbhs_priv_to_dev(priv);
> int ret;
> + unsigned long flags;
>
> + local_irq_save(flags);
Adding "local_irq_save()" without a spinlock is usually not correct.
I'm a bit confused here. usbhsf_pkt_handler() itself calls
usbhs_lock(priv, flags);
which is actually
spin_lock_irqsave(usbhs_priv_to_lock(priv), flags)
so it does disable interrupts internally?
Or is this about protecting the call to
pkt->done(priv, pkt);
at the end of usbhsf_pkt_handler(), which is done after releasing the
spinlock?
Still, that would need some better protection, as local_irq_save() disables
interrupts only on the CPU it's running on, not on other CPUs in a
multiprocessor system.
> ret = usbhsf_pkt_handler(pipe, USBHSF_PKT_DMA_DONE);
> if (ret < 0)
> dev_err(dev, "dma_complete run_error %d : %d\n",
> usbhs_pipe_number(pipe), ret);
> + local_irq_restore(flags);
> }
>
> void usbhs_fifo_clear_dcp(struct usbhs_pipe *pipe)
> --
> 1.7.9.5
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
[PATCH 1/4] usb: renesas_usbhs: fix spinlock recursion by usbhsf_dma_complete()
The usbhsf_pkt_handler(pipe, USBHSF_PKT_DMA_DONE) in usbhsf_dma_complete() will call the complete function of a usb gadget driver finally. According to the gadget.h, "The function will always be called with interrupts disabled". So, this patch adds a local_irq_save/local_irq_restore in the usbhsf_dma_complete() because a dmaengine driver may call this callback function when interrupts enabled (e.g. in tasklet). Signed-off-by: Yoshihiro Shimoda --- drivers/usb/renesas_usbhs/fifo.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c index d891bff..b1440d0 100644 --- a/drivers/usb/renesas_usbhs/fifo.c +++ b/drivers/usb/renesas_usbhs/fifo.c @@ -1165,11 +1165,14 @@ static void usbhsf_dma_complete(void *arg) struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe); struct device *dev = usbhs_priv_to_dev(priv); int ret; + unsigned long flags; + local_irq_save(flags); ret = usbhsf_pkt_handler(pipe, USBHSF_PKT_DMA_DONE); if (ret < 0) dev_err(dev, "dma_complete run_error %d : %d\n", usbhs_pipe_number(pipe), ret); + local_irq_restore(flags); } void usbhs_fifo_clear_dcp(struct usbhs_pipe *pipe) -- 1.7.9.5 -- 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
