Re: [RESEND PATCH v2 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

Hi Peter,

On 10/20/2016 05:08 PM, Peter Zijlstra wrote:
> 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.
>

Thank you again.

Your comments make sense to me. I will remove the lock and work
queue usages in the new version.

Best regards,
Lu Baolu
--
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

[Lu Baolu]

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

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

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

As I said above, I need a worker to check the 

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

[RESEND PATCH v2 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu]

xHCI debug capability (DbC) is an optional but standalone
functionality provided by an xHCI host controller. Software
learns this capability by walking through the extended
capability list of the host. xHCI specification describes
DbC in section 7.6.

This patch introduces the code to probe and initialize the
debug capability hardware during early boot. With hardware
initialized, the debug target (system on which this code is
running) will present a debug device through the debug port
(normally the first USB3 port). The debug device is fully
compliant with the USB framework and provides the equivalent
of a very high performance (USB3) full-duplex serial link
between the debug host and target. The DbC functionality is
independent of xHCI host. There isn't any precondition from
xHCI host side for DbC to work.

This patch also includes bulk out and bulk in interfaces.
These interfaces could be used to implement early printk
bootconsole or hook to various system debuggers.

This code is designed to be only used for kernel debugging
when machine crashes very early before the console code is
initialized. For normal operation it is not recommended.

Cc: Mathias Nyman 
Signed-off-by: Lu Baolu 
---
 arch/x86/Kconfig.debug|   14 +
 drivers/usb/Kconfig   |3 +
 drivers/usb/Makefile  |2 +-
 drivers/usb/early/Makefile|1 +
 drivers/usb/early/xhci-dbc.c  | 1097 +
 drivers/usb/early/xhci-dbc.h  |  206 
 include/linux/usb/xhci-dbgp.h |   22 +
 7 files changed, 1344 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/early/xhci-dbc.c
 create mode 100644 drivers/usb/early/xhci-dbc.h
 create mode 100644 include/linux/usb/xhci-dbgp.h

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 67eec55..13e85b7 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -29,6 +29,7 @@ config EARLY_PRINTK
 config EARLY_PRINTK_DBGP
bool "Early printk via EHCI debug port"
depends on EARLY_PRINTK && PCI
+   select USB_EARLY_PRINTK
---help---
  Write kernel log output directly into the EHCI debug port.
 
@@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
  This is useful for kernel debugging when your machine crashes very
  early before the console code is initialized.
 
+config EARLY_PRINTK_XDBC
+   bool "Early printk via xHCI debug port"
+   depends on EARLY_PRINTK && PCI
+   select USB_EARLY_PRINTK
+   ---help---
+ Write kernel log output directly into the xHCI debug port.
+
+ This is useful for kernel debugging when your machine crashes very
+ early before the console code is initialized. For normal operation
+ it is not recommended because it looks ugly and doesn't cooperate
+ with klogd/syslogd or the X server. You should normally N here,
+ unless you want to debug such a crash.
+
 config X86_PTDUMP_CORE
def_bool n
 
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 644e978..860d81b1 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -19,6 +19,9 @@ config USB_EHCI_BIG_ENDIAN_MMIO
 config USB_EHCI_BIG_ENDIAN_DESC
bool
 
+config USB_EARLY_PRINTK
+   bool
+
 menuconfig USB_SUPPORT
bool "USB support"
depends on HAS_IOMEM
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index dca7856..dd91ca1 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -48,7 +48,7 @@ obj-$(CONFIG_USB_MICROTEK)+= image/
 obj-$(CONFIG_USB_SERIAL)   += serial/
 
 obj-$(CONFIG_USB)  += misc/
-obj-$(CONFIG_EARLY_PRINTK_DBGP)+= early/
+obj-$(CONFIG_USB_EARLY_PRINTK) += early/
 
 obj-$(CONFIG_USB_ATM)  += atm/
 obj-$(CONFIG_USB_SPEEDTOUCH)   += atm/
diff --git a/drivers/usb/early/Makefile b/drivers/usb/early/Makefile
index 24bbe51..2db5906 100644
--- a/drivers/usb/early/Makefile
+++ b/drivers/usb/early/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o
+obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
new file mode 100644
index 000..939fff2
--- /dev/null
+++ b/drivers/usb/early/xhci-dbc.c
@@ -0,0 +1,1097 @@
+/**
+ * xhci-dbc.c - xHCI debug capability early driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Author: Lu Baolu 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt)KBUILD_MODNAME ":%s: " fmt, __func__
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "../host/xhci.h"
+#include "xhci-dbc.h"
+
+static struct xdbc_state xdbc;
+static int