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

From: Ingo Molnar
Date: Thu Jan 19 2017 - 04:41:26 EST

Next message: Quatif Group of Companies: "Business Proposal"
Previous message: Greg KH: "Re: [PATCH] tools: usb usbip - update README USB/IP driver location"
Next in thread: Lu Baolu: "Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

* Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:

> 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 <mathias.nyman@xxxxxxxxxxxxxxx>
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
> arch/x86/Kconfig.debug | 14 +
> drivers/usb/Kconfig | 3 +
> drivers/usb/Makefile | 2 +-
> drivers/usb/early/Makefile | 1 +
> drivers/usb/early/xhci-dbc.c | 1068 +++++++++++++++++++++++++++++++++++++++++
> drivers/usb/early/xhci-dbc.h | 205 ++++++++
> include/linux/usb/xhci-dbgp.h | 22 +
> 7 files changed, 1314 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.

Could we please do this rename:

s/EARLY_PRINTK_XDBC
EARLY_PRINTK_USB_XDBC

?

As many people will not realize what 'xdbc' means, standalone - while "it's an
USB serial logging variant" is a lot more natural.

> +config USB_EARLY_PRINTK
> + bool

Also, could we standardize the nomencalture to not be a mixture of prefixes and
postfixes - i.e. standardize on postfixes (as commonly done in the Kconfig space)
and rename this one to EARLY_PRINTK_USB or so?

You can see the prefix/postfix inconsistency here already:

> -obj-$(CONFIG_EARLY_PRINTK_DBGP) += early/
> +obj-$(CONFIG_USB_EARLY_PRINTK) += early/
> +obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o

> +static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
> +{
> + u32 val, sz;
> + u64 val64, sz64, mask64;
> + u8 byte;
> + void __iomem *base;
> +
> + val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, ~0);
> + sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, val);
> + if (val == 0xffffffff || sz == 0xffffffff) {
> + pr_notice("invalid mmio bar\n");
> + return NULL;
> + }

> + if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> + PCI_BASE_ADDRESS_MEM_TYPE_64) {

Please don't break the line here.

> + val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, ~0);
> + sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, val);
> +
> + val64 |= ((u64)val << 32);
> + sz64 |= ((u64)sz << 32);
> + mask64 |= ((u64)~0 << 32);

Unnecessary parentheses.

> + }
> +
> + sz64 &= mask64;
> +
> + if (sizeof(dma_addr_t) < 8 || !sz64) {
> + pr_notice("invalid mmio address\n");
> + return NULL;
> + }

So this doesn't work on regular 32-bit kernels?

