From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751375AbdASJjr (ORCPT ); Thu, 19 Jan 2017 04:39:47 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:34265 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751039AbdASJil (ORCPT ); Thu, 19 Jan 2017 04:38:41 -0500 Date: Thu, 19 Jan 2017 10:37:43 +0100 From: Ingo Molnar To: Lu Baolu Cc: Greg Kroah-Hartman , Mathias Nyman , Ingo Molnar , tglx@linutronix.de, peterz@infradead.org, linux-usb@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Message-ID: <20170119093743.GC22865@gmail.com> References: <1479189731-2728-1-git-send-email-baolu.lu@linux.intel.com> <1479189731-2728-2-git-send-email-baolu.lu@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1479189731-2728-2-git-send-email-baolu.lu@linux.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Lu Baolu 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 > 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 | 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