From: Ingo Molnar <mingo@kernel.org>
To: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Mathias Nyman <mathias.nyman@linux.intel.com>,
Ingo Molnar <mingo@redhat.com>,
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
Date: Thu, 19 Jan 2017 10:37:43 +0100 [thread overview]
Message-ID: <20170119093743.GC22865@gmail.com> (raw)
In-Reply-To: <1479189731-2728-2-git-send-email-baolu.lu@linux.intel.com>
* 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:
> -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 prev parent reply other threads:[~2017-01-19 9:39 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-15 6:02 [PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port Lu Baolu
2016-11-15 6:02 ` [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu
2017-01-19 9:37 ` Ingo Molnar [this message]
2017-01-20 2:47 ` Lu Baolu
2017-01-22 9:04 ` Ingo Molnar
2017-01-24 4:44 ` Lu Baolu
2017-01-24 8:20 ` Ingo Molnar
2017-01-25 5:28 ` Lu Baolu
2017-01-25 9:23 ` Ingo Molnar
2017-01-25 9:57 ` Peter Zijlstra
2017-01-25 12:27 ` Lu Baolu
2017-01-25 14:38 ` Peter Zijlstra
2017-01-25 15:51 ` Lu Baolu
2017-01-25 16:16 ` Peter Zijlstra
2017-01-26 3:37 ` Lu Baolu
2017-01-26 7:19 ` Ingo Molnar
2017-01-26 7:49 ` Lu Baolu
2017-01-26 8:17 ` Ingo Molnar
2017-01-26 10:28 ` Peter Zijlstra
2017-01-26 16:01 ` Ingo Molnar
2017-01-26 17:39 ` Peter Zijlstra
2017-01-27 6:51 ` Ingo Molnar
2017-02-09 5:59 ` Lu Baolu
2017-01-26 7:22 ` Ingo Molnar
2017-02-09 7:37 ` Lu Baolu
2017-01-25 12:17 ` Lu Baolu
2017-01-26 3:26 ` Lu Baolu
2016-11-15 6:02 ` [PATCH v5 2/4] x86: add support for earlyprintk via USB3 debug port Lu Baolu
2017-01-19 9:38 ` Ingo Molnar
2017-01-20 2:48 ` Lu Baolu
2016-11-15 6:02 ` [PATCH v5 3/4] usb: serial: usb_debug: add support for dbc debug device Lu Baolu
2017-01-19 9:39 ` Ingo Molnar
2017-01-20 2:50 ` Lu Baolu
2016-11-15 6:02 ` [PATCH v5 4/4] usb: doc: add document for USB3 debug port usage Lu Baolu
2017-01-19 9:41 ` Ingo Molnar
2017-01-20 2:53 ` Lu Baolu
2017-01-18 6:20 ` [PATCH v5 0/4] usb: early: add support for early printk through USB3 debug port Lu Baolu
2017-01-19 9:06 ` Greg Kroah-Hartman
2017-01-19 9:09 ` Ingo Molnar
2017-01-19 11:24 ` Mathias Nyman
2017-01-19 9:12 ` Ingo Molnar
2017-01-20 2:56 ` Lu Baolu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170119093743.GC22865@gmail.com \
--to=mingo@kernel.org \
--cc=baolu.lu@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@linux.intel.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.