Date:   Sun, 22 Jan 2017 10:31:29 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Lu Baolu <baolu.lu@...ux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mathias Nyman <mathias.nyman@...ux.intel.com>,
        tglx@...utronix.de, peterz@...radead.org,
        linux-usb@...r.kernel.org, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/4] usb: dbc: early driver for xhci debug capability


* Lu Baolu <baolu.lu@...ux.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.

s/xHCI host/the xHCI host

> 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.

s/out/output
s/in/input

> +config EARLY_PRINTK_USB_XDBC
> +	bool "Early printk via xHCI debug port"

s/xHCI/the xHCI

I remarked on this in my first review as well - mind checking the whole series for 
uses of 'xHCI'?

> +	depends on EARLY_PRINTK && PCI
> +	select EARLY_PRINTK_USB
> +	---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.
> +
>  config X86_PTDUMP_CORE
>  	def_bool n
>  
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index fbe493d..9313fff 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -19,6 +19,9 @@ config USB_EHCI_BIG_ENDIAN_MMIO
>  config USB_EHCI_BIG_ENDIAN_DESC
>  	bool
>  
> +config USB_EARLY_PRINTK
> +	bool
> +
>  menuconfig USB_SUPPORT
>  	bool "USB support"
>  	depends on HAS_IOMEM
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index 7791af6..53d1356 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -49,7 +49,7 @@ obj-$(CONFIG_USB_MICROTEK)	+= image/
>  obj-$(CONFIG_USB_SERIAL)	+= serial/
>  
>  obj-$(CONFIG_USB)		+= misc/
> -obj-$(CONFIG_EARLY_PRINTK_DBGP)	+= early/
> +obj-$(CONFIG_EARLY_PRINTK_USB)	+= early/
>  
>  obj-$(CONFIG_USB_ATM)		+= atm/
>  obj-$(CONFIG_USB_SPEEDTOUCH)	+= atm/
> diff --git a/drivers/usb/early/Makefile b/drivers/usb/early/Makefile
> index 24bbe51..fcde228 100644
> --- a/drivers/usb/early/Makefile
> +++ b/drivers/usb/early/Makefile
> @@ -3,3 +3,4 @@
>  #
>  
>  obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o
> +obj-$(CONFIG_EARLY_PRINTK_USB_XDBC) += xhci-dbc.o
> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
> new file mode 100644
> index 0000000..72480c4
> --- /dev/null
> +++ b/drivers/usb/early/xhci-dbc.c
> @@ -0,0 +1,1027 @@
> +/**
> + * xhci-dbc.c - xHCI debug capability early driver
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * Author: Lu Baolu <baolu.lu@...ux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt)	KBUILD_MODNAME ":%s: " fmt, __func__
> +
> +#include <linux/console.h>
> +#include <linux/pci_regs.h>
> +#include <linux/pci_ids.h>
> +#include <linux/bootmem.h>
> +#include <linux/io.h>
> +#include <asm/pci-direct.h>
> +#include <asm/fixmap.h>
> +#include <linux/bcd.h>
> +#include <linux/export.h>
> +#include <linux/version.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +
> +#include "../host/xhci.h"
> +#include "xhci-dbc.h"
> +
> +static struct xdbc_state xdbc;
> +static int early_console_keep;
> +
> +#ifdef XDBC_TRACE
> +#define	xdbc_trace	trace_printk
> +#else
> +static inline void xdbc_trace(const char *fmt, ...) { }
> +#endif /* XDBC_TRACE */
> +
> +static int xdbc_bulk_transfer(void *data, int size, bool read);
> +
> +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;
> +	}
> +
> +	val64 = val & PCI_BASE_ADDRESS_MEM_MASK;
> +	sz64 = sz & PCI_BASE_ADDRESS_MEM_MASK;
> +	mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;

Is this cast necessary?

> +
> +	if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +		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;

Could these type casts be avoided by declaring 'val' and 'sz' as u64 to begin 
with?

> +		mask64 |= (u64)~0 << 32;

Type cast could be avoided by using 0L.

> +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);
> +				if ((class >> 8) != PCI_CLASS_SERIAL_USB_XHCI)
> +					continue;
> +
> +				if (xdbc_num-- != 0)
> +					continue;
> +
> +				*b = bus;
> +				*d = dev;
> +				*f = func;
> +
> +				return 0;
> +			}
> +		}
> +	}

Nit: to me it would look more balanced visually if the body of the innermost loop 
started with an empty newline.

> +	ext_cap_offset = xhci_find_next_ext_cap(xdbc.xhci_base,
> +						0, XHCI_EXT_CAPS_LEGACY);

Please don't break this line. You can shorten the variable name if you want to 
save line length.

> +	/* populate the contexts */
> +	context = (struct xdbc_context *)xdbc.dbcc_base;
> +	context->info.string0 = cpu_to_le64(xdbc.string_dma);
> +	context->info.manufacturer = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
> +	context->info.product = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
> +	context->info.serial = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
> +	context->info.length = cpu_to_le32(string_length);

Wouldn't this (and some of the other initializations) look nicer this way:

	/* Populate the contexts: */
	context = (struct xdbc_context *)xdbc.dbcc_base;

	context->info.string0		= cpu_to_le64(xdbc.string_dma);
	context->info.manufacturer	= cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
	context->info.product		= cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
	context->info.serial		= cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
	context->info.length		= cpu_to_le32(string_length);

?

> +	/* Check completion of the previous request. */

Could you please standardize the capitalization and punctuation of all the 
comments in the patches? Some are lower case, some are upper case, some have full 
stops, some don't.

One good style would be to use this form when a comment refers to what the next 
statement does:

	/* Check completion of the previous request: */

> +/**
> + * struct xdbc_regs - xHCI Debug Capability Register interface.
> + */
> +struct xdbc_regs {
> +	__le32	capability;
> +	__le32	doorbell;
> +	__le32	ersts;		/* Event Ring Segment Table Size*/
> +	__le32	__reserved_0;	/* 0c~0f reserved bits */
> +	__le64	erstba;		/* Event Ring Segment Table Base Address */
> +	__le64	erdp;		/* Event Ring Dequeue Pointer */
> +	__le32	control;
> +#define DEBUG_MAX_BURST(p)	(((p) >> 16) & 0xff)
> +#define CTRL_DCR		BIT(0)		/* DbC Run */
> +#define CTRL_PED		BIT(1)		/* Port Enable/Disable */
> +#define CTRL_HOT		BIT(2)		/* Halt Out TR */
> +#define CTRL_HIT		BIT(3)		/* Halt In TR */
> +#define CTRL_DRC		BIT(4)		/* DbC run change */
> +#define CTRL_DCE		BIT(31)		/* DbC enable */
> +	__le32	status;
> +#define DCST_DPN(p)		(((p) >> 24) & 0xff)
> +	__le32	portsc;		/* Port status and control */
> +#define PORTSC_CCS		BIT(0)
> +#define PORTSC_CSC		BIT(17)
> +#define PORTSC_PRC		BIT(21)
> +#define PORTSC_PLC		BIT(22)
> +#define PORTSC_CEC		BIT(23)
> +	__le32	__reserved_1;	/* 2b~28 reserved bits */
> +	__le64	dccp;		/* Debug Capability Context Pointer */
> +	__le32	devinfo1;	/* Device Descriptor Info Register 1 */
> +	__le32	devinfo2;	/* Device Descriptor Info Register 2 */
> +};

So why are defines intermixed with the fields? If the defines relate to the field 
then their naming sure does not express that ...

I was thinking of something like:

/**
 * struct xdbc_regs - xHCI Debug Capability Register interface.
 */
struct xdbc_regs {
	__le32	capability;
	__le32	doorbell;
	__le32	ersts;		/* Event Ring Segment Table Size*/
	__le32	__reserved_0;	/* 0c~0f reserved bits */
	__le64	erstba;		/* Event Ring Segment Table Base Address */
	__le64	erdp;		/* Event Ring Dequeue Pointer */
	__le32	control;
	__le32	status;
	__le32	portsc;		/* Port status and control */
	__le32	__reserved_1;	/* 2b~28 reserved bits */
	__le64	dccp;		/* Debug Capability Context Pointer */
	__le32	devinfo1;	/* Device Descriptor Info Register 1 */
	__le32	devinfo2;	/* Device Descriptor Info Register 2 */
};

#define DEBUG_MAX_BURST(p)	(((p) >> 16) & 0xff)

#define	CTRL_DCR		BIT(0)		/* DbC Run */
#define	CTRL_PED		BIT(1)		/* Port Enable/Disable */
#define	CTRL_HOT		BIT(2)		/* Halt Out TR */
#define	CTRL_HIT		BIT(3)		/* Halt In TR */
#define	CTRL_DRC		BIT(4)		/* DbC run change */
#define CTRL_DCE		BIT(31)		/* DbC enable */

#define DCST_DPN(p)		(((p) >> 24) & 0xff)

#define PORTSC_CCS		BIT(0)
#define PORTSC_CSC		BIT(17)
#define PORTSC_PRC		BIT(21)
#define PORTSC_PLC		BIT(22)
#define PORTSC_CEC		BIT(23)

Also, the abbreviations suck big time. What's wrong with:

#define	CTRL_DBC_RUN		BIT(0)
#define	CTRL_PORT_ENABLE	BIT(1)
#define	CTRL_HALT_OUT_TR	BIT(2)
#define	CTRL_HALT_IN_TR		BIT(3)
#define	CTRL_DBC_RUN_CHANGE	BIT(4)
#define CTRL_DBC_ENABLE		BIT(31)

?

Also note how the comments became superfluous once descriptive names are chosen...

Please review the rest of the defines and series for similar problems and fix 
them, there are more of the same type.

Thanks,

	Ingo

Powered by blists - more mailing lists