Hi Ingo, I'm very appreciated for your review comments. I've put my replies in lines.
On 01/19/2017 05:37 PM, Ingo Molnar wrote: > * Lu Baolu <[email protected]> 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 <[email protected]> >> Signed-off-by: Lu Baolu <[email protected]> >> --- >> 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: Sure. I will fix the names. Thanks. > >> -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. Sure. Will fix it. > >> + 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. Sure. Will fix it. > >> + } >> + >> + 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? I will run my code on a 32-bit kernel and remove this check if it passes the test. > >> +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. Sorry. I will fix it. > >> +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. I tried udelay() in the early code. It's not precise enough for the hardware handshaking. > >> +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) Sure. I will fix it. > >> + 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. Sure. These line breaks were added to make checkpatch.pl happy. I will review all the line breaks and make them more readable. > >> +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 ... Yes. The usb ehci (usb2) debug port driver (drivers/usb/early/ehci-dbgp.c) does this. I kept the same for xhci (usb3). It turns out to be good for display on host 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 > > ? > Sure. I will fix it. Thanks. >> + */ >> + if (early_xdbc_console.index == -1 || >> + (early_xdbc_console.flags & CON_BOOT)) { >> + xdbc_trace("hardware not used any more\n"); > s/any more > anymore Sure. I will fix it. Thanks. > >> + raw_spin_lock_irqsave(&xdbc.lock, flags); >> + base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length); > Ugh, ioremap() can sleep ... Oh, right. I will remove the remapping code and let it use the previously mapped one. > >> +/** >> + * 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. Sure. I will fix it. Thanks. > >> + __le32 rsvd; >> + __le32 rsvdz[7]; >> + __le32 rsvd0[11]; > ditto. I will fix them. > >> +#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. Sure. > >> + 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 > > ? Sure. > >> +}; >> + >> +#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. Sure. I will fix this. Thank you. Best regards, Lu Baolu
