Re: dvb usb issues since kernel 4.9

[Peter Zijlstra]

On Mon, Jan 08, 2018 at 10:31:09PM +0100, Jesper Dangaard Brouer wrote:
> I did expected the issue to get worse, when you load the Pi with
> network traffic, as now the softirq time-budget have to be shared
> between networking and USB/DVB. Thus, I guess you are running TCP and
> USB/mpeg2ts on the same CPU (why when you have 4 CPUs?...)

Isn't networking also over USB on the Pi ?
--
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 v8 2/5] usb: early: add driver for xhci debug capability

[Peter Zijlstra]

On Thu, Jun 01, 2017 at 10:15:24AM +0200, Vlastimil Babka wrote:
> Thanks. I didn't make it clear that the trace_printk() warning is there
> even if the code using it doesn't actually execute (i.e. I didn't
> specify any early_printk bootparam). There are some roastedy tricks to
> detect the potential users, so that the buffers can be allocated in
> advance to allow the first trace_printk() from any context, I guess.
> 
> I'm not sure if there's a way to change it so that your driver reports
> the trace_printk usage only in response to the bootparam (which could
> also be a safe point to allocate ftrace buffers?).

No, nor do we want to. There should not be a single caller to
trace_printk() in normal kernels.

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

[Peter Zijlstra]

On Thu, Jan 26, 2017 at 05:01:05PM +0100, Ingo Molnar wrote:
> > > 
> > > I.e. if there's any polling component then it would be reasonable to add 
> > > an error 
> > > component: poll the status and if it goes 'disconnected' then disable 
> > > early-printk 
> > > altogether in this case and trigger an emergency printk() so that there's 
> > > chance 
> > > that the user notices [if the system does not misbehave otherwise].
> > 
> > That'll be fun when printk() == early_printk() :-)
> 
> My suggestion would be to just print into the printk buffer directly in this 
> case, 
> without console output - the developer will notice it in 'dmesg'.

When you map printk() onto early_printk() dmesg will be empty, there
will be nothing there, and therefore no reason what so ever to look
there. I certainly don't ever look there.

Note that the printk buffer itself is a major part of why printk sucks
donkey balls.  Not to mention that you really cannot have an
early_printk() implementation that depends on printk().

> > I myself wouldn't mind the system getting stuck until the link is
> > re-established. My own damn fault for taking that cable out etc.
> 
> That's fine too, although beyond the obvious "yanked the cable without 
> realizing 
> it" case there are corner cases where usability is increased massively if the 
> kernel is more proactive about error conditions: for example there are 
> sub-standard USB cables and there are too long USB pathways from overloaded 
> USB 
> hubs which can result in intermittent behavior, etc.
> 
> A clear diagnostic message in 'dmesg' that the USB host controller is unhappy 
> about the USB-debug dongle device is a _lot_ more useful when troubleshooting 
> such 
> problems than the occasional weird, non-deterministic hang...

Sure, I'm just not sure what or where makes sense.

If your serial cable is bad you notice because you don't receive the
right amount of characters and or stuff gets mangled. You chuck the
cable and get a new one.

I think the most important part is re-establishing the link when the
cable gets re-inserted. Maybe we should just drop all characters written
when there's no link and leave it at that, same as serial.

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

[Peter Zijlstra]

On Thu, Jan 26, 2017 at 08:19:37AM +0100, Ingo Molnar wrote:
> 
> * Lu Baolu  wrote:
> 
> > Fair enough.
> > 
> > USB connection is stable enough, unless the user unplugs the
> > USB cable during debugging.
> 
> What does the hardware do in this case? The XHCI registers are in the host 
> hardware, so they won't disappear, right? Is there some cable connection 
> status 
> bit we can extract without interrupts?
> 
> I.e. if there's any polling component then it would be reasonable to add an 
> error 
> component: poll the status and if it goes 'disconnected' then disable 
> early-printk 
> altogether in this case and trigger an emergency printk() so that there's 
> chance 
> that the user notices [if the system does not misbehave otherwise].

That'll be fun when printk() == early_printk() :-)

I myself wouldn't mind the system getting stuck until the link is
re-established. My own damn fault for taking that cable out etc.
--
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 v5 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Wed, Jan 25, 2017 at 11:51:34PM +0800, Lu Baolu wrote:

> > What is timeout and why?
> 
> Put it in simple:
> 
> The driver sets the RUN bit in control register and polls READY
> bit in status register for the successful USB device enumeration.
> As the USB device enumeration might fail and the READY bit will
> never be set, the driver must have a timeout logic to avoid
> endless loop.
> 
> More details:
> 
> The operational model is that driver sets up all necessary registers
> and data structures, and then starts the debug engine by setting
> the RUN/STOP bit in the control register.
> 
> The debug engine then brings up itself as a ready-for-enumeration
> USB device. The USB link between host and device starts link training
> and then host will detect the connected device. The hub driver in
> host will then starts the USB device enumeration processes (as defined
> in USB spec). If everything goes smoothly, the device gets enumerated
> and host can talk with the debug device.
> 
> After that, xdbc firmware will set the READY bit in status register. And
> the driver can go ahead with data transfer over USB.

I have vague memories from a prior discussion where you said this READY
state can be lost at any time (cable unplug or whatnot) and at that
point the driver should re-start the setup, right?

> >  If there is an error other than !ready, I would
> > expect the hardware to inform you of this through another status bit,
> > no?
> 
> Yeah, this might be another choice of hardware design. But it's not a
> topic for this driver.

So is there really no way to way to distinguish between "I did setup and
am waiting for READY", "I did setup, am waiting for READY, but things
got hosed" and "I was READY, things be hosed" ?

I suppose the first and last can be distinguished by remembering if you
ever saw READY, but the first and second are the interesting case I
think.

> > So why can't you poll indefinitely for either ready or error?
> >
> 
> Even if the hardware has both ready and error status bits, it's still
> nice to have a time out watch dog. Buggy hardware or firmware
> might not set any of these bits. Polling indefinitely might result in
> a endless loop.

Loosing output, esp. without indication, is very _very_ annoying when
you're debugging things. Its just about on par with a stuck system, at
least then you know something bad happened.

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

[Peter Zijlstra]

On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote:
> In my driver, udelay() is mostly used to handle time out.
> 
> Xdbc hides most USB things in its firmware. Early printk driver only needs
> to setup the registers/data structures and wait until link ready or time out.
> Without udelay(), I have no means to convert the polling times into waiting
> time.

What is timeout and why? If there is an error other than !ready, I would
expect the hardware to inform you of this through another status bit,
no?

So why can't you poll indefinitely for either ready or error?

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

[Peter Zijlstra]

On Wed, Jan 25, 2017 at 10:23:55AM +0100, Ingo Molnar wrote:
> 
> * Lu Baolu  wrote:
> 
> > > Hiding essentially an early udelay() implementation in an early-printk 
> > > driver is 
> > > ugly and counterproductive.

> Yeah - so could we do this in a more generic fashion, not in the early-printk 
> driver but in core x86 code?

So ideally early_printk() would not depend on udelay() being setup.

In fact, ideally early_printk() wouldn't even use udelay -- this very
much includes its own copy.

Why is udelay() required? Can't the thing simply poll its own register
state to wait for completion?

This all sounds like xdbc cruft is still unreliably garbage..
--
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

[Peter Zijlstra]

On Fri, Nov 11, 2016 at 12:33:29PM +0800, Lu Baolu wrote:

