lkml.org 
[lkml]   [2017]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
    From
    Date
    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 <baolu.lu@linux.intel.com> 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@linux.intel.com>
    >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
    >> ---
    >> 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

    \
     
     \ /
      Last update: 2017-01-20 03:48    [W:3.078 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site