Re: [PATCH] USB: add SPDX identifiers to all files in drivers/usb/

[Thomas Gleixner]

On Thu, 19 Oct 2017, Geert Uytterhoeven wrote:
> On Thu, Oct 19, 2017 at 10:52 AM, Greg Kroah-Hartman
> <gre...@linuxfoundation.org> wrote:
> > On Thu, Oct 19, 2017 at 10:49:47AM +0200, Geert Uytterhoeven wrote:
> >> On Thu, Oct 19, 2017 at 10:38 AM, Greg Kroah-Hartman
> >> <gre...@linuxfoundation.org> wrote:
> >> > It's good to have SPDX identifiers in all files to make it easier to
> >> > audit the kernel tree for correct licenses.  This patch adds these
> >> > identifiers to all files in drivers/usb/ based on a script and data from
> >> > Thomas Gleixner, Philippe Ombredanne, and Kate Stewart.
> >> >
> >> > Cc: Thomas Gleixner <t...@linutronix.de>
> >> > Cc: Kate Stewart <kstew...@linuxfoundation.org>
> >> > Cc: Philippe Ombredanne <pombreda...@nexb.com>
> >> > Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> >> > ---
> >> > Unless someone really complains, I'm going to add this to my tree for
> >> > 4.15-rc1.
> >>
> >> It would be good to include a diffstat, esp. when touching +600 files.
> >
> > I wanted to, but I was worried it would be too long to prevent the patch
> > from hitting the list :)
> >
> > Here it is:
> 
> [...]
> 
> Thanks!
> 
> BTW, some files seem to be "SPDX-License-Identifier: GPL-1.0+".
> Was this intentional, given COPYING says the default is v2?

Yes. The license mentioned in the file says something like:

 This is licensed under GPL.

which is equivalent to GPL-1.0+.

Thanks,

tglx
--
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] USB: add SPDX identifiers to all files in drivers/usb/

[Thomas Gleixner]