> Things become complicated when it comes to USB debug port.
> But it's still addressable.
> 
> At this time, we can do it like this.
> 
> write()
> {
>   if (in_nmi() && raw_spin_is_locked())
>   return;
> 
>   raw_spinlock_irqsave(, flags);
>   
> 

Please use raw_spin_trlock_irqsave() instead, spin_is_locked() is fairly
icky.

Also, there's a bunch of exception contexts that do not set in_nmi().
That is in_nmi() is really only set for #NM. #MC and #DB and
others do not set this.

> This will filter some messages from NMI handler in case that
> another thread is holding the spinlock. I have no idea about
> how much chance could a debug user faces this. But it might
> further be fixed with below enhancement.
> 
> write()
> {
>   if (in_nmi() && raw_spin_is_locked()) {
>   produce_a_pending_item_in_ring();
>   return;
>   }
> 
>   raw_spinlock_irqsave(, flags);
> 
>   while (!pending_item_ring_is_empty)
>   consume_a_pending_item_in_ring();
> 
>   
> 
> 
> We can design the pending item ring in a producer-consumer
> model. It's easy to avoid race between the producer and
> consumer.

Problem is that the consumer might never happen, those are the fun most
bugs.

Not being able to deal with random nested exception context really
reduces the utility of this thing.

Again, a UART rules. Make a virtual UART in hardware, that'd be totally
awesome. This thing, I'm not convinced its worth having.
--
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

[Peter Zijlstra]

On Thu, Nov 10, 2016 at 09:56:41AM +0100, Thomas Gleixner wrote:
> On Thu, 10 Nov 2016, Lu Baolu wrote:

> > 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);
>}

Right, which is why actual UARTs rule. If only laptops still had pinouts
for them life would be so much better.

Ideally the USB debug port would be a virtual UART and its interface as
simple and robust.

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