> +static u32 __init xdbc_find_dbgp(int xdbc_num, u32 *b, u32 *d, u32 *f)
> +{
> + u32 bus, dev, func, class;
> +
> + for (bus = 0; bus < XDBC_PCI_MAX_BUSES; bus++) {
> + for (dev = 0; dev < XDBC_PCI_MAX_DEVICES; dev++) {
> + for (func = 0; func < XDBC_PCI_MAX_FUNCTION; func++) {
> + class = read_pci_config(bus, dev, func,
> + PCI_CLASS_REVISION);

Please no ugly linebreaks.

> +static void xdbc_runtime_delay(unsigned long count)
> +{
> + udelay(count);
> +}

> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;

Is this udelay() complication really necessary? udelay() should work fine even in
early code. It might not be precisely calibrated, but should be good enough.

> +static int handshake(void __iomem *ptr, u32 mask, u32 done,
> + int wait, int delay)

Please break lines more intelligently:

static int
handshake(void __iomem *ptr, u32 mask, u32 done, int wait, int delay)

> + ext_cap_offset = xhci_find_next_ext_cap(xdbc.xhci_base,
> + 0, XHCI_EXT_CAPS_LEGACY);

No ugly linebreaks please. There's a ton more in other parts of this patch and
other patches: please review all the other linebreaks (and ignore checkpatch.pl).

For example this:

> + xdbc.erst_base = xdbc.table_base +
> + index * XDBC_TABLE_ENTRY_SIZE;
> + xdbc.erst_dma = xdbc.table_dma +
> + index * XDBC_TABLE_ENTRY_SIZE;

should be:

xdbc.erst_base = xdbc.table_base + index*XDBC_TABLE_ENTRY_SIZE;
xdbc.erst_dma = xdbc.table_dma + index*XDBC_TABLE_ENTRY_SIZE;

which makes it much more readable, etc.

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

Hm, why are newlines converted to \r\n unconditionally? Makes for a crappy minicom
log on the other side ...

> +static int __init xdbc_init(void)
> +{
> + unsigned long flags;
> + void __iomem *base;
> + u32 offset;
> + int ret = 0;
> +
> + if (!(xdbc.flags & XDBC_FLAGS_INITIALIZED))
> + return 0;
> +
> + xdbc_delay = xdbc_runtime_delay;
> +
> + /*
> + * It's time to shutdown DbC, so that the debug
> + * port could be reused by the host controller.

s/shutdown DbC
/shut down the DbC

s/could be reused
/can be reused

?

> + */
> + if (early_xdbc_console.index == -1 ||
> + (early_xdbc_console.flags & CON_BOOT)) {
> + xdbc_trace("hardware not used any more\n");

s/any more
anymore

> + raw_spin_lock_irqsave(&xdbc.lock, flags);
> + base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);

Ugh, ioremap() can sleep ...

> +/**
> + * struct xdbc_regs - xHCI Debug Capability Register interface.
> + */
> +struct xdbc_regs {
> + __le32 capability;
> + __le32 doorbell;
> + __le32 ersts; /* Event Ring Segment Table Size*/
> + __le32 rvd0; /* 0c~0f reserved bits */

Yeah, so thsbbrvtnssck. (these abbreviations suck)

Why 'rvd0' - did we run out of letters? Please name it __reserved_0 and
__reserved_1 like we typically do in kernel code.

> + __le32 rsvd;

> + __le32 rsvdz[7];

> + __le32 rsvd0[11];

ditto.

> +#define XDBC_INFO_CONTEXT_SIZE 48
> +
> +#define XDBC_MAX_STRING_LENGTH 64
> +#define XDBC_STRING_MANUFACTURE "Linux"
> +#define XDBC_STRING_PRODUCT "Remote GDB"
> +#define XDBC_STRING_SERIAL "0001"
> +struct xdbc_strings {

Please put a newline between different types of definitions.

> + char string0[XDBC_MAX_STRING_LENGTH];
> + char manufacture[XDBC_MAX_STRING_LENGTH];
> + char product[XDBC_MAX_STRING_LENGTH];
> + char serial[XDBC_MAX_STRING_LENGTH];

s/manufacture/manufacturer

?

> +};
> +
> +#define XDBC_PROTOCOL 1 /* GNU Remote Debug Command Set */
> +#define XDBC_VENDOR_ID 0x1d6b /* Linux Foundation 0x1d6b */
> +#define XDBC_PRODUCT_ID 0x0004 /* __le16 idProduct; device 0004 */
> +#define XDBC_DEVICE_REV 0x0010 /* 0.10 */
> +
> +/*
> + * software state structure
> + */
> +struct xdbc_segment {
> + struct xdbc_trb *trbs;
> + dma_addr_t dma;
> +};
> +
> +#define XDBC_TRBS_PER_SEGMENT 256
> +
> +struct xdbc_ring {
> + struct xdbc_segment *segment;
> + struct xdbc_trb *enqueue;
> + struct xdbc_trb *dequeue;
> + u32 cycle_state;
> +};
> +
> +#define XDBC_EPID_OUT 2
> +#define XDBC_EPID_IN 3
> +
> +struct xdbc_state {
> + /* pci device info*/
> + u16 vendor;
> + u16 device;
> + u32 bus;
> + u32 dev;
> + u32 func;
> + void __iomem *xhci_base;
> + u64 xhci_start;
> + size_t xhci_length;
> + int port_number;
> +#define XDBC_PCI_MAX_BUSES 256
> +#define XDBC_PCI_MAX_DEVICES 32
> +#define XDBC_PCI_MAX_FUNCTION 8
> +
> + /* DbC register base */
> + struct xdbc_regs __iomem *xdbc_reg;
> +
> + /* DbC table page */
> + dma_addr_t table_dma;
> + void *table_base;
> +
> +#define XDBC_TABLE_ENTRY_SIZE 64
> +#define XDBC_ERST_ENTRY_NUM 1
> +#define XDBC_DBCC_ENTRY_NUM 3
> +#define XDBC_STRING_ENTRY_NUM 4
> +
> + /* event ring segment table */
> + dma_addr_t erst_dma;
> + size_t erst_size;
> + void *erst_base;
> +
> + /* event ring segments */
> + struct xdbc_ring evt_ring;
> + struct xdbc_segment evt_seg;
> +
> + /* debug capability contexts */
> + dma_addr_t dbcc_dma;
> + size_t dbcc_size;
> + void *dbcc_base;
> +
> + /* descriptor strings */
> + dma_addr_t string_dma;
> + size_t string_size;
> + void *string_base;
> +
> + /* bulk OUT endpoint */
> + struct xdbc_ring out_ring;
> + struct xdbc_segment out_seg;
> + void *out_buf;
> + dma_addr_t out_dma;
> +
> + /* bulk IN endpoint */
> + struct xdbc_ring in_ring;
> + struct xdbc_segment in_seg;
> + void *in_buf;
> + dma_addr_t in_dma;

Please make the vertical tabulation of the fields consistent throughout the
structure. Look at it in a terminal and convince yourself that it's nice and
beautiful to look at!

Also, if you mix CPP #defines into structure definitions then tabulate them in a
similar fashion.

Thanks,

Ingo

Next message: Quatif Group of Companies: "Business Proposal"
Previous message: Greg KH: "Re: [PATCH] tools: usb usbip - update README USB/IP driver location"
Next in thread: Lu Baolu: "Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]