Hi Geert-san,
Thank you for the reply!
> Hi Shimoda-san,
>
> On Mon, Feb 9, 2015 at 9:16 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> 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 <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > ---
> > 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: G W 3.19.0-rc4-01754-gfd5b84e-dirty #181
Hardware name: Generic R8A7791 (Flattened Device Tree)
Backtrace:
[<c0011e78>] (dump_backtrace) from [<c0012084>] (show_stack+0x18/0x1c)
r6:00000000 r5:ee1f5d5c r4:00000000 r3:00208040
[<c001206c>] (show_stack) from [<c046c45c>] (dump_stack+0x7c/0x98)
[<c046c3e0>] (dump_stack) from [<c046b754>] (spin_dump+0x80/0x94)
r4:ee967240 r3:c0653334
[<c046b6d4>] (spin_dump) from [<c046b794>] (spin_bug+0x2c/0x30)
r5:c059b229 r4:ee1f5d5c
[<c046b768>] (spin_bug) from [<c005578c>] (do_raw_spin_lock+0x50/0x190)
r5:0000000f r4:60000113
[<c005573c>] (do_raw_spin_lock) from [<c0470270>] (_raw_spin_lock_irqsave+0x18/0x20)
r9:ee1f5d5c r8:ee1f5d40 r7:eeaea434 r6:00000000 r5:0000000f r4:60000113
[<c0470258>] (_raw_spin_lock_irqsave) from [<bf0006e8>] (eth_start_xmit+0xd0/0x37c [u_ether])
r4:ee1f5800 r3:00000000
[<bf000618>] (eth_start_xmit [u_ether]) from [<bf01b0e8>] (ncm_tx_tasklet+0x44/0x4c [usb_f_ncm])
r10:eea5fd38 r9:c06987c0 r8:00000000 r7:00000000 r6:c0653274 r5:00000000
r4:ee115600
[<bf01b0a4>] (ncm_tx_tasklet [usb_f_ncm]) from [<c0029868>] (tasklet_action+0x94/0xf4)
r5:ee1156e0 r4:ee1156dc
[<c00297d4>] (tasklet_action) from [<c0028f3c>] (__do_softirq+0xec/0x220)
r8:c0658098 r7:00000100 r6:c0658088 r5:00000030 r4:eea5e000 r3:40000004
[<c0028e50>] (__do_softirq) from [<c00292ec>] (irq_exit+0x8c/0xfc)
r10:00000002 r9:60000013 r8:00000001 r7:ee806000 r6:00000000 r5:c0653ac8
r4:00000000
[<c0029260>] (irq_exit) from [<c005b1d8>] (__handle_domain_irq+0x94/0xb8)
r4:00000000 r3:000000a2
[<c005b144>] (__handle_domain_irq) from [<c0009394>] (gic_handle_irq+0x40/0x64)
r8:eea4f8b0 r7:eea5fe1c r6:c065e95c r5:eea5fde8 r4:f0002000 r3:eea5fde8
[<c0009354>] (gic_handle_irq) from [<c0012bc0>] (__irq_svc+0x40/0x54)
Exception stack(0xeea5fde8 to 0xeea5fe30)
fde0: ee1f5d5c ac4aac49 ee0b65e4 ee1f5d40 ee1f5d40 ee0b65c0
fe00: eeb5b280 ee1f5d5c eea4f8b0 60000013 00000002 eea5fe4c eea5fe20 eea5fe30
fe20: c0470254 bf0005b8 60000013 ffffffff
r6:ffffffff r5:60000013 r4:bf0005b8 r3:c0470254
[<bf000540>] (tx_complete [u_ether]) from [<c02d6a04>] (usb_gadget_giveback_request+0x14/0x18)
r7:eea4f810 r6:ee8e206c r5:00000000 r4:ee0b65f4
[<c02d69f0>] (usb_gadget_giveback_request) from [<c02d5d14>] (usbhsg_queue_done+0x2c/0x30)
[<c02d5ce8>] (usbhsg_queue_done) from [<c02d560c>] (usbhsf_pkt_handler+0xfc/0x114)
[<c02d5510>] (usbhsf_pkt_handler) from [<c02d5644>] (usbhsf_dma_complete+0x20/0x58)
r10:00000000 r9:00000001 r8:00200200 r7:00100100 r6:eeb118c8 r5:ee9a5a00
r4:ee8e206c
[<c02d5624>] (usbhsf_dma_complete) from [<c01f1aec>] (usb_dmac_isr_channel_thread+0x8c/0xcc)
r5:eeb11894 r4:ee9e852c
[<c01f1a60>] (usb_dmac_isr_channel_thread) from [<c005c2ec>] (irq_thread_fn+0x24/0x3c)
r8:c005c2c8 r7:ee1bbfa0 r6:ee998640 r5:eea5e000 r4:ee1bbf80 r3:c01f1a60
[<c005c2c8>] (irq_thread_fn) from [<c005c5c0>] (irq_thread+0xf4/0x16c)
r6:ee1bbf80 r5:eea5e000 r4:ee998640 r3:00000004
[<c005c4cc>] (irq_thread) from [<c003d7b4>] (kthread+0xf0/0x104)
r10:00000000 r9:00000000 r8:00000000 r7:c005c4cc r6:ee1bbf80 r5:00000000
r4:ee1bbf40
[<c003d6c4>] (kthread) from [<c000ebd8>] (ret_from_fork+0x14/0x3c)
r7:00000000 r6:00000000 r5:c003d6c4 r4:ee1bbf40
BUG: spinlock lockup suspected on CPU#1, iperf/2473
lock: 0xee1f5d5c, .magic: dead4ead, .owner: irq/102-e65a000/546, .owner_cpu: 0
CPU: 1 PID: 2473 Comm: iperf Tainted: G W 3.19.0-rc4-01754-gfd5b84e-dirty #181
Hardware name: Generic R8A7791 (Flattened Device Tree)
Backtrace:
[<c0011e78>] (dump_backtrace) from [<c0012084>] (show_stack+0x18/0x1c)
r6:00989680 r5:ee1f5d5c r4:00000000 r3:00404040
[<c001206c>] (show_stack) from [<c046c45c>] (dump_stack+0x7c/0x98)
[<c046c3e0>] (dump_stack) from [<c046b754>] (spin_dump+0x80/0x94)
r4:ee967240 r3:c0653334
[<c046b6d4>] (spin_dump) from [<c0055878>] (do_raw_spin_lock+0x13c/0x190)
r5:00000000 r4:00989680
[<c005573c>] (do_raw_spin_lock) from [<c0470270>] (_raw_spin_lock_irqsave+0x18/0x20)
r9:ee1f5d5c r8:ee1f5d40 r7:eeaea434 r6:ee28f580 r5:0000000f r4:20000013
[<c0470258>] (_raw_spin_lock_irqsave) from [<bf0006e8>] (eth_start_xmit+0xd0/0x37c [u_ether])
r4:ee1f5800 r3:00000001
[<bf000618>] (eth_start_xmit [u_ether]) from [<c0396c70>] (dev_hard_start_xmit+0x250/0x2e8)
r10:ee346c60 r9:ee2a2100 r8:ee1f5800 r7:00000000 r6:ee346c00 r5:ee28f580
r4:00000001
[<c0396a20>] (dev_hard_start_xmit) from [<c03aeb58>] (sch_direct_xmit+0xa8/0x1d0)
r10:ee346c60 r9:ee1f5800 r8:ee28f580 r7:ee2a2100 r6:ee346c00 r5:ee346c00
r4:00000001
[<c03aeab0>] (sch_direct_xmit) from [<c0396f54>] (__dev_queue_xmit+0x24c/0x498)
r10:ee346c60 r9:00000000 r8:00000000 r7:ee2a2100 r6:ee1f5800 r5:ee346c00
r4:ee28f580
[<c0396d08>] (__dev_queue_xmit) from [<c03971b4>] (dev_queue_xmit+0x14/0x18)
r10:00000000 r9:ee3380cc r8:ee3380bc r7:00000000 r6:ee28f580 r5:0000000e
r4:ee338000
[<c03971a0>] (dev_queue_xmit) from [<c03bd1e8>] (ip_finish_output+0x8e4/0x96c)
[<c03bc904>] (ip_finish_output) from [<c03be674>] (ip_output+0xa8/0xb4)
r9:001105c6 r8:00000000 r7:eeaabef0 r6:eeaabc80 r5:ee1f5800 r4:ee28f580
[<c03be5cc>] (ip_output) from [<c03bdf0c>] (ip_local_out_sk+0x3c/0x40)
r5:eeaabc80 r4:ee28f580
[<c03bded0>] (ip_local_out_sk) from [<c03bf0cc>] (ip_send_skb+0x18/0xa0)
r5:c068f3c0 r4:ee28f580
[<c03bf0b4>] (ip_send_skb) from [<c03df984>] (udp_send_skb+0x188/0x234)
r5:edc0f824 r4:ee28f580
[<c03df7fc>] (udp_send_skb) from [<c03e1944>] (udp_sendmsg+0x4f0/0x6dc)
r9:00008913 r8:000005be r7:eeaabef0 r6:1616a8c0 r5:ee95be84 r4:eeaabc80
[<c03e1454>] (udp_sendmsg) from [<c03ea704>] (inet_sendmsg+0x84/0xb4)
r10:00000000 r9:0001e5b0 r8:ee326100 r7:000005be r6:ee95be84 r5:ee95bee8
r4:eeaabc80
[<c03ea680>] (inet_sendmsg) from [<c037fd30>] (sock_aio_write+0xe4/0xf4)
r7:ee95be64 r6:ee95bee8 r5:ee40a580 r4:000005be
[<c037fc4c>] (sock_aio_write) from [<c00c7b3c>] (do_sync_write+0x74/0x98)
r8:000005be r7:ee326100 r6:ee95bf78 r5:00000000 r4:00000000
[<c00c7ac8>] (do_sync_write) from [<c00c83e0>] (vfs_write+0xe4/0x194)
r8:000005be r7:000005be r6:ee95bf78 r5:0001e5b0 r4:ee326100
[<c00c82fc>] (vfs_write) from [<c00c8980>] (SyS_write+0x44/0x84)
r9:0001e5b0 r8:000005be r7:ee326101 r6:ee326100 r5:00000000 r4:00000000
[<c00c893c>] (SyS_write) from [<c000eb40>] (ret_fast_syscall+0x0/0x30)
r9:ee95a000 r8:c000ece4 r7:00000004 r6:ffffc3ef r5:0001eb78 r4:0001e1c8
=====================
Best regards,
Yoshihiro Shimoda
> > 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 -- geert@xxxxxxxxxxxxxx
>
> 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
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