I would also suggest telling the hardware people they have designed
something near the brink of useless. If you cannot do random exception
context debugging (#DB, #NMI, #MCE etc..) then there's a whole host of
problems that simply cannot be debugged.

Also note that kdb runs from NMI context, so you'll not be able to
support that either.
--
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: [RESEND PATCH v2 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Thu, Oct 20, 2016 at 10:41:32AM +0200, Peter Zijlstra wrote:
> I'm already only using early_printk() because regular printk() is an
> unfixable piece of crap, and now you're making early_printk() useless
> too.

Note that the existing USB debug port stuff doesn't seem to have a
single lock in. Its just polling on readl and doing writel without any
serialization what so ever.

Not that I've ever actually used it (lack of the magic cable), but it at
least looks like it should mostly work.
--
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: [RESEND PATCH v2 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Thu, Oct 20, 2016 at 04:08:17PM +0800, Lu Baolu wrote:
> Hi Peter,
> 
> Thanks for your comments.
> 
> On 10/19/2016 09:09 PM, Peter Zijlstra wrote:
> > On Wed, Oct 19, 2016 at 08:18:22AM +0800, Lu Baolu wrote:
> >> +++ b/drivers/usb/early/xhci-dbc.c
> >> +static int xdbc_bulk_write(const char *bytes, int size)
> >> +{
> >> +  unsigned long flags;
> >> +  int ret, timeout = 0;
> >> +
> >> +  spin_lock_irqsave(, flags);
> > Yikes!!
> >
> > So how is this supposed to work from NMI context and the like?
> >
> > (also, at the very least, that should be a raw_spinlock_t)
> 
> Totally agree. We should put it as a raw_spinlock_t().

Well, raw_spinlock_t still doesn't allow for NMI context operation. So
ideally you'd manage without any locks at all.

> > What do you need the spinlock for? Afaict this is a 'simple' polling
> > event handling loop on MMIO, right?
> 
> Not only for polling registers in MMIO, but also for handling the
> events in the event ring. The event ring is a memory block,
> which is allocated during hardware initialization and saved
> in a register in MMIO.
> 
> There is a single event ring for all events (read completion,
> write completion, port status change and transfer errors etc).
> The debugging hardware doesn't support interrupt, so software
> has to poll the event ring whenever it needs to.
> 
> Event ring polling happens at least in write interface (to make
> sure the previous transfer has been completed), and a worker
> (to check the read events and other things). That's the reason
> why I need a spin_lock here.

I'm not sure I understand. Sure you need someone polling, and you need
only a single CPU polling at the same time.

But the serialization I pointed to provides you that.

Sure, it get a tad tricky to allow a nested context to take over
processing in the middle of things, but that just means you should use
some cmpxchg and stay away from stack based variables.

> >> +static void xdbc_scrub_function(struct work_struct *work)
> >> +{
> >> +  unsigned long flags;
> >> +
> >> +  spin_lock_irqsave(, flags);
> >> +
> >> +  /*
> >> +   * DbC is running, check the event ring and
> >> +   * handle the events.
> >> +   */
> >> +  if (readl(_reg->control) & CTRL_DRC)
> >> +  xdbc_handle_events();
> >> +
> >> +  /*
> >> +   * External reset happened. Need to restart the
> >> +   * debugging hardware.
> >> +   */
> >> +  if (unlikely(!(readl(_reg->control) & CTRL_DCE)))
> >> +  xdbc_handle_external_reset();
> >> +
> >> +  spin_unlock_irqrestore(, flags);
> >> +
> >> +  queue_delayed_work(xdbc_wq, , usecs_to_jiffies(100));
> >> +}
> > Excuse my total lack of USB knowledge, but WTH does this do and what do
> > we need it for?
> >
> 
> As I said above, I need a worker to check the read completion
> events and other hardware situations.
> 
> One hardware situation that needs to check regularly is that
> it might be aborted by the host controller itself. The xhci spec
> allows the debug hardware to share some logics with the host
> controller (to reduce cost?). As the result, when host controller
> driver resets the host (always happens in driver probe or
> resume) the debug hardware resets as well. Software needs
> to re-initialize and bring it back.
> 
> Early printk doesn't need to read anything from debug host.
> But if we use it for kernel debugging with kgdb (it's in my work
> queue),  we need a read interface. We need to check the event
> ring regularly for read completion events.

Urgh, but this is very non-robust. Who says the workqueue stuff still
works?

So now you're having your early_printk driver depend on the scheduler
still working and the workqueue stuff and..

As it stands, that renders the entire thing completely useless for
debugging the scheduler, workqueues and anything NMI. IOW, its
completely useless full stop.

I'm already only using early_printk() because regular printk() is an
unfixable piece of crap, and now you're making early_printk() useless
too.
--
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: [RESEND PATCH v2 1/4] usb: dbc: early driver for xhci debug capability

[Peter Zijlstra]

On Wed, Oct 19, 2016 at 08:18:22AM +0800, Lu Baolu wrote:
> +++ b/drivers/usb/early/xhci-dbc.c

> +static int xdbc_bulk_write(const char *bytes, int size)
> +{
> + unsigned long flags;
> + int ret, timeout = 0;
> +
> + spin_lock_irqsave(, flags);

Yikes!!

So how is this supposed to work from NMI context and the like?

(also, at the very least, that should be a raw_spinlock_t)

What do you need the spinlock for? Afaict this is a 'simple' polling
event handling loop on MMIO, right?

All we really need to guarantee is that there's only a single CPU trying
to do that at any one time.

Wouldn't something like:

  https://marc.info/?l=linux-kernel=147681099108509=2

already take care of that? Then you can drop the lock and things will
work 'nested'.

> +
> + xdbc_handle_events();
> +
> + /* Check completion of the previous request. */
> + while (xdbc.flags & XDBC_FLAGS_OUT_PROCESS) {
> + if (timeout > 100)
> + break;
> +
> + spin_unlock_irqrestore(, flags);
> + xdbc_delay(100);
> + spin_lock_irqsave(, flags);
> + timeout += 100;
> +
> + xdbc_handle_events();
> + }
> +
> + if (xdbc.flags & XDBC_FLAGS_OUT_PROCESS) {
> + spin_unlock_irqrestore(, flags);
> +
> + /*
> +  * Oops, hardware wasn't able to complete the
> +  * previous transfer.
> +  */
> + xdbc_trace("oops: previous transfer not completed yet\n");
> +
> + return -ETIMEDOUT;
> + }
> +
> + ret = xdbc_bulk_transfer((void *)bytes, size, false);
> +
> + spin_unlock_irqrestore(, flags);
> +
> + return ret;
> +}
> +
> +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);
> + while (n > 0) {
> + for (chunk = 0; chunk < XDBC_MAX_PACKET && n > 0;
> + str++, chunk++, n--) {
> + if (!use_cr && *str == '\n') {
> + use_cr = 1;
> + buf[chunk] = '\r';
> + str--;
> + n++;
> + continue;
> + }
> + if (use_cr)
> + use_cr = 0;
> + buf[chunk] = *str;
> + }
> + if (chunk > 0) {
> + ret = xdbc_bulk_write(buf, chunk);
> + if (ret < 0)
> + break;
> + }
> + }
> +}
> +
> +static struct console early_xdbc_console = {
> + .name = "earlyxdbc",
> + .write =early_xdbc_write,
> + .flags =CON_PRINTBUFFER,
> + .index =-1,
> +};
> +
> +void __init early_xdbc_register_console(void)
> +{
> + if (early_console)
> + return;
> +
> + early_console = _xdbc_console;
> + if (early_console_keep)
> + early_console->flags &= ~CON_BOOT;
> + else
> + early_console->flags |= CON_BOOT;
> + register_console(early_console);
> +}
> +
> +static void xdbc_scrub_function(struct work_struct *work)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(, flags);
> +
> + /*
> +  * DbC is running, check the event ring and
> +  * handle the events.
> +  */
> + if (readl(_reg->control) & CTRL_DRC)
> + xdbc_handle_events();
> +
> + /*
> +  * External reset happened. Need to restart the
> +  * debugging hardware.
> +  */
> + if (unlikely(!(readl(_reg->control) & CTRL_DCE)))
> + xdbc_handle_external_reset();
> +
> + spin_unlock_irqrestore(, flags);
> +
> + queue_delayed_work(xdbc_wq, , usecs_to_jiffies(100));
> +}

Excuse my total lack of USB knowledge, but WTH does this do and what do
we need it for?
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Tue, Sep 06, 2016 at 10:46:55AM -0400, Alan Stern wrote:

Not knowing where INFO() goes, you should use trace_printk() not
printk(), as the former is strictly per cpu, while the latter is
globally serialized and can hide all these problems.

> Index: usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
> ===
> --- usb-4.x.orig/drivers/usb/gadget/function/f_mass_storage.c
> +++ usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
> @@ -485,6 +485,8 @@ static void bulk_out_complete(struct usb
>   spin_lock(>lock);
>   bh->outreq_busy = 0;
>   bh->state = BUF_STATE_FULL;
> + if (bh->bulk_out_intended_length == US_BULK_CB_WRAP_LEN)
> + INFO(common, "compl: bh %p state %d\n", bh, bh->state);
>   wakeup_thread(common);
>   spin_unlock(>lock);
>  }
> @@ -2207,6 +2209,7 @@ static int get_next_command(struct fsg_c
>   rc = sleep_thread(common, true);
>   if (rc)
>   return rc;
> + INFO(common, "next: bh %p state %d\n", bh, bh->state);
>   }
>   smp_rmb();
>   rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO;
> 
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Tue, Sep 06, 2016 at 01:49:37PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 06, 2016 at 02:43:39PM +0300, Felipe Balbi wrote:

> > My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
> > adds extra overhead which makes the problem much, much less likely to
> > happen. Does that sound plausible to you?
> 
> I did consider that, but I've not sufficiently grokked the code to rule
> out actual fail. So let me stare at this a bit more.

OK, so I'm really not seeing it, we've got:

while (bh->state != FULL) {
for (;;) {
set_current_state(INTERRUPTIBLE); /* MB after */
if (signal_pending(current))
return -EINTR;
if (common->thread_wakeup_needed)
break;
schedule(); /* MB */
}
__set_current_state(RUNNING);
common->thread_wakeup_needed = 0;
smp_rmb(); /* NOP */
}


VS.


spin_lock(>lock); /* MB */
bh->state = FULL;
smp_wmb(); /* NOP */
common->thread_wakeup_needed = 1;
wake_up_process(common->thread_task); /* MB before */
spin_unlock(>lock);



(the MB annotations specific to x86, not true in general)


If we observe thread_wakeup_needed, we must also observe bh->state.

And the sleep/wakeup ordering is also correct, we either see
thread_wakeup_needed and continue, or we see task->state == RUNNING
(from the wakeup) and NO-OP schedule(). The MB from set_current_statE()
then matches with the MB from wake_up_process() to ensure we must see
thead_wakeup_needed.

Or, we go sleep, and get woken up, at which point the same happens.
Since the waking CPU gets the task back on its RQ the happens-before
chain includes the waking CPUs state along with the state of the task
itself before it went to sleep.

At which point we're back where we started, once we see
thread_wakeup_needed we must then also see bh->state (and all state
prior to that on the waking CPU).



There's enough cruft in the while-sleep loop to force reload bh->state.

Load/store tearing cannot be a problem because all values are single
bytes (the variables are multi bytes, but all values used only affect
the LSB).

Colour me puzzled.
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Tue, Sep 06, 2016 at 02:43:39PM +0300, Felipe Balbi wrote:

> > Could you confirm that bulk_{in,out}_complete() work on different
> > usb_request structures, and they can not, at any time, get called on the
> > _same_ request?
> 
> usb_requests are allocated for a specific endpoint and USB Device
> Controller (UDC) drivers refuse to queue requests allocated for epX to
> epY, so this really can never happen.

Good, thanks!

> My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
> adds extra overhead which makes the problem much, much less likely to
> happen. Does that sound plausible to you?

I did consider that, but I've not sufficiently grokked the code to rule
out actual fail. So let me stare at this a bit more.
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Mon, Sep 05, 2016 at 11:29:26AM -0400, Alan Stern wrote:
> On Mon, 5 Sep 2016, Peter Zijlstra wrote:
> 
> > > Actually, seeing it written out like this, one realizes that it really 
> > > ought to be:
> > > 
> > >   CPU 0   CPU 1
> > > - -
> > > Start DMA Handle DMA-complete irq
> > > Sleep until bh->state smp_mb()
> > >   set bh->state
> > >   Wake up CPU 0
> > >   smp_mb()
> > >   Compute rc based on contents of the DMA buffer
> > > 
> > > (Bear in mind also that on some platforms, the I/O operation is carried 
> > > out by PIO rather than DMA.)

> > Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC.
> > Its only defined to do CPU/CPU interactions.
> 
> Suppose the DMA master finishes filling up an input buffer and issues a
> completion irq to CPU1.  Presumably the data would then be visible to
> CPU1 if the interrupt handler looked at it.  So if CPU1 executes smp_mb
> before setting bh->state and waking up CPU0, and if CPU0 executes
> smp_mb after testing bh->state and before reading the data buffer,
> wouldn't CPU0 then see the correct data in the buffer?  Even if CPU0 
> never did go to sleep?

Couple of notes here; I would expect the DMA master to make its stores
_globally_ visible on 'completion'. Because I'm not sure our smp_mb()
would help make it globally visible, since its only defined on CPU/CPU
interactions, not external.

Note that for example ARM has the distinction where smp_mb() uses
DMB-ISH barrier, dma_[rw]mb() uses DMB-OSH{LD,ST} and mb() uses DSB-SY.

The ISH domain is the Inner-SHarable (IIRC) and only includes CPUs. The
OSH is Outer-SHarable and adds external agents like DMA (also includes
CPUs). The DSB-SY thing is even heavier and syncs world or something; I
always forget these details.

> Or would something more be needed?

The thing is, this is x86 (TSO). Most everything is globally
ordered/visible and full barriers.

The only reorder allowed by TSO is a later read can happen before a
prior store. That is, only:

X = 1;
smp_mb();
r = Y;

actually needs a full barrier. All the other variants are no-ops.

Note that x86's dma_[rw]mb() are no-ops (like the regular smp_[rw]mb()).
Which seems to imply DMA is coherent and globally visible.

> > I would very much expect the device IO stuff to order things for us in
> > this case. "Start DMA" should very much include sufficient fences to
> > ensure the data under DMA is visible to the DMA engine, this would very
> > much include things like flushing store buffers and maybe even writeback
> > caches, depending on platform needs.
> > 
> > At the same time, I would expect "Handle DMA-complete irq", even if it
> > were done with a PIO polling loop, to guarantee ordering against later
> > operations such that 'complete' really means that.
> 
> That's what I would expect too.
> 
> Back in the original email thread where the problem was first reported, 
> Felipe says that the problem appears to be something else.  Here's what 
> it looks like now, in schematic form:
> 
>   CPU0
>   
>   get_next_command():
> while (bh->state != BUF_STATE_EMPTY)
>   sleep_thread();
> start an input request for bh
> while (bh->state != BUF_STATE_FULL)
>   sleep_thread();
> 
> As mentioned above, the input involves DMA and is terminated by an irq.
> The request's completion handler is bulk_out_complete():
> 
>   CPU1
>   
>   bulk_out_complete():
> bh->state = BUF_STATE_FULL;
> wakeup_thread();
> 
> According to Felipe, when CPU0 wakes up and checks bh->state, it sees a 
> value different from BUF_STATE_FULL.  So it goes back to sleep again 
> and doesn't make any forward progress.
> 
> It's possible that something else is changing bh->state when it 
> shouldn't.  But if this were the explanation, why would Felipe see that 
> the problem goes away when he changes the memory barriers in 
> sleep_thread() and wakeup_thread() to smp_mb()?

Good question, let me stare at the code more.

Could you confirm that bulk_{in,out}_complete() work on different
usb_request structures, and they can not, at any time, get called on the
_same_ request?


--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Mon, Sep 05, 2016 at 10:43:11AM +0100, Will Deacon wrote:
> On Sat, Sep 03, 2016 at 12:16:29AM +0200, Peter Zijlstra wrote:

> > Forgot to Cc Will. Will, does ARM64 need to make smp_mb__before_spinlock
> > smp_mb() too?
> 
> Yes, probably. Just to confirm, the test is something like:
> 
> 
> CPU0
> 
> 
> Wx=1
> smp_mb__before_spinlock()
> LOCK(y)
> Rz=0
> 
> CPU1
> 
> 
> Wz=1
> smp_mb()
> Rx=0
> 
> 
> and that should be forbidden?

Indeed.
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Sat, Sep 03, 2016 at 10:49:39AM -0400, Alan Stern wrote:
> On Sat, 3 Sep 2016, Alan Stern wrote:
> 
> > In other words, we have:
> > 
> > CPU 0   CPU 1
> > -   -
> > Start DMA   Handle DMA-complete irq
> > Sleep until bh->state   Set bh->state
> > smp_wmb()
> > Wake up CPU 0
> > smp_rmb()
> > Compute rc based on contents
> > of the DMA buffer
> > 
> > This was written many years ago, at a time when I did not fully
> > understand all the details of memory ordering.  Do you agree that both
> > of those barriers should really be smp_mb()?  That's what Felipe has
> > been testing.
> 
> Actually, seeing it written out like this, one realizes that it really 
> ought to be:
> 
>   CPU 0   CPU 1
> - -
> Start DMA Handle DMA-complete irq
> Sleep until bh->state smp_mb()
>   set bh->state
>   Wake up CPU 0
>   smp_mb()
>   Compute rc based on contents of the DMA buffer
> 
> (Bear in mind also that on some platforms, the I/O operation is carried 
> out by PIO rather than DMA.)

I'm sorry, but I still don't follow. This could be because I seldom
interact with DMA agents and therefore am not familiar with that stuff.

Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC.
Its only defined to do CPU/CPU interactions.

I would very much expect the device IO stuff to order things for us in
this case. "Start DMA" should very much include sufficient fences to
ensure the data under DMA is visible to the DMA engine, this would very
much include things like flushing store buffers and maybe even writeback
caches, depending on platform needs.

At the same time, I would expect "Handle DMA-complete irq", even if it
were done with a PIO polling loop, to guarantee ordering against later
operations such that 'complete' really means that.
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Sat, Sep 03, 2016 at 04:51:07PM +0300, Felipe Balbi wrote:
> > That said, I cannot spot an obvious fail,
> 
> okay, but a fail does exist. Any hints on what extra information I could
> capture to help figuring this one out?

More information on which sleep is not waking woudl help I suppose. That
greatly limits the amount of code one has to stare at.

Also, a better picture of all threads involved in the wakeup. Alan seems
to suggest there's multiple CPUs involved in the wakeup.
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Sat, Sep 03, 2016 at 10:16:31AM -0400, Alan Stern wrote:

> > Sorry, but that is horrible code. A barrier cannot ensure writes are
> > 'complete', at best they can ensure order between writes (or reads
> > etc..).
> 
> The code is better than the comment.  What I really meant was that the 
> write of bh->state needs to be visible to the thread after it wakes up 
> (or after it checks the wakeup condition and skips going to sleep).

Yeah, I got that.

> > Also, looking at that thing, that common->thread_wakeup_needed variable
> > is 100% redundant. All sleep_thread() invocations are inside a loop of
> > sorts and basically wait for other conditions to become true.
> > 
> > For example:
> > 
> > while (bh->state != BUF_STATE_EMPTY) {
> > rc = sleep_thread(common, false);
> > if (rc)
> > return rc;
> > }
> > 
> > All you care about there is bh->state, _not_
> > common->thread_wakeup_needed.
> 
> You know, I never went through and verified that _all_ the invocations 
> of sleep_thread() are like that. 

Well, thing is, they're all inside a loop which checks other conditions
for forward progress. Therefore the loop inside sleep_thread() is
pointless. Even if you were to return early, you'd simply loop in the
outer loop and go back to sleep again.

> In fact, I wrote the sleep/wakeup 
> routines _before_ the rest of the code, and I didn't know in advance 
> exactly how they were going to be called.

Still seems strange to me, why not use wait-queues for the first cut?

Only if you find a performance issue with wait-queues, which cannot be
fixed in the wait-queue proper, then do you do custom thingies.

Starting with a custom sleeper, just doesn't make sense to me.

> > That said, I cannot spot an obvious fail, but the code can certainly use
> > help.
> 
> The problem may be that when the thread wakes up (or skips going to 
> sleep), it needs to see more than just bh->state.  Those other values 
> it needs are not written by the same CPU that calls wakeup_thread(), 
> and so to ensure that they are visible that smp_wmb() really ought to 
> be smp_mb() (and correspondingly in the thread.  That's what Felipe has 
> been testing.

So you're saying something like:


CPU0CPU1CPU2

X = 1   sleep_thread()
wakeup_thread()
r = X

But how does CPU1 know to do the wakeup? That is, how are CPU0 and CPU1
coupled.


--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Sat, Sep 03, 2016 at 09:58:09AM +0300, Felipe Balbi wrote:

> > What arch are you seeing this on?
> 
> x86. Skylake to be exact.

So it _cannot_ be the thing Alan mentioned. By the simple fact that
spin_lock() is a full barrier on x86 (every LOCK prefixed instruction
is).

> The following change survived through the night:
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
> b/drivers/usb/gadget/function/f_mass_storage.c
> index 8f3659b65f53..d31581dd5ce5 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct 
> usb_ep *ep)
>  /* Caller must hold fsg->lock */
>  static void wakeup_thread(struct fsg_common *common)
>  {
> - smp_wmb();  /* ensure the write of bh->state is complete */
> + smp_mb();   /* ensure the write of bh->state is complete */
>   /* Tell the main thread that something has happened */
>   common->thread_wakeup_needed = 1;
>   if (common->thread_task)
> @@ -626,7 +626,7 @@ static int sleep_thread(struct fsg_common *common, bool 
> can_freeze)
>   }
>   __set_current_state(TASK_RUNNING);
>   common->thread_wakeup_needed = 0;
> - smp_rmb();  /* ensure the latest bh->state is visible */
> + smp_mb();   /* ensure the latest bh->state is visible */
>   return rc;
>  }