On Thu, 19 Oct 2017, Greg Kroah-Hartman wrote:
> On Thu, Oct 19, 2017 at 10:50:44AM +0200, Thomas Gleixner wrote:
> > The last discussion about this was to add the identifier as the first line
> > of the file or as the second in case of files with a shebang in the first
> > one.
> > 
> > I think you missed the last version of the script. Attached.
> 
> Ugh, that's ugly, creating stuff like this:
> 
> --- a/drivers/usb/atm/speedtch.c
> +++ b/drivers/usb/atm/speedtch.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0+
>  
> /**
>   *  speedtch.c  -  Alcatel SpeedTouch USB xDSL modem driver
>   *
> diff --git a/drivers/usb/atm/ueagle-atm.c b/drivers/usb/atm/ueagle-atm.c
> 
> 
> Are we really ok with the '//' comments?

That's what Linus suggested so it stands out.

Thanks,

tglx
--
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] USB: add SPDX identifiers to all files in drivers/usb/

[Thomas Gleixner]

On Thu, 19 Oct 2017, Greg Kroah-Hartman wrote:

> It's good to have SPDX identifiers in all files to make it easier to
> audit the kernel tree for correct licenses.  This patch adds these
> identifiers to all files in drivers/usb/ based on a script and data from
> Thomas Gleixner, Philippe Ombredanne, and Kate Stewart.
> 
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Kate Stewart <kstew...@linuxfoundation.org>
> Cc: Philippe Ombredanne <pombreda...@nexb.com>
> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> ---
> Unless someone really complains, I'm going to add this to my tree for
> 4.15-rc1.
> 
> 
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index 9650b351c26c..cb8d902b801d 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -1,6 +1,7 @@
>  #
>  # Makefile for the kernel USB device drivers.
>  #
> +# SPDX-License-Identifier: GPL-2.0

The last discussion about this was to add the identifier as the first line
of the file or as the second in case of files with a shebang in the first
one.

I think you missed the last version of the script. Attached.

Thanks,

tglx



#!/usr/bin/env python
#
import sys
import os

def insert_at(srclines, pos, tag, style):
srclines.insert(pos, '%s SPDX-License-Identifier: %s\n' %(style, tag))
return True

def handle_c_h(srclines, tag):
return insert_at(srclines, 0, tag, '//')

def handle_asm(srclines, tag):
# Stupid search for a proper style to comment the SPDX tag
pos = 0
style = None
for line in srclines:
pos += 1
if line.startswith(';;;'):
style = ';;;'
elif line.startswith(';;'):
style = ';;'
elif line.startswith(';'):
style = ';'
elif line.startswith('|'):
style = '|'
elif line.startswith('!'):
style = '!'
elif line.startswith('//'):
style = '//'
elif line.startswith("/*"):
style = '//'
else:
continue
return insert_at(srclines, 0, tag, style)
return False

def handle_sh(srclines, tag):
return insert_at(srclines, 1, tag, '#')

tf = open(sys.argv[1])

for entry in tf.readlines():

if len(entry.strip()) == 0:
continue

nr, fname, tag = entry.strip().split(',')
# FIXME: Use a proper encoder
fname = fname.replace('%2C',',')
if tag == 'NOTAG':
print("Skipping %s" %fname)
continue

if not os.path.isfile(fname):
print("FAIL: File %s does not exist anymore" %fname)
continue

srclines = open(fname).readlines()

done = False
for line in srclines:
if line.find('SPDX-License-Identifier') >= 0:
done = True
break

if done:
print("SPDX id exists already in %s" %fname)
continue

ok = False
if fname.endswith('.c') or fname.endswith('.h') or fname.endswith('.uc'):
ok = handle_c_h(srclines, tag)

elif fname.endswith('.S'):
ok = handle_asm(srclines, tag)

elif fname.endswith('.cocci'):
ok = insert_at(srclines, 0, tag, '//')

elif fname.endswith('.dts') or fname.endswith('.dtsi'):
ok = insert_at(srclines, 0, tag, '//')

elif fname.endswith('.py') or fname.endswith('.tc') or fname.endswith('.sh'):
ok = insert_at(srclines, 1, tag, '#')

elif fname.endswith('Makefile') or fname.find('Kconfig') > 0 or fname.find('Kbuild') > 0:
ok = insert_at(srclines, 0, tag, '#')

else:
print("Unhandled or ignored file %s" %fname)
continue

if ok:
open(fname, 'w').writelines(srclines)
print("Inserted %s into %s" %(tag, fname))
else:
print("FAIL: No place for insertion found %s" %fname)

Re: v4.10-rc8 (-rc6) boot regression on Intel desktop, does not boot after cold boots, boots after reboot

[Thomas Gleixner]

On Fri, 17 Feb 2017, Frederic Weisbecker wrote:
> On Thu, Feb 16, 2017 at 08:34:45PM +0100, Thomas Gleixner wrote:
> > On Thu, 16 Feb 2017, Frederic Weisbecker wrote:
> > > On Thu, Feb 16, 2017 at 10:20:14AM -0800, Linus Torvalds wrote:
> > > > On Thu, Feb 16, 2017 at 10:13 AM, Frederic Weisbecker
> > > > <fweis...@gmail.com> wrote:
> > > > >
> > > > > I haven't followed the discussion but this patch has a known issue 
> > > > > which is fixed
> > > > > with:
> > > > > 7bdb59f1ad474bd7161adc8f923cdef10f2638d1
> > > > > "tick/nohz: Fix possible missing clock reprog after tick soft 
> > > > > restart"
> > > > >
> > > > > I hope this fixes your issue.
> > > > 
> > > > No, Pavel saw the problem with rc8 too, which already has that fix.
> > > > 
> > > > So I think we'll just need to revert that original patch (and that
> > > > means that we have to revert the commit you point to as well, since
> > > > that ->next_tick field was added by the original commit).
> > > 
> > > Aw too bad, but indeed that late we don't have the choice.
> > 
> > Hint: Look for CPU hotplug interaction of these patches. I bet something
> > becomes stale when the CPU goes down and does not get reset when it comes
> > back online.
> 
> Indeed I should check that. But Pavel is seeing this on boot, where the

I don't think so. He observed it on suspend resume and by doing hotplug
operations in a loop. But I might be wrong as usual.

> only hotplug operations that happen are CPU UP without preceding CPU DOWN
> that may have retained stale values. I think the value of ts->next_tick should
> be initially 0 for all CPUs. So perhaps that 0 value confuses stuff. But
> looking at the code I don't see how. It maybe something more subtle.
> 
--
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: v4.10-rc8 (-rc6) boot regression on Intel desktop, does not boot after cold boots, boots after reboot

[Thomas Gleixner]

On Thu, 16 Feb 2017, Frederic Weisbecker wrote:
> On Thu, Feb 16, 2017 at 10:20:14AM -0800, Linus Torvalds wrote:
> > On Thu, Feb 16, 2017 at 10:13 AM, Frederic Weisbecker
> >  wrote:
> > >
> > > I haven't followed the discussion but this patch has a known issue which 
> > > is fixed
> > > with:
> > > 7bdb59f1ad474bd7161adc8f923cdef10f2638d1
> > > "tick/nohz: Fix possible missing clock reprog after tick soft restart"
> > >
> > > I hope this fixes your issue.
> > 
> > No, Pavel saw the problem with rc8 too, which already has that fix.
> > 
> > So I think we'll just need to revert that original patch (and that
> > means that we have to revert the commit you point to as well, since
> > that ->next_tick field was added by the original commit).
> 
> Aw too bad, but indeed that late we don't have the choice.

Hint: Look for CPU hotplug interaction of these patches. I bet something
becomes stale when the CPU goes down and does not get reset when it comes
back online.

Thanks,

tglx



--
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 v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Sat, 12 Nov 2016, Lu Baolu wrote:
> On 11/11/2016 08:28 PM, Peter Zijlstra wrote:
> > Again, a UART rules. Make a virtual UART in hardware, that'd be totally
> > awesome. This thing, I'm not convinced its worth having.
> 
> This is the initial work. It helps at least in cases where people need
> to dump kernel messages but lacking of a console. It's also a cheap
> way, people don't need to buy any third-party devices.
> 
> With more and more people trying and enhancing it, it will become
> more robust and helpful.

Well, not really. The hardware does not allow you what an UART allows you
to do and never will, no matter how many people are trying to enhance the
software part of it.

Thanks,

tglx
--
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 v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Thu, 10 Nov 2016, Lu Baolu wrote:
> On 11/09/2016 05:37 PM, Thomas Gleixner wrote:
> > On Tue, 1 Nov 2016, Lu Baolu wrote:
> >> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
> >> +{
> >> +  int chunk, ret;
> >> +  static char buf[XDBC_MAX_PACKET];
> >> +  int use_cr = 0;
> >> +
> >> +  if (!xdbc.xdbc_reg)
> >> +  return;
> >> +  memset(buf, 0, XDBC_MAX_PACKET);
> > How is that dealing with reentrancy?
> >
> > early_printk() does not protect against it. Peter has a patch to prevent
> > concurrent access from different cpus, but it cannot and will never prevent
> > reentrancy on the same cpu (interrupt, nmi).
> 
> I can use a spinlock_irq to protect reentrancy of interrupt on the same
> cpu. But I have no idea about the nmi one.

spinlock wont work due to NMIs.

> This seems to be a common issue for all early printk drivers.

No. The other early printk drivers like serial do not have that problem as
they simply do:

   while (*buf) {
  while (inb(UART) & TX_BUSY)
 cpu_relax();
  outb(*buf++, UART);
   }

The wait for the UART to become ready is independent of the context as it
solely depends on the hardware.

As a result you can see the output from irq/nmi intermingled with the one
from thread context, but that's the only problem they have.

The only thing you can do to make this work is to prevent printing in NMI
context:

write()
{
if (in_nmi())
return;

raw_spinlock_irqsave(, flags);


That fully serializes the writes and just ignores NMI context printks. Not
optimal, but I fear that's all you can do.

Thanks,

tglx
--
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 v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Tue, 1 Nov 2016, Lu Baolu wrote:
> +static void early_xdbc_write(struct console *con, const char *str, u32 n)
> +{
> + int chunk, ret;
> + static char buf[XDBC_MAX_PACKET];
> + int use_cr = 0;
> +
> + if (!xdbc.xdbc_reg)
> + return;
> + memset(buf, 0, XDBC_MAX_PACKET);

How is that dealing with reentrancy?

early_printk() does not protect against it. Peter has a patch to prevent
concurrent access from different cpus, but it cannot and will never prevent
reentrancy on the same cpu (interrupt, nmi).

Thanks,

tglx
--
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 v4 1/4] usb: dbc: early driver for xhci debug capability

[Thomas Gleixner]

On Tue, 1 Nov 2016, Lu Baolu wrote:
> +static int __init xdbc_init(void)
> +{
...
> + base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
> + if (!base) {
> + xdbc_trace("failed to remap the io address\n");
> + ret = -ENOMEM;
> + goto free_and_quit;
> + }
> +
> + early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
> + xdbc_trace("early mapped IO address released\n");
> +
> + xdbc.xhci_base = base;
> + offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_DEBUG);
> + xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);

This is broken. What prevents that 

 - a printk is in progress on another cpu?

 - a printk happens between the unmap and storing the new base ?

Nothing AFAICT. So this needs to be done in a safe way. And just making it

oldbase = xdbc.xhci_base;
base = ioremap();
xdbc.xhci_base = base;
early_iounmap(oldbase);

does not work either because the compiler can rightfully cache
xdbc.xhci_base in the write related functions. The same issue with
xdbc.xdbc_reg.

Thanks,

tglx
--
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: [BUG] Lockdep splat in usb network gadget

[Thomas Gleixner]

On Wed, 23 Jul 2014, Li RongQing wrote:

 This commit introduced this bug
 
 commit a9232076374334ca2bc2a448dfde96d38a54349a
 Author: Jeff Westfahl jeff.westf...@ni.com
 Date:   Thu May 29 09:49:41 2014 +0300
 
 usb: gadget: u_ether: synchronize with transmit when stopping queue
 
 When disconnecting, it's possible that another thread has already made it
 into eth_start_xmit before we call netif_stop_queue. This can lead to a
 crash as eth_start_xmit tries to use resources that gether_disconnect is
 freeing. Use netif_tx_lock/unlock around netif_stop_queue to ensure no
 threads are executing during the remainder of gether_disconnect.
 
 Signed-off-by: Jeff Westfahl jeff.westf...@ni.com
 Tested-by: Jaeden Amero jaeden.am...@ni.com
 Signed-off-by: Felipe Balbi ba...@ti.com

So what's the fix?
 
 diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
 index 3d78a88..97b0277 100644
 --- a/drivers/usb/gadget/u_ether.c
 +++ b/drivers/usb/gadget/u_ether.c
 @@ -1120,7 +1120,10 @@ void gether_disconnect(struct gether *link)
 
 DBG(dev, %s\n, __func__);
 
 +   netif_tx_lock(dev-net);
 netif_stop_queue(dev-net);
 +   netif_tx_unlock(dev-net);
 +
 netif_carrier_off(dev-net);
--
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

[BUG] Lockdep splat in usb network gadget

[Thomas Gleixner]

On a Intel E660/EG20T system I can observe the following lockdep splats:

[   10.154920] =
[   10.156026] [ INFO: inconsistent lock state ]
[   10.156026] 3.16.0-rc5+ #13 Not tainted
[   10.156026] -
[   10.156026] inconsistent {IN-HARDIRQ-W} - {HARDIRQ-ON-W} usage.
[   10.156026] swapper/1/0 [HC0[0]:SC1[5]:HE1:SE0] takes:
[   10.156026]  (_xmit_ETHER){?.-...}, at: [80948b6a] 
sch_direct_xmit+0x7a/0x250
[   10.156026] {IN-HARDIRQ-W} state was registered at:
[   10.156026]   [804811f0] __lock_acquire+0x800/0x17a0
[   10.156026]   [804828ba] lock_acquire+0x6a/0xf0
[   10.156026]   [809ed477] _raw_spin_lock+0x27/0x40
[   10.156026]   [8088d508] gether_disconnect+0x68/0x280
[   10.156026]   [8088e777] eem_set_alt+0x37/0xc0
[   10.156026]   [808847ce] composite_setup+0x30e/0x1240
[   10.156026]   [8088b8ae] pch_udc_isr+0xa6e/0xf50
[   10.156026]   [8048abe8] handle_irq_event_percpu+0x38/0x1e0
[   10.156026]   [8048adc1] handle_irq_event+0x31/0x50
[   10.156026]   [8048d94b] handle_fasteoi_irq+0x6b/0x140
[   10.156026]   [804040a5] handle_irq+0x65/0x80
[   10.156026]   [80403cfc] do_IRQ+0x3c/0xc0
[   10.156026]   [809ee6ae] common_interrupt+0x2e/0x34
[   10.156026]   [804668c5] finish_task_switch+0x65/0xd0
[   10.156026]   [809e89df] __schedule+0x20f/0x7d0
[   10.156026]   [809e94aa] schedule_preempt_disabled+0x2a/0x70
[   10.156026]   [8047bf03] cpu_startup_entry+0x143/0x410
[   10.156026]   [809e2e61] rest_init+0xa1/0xb0
[   10.156026]   [80ce2a3b] start_kernel+0x336/0x33b
[   10.156026]   [80ce22ab] i386_start_kernel+0x79/0x7d
[   10.156026] irq event stamp: 52070
[   10.156026] hardirqs last  enabled at (52070): [809375de] 
neigh_resolve_output+0xee/0x2a0
[   10.156026] hardirqs last disabled at (52069): [809375a8] 
neigh_resolve_output+0xb8/0x2a0
[   10.156026] softirqs last  enabled at (52020): [8044401f] 
_local_bh_enable+0x1f/0x50
[   10.156026] softirqs last disabled at (52021): [80404036] 
do_softirq_own_stack+0x26/0x30
[   10.156026] 
[   10.156026] other info that might help us debug this:
[   10.156026]  Possible unsafe locking scenario:
[   10.156026] 
[   10.156026]CPU0
[   10.156026]
[   10.156026]   lock(_xmit_ETHER);
[   10.156026]   Interrupt
[   10.156026] lock(_xmit_ETHER);
[   10.156026] 
[   10.156026]  *** DEADLOCK ***
[   10.156026] 
[   10.156026] 4 locks held by swapper/1/0:
[   10.156026]  #0:  (((idev-mc_ifc_timer))){+.-...}, at: [8044b100] 
call_timer_fn+0x0/0x190
[   10.156026]  #1:  (rcu_read_lock){..}, at: [a0577c40] 
mld_sendpack+0x0/0x590 [ipv6]
[   10.156026]  #2:  (rcu_read_lock_bh){..}, at: [a055680c] 
ip6_finish_output2+0x4c/0x7f0 [ipv6]
[   10.156026]  #3:  (rcu_read_lock_bh){..}, at: [8092e510] 
__dev_queue_xmit+0x0/0x5f0
[   10.156026] 
[   10.156026] stack backtrace:
[   10.156026] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.16.0-rc5+ #13
[   10.156026]  811dbb10  9e919d10 809e6785 9e8b8000 9e919d3c 809e561e 
80b95511
[   10.156026]  80b9545a 80b9543d 80b95450 80b95441 80b957e4 9e8b84e0 0002 
8047f7b0
[   10.156026]  9e919d5c 8048043b 0002  9e8b8000 0001 0004 
9e8b8000
[   10.156026] Call Trace:
[   10.156026]  [809e6785] dump_stack+0x48/0x69
[   10.156026]  [809e561e] print_usage_bug+0x18f/0x19c
[   10.156026]  [8047f7b0] ? print_shortest_lock_dependencies+0x170/0x170
[   10.156026]  [8048043b] mark_lock+0x53b/0x5f0
[   10.156026]  [804810cf] __lock_acquire+0x6df/0x17a0
[   10.156026]  [804828ba] lock_acquire+0x6a/0xf0
[   10.156026]  [80948b6a] ? sch_direct_xmit+0x7a/0x250
[   10.156026]  [809ed477] _raw_spin_lock+0x27/0x40
[   10.156026]  [80948b6a] ? sch_direct_xmit+0x7a/0x250
[   10.156026]  [80948b6a] sch_direct_xmit+0x7a/0x250
[   10.156026]  [8092e6bf] __dev_queue_xmit+0x1af/0x5f0
[   10.156026]  [80947fc0] ? ether_setup+0x80/0x80
[   10.156026]  [8092eb0f] dev_queue_xmit+0xf/0x20
[   10.156026]  [8093764c] neigh_resolve_output+0x15c/0x2a0
[   10.156026]  [a0556927] ip6_finish_output2+0x167/0x7f0 [ipv6]
[   10.156026]  [a0559b05] ip6_finish_output+0x85/0x1c0 [ipv6]
[   10.156026]  [a0559cb7] ip6_output+0x77/0x240 [ipv6]
[   10.156026]  [a0578163] mld_sendpack+0x523/0x590 [ipv6]
[   10.156026]  [80480501] ? mark_held_locks+0x11/0x90
[   10.156026]  [a057947d] mld_ifc_timer_expire+0x15d/0x280 [ipv6]
[   10.156026]  [8044b168] call_timer_fn+0x68/0x190
[   10.156026]  [a0579320] ? igmp6_group_added+0x150/0x150 [ipv6]
[   10.156026]  [8044b3fa] run_timer_softirq+0x16a/0x240
[   10.156026]  [a0579320] ? igmp6_group_added+0x150/0x150 [ipv6]
[   10.156026]  [80444984] __do_softirq+0xd4/0x2f0
[   10.156026]  [804448b0] ? tasklet_action+0x100/0x100
[   10.156026]  [80404036] do_softirq_own_stack+0x26/0x30
[   10.156026]  IRQ  [80444d05] irq_exit+0x65/0x70
[   10.156026]  [8042d758] smp_apic_timer_interrupt+0x38/0x50
[   10.156026]  [809ee91f] apic_timer_interrupt+0x2f/0x34
[   10.156026]  [8048007b] ? mark_lock+0x17b/0x5f0
[   10.156026]  

usb: musb: Ensure that cppi41 timer gets armed on premature DMA TX irq

[Thomas Gleixner]

Some TI chips raise the DMA complete interrupt before the actual
transfer has been completed. The code tries to busy wait for a few
microseconds and if that fails it arms an hrtimer to recheck. So far
so good, but that has the following issue:

CPU 0   CPU1

start_next_transfer(RQ1);

DMA interrupt
  if (premature_irq(RQ1))
if (!hrtimer_active(timer))
   hrtimer_start(timer);

hrtimer expires
  timer-state = CALLBACK_RUNNING;
  timer-fn() 
cppi41_recheck_tx_req()
  complete_request(RQ1); 
  if (requests_pending())
start_next_transfer(RQ2);

DMA interrupt
  if (premature_irq(RQ2))
if (!hrtimer_active(timer))
   hrtimer_start(timer);
  timer-state = INACTIVE; 

The premature interrupt of request2 on CPU1 does not arm the timer and
therefor the request completion never happens because it checks for
!hrtimer_active(). hrtimer_active() evaluates:

  timer-state != HRTIMER_STATE_INACTIVE

which of course evaluates to true in the above case as timer-state is
CALLBACK_RUNNING.

That's clearly documented:

 * A timer is active, when it is enqueued into the rbtree or the
 * callback function is running or it's in the state of being migrated  
 * to another cpu. 

But that's not what the code wants to check. The code wants to check
whether the timer is queued, i.e. whether its armed and waiting for
expiry.

We have a helper function for this: hrtimer_is_queued(). This
evaluates:

  timer-state  HRTIMER_STATE_QUEUED

So in the above case this evaluates to false and therefor forces the
DMA interrupt on CPU1 to call hrtimer_start().

Use hrtimer_is_queued() instead of hrtimer_active() and evrything is
good.

Reported-by: Torben Hohn torb...@linutronix.de
Signed-off-by: Thomas Gleixner t...@linutronix.de
Cc: sta...@vger.kernel.org
---
 drivers/usb/musb/musb_cppi41.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/usb/musb/musb_cppi41.c
===
--- linux.orig/drivers/usb/musb/musb_cppi41.c
+++ linux/drivers/usb/musb/musb_cppi41.c
@@ -318,7 +318,7 @@ static void cppi41_dma_callback(void *pr
}
list_add_tail(cppi41_channel-tx_check,
controller-early_tx_list);
-   if (!hrtimer_active(controller-early_tx)) {
+   if (!hrtimer_is_queued(controller-early_tx)) {
hrtimer_start_range_ns(controller-early_tx,
ktime_set(0, 140 * NSEC_PER_USEC),
40 * NSEC_PER_USEC,
--
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] usb: mark *hci_pci irqs with IRQF_NO_THREAD

[Thomas Gleixner]

On Mon, 17 Feb 2014, Stanislaw Gruszka wrote:

 There is threadirqs kenel boot option which allow to force interrupt
 routines to be performed as thread. 
 
 USB irq routines use spin_lock(*hci-lock) variant without disabling
 interrupts, what is perfectly fine, but that can cause deadlock when
 forced thread irqs are used. Deadlock scenario is quite reproducible for
 me, as I can not boot system with threadirqs option, when some USB
 device is connected. Patch marks USB irq routines with IRQF_NO_THREAD
 to prevent forced threading.
 
 Signed-off-by: Stanislaw Gruszka sgrus...@redhat.com

NACK.

This is the wrong approach and will break RT. See the discussion about
EHCI.

Thanks,

tglx


 ---
  drivers/usb/core/hcd-pci.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
 index d59d993..b4fa1a5 100644
 --- a/drivers/usb/core/hcd-pci.c
 +++ b/drivers/usb/core/hcd-pci.c
 @@ -264,7 +264,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct 
 pci_device_id *id)
   down_write(companions_rwsem);
   dev_set_drvdata(dev-dev, hcd);
   for_each_companion(dev, hcd, ehci_pre_add);
 - retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
 + retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED | 
 IRQF_NO_THREAD);
   if (retval != 0)
   dev_set_drvdata(dev-dev, NULL);
   for_each_companion(dev, hcd, ehci_post_add);
 @@ -272,7 +272,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct 
 pci_device_id *id)
   } else {
   down_read(companions_rwsem);
   dev_set_drvdata(dev-dev, hcd);
 - retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
 + retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED | 
 IRQF_NO_THREAD);
   if (retval != 0)
   dev_set_drvdata(dev-dev, NULL);
   else
 -- 
 1.7.11.7
 
 
--
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: USB storage vanilla kernel 3.13 hang on DELL PRECISION M6400

[Thomas Gleixner]

On Fri, 14 Feb 2014, Stefani Seibold wrote:
 Am Freitag, den 14.02.2014, 10:32 -0500 schrieb Alan Stern:
  On Fri, 14 Feb 2014, Stefani Seibold wrote:
  
   Hi Alan;
   
   Am Donnerstag, den 13.02.2014, 15:55 -0500 schrieb Alan Stern:
   
Stefani, please try the patch below, with threadirq and the plain
vanilla kernel.  It ought to fix your problem, but let's make certain.
   
   I tried the patch on 3.13.2 together with the commit 88ed9fd50e57 and it
   seems that it solves the problem.
   
   I also tried the current 3.14.0-rc2-00271-g4675348 without any
   modifications and without our patch... and it also works.
  
  3.14-rc2 works with no modifications at all?  Then it looks like the 
  patch is unnecessary.
  
  But I'm surprised that it works.  As far as I know, there haven't been 
  any changes since 3.13 that would fix this problem.
  
 
 I surprised too. I think it is a kind of race condition and IMHO the
 patch is necessary.

Lockdep does not care about race conditions and humble opinions at
all. It observes that one place takes the lock with interrupts enabled
and the other takes it in hardirq context. It's that simple.

So now the question is why one kernel triggers the lockdep warning and
a later version does not while all involved parties claim that nothing
has changed.

Before we jump into detailed analysis, let's make sure that this
happens

  - with the same .config (i.e. lockdep and the device driver in
question enabled)

  - the same system (i.e. the same drivers loaded and handling the same
devices)

  - the device driver actually doing something which might cause
lockdep to yell.

When this is the case, then the claim that nothing has changed is
simply wrong and we need to figure out which allegedly unrelated
change has caused this change in behaviour.

The patch is needed from a code analysis POV. I've been wrong before,
but you really need to provide evidence why 3.14-rc2 does not show the
behaviour and which change has caused that before I'm going to back
off. IMHO is not a convincing argument!

Thanks,

tglx
--
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: USB storage vanilla kernel 3.13 hang on DELL PRECISION M6400

[Thomas Gleixner]

On Wed, 12 Feb 2014, Alan Stern wrote:
 I have no idea what might have changed between 3.12 and 3.13 to cause 
 this problem.  Maybe Thomas can figure it out.
 
  And yes, the issues goes away when no thread irqs are used (with and
  without the patch).
 
 Thomas, there must be some reason why the patch below is wrong, but I
 don't know enough about the IRQ subsystem to tell what's really going
 on.  Can you explain it?

If we force all interrupts into threading, then there is no reason to
disable interrupts and lockdep should not complain at all because all
accesses happen in thread context.

Now the ehci case is different: 

The hrtimer callback (ehci_hrtimer_func) still runs from hard
interrupt context and is of course taking the lock which in turn
causes lockdep to yell.

So the local_irq_save() in the usb interrupt handler was hiding the
issue so far. That's why reverting 88ed9fd50e cures it.

On RT we cure it differently as we move all hrtimers which are not
explicitely marked as hardirq safe into the threaded softirq context.

Not sure what's the best solution to solve this, but I really want to
avoid the general interrupt disable in the forced threaded mode.

The simple solution is of course to take the lock in ehci_irq with
spin_lock_irqsave(), but it's not that pretty either ...

Thanks,

tglx
--
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: USB storage vanilla kernel 3.13 hang on DELL PRECISION M6400

[Thomas Gleixner]

On Thu, 13 Feb 2014, Alan Stern wrote:
 On Thu, 13 Feb 2014, Thomas Gleixner wrote:
 I can think of two other solutions.  The first is to move the
 non-hardirq-safe hrtimers into threaded softirq context, as mentioned
 above.

Right, but that's major surgery.
 
 The second is for ehci_hrtimer_func to schedule a tasklet instead of
 doing its work directly.  But this seems like an awkward workaround for 
 something which shouldn't be a problem in the first place.

Eeew.
 
 Of the three, using spin_lock_irqsave in ehci_irq is the simplest.  If 
 the hrtimers can't be moved to threaded context, that's what I'll end 
 up doing.

I think, that's the simplest option for now. We'll look into the
hrtimer issue anyway, but as I said it's not a two lines patch.

Thanks,

tglx
--
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: [RFC PATCH 0/4] USB: HCD/EHCI: giveback of URB in tasklet context

[Thomas Gleixner]

On Fri, 14 Jun 2013, Alan Stern wrote:

 On Fri, 14 Jun 2013, Ming Lei wrote:
 
   With the two trace points of irq_handler_entry and irq_handler_exit, the
   interrupt latency(or the time taken by hard irq handler) isn't hard to 
   measure.
   One simple script can figure out the average/maximum latency for one irq
   handler, like I did in 4/4.
  
   But that doesn't measure the time between when the IRQ request is
   issued and when irq_handler_entry runs.
  
  That might be hard to measure, also I am wondering if the time can be
  measured only by software, but these patches only focus on the time
  between HCD irq entry and irq exit.
 
 Not entirely.  On a UP system, leaving interrupts disabled for a long
 time (which is what we do now) increases the delay between when the IRQ
 is raised and when it is serviced.  On an SMP system, a long-running 
 interrupt handler will delay servicing a different device that shares 
 the same IRQ line.
 
And on UP it delays ALL other interrupts. I've seen 500us+ caused by
the USB interrupt handlers...

Thanks,

tglx
--
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: [RFC PATCH 0/4] USB: HCD/EHCI: giveback of URB in tasklet context

[Thomas Gleixner]

On Wed, 12 Jun 2013, Ming Lei wrote:
 On Wed, Jun 12, 2013 at 3:10 AM, Alan Stern st...@rowland.harvard.edu wrote:
  Also the tasklet running in CPU0 can handle the work which should
  have been done by the same tasket scheduled in CPU1,  so we can
  avoid busy-waitting in CPU1 which may be introduced by tasklet_schedule()
  in CPU1.
 
  Or you could have a separate tasklet for each host controller.
 
 Yes, but I will compare tasklet with interrupt threaded handler first.

Yes, please. I read through the thread and I really recommend that you
get rid of the tasklet. tasklets are a complete disaster by design and
we really should make efforts to get rid of the last users instead of
trying to work around their semantical shortcomings.

Thanks,

tglx
--
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 Revert USB/host: Cleanup unneccessary irq disable code added to usb tree

[Thomas Gleixner]

On Wed, 14 Nov 2012, Alan Stern wrote:

 On Tue, 13 Nov 2012 gre...@linuxfoundation.org wrote:
 
  From e592c5d0b7db94b485b4a2342db041a1882b7f75 Mon Sep 17 00:00:00 2001
  From: Greg Kroah-Hartman gre...@linuxfoundation.org
  Date: Tue, 13 Nov 2012 10:52:52 -0800
  Subject: Revert USB/host: Cleanup unneccessary irq disable code
  
  This reverts commit 73d4066055e0e2830533041f4b91df8e6e5976ff.
  
  Martin Steigerwald reported that this change caused a hard lockup when
  using USB if threadirqs are enabled.  Thomas pointed out that this patch
  is incorrect, and can cause problems.  So revert it to get the
  previously working functionality back.
 
 This reminds me -- it might be a good idea to start migrating the USB 
 host controller drivers to use threaded interrupts.  The difficulty is 
 that I'm not sure what would be required.

I had some rough go at it two years ago, but hell don't ask me where I
buried those patches.

The main thing is that you need a real primary handler, which runs in
hard interrupt context, because the USB interrupts are often shared
and you don't want and cannot block out the other devices while you
are handling your device.

So the primary handler has to figure out (as it does now) whether the
interrupt originates from the USB device. If not it returns IRQ_NONE
as now. If yes, it has to disable the interrupt at the host device
level and return IRQ_WAKE_THREAD. IIRC one of the problems I ran into
was that not all host controllers preserve the interrupt cause when
you have read it out already, but my memory is really blurry.
 
Now the threaded handler does the device handling and at the end of
it, it reenables the interrupts at the device level again for the next
round.

So the only thing you have to worry about vs. interupts is protecting
the access to the irq related hardware registers and of course vs. the
timers. Btw, what's the reason for using hrtimers in USB? 

Thanks,

tglx

--
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 Revert USB/host: Cleanup unneccessary irq disable code added to usb tree

[Thomas Gleixner]

On Thu, 15 Nov 2012, Alan Stern wrote:
 On Thu, 15 Nov 2012, Thomas Gleixner wrote:
  The main thing is that you need a real primary handler, which runs in
  hard interrupt context, because the USB interrupts are often shared
  and you don't want and cannot block out the other devices while you
  are handling your device.
  
  So the primary handler has to figure out (as it does now) whether the
  interrupt originates from the USB device. If not it returns IRQ_NONE
  as now. If yes, it has to disable the interrupt at the host device
  level and return IRQ_WAKE_THREAD. IIRC one of the problems I ran into
  was that not all host controllers preserve the interrupt cause when
  you have read it out already, but my memory is really blurry.
 
 Right, I follow that.  It's easy to save a copy of the interrupt cause.
 
  Now the threaded handler does the device handling and at the end of
  it, it reenables the interrupts at the device level again for the next
  round.
 
 Does this depend on how much work the primary handler needs to do in
 order to turn off the interrupt source?  For example, if the interrupt
 source can be turned off simply by clearing a bit in a status register,
 couldn't the primary handler do that and then leave device interrupts
 enabled?  If multiple interrupts arrived before the threaded handler
 started running, they could all be OR'ed together in the saved
 interrupt-cause value.

Yeah, that's fine.
 
 I guess this amounts to the same thing as asking what happens if a
 device interrupt occurs while the threaded handler is running.  Would
 the kernel then schedule the threaded handler to run a second time?  
 Or is it not prepared to handle this sort of thing?

Right now, it reschedules it, but I have some idea how to deal with
that.
 
  So the only thing you have to worry about vs. interupts is protecting
  the access to the irq related hardware registers and of course vs. the
  timers. Btw, what's the reason for using hrtimers in USB? 
 
 The ehci-hcd driver often needs delays ranging between 1 and 10 ms.  
 hrtimers seemed to be the best way of getting them.

Are these delays related to the interrupt flow or do they come from a
separate control entity?
 
 To prevent the timer handler from interfering with the threaded 
 handler, should the threaded handler use spin_{un}lock_bh()?

_irqsave() unfortunately as the hrtimer callbacks run in hard
interrupt context.

Thanks,

tglx
--
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 Revert USB/host: Cleanup unneccessary irq disable code added to usb tree

[Thomas Gleixner]

On Thu, 15 Nov 2012, Alan Stern wrote:

 On Thu, 15 Nov 2012, Thomas Gleixner wrote:
 
   I guess this amounts to the same thing as asking what happens if a
   device interrupt occurs while the threaded handler is running.  Would
   the kernel then schedule the threaded handler to run a second time?  
   Or is it not prepared to handle this sort of thing?
  
  Right now, it reschedules it, but I have some idea how to deal with
  that.
 
 Isn't rescheduling the right thing to do?  Presumably the threaded 
 handler is only aware of the hardware events that occurred before it 
 started running.  If new events occur while the handler is running, it 
 should be rescheduled so that it can take care of them.

Yes, that's what the code does now. Though there were people asking in
some other context to have an interface which lets them poll from
their interrupt handler thread code whether there is a new event
pending and clear that to avoid a full round trip.
 
So the only thing you have to worry about vs. interupts is protecting
the access to the irq related hardware registers and of course vs. the
timers. Btw, what's the reason for using hrtimers in USB? 
   
   The ehci-hcd driver often needs delays ranging between 1 and 10 ms.  
   hrtimers seemed to be the best way of getting them.
  
  Are these delays related to the interrupt flow or do they come from a
  separate control entity?
 
 Most of them come from a separate control entity.  A couple are related
 to the interrupt flow.  (For example, sometimes an interrupt occurs and
 the interrupt handler needs to schedule a timer for 2 ms in the
 future.)
 
   To prevent the timer handler from interfering with the threaded 
   handler, should the threaded handler use spin_{un}lock_bh()?
  
  _irqsave() unfortunately as the hrtimer callbacks run in hard
  interrupt context.
 
 Hmmm.  Doesn't that mean there would be no benefit from using threaded 
 interrupt handlers?

Not versus the timers. Threaded interrupt handlers are handy, if you
want to be able to schedule or run complex code w/o hogging the
(soft)interrupt system for a long time. It also allows you to move stuff
which you hand off to tasklets, workqueues or other deferred entities
into your handler which reduced locking complexities and other problems.

But again, that really depends on what your driver/interrupt handler
has to do and whether avoiding the handoff of work makes sense or not.

Thanks,

tglx


--
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 Revert USB/host: Cleanup unneccessary irq disable code added to usb tree

[Thomas Gleixner]

On Thu, 15 Nov 2012, Thomas Gleixner wrote:
 On Thu, 15 Nov 2012, Alan Stern wrote:
To prevent the timer handler from interfering with the threaded 
handler, should the threaded handler use spin_{un}lock_bh()?
   
   _irqsave() unfortunately as the hrtimer callbacks run in hard
   interrupt context.
  
  Hmmm.  Doesn't that mean there would be no benefit from using threaded 
  interrupt handlers?
 
 Not versus the timers. Threaded interrupt handlers are handy, if you
 want to be able to schedule or run complex code w/o hogging the
 (soft)interrupt system for a long time. It also allows you to move stuff
 which you hand off to tasklets, workqueues or other deferred entities
 into your handler which reduced locking complexities and other problems.
 
 But again, that really depends on what your driver/interrupt handler
 has to do and whether avoiding the handoff of work makes sense or not.

Bah. Hit send too fast. :)

That said, it might even make sense versus timers, if you have to
delay operations on the device for time X. You can even just sleep in
your interrupt thread and get woken, when either the timer expires or
another interrupt comes in.

Though I'm not familiar enough with the USB code to tell whether any
of this helps or not. Its definitely pointless just to enforce the
threaded handler and not make use of its extended abilities.

Thanks,

tglx

--
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 Revert USB/host: Cleanup unneccessary irq disable code added to usb tree

[Thomas Gleixner]

On Thu, 15 Nov 2012, Alan Stern wrote:
 On Thu, 15 Nov 2012, Thomas Gleixner wrote:
 
Hmmm.  Doesn't that mean there would be no benefit from using threaded 
interrupt handlers?
   
   Not versus the timers. Threaded interrupt handlers are handy, if you
   want to be able to schedule or run complex code w/o hogging the
   (soft)interrupt system for a long time. It also allows you to move stuff
   which you hand off to tasklets, workqueues or other deferred entities
   into your handler which reduced locking complexities and other problems.
   
   But again, that really depends on what your driver/interrupt handler
   has to do and whether avoiding the handoff of work makes sense or not.
  
  Bah. Hit send too fast. :)
  
  That said, it might even make sense versus timers, if you have to
  delay operations on the device for time X. You can even just sleep in
  your interrupt thread and get woken, when either the timer expires or
  another interrupt comes in.
 
 How does that work?  Does the thread simply run in a big loop, spending
 most of its time in an interruptible sleep, and calling the threaded
 handler routine whenever it gets woken up?  And it gets woken up
 whenever the primary handler returns IRQ_WAKE_THREAD?

The way I've seen people using it is:

threaded_handler()
 again:
  handle_stuff();
case A:
   /* Wait for timer or new interrupt */ 
   if (hrtimer_sleep(...))
  goto again;
   handle_timeout();
   break;
case B:
  
 return; 
 
 That would mean the timer routine could simply wake up the handler
 thread, and the rest of the work could be done in the thread's context.  
 All the thread would need to do is check, each time it started up,
 whether the timer had expired.  No locking would be necessary (vs the 
 timer, at least).

Correct.
 
  Though I'm not familiar enough with the USB code to tell whether any
  of this helps or not. Its definitely pointless just to enforce the
  threaded handler and not make use of its extended abilities.
 
 I see.  The USB code does do a reasonably large amount of work in
 interrupt context.  It doesn't have to be that way; it could be in
 process context -- but the design of the USB subsystem requires that
 much of the work be carried out with interrupts disabled.  (The API
 states that all the driver callbacks occur with disabled interrupts.)  

Well, the reason why the API says that is because you are doing lots
of work in interrupt context and you probably not only disable
interrupts for that, you also hold the proper locks to protect against
the eventually incoming interrupt on the other cpu.

So sure, this needs some thought whether it's worth the trouble, but
last I looked a few years ago (when I just had a stab on it), it
definitely looked worthwhile to simplify a lot of things by simply
serializing them naturaly. Don't misunderstand me. You still will need
locks :)

Thanks,

tglx
--
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