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

[Lu Baolu <baolu.lu@linux.intel.com>]

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