Sorry, but that is horrible code. A barrier cannot ensure writes are
'complete', at best they can ensure order between writes (or reads
etc..).

Also, looking at that thing, that common->thread_wakeup_needed variable
is 100% redundant. All sleep_thread() invocations are inside a loop of
sorts and basically wait for other conditions to become true.

For example:

while (bh->state != BUF_STATE_EMPTY) {
rc = sleep_thread(common, false);
if (rc)
return rc;
}

All you care about there is bh->state, _not_
common->thread_wakeup_needed.

That said, I cannot spot an obvious fail, but the code can certainly use
help.
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Fri, Sep 02, 2016 at 04:29:19PM -0400, Alan Stern wrote:
> I'm afraid so.  The code doesn't use wait_event(), in part because
> there's no wait_queue (since only one task is involved).

You can use wait_queue fine with just one task, and it would clean up
the code tremendously.

You can replace things like the earlier mentioned:

while (bh->state != BUF_STATE_EMPTY) {
rc = sleep_thread(common, false);
if (rc)
return rc;
}

with:

rc = wait_event_interruptible(>wq, bh->state == 
BUF_STATE_EMPTY);
if (rc)
return rc;

> But maybe there's another barrier which needs to be fixed.  Felipe, can
> you check to see if received_cbw() is getting called in
> get_next_command(), and if so, what value it returns?  Or is the
> preceding sleep_thread() the one that never wakes up?
> 
> It could be that the smp_wmb() in wakeup_thread() needs to be smp_mb().  
> The reason being that get_next_command() runs outside the protection of 
> the spinlock.

