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

[Ingo Molnar <mingo@kernel.org>]

* 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