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

[Lu Baolu]

Hi Ingo,

On 01/22/2017 05:31 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> + *
>> + * 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.

Thank you for the comments.

I will correct all the code style issues you pointed out here. And, I will
review all patches and fix the similar code styles.

Best regards,
Lu Baolu