Being somewhat confused by the code, I fail to follow that argument.
wakeup_thread() is always called under that spinlock(), but since the
critical section is 2 stores, I fail to see how a smp_mb() can make any
difference over the smp_wmb() already there.
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> Felipe, your tests will show whether my guess was totally off-base.  
> For the new people, Felipe is tracking down a problem that involves
> exactly the code sequence listed at the top of the email, where we know
> that the wakeup routine runs but nevertheless the task sleeps.  At
> least, that's what it looks like at the moment.

What arch are you seeing this on?
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Sat, Sep 03, 2016 at 12:14:13AM +0200, Peter Zijlstra wrote:
> On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> > 
> > Actually, that's not entirely true (although presumably it works okay
> > for most architectures).
> 
> Yeah, all load-store archs (with exception of PowerPC and ARM64 and
> possibly MIPS) implement ACQUIRE with a general fence (after the ll/sc).
> 
> ( and MIPS doesn't use their fancy barriers in Linux )
> 
> PowerPC does the full fence for smp_mb__before_spinlock, which leaves
> ARM64, I'm not sure its correct, but I'm way too tired to think about
> that now.
> 
> The TSO archs imply full barriers with all atomic RmW ops and are
> therefore also good.
> 

Forgot to Cc Will. Will, does ARM64 need to make smp_mb__before_spinlock
smp_mb() too?
--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote:
> 
> Actually, that's not entirely true (although presumably it works okay
> for most architectures).

Yeah, all load-store archs (with exception of PowerPC and ARM64 and
possibly MIPS) implement ACQUIRE with a general fence (after the ll/sc).

( and MIPS doesn't use their fancy barriers in Linux )

PowerPC does the full fence for smp_mb__before_spinlock, which leaves
ARM64, I'm not sure its correct, but I'm way too tired to think about
that now.

The TSO archs imply full barriers with all atomic RmW ops and are
therefore also good.

--
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: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Fri, Sep 02, 2016 at 02:10:13PM -0400, Alan Stern wrote:
> Paul, Peter, and Ingo:
> 
> This must have come up before, but I don't know what was decided.
> 
> Isn't it often true that a memory barrier is needed before a call to 
> wake_up_process()?  A typical scenario might look like this:
> 
>   CPU 0
>   -
>   for (;;) {
>   set_current_state(TASK_INTERRUPTIBLE);
>   if (signal_pending(current))
>   break;
>   if (wakeup_flag)
>   break;
>   schedule();
>   }
>   __set_current_state(TASK_RUNNING);
>   wakeup_flag = 0;
> 
> 
>   CPU 1
>   -
>   wakeup_flag = 1;
>   wake_up_process(my_task);
> 
> The underlying pattern is:
> 
>   CPU 0   CPU 1
>   -   -
>   write current->statewrite wakeup_flag
>   smp_mb();
>   read wakeup_flagread my_task->state
> 
> where set_current_state() does the write to current->state and 
> automatically adds the smp_mb(), and wake_up_process() reads 
> my_task->state to see whether the task needs to be woken up.
> 
> The kerneldoc for wake_up_process() says that it has no implied memory
> barrier if it doesn't actually wake anything up.  And even when it
> does, the implied barrier is only smp_wmb, not smp_mb.
> 
> This is the so-called SB (Store Buffer) pattern, which is well known to
> require a full smp_mb on both sides.  Since wake_up_process() doesn't
> include smp_mb(), isn't it correct that the caller must add it
> explicitly?
> 
> In other words, shouldn't the code for CPU 1 really be:
> 
>   wakeup_flag = 1;
>   smp_mb();
>   wake_up_process(task);
> 

No, it doesn't need to do that. try_to_wake_up() does the right thing.

It does:

smp_mb__before_spinlock();
raw_spin_lock_irqsave(>pi_lock);

Now, smp_mb__before_spinlock() is a bit of an odd duck, if you look at
its comment it says:

/*
 * Despite its name it doesn't necessarily has to be a full barrier.
 * It should only guarantee that a STORE before the critical section
 * can not be reordered with LOADs and STOREs inside this section.
 * spin_lock() is the one-way barrier, this LOAD can not escape out
 * of the region. So the default implementation simply ensures that
 * a STORE can not move into the critical section, smp_wmb() should
 * serialize it with another STORE done by spin_lock().
 */
#ifndef smp_mb__before_spinlock
#define smp_mb__before_spinlock()   smp_wmb()
#endif


So per default it ends up being:

WMB
LOCK

Which is sufficient to order the prior store vs the later load as is
required. Note that a spinlock acquire _must_ imply a store (we need to
mark the lock as taken), therefore the prior store is ordered against
the lock store per the wmb, and since the lock must imply an ACQUIRE
that limits the load.


Now, PowerPC defines smp_mb__before_spinlock as smp_mb(), and this is
because PowerPC ACQUIRE is a bit of an exception, if you want more
details I'm sure I or Paul can dredge them up :-)
--
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] usbhid: Fix lockdep unannotated irqs-off warning

[Peter Zijlstra]

On Wed, Mar 02, 2016 at 11:35:35AM -0500, Steven Rostedt wrote:
> On Wed, 2 Mar 2016 17:24:12 +0100
> Peter Zijlstra <pet...@infradead.org> wrote:
> 
> > On Wed, Mar 02, 2016 at 04:53:36PM +0100, Sedat Dilek wrote:
> > > 8110f570 :
> > > 8110f570: 55  push   %rbp
> > > 8110f571: 48 89 e5mov%rsp,%rbp
> > > 8110f574: 41 57   push   %r15
> > > 8110f576: 41 56   push   %r14
> > > 8110f578: 53  push   %rbx
> > > 8110f579: 48 83 ec 28 sub$0x28,%rsp  
> > 
> > stack offset is 0x28 bytes [*]
> 
> Actually, isn't it really 0x40 bytes? The stack pushed 3 words (8 bytes
> each) before doing the subtract. 0x28 == 40 bytes, 3 * 8 = 24,
> 40 + 24 = 64 == 0x40.

> > > 8110f5fd: 4c 89 75 d0 mov%r14,-0x30(%rbp)
> > > 8110f601: ff 75 d0pushq  -0x30(%rbp)
> > > 8110f604: 9d  popfq
> > 
> > put r14 into -0x30(rbp) and pushes/pops that, see [*] this is 8 bytes
> > over stack ?!
> 
> But from rbp, the stack is 0x40 bytes. This may be fine.

Ah indeed.
--
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] usbhid: Fix lockdep unannotated irqs-off warning

[Peter Zijlstra]

On Wed, Mar 02, 2016 at 04:53:36PM +0100, Sedat Dilek wrote:
> 8110f570 :
> 8110f570: 55  push   %rbp
> 8110f571: 48 89 e5mov%rsp,%rbp
> 8110f574: 41 57   push   %r15
> 8110f576: 41 56   push   %r14
> 8110f578: 53  push   %rbx
> 8110f579: 48 83 ec 28 sub$0x28,%rsp

stack offset is 0x28 bytes [*]

> 8110f57d: 48 89 fbmov%rdi,%rbx
> 8110f580: e8 6b 6e 80 00  callq  819163f0 
> 8110f585: e8 66 6e 80 00  callq  819163f0 
> 8110f58a: e8 61 6e 80 00  callq  819163f0 
> 8110f58f: e8 5c 6e 80 00  callq  819163f0 

Your compiler is on drugs!

> 8110f594: 9c  pushfq 
> 8110f595: 8f 45 e0popq   -0x20(%rbp)

Saves flags in -0x20(%rbp)

> 8110f598: 4c 8b 7d e0 mov-0x20(%rbp),%r15

And in %r15

/me wonders what's wrong with: popf %r15

> 8110f59c: e8 4f 6e 80 00  callq  819163f0 
> 8110f5a1: e8 4a 6e 80 00  callq  819163f0 
> 8110f5a6: fa  cli
> 8110f5a7: e8 84 cb fc ff  callq  810dc130 
> 
> 8110f5ac: 4c 8d 73 50 lea0x50(%rbx),%r14
> 8110f5b0: 48 c7 04 24 b0 f5 10movq   
> $0x8110f5b0,(%rsp)
> 8110f5b7: 81 
> 8110f5b8: 31 f6   xor%esi,%esi
> 8110f5ba: 31 d2   xor%edx,%edx
> 8110f5bc: 31 c9   xor%ecx,%ecx
> 8110f5be: 41 b8 01 00 00 00   mov$0x1,%r8d
> 8110f5c4: 45 31 c9xor%r9d,%r9d
> 8110f5c7: 4c 89 f7mov%r14,%rdi
> 8110f5ca: e8 c1 e5 fc ff  callq  810ddb90 
> 
> 8110f5cf: be 01 00 00 00  mov$0x1,%esi
> 8110f5d4: 48 c7 c2 cf f5 10 81mov$0x8110f5cf,%rdx
> 8110f5db: 4c 89 f7mov%r14,%rdi
> 8110f5de: e8 8d 08 fd ff  callq  810dfe70 
> 
> 8110f5e3: e8 08 6e 80 00  callq  819163f0 

> 8110f5e8: 4c 89 f8mov%r15,%rax
> 8110f5eb: 49 89 c6mov%rax,%r14

Moves r15 into r14 through rax


> 8110f5ee: f6 c4 02test   $0x2,%ah
> 8110f5f1: 75 19   jne8110f60c 
> 
> 8110f5f3: e8 f8 6d 80 00  callq  819163f0 
> 8110f5f8: e8 f3 6d 80 00  callq  819163f0 


> 8110f5fd: 4c 89 75 d0 mov%r14,-0x30(%rbp)
> 8110f601: ff 75 d0pushq  -0x30(%rbp)
> 8110f604: 9d  popfq  

put r14 into -0x30(rbp) and pushes/pops that, see [*] this is 8 bytes
over stack ?!

> 8110f605: e8 26 cb fc ff  callq  810dc130 
> 
> 8110f60a: eb 17   jmp8110f623 
> 
> 8110f60c: e8 2f cb fc ff  callq  810dc140 
> 
> 8110f611: e8 da 6d 80 00  callq  819163f0 
> 8110f616: e8 d5 6d 80 00  callq  819163f0 
> 8110f61b: 4c 89 75 d8 mov%r14,-0x28(%rbp)
> 8110f61f: ff 75 d8pushq  -0x28(%rbp)
> 8110f622: 9d  popfq  

puts r14 into -0x28(rbp) and pushes/pops that

> 8110f623: e8 c8 6d 80 00  callq  819163f0 
> 8110f628: 65 8b 04 25 d4 ae 00mov%gs:0xaed4,%eax
> 8110f62f: 00 
> 8110f630: a9 00 00 0f 00  test   $0xf,%eax
> 8110f635: 74 25   je 8110f65c 
> 
> 8110f637: f6 43 2a 20 testb  $0x20,0x2a(%rbx)
> 8110f63b: 75 1f   jne8110f65c 
> 
> 8110f63d: 48 c7 c7 04 54 c5 81mov$0x81c55404,%rdi
> 8110f644: be 61 04 00 00  mov$0x461,%esi
> 8110f649: e8 12 c4 f6 ff  callq  8107ba60 
> 
> 8110f64e: eb 0c   jmp8110f65c 
> 
> 8110f650: e8 9b 6d 80 00  callq  819163f0 
> 8110f655: e8 96 6d 80 00  callq  819163f0 
> 8110f65a: f3 90   pause  
> 8110f65c: 48 89 dfmov%rbx,%rdi
> 8110f65f: e8 4c fe ff ff  callq  8110f4b0 

Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning

[Peter Zijlstra]

On Wed, Mar 02, 2016 at 04:00:49PM +0100, Sedat Dilek wrote:
> >
> > Right, most odd. Sedat, could you provide objdump -D of the relevant
> > sections of vmlinux ?
> >
> 
> Can you give some clear instructions - for what shall I look for in special?

objdump -D vmlinux | awk '/<[^>]*>:$/ { p=0; } /:/ { p=1; } { 
if (p) print $0; }'

might be a good start, esp. if the output differs between the LLVM and
GCC cases (+- address muck).
--
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] usbhid: Fix lockdep unannotated irqs-off warning

[Peter Zijlstra]

On Tue, Mar 01, 2016 at 10:07:40AM -0500, Steven Rostedt wrote:
> On Tue, 1 Mar 2016 11:05:42 +0100
> Sedat Dilek  wrote:
> 
> 
> > [ FACT #3: TEST-CASE #2 ]
> > 
> > The most reliable test-case is to simply unplug my external Logitech
> > USB mouse - saw this by accident.
> > YES, it was so simple.
> 
> Just to clarify, this happens on gcc and clang?

Just clang from what I gather.

> > --- dmesg_4.5.0-rc6-2-llvmlinux-amd64.txt   2016-02-29
> > 21:23:56.399691975 +0100
> > +++ dmesg_4.5.0-rc6-2-llvmlinux-amd64_usbmouse-unplugged.txt
> > 2016-02-29 21:28:14.401832240 +0100
> > @@ -832,3 +832,75 @@
> >  [   66.529779] PPP BSD Compression module registered
> >  [   66.563013] PPP Deflate Compression module registered
> >  [   66.978977] usb 2-1.5: USB disconnect, device number 4
> > +[  321.937369] usb 2-1.4: USB disconnect, device number 3
> > +[  321.950810] BUG: sleeping function called from invalid context at
> > kernel/workqueue.c:2785
> > +[  321.950816] in_atomic(): 0, irqs_disabled(): 1, pid: 44, name: 
> > kworker/2:1
> > +[  321.950819] 9 locks held by kworker/2:1/44:
> > +[  321.950820]  #0:  ("usb_hub_wq"){.+.+.+}, at: []
> > process_one_work+0x30f/0x8d0
> > +[  321.950830]  #1:  ((>events)){+.+.+.}, at:
> > [] process_one_work+0x33c/0x8d0
> > +[  321.950836]  #2:  (>mutex){..}, at: []
> > hub_event+0x50/0x15b0
> > +[  321.950844]  #3:  (>mutex){..}, at: []
> > usb_disconnect+0x5f/0x2c0
> > +[  321.950849]  #4:  (>mutex){..}, at: []
> > device_release_driver+0x22/0x40
> > +[  321.950856]  #5:  (>mutex){..}, at: []
> > device_release_driver+0x22/0x40
> > +[  321.950862]  #6:  (input_mutex){+.+.+.}, at: []
> > __input_unregister_device+0x9a/0x190
> > +[  321.950869]  #7:  (>mutex#2){+.+...}, at:
> > [] input_close_device+0x27/0x70
> > +[  321.950875]  #8:  (hid_open_mut){+.+...}, at: []
> > usbhid_close+0x28/0xb0 [usbhid]
> > +[  321.950883] irq event stamp: 47770
> > +[  321.950885] hardirqs last  enabled at (47769):
> > [] _raw_spin_unlock_irq+0x32/0x60
> > +[  321.950889] hardirqs last disabled at (47770):
> > [] del_timer_sync+0x3c/0x110
> 
> According to lockdep, interrupts were last disabled in del_timer_sync,
> and they were never enabled. The numbers in parenthesis show the order
> of events. _raw_spin_unlock_irq() at 47769, then del_timer_sync at
> 47770.
> 
> But why did they not get enabled again? We have:
> 
>   local_irq_save(flags);
>   lock_map_acquire(>lockdep_map);
>   lock_map_release(>lockdep_map);
>   local_irq_restore(flags);
> 
> If this caused an issue, then you would have a lockdep splat. But
> perhaps something corrupted "flags", and interrupts were not re-enabled?

Right, most odd. Sedat, could you provide objdump -D of the relevant
sections of vmlinux ?
--
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: Stack dump when try to store uframe_periodic_max

[Peter Zijlstra]

On Mon, Aug 17, 2015 at 10:32:20AM -0400, Alan Stern wrote:
 On Mon, 17 Aug 2015, Peter Chen wrote:
 
  Hi Alan,
  
  When run echo 105   
  /sys/bus/platform/devices/ci_hdrc.1/uframe_periodic_max,
  if lockdep check is enabled, there is below dump, I am afraid it can't
  find how to fix it, the warning shows the key is not persistent.
  (see line 757, kernel/locking/lockdep.c).
  
  
  [   70.190430] INFO: trying to register non-static key.
  [   70.195437] the code is fine but needs lockdep annotation.
  [   70.200939] turning off the locking correctness validator.

 I don't understand this either.
 
 Maybe Peter Z. can help.  This dump is triggered by the
 spin_lock_irqsave() call in
 drivers/usb/host/ehci-sysfs.c:store_uframe_periodic_max().  That
 function doesn't appear to be unusual in any way.

Can it happen this code is called before a corresponding
spin_lock_init()?

Alternatively, memory corruption is an option. If something stomped on
the lockdep state you can trigger this I suppose.
--
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: Linux 3.16-rc6

[Peter Zijlstra]

On Mon, Jul 28, 2014 at 12:37:14PM -0400, Waiman Long wrote:

 I am planning to take out the check in check_deadlock and only have the test
 in lock_acquire which change a 3 to 2 when in interrupt context. Now my
 question is whether to do it as a new patch on top of the existing one in
 tip or a total replacement. I also intend to use symbolic names for the read
 states for better readability as suggested by John.

Send new patches, the patches magically went away from tip.

I don't care too much about the symbolic thing, partly because the
actual value is not irrelevant seeing how we're peddling with bitfields.

Also, its an unrelated cleanup at best.

When you do re-submit extend the locking self test scenarios to cover
the new semantics as well.


pgpfULko6CLvS.pgp
Description: PGP signature

Re: Linux 3.16-rc6

[Peter Zijlstra]

On Thu, Jul 24, 2014 at 04:38:28PM -0400, Waiman Long wrote:
 Yes, I think I may have a solution for that.
 
 Borislav, can you apply the following patch on top of the lockdep patch to
 see if it can fix the problem?
 
 diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
 index d24e433..507a8ce 100644
 --- a/kernel/locking/lockdep.c
 +++ b/kernel/locking/lockdep.c
 @@ -3595,6 +3595,12 @@ void lock_acquire(struct lockdep_map *lock, unsigned 
 int
 raw_local_irq_save(flags);
 check_flags(flags);
 
 +   /*
 +* An interrupt recursive read in interrupt context can be considered
 +* to be the same as a recursive read from checking perspective.
 +*/
 +   if ((read == 3)  in_interrupt())
 +   read = 2;
 current-lockdep_recursion = 1;
 trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock, 
 ip);
 __lock_acquire(lock, subclass, trylock, read, check,

Just had another look at the initial patch and it cannot be right, even
with the above.

The problem is you cannot use in_interrupt() in check_deadlock().
Check_deadlock() must be context invariant, it should only test the
chain state and not rely on where or when its called.




pgpEYYMbqGydu.pgp
Description: PGP signature

Re: Linux 3.16-rc6

[Peter Zijlstra]

On Wed, Jul 23, 2014 at 05:37:43PM -0700, Linus Torvalds wrote:
 On Wed, Jul 23, 2014 at 2:53 AM, Borislav Petkov b...@alien8.de wrote:
 
  Well, it looks like we f*cked up something after -rc5 since I'm starting
  to see lockdep splats all over the place which I didn't see before. I'm
  running rc6 + tip/master.
 
  There was one in r8169 yesterday:
 
  https://lkml.kernel.org/r/20140722081840.ga6...@pd.tnic
 
  and now I'm seeing the following in a kvm guest. I'm adding some more
  lists to CC which look like might be related, judging from the stack
  traces.
 
 Hmm. I'm not seeing the reason for this.
 
  [   31.704282] [ INFO: possible irq lock inversion dependency detected ]
  [   31.704282] 3.16.0-rc6+ #1 Not tainted
  [   31.704282] -
  [   31.704282] Xorg/3484 just changed the state of lock:
  [   31.704282]  (tasklist_lock){.?.+..}, at: [81184b19] 
  send_sigio+0x59/0x1b0
  [   31.704282] but this lock took another, HARDIRQ-unsafe lock in the past:
  [   31.704282]  ((p-alloc_lock)-rlock){+.+...}
 
 Ok, so the claim is that there's a 'p-alloc_lock' (ie task_lock())
 that is inside the tasklist_lock, which would indeed be wrong. But I'm
 not seeing it. The shortest dependencies thing seems to imply
 __set_task_comm(), but that only takes task_lock.
 
 Unless there is something in tip/master. 

lkml.kernel.org/r/tip-e0645a111cb44e01adc6bfff34f683323863f...@git.kernel.org

Its supposed to change lockdep to the stricter semantics provided by the
qrwlock.

Where the rwlock used to be unfair and reader biased, qrwlock is 'fair'
and only allows interrupt recursion.

 Can you check that this is
 actually in plain -rc6?
 
 Or maybe I'm just blind. Those lockdep splats are easy to get wrong.
 Adding PeterZ and Ingo to the list just because they are my lockdep
 go-to people.

I've been staring at this splat from borislav since yesterday morning
and confusing myself properly.. I'll continue doing so until I'm
decided.
--
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: Linux 3.16-rc6

[Peter Zijlstra]

On Thu, Jul 24, 2014 at 02:25:13PM +0200, Borislav Petkov wrote:
 On Thu, Jul 24, 2014 at 10:41:27AM +0200, Borislav Petkov wrote:
  you can easily reproduce by booting a kvm guest with rc6 + tip/master.
 
 Right, so reverting
 
 586fefe5bbdc (locking/selftest: Support queued rwlock)
 e0645a111cb4 (locking/lockdep: Restrict the use of recursive read_lock() 
 with qrwlock)
 
 from the top of tip/locking/core seems to fix the issue, with the kvm
 guests at least.

Well, it makes the report go away.. I'm currently leaning towards that
the report is valid. We did after all change rwlock semantics, and that
lockdep patch is making lockdep match those new semantics.

Of course, its also possible the lockdep patch is wrong. But I'm leaning
towards that the report is valid.

So going by the nifty picture rostedt made:

[   61.454336]CPU0CPU1
[   61.454336]
[   61.454336]   lock((p-alloc_lock)-rlock);
[   61.454336]local_irq_disable();
[   61.454336]lock(tasklist_lock);
[   61.454336]lock((p-alloc_lock)-rlock);
[   61.454336]   Interrupt
[   61.454336] lock(tasklist_lock);

the fact that CPU1 holds tasklist_lock for reading, does not
automagically allow CPU0 to acquire tasklist_lock for reading too, for
example if CPU2 (not in the picture) is waiting to acquire tasklist_lock
for writing, CPU0's read acquire is made to wait.

The only kind of recursion that's safe is same CPU interrupt.

Of course we should have made the lockdep change before merging qrwlock,
and that's entirely my fail, but with qrwlock in these new semantics are
already a reality.

We could of course disable qrwlock until all such issues are cleared up
(its the safe option)...


pgpObnJ6NCL62.pgp
Description: PGP signature

Re: Linux 3.16-rc6

[Peter Zijlstra]

On Thu, Jul 24, 2014 at 11:18:16AM -0700, Linus Torvalds wrote:
 On Thu, Jul 24, 2014 at 5:58 AM, Peter Zijlstra pet...@infradead.org wrote:
 
  So going by the nifty picture rostedt made:
 
  [   61.454336]CPU0CPU1
  [   61.454336]
  [   61.454336]   lock((p-alloc_lock)-rlock);
  [   61.454336]local_irq_disable();
  [   61.454336]lock(tasklist_lock);
  [   61.454336]
  lock((p-alloc_lock)-rlock);
  [   61.454336]   Interrupt
  [   61.454336] lock(tasklist_lock);
 
 So this *should* be fine. It always has been in the past, and it was
 certainly the *intention* that it should continue to work with
 qrwlock, even in the presense of pending writers on other cpu's.
 
 The qrwlock rules are that a read-lock in an interrupt is still going
 to be unfair and succeed if there are other readers.

Ah, indeed. Should have checked :/

 So it sounds to me like the new lockdep rules in tip/master are too
 strict and are throwing a false positive.

Right. Waiman can you have a look?
--
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

[Peter Zijlstra]

On Thu, Feb 13, 2014 at 05:27:20PM +0100, Thomas Gleixner wrote:
 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.

Yeah, and don't forget the reason we ripped that hrtimer softirq stuff
out of mainline :-)
--
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: Audio I/O parameters

[Peter Zijlstra]

On Fri, Aug 02, 2013 at 10:14:39AM -0400, Alan Stern wrote:
 URL Clas-32862d.h.1us : local_clock -perf_event_update_userpage
 URL Clas-32862d.h.2us : watchdog_overflow_callback 
 -__perf_event_overflow
 URL Clas-32862d.h.3us : arch_trigger_all_cpu_backtrace_handler 
 -nmi_handle.isra.0
 URL Clas-32862d.h.3us : perf_ibs_nmi_handler -nmi_handle.isra.0
 URL Clas-32862d.h.3us : perf_ibs_handle_irq -perf_ibs_nmi_handler
 URL Clas-32862d.h.4us : perf_ibs_handle_irq -perf_ibs_nmi_handler
 URL Clas-32862d.h.4us!: rcu_nmi_exit -do_nmi

What's cute is that the trace starts when the NMI handler does
local_irq_save(), _not_ when the NMI starts, which is where the hardware
actually disabled interrupts.

 URL Clas-32862d...  879us : smp_apic_timer_interrupt 
 -apic_timer_interrupt

And then we have hit massive jump.. out of nowhere. weirdness.
--
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: Audio I/O parameters

[Peter Zijlstra]

On Fri, Aug 02, 2013 at 02:57:26PM -0400, Steven Rostedt wrote:
 The other thing you can do is not run perf while this is going on.

URL Clas-32862d.h.1us : local_clock -perf_event_update_userpage
URL Clas-32862d.h.2us : watchdog_overflow_callback 
-__perf_event_overflow
URL Clas-32862d.h.3us : arch_trigger_all_cpu_backtrace_handler 
-nmi_handle.isra.0
URL Clas-32862d.h.3us : perf_ibs_nmi_handler -nmi_handle.isra.0
URL Clas-32862d.h.3us : perf_ibs_handle_irq -perf_ibs_nmi_handler
URL Clas-32862d.h.4us : perf_ibs_handle_irq -perf_ibs_nmi_handler


So they're not running perf as such; it looks to be the NMI watchdog.  It also
looks like they're actually triggering the NMI watchdog --
arch_trigger_all_cpu_backtrace_handler() will cause unconditional 'fun' on all
CPUS.

Weirdly the report doesn't actually say this; one would think that an NMI
watchdog trigger would be a fair clue that something isn't right.

You can disable that with: echo 0  /proc/sys/kernel/nmi_watchdog

This also raises the question why those events even bother calculating time
values; there's no (possible) consumer for them at all.

---
Subject: perf: Do not compute time values unnecessarily

We should not be calling calc_timer_values() for events that do not actually
have an mmap()'ed userpage.

Signed-off-by: Peter Zijlstra pet...@infradead.org
---
 kernel/events/core.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 916cf1f..8643069 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3665,6 +3665,10 @@ void perf_event_update_userpage(struct perf_event *event)
u64 enabled, running, now;
 
rcu_read_lock();
+   rb = rcu_dereference(event-rb);
+   if (!rb)
+   goto unlock;
+
/*
 * compute total_time_enabled, total_time_running
 * based on snapshot values taken when the event
@@ -3675,12 +3679,8 @@ void perf_event_update_userpage(struct perf_event *event)
 * NMI context
 */
calc_timer_values(event, now, enabled, running);
-   rb = rcu_dereference(event-rb);
-   if (!rb)
-   goto unlock;
 
userpg = rb-user_page;
-
/*
 * Disable preemption so as to not let the corresponding user-space
 * spin too long if we get preempted.
--
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: Audio I/O parameters

[Peter Zijlstra]

On Tue, Jul 30, 2013 at 02:05:29PM -0400, Steven Rostedt wrote:
 On Tue, 2013-07-30 at 11:42 -0400, Alan Stern wrote:
  On Mon, 29 Jul 2013, James Stone wrote:
 
 Just an FYI, it is usually better to email my rost...@goodmis.org
 account. I don't always read my redhat email. That's reserved for
 bugzillas assigned to me ;-)
 
  
   OK, having said that, I just got it to happen - listening to audacity
   and just logging into Facebook (of all things!! Meh!)
   
   This is the contents of the trace file (as per instructions on bug 
   #1191603)
   
   # tracer: irqsoff
   #
   # irqsoff latency trace v1.1.5 on 3.10.0-ver5
   # 
   # latency: 2173 us, #4/4, CPU#0 | (M:desktop VP:0, KP:0, SP:0 HP:0 #P:4)
   #-
   #| task: apt-check-3628 (uid:1000 nice:19 policy:0 rt_prio:0)
   #-
   #  = started at: perf_event_update_userpage
   #  = ended at:   retint_careful
   #
   #
   #  _--= CPU#
   # / _-= irqs-off
   #| / _= need-resched
   #|| / _---= hardirq/softirq
   #||| / _--= preempt-depth
   # / delay
   #  cmd pid   | time  |   caller
   # \   /  |  \|   /
   apt-chec-36280d.h.0us!: local_clock -perf_event_update_userpage
 
 perf_event_update_userpage should not be taking 2ms to complete.
 
  This suggests that the IRQ line was disabled or interrupts were
  disabled on all four CPUs for 7 ms.  However, the irqsoff trace shows
  that the maximum latency was 2 ms.  Something strange is going on, but
  I don't know what or how to find it.
  
  Steve, do you have any ideas about this?  Or suggestions for other 
  people to ask?
 
 Is there SMIs on this box? Those will not be detected by the latency
 tracers directly. But we do have other tools to detect them.

Right, NMI shouldn't be on all CPUs concurrently, and while its possible
to get very long NMIs with perf (callchains tend to take a while) 7ms
sounds like very long on a regular machine (there's issues with SGI
class NUMA machines).


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