Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1562531 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Ingo Molnar <mingo@kernel.org> - 2017-01-19 10:50 +0100
    Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu <baolu.lu@linux.intel.com> - 2017-01-20 03:50 +0100
      Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Ingo Molnar <mingo@kernel.org> - 2017-01-22 10:10 +0100
        Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu <baolu.lu@linux.intel.com> - 2017-01-24 05:50 +0100
          Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Ingo Molnar <mingo@kernel.org> - 2017-01-24 09:30 +0100
            Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu <baolu.lu@linux.intel.com> - 2017-01-25 06:30 +0100
              Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Ingo Molnar <mingo@kernel.org> - 2017-01-25 10:30 +0100
                Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Peter Zijlstra <peterz@infradead.org> - 2017-01-25 11:00 +0100
                  Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu <baolu.lu@linux.intel.com> - 2017-01-25 13:30 +0100
                    Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Peter Zijlstra <peterz@infradead.org> - 2017-01-25 15:40 +0100
                      Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu <baolu.lu@linux.intel.com> - 2017-01-25 17:00 +0100
                        Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Peter Zijlstra <peterz@infradead.org> - 2017-01-25 17:20 +0100
                          Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu <baolu.lu@linux.intel.com> - 2017-01-26 04:40 +0100
                            Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Ingo Molnar <mingo@kernel.org> - 2017-01-26 08:20 +0100
                              Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu <baolu.lu@linux.intel.com> - 2017-01-26 08:50 +0100
                                Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Ingo Molnar <mingo@kernel.org> - 2017-01-26 09:20 +0100
                              Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Peter Zijlstra <peterz@infradead.org> - 2017-01-26 11:30 +0100
                                Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Ingo Molnar <mingo@kernel.org> - 2017-01-26 17:10 +0100
                                  Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Peter Zijlstra <peterz@infradead.org> - 2017-01-26 18:50 +0100
                                    Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Ingo Molnar <mingo@kernel.org> - 2017-01-27 08:00 +0100
                        Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Ingo Molnar <mingo@kernel.org> - 2017-01-26 08:30 +0100
                Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu <baolu.lu@linux.intel.com> - 2017-01-25 13:20 +0100
                Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability Lu Baolu <baolu.lu@linux.intel.com> - 2017-01-26 04:30 +0100

Page 1 of 2  [1] 2  Next page →

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

* 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

[toc] | [next] | [standalone]

#1563251

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

[toc] | [prev] | [next] | [standalone]

#1564345

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

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

Possibly because on x86 early udelay() did not work at all - i.e. there's no delay 
whatsoever.

Could you try it on top of this commit in tip:timers/core:

  4c45c5167c95 x86/timer: Make delay() work during early bootup

?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1565470

[Multipart message — attachments visible in raw view] — view raw

Hi Ingo,

On 01/22/2017 05:04 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>>>> +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.
> Possibly because on x86 early udelay() did not work at all - i.e. there's no delay 
> whatsoever.

Yes.

>
> Could you try it on top of this commit in tip:timers/core:
>
>   4c45c5167c95 x86/timer: Make delay() work during early bootup
>
> ?

I tried tip:timers/core. It's not precise enough for my context either.

__const_udelay().

157 inline void __const_udelay(unsigned long xloops)
158 {
159         unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy) ? : loops_per_jiffy;
160         int d0;
161
162         xloops *= 4;
163         asm("mull %%edx"
164                 :"=d" (xloops), "=&a" (d0)
165                 :"1" (xloops), "0" (lpj * (HZ / 4)));
166
167         __delay(++xloops);
168 }


In my early  code, loops_per_jiffy is not initialized yet. Hence "lpj" for the asm line
is 4096 (default value).

The  cpu_info.loops_per_jiffy actually reads 8832000 after initialization. They are
about 2000 times different.

I did a hacky test in kernel to check the difference between these two different
"lpj" values. (The hacky patch is attached.) Below is the output for 100ms delay.

[    2.494751] udelay_test uninitialized ---->start
[    2.494820] udelay_test uninitialized ---->end
[    2.494828] udelay_test initialized ---->start
[    2.595234] udelay_test initialized ---->end

For 100ms delay, udelay() with uninitialized loops_per_jiffy only gives a delay of
only 69us.

Best regards,
Lu Baolu

[toc] | [prev] | [next] | [standalone]

#1565653

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Ingo,
> 
> On 01/22/2017 05:04 PM, Ingo Molnar wrote:
> > * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >
> >>>> +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.
> > Possibly because on x86 early udelay() did not work at all - i.e. there's no delay 
> > whatsoever.
> 
> Yes.
> 
> >
> > Could you try it on top of this commit in tip:timers/core:
> >
> >   4c45c5167c95 x86/timer: Make delay() work during early bootup
> >
> > ?
> 
> I tried tip:timers/core. It's not precise enough for my context either.
> 
> __const_udelay().
> 
> 157 inline void __const_udelay(unsigned long xloops)
> 158 {
> 159         unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy) ? : loops_per_jiffy;
> 160         int d0;
> 161
> 162         xloops *= 4;
> 163         asm("mull %%edx"
> 164                 :"=d" (xloops), "=&a" (d0)
> 165                 :"1" (xloops), "0" (lpj * (HZ / 4)));
> 166
> 167         __delay(++xloops);
> 168 }
> 
> 
> In my early  code, loops_per_jiffy is not initialized yet. Hence "lpj" for the asm line
> is 4096 (default value).
> 
> The  cpu_info.loops_per_jiffy actually reads 8832000 after initialization. They are
> about 2000 times different.
> 
> I did a hacky test in kernel to check the difference between these two different
> "lpj" values. (The hacky patch is attached.) Below is the output for 100ms delay.
> 
> [    2.494751] udelay_test uninitialized ---->start
> [    2.494820] udelay_test uninitialized ---->end
> [    2.494828] udelay_test initialized ---->start
> [    2.595234] udelay_test initialized ---->end
> 
> For 100ms delay, udelay() with uninitialized loops_per_jiffy only gives a delay of
> only 69us.

Ok, then could we add some simple calibration to make udelay work much better - or 
perhaps move the udelay calibration up earlier?

Hiding essentially an early udelay() implementation in an early-printk driver is 
ugly and counterproductive.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1566327

Hi Ingo,

On 01/24/2017 04:20 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Hi Ingo,
>>
>> On 01/22/2017 05:04 PM, Ingo Molnar wrote:
>>> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>>
>>>>>> +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.
>>> Possibly because on x86 early udelay() did not work at all - i.e. there's no delay 
>>> whatsoever.
>> Yes.
>>
>>> Could you try it on top of this commit in tip:timers/core:
>>>
>>>   4c45c5167c95 x86/timer: Make delay() work during early bootup
>>>
>>> ?
>> I tried tip:timers/core. It's not precise enough for my context either.
>>
>> __const_udelay().
>>
>> 157 inline void __const_udelay(unsigned long xloops)
>> 158 {
>> 159         unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy) ? : loops_per_jiffy;
>> 160         int d0;
>> 161
>> 162         xloops *= 4;
>> 163         asm("mull %%edx"
>> 164                 :"=d" (xloops), "=&a" (d0)
>> 165                 :"1" (xloops), "0" (lpj * (HZ / 4)));
>> 166
>> 167         __delay(++xloops);
>> 168 }
>>
>>
>> In my early  code, loops_per_jiffy is not initialized yet. Hence "lpj" for the asm line
>> is 4096 (default value).
>>
>> The  cpu_info.loops_per_jiffy actually reads 8832000 after initialization. They are
>> about 2000 times different.
>>
>> I did a hacky test in kernel to check the difference between these two different
>> "lpj" values. (The hacky patch is attached.) Below is the output for 100ms delay.
>>
>> [    2.494751] udelay_test uninitialized ---->start
>> [    2.494820] udelay_test uninitialized ---->end
>> [    2.494828] udelay_test initialized ---->start
>> [    2.595234] udelay_test initialized ---->end
>>
>> For 100ms delay, udelay() with uninitialized loops_per_jiffy only gives a delay of
>> only 69us.
> Ok, then could we add some simple calibration to make udelay work much better - or 
> perhaps move the udelay calibration up earlier?
>
> Hiding essentially an early udelay() implementation in an early-printk driver is 
> ugly and counterproductive.

Sure. How about below change?

diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index d3f0c84..940989e 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool read)
        return size;
 }
 
+static void __init xdbc_udelay_calibration(void)
+{
+       unsigned long lpj = 0;
+       unsigned int tsc_khz, cpu_khz;
+
+       if (!boot_cpu_has(X86_FEATURE_TSC))
+               goto calibration_out;
+
+       cpu_khz = x86_platform.calibrate_cpu();
+       tsc_khz = x86_platform.calibrate_tsc();
+
+       if (tsc_khz == 0)
+               tsc_khz = cpu_khz;
+       else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
+               cpu_khz = tsc_khz;
+
+       if (!tsc_khz)
+               goto calibration_out;
+
+       lpj = tsc_khz * 1000;
+       do_div(lpj, HZ);
+
+calibration_out:
+       if (!lpj)
+               lpj = 1 << 22;
+
+       loops_per_jiffy = lpj;
+}
+
 static int __init xdbc_early_setup(void)
 {
        int ret;
@@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
        }
        xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
 
+       xdbc_udelay_calibration();
+
        return 0;
 }

Best regards,
Lu Baolu

[toc] | [prev] | [next] | [standalone]

#1566407

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> > Hiding essentially an early udelay() implementation in an early-printk driver is 
> > ugly and counterproductive.
> 
> Sure. How about below change?
> 
> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
> index d3f0c84..940989e 100644
> --- a/drivers/usb/early/xhci-dbc.c
> +++ b/drivers/usb/early/xhci-dbc.c
> @@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool read)
>         return size;
>  }
>  
> +static void __init xdbc_udelay_calibration(void)
> +{
> +       unsigned long lpj = 0;
> +       unsigned int tsc_khz, cpu_khz;
> +
> +       if (!boot_cpu_has(X86_FEATURE_TSC))
> +               goto calibration_out;
> +
> +       cpu_khz = x86_platform.calibrate_cpu();
> +       tsc_khz = x86_platform.calibrate_tsc();
> +
> +       if (tsc_khz == 0)
> +               tsc_khz = cpu_khz;
> +       else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
> +               cpu_khz = tsc_khz;
> +
> +       if (!tsc_khz)
> +               goto calibration_out;
> +
> +       lpj = tsc_khz * 1000;
> +       do_div(lpj, HZ);
> +
> +calibration_out:
> +       if (!lpj)
> +               lpj = 1 << 22;
> +
> +       loops_per_jiffy = lpj;
> +}
> +
>  static int __init xdbc_early_setup(void)
>  {
>         int ret;
> @@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
>         }
>         xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
>  
> +       xdbc_udelay_calibration();
> +
>         return 0;
>  }

Yeah - so could we do this in a more generic fashion, not in the early-printk 
driver but in core x86 code?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1566438

On Wed, Jan 25, 2017 at 10:23:55AM +0100, Ingo Molnar wrote:
> 
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
> > > Hiding essentially an early udelay() implementation in an early-printk driver is 
> > > ugly and counterproductive.

> Yeah - so could we do this in a more generic fashion, not in the early-printk 
> driver but in core x86 code?

So ideally early_printk() would not depend on udelay() being setup.

In fact, ideally early_printk() wouldn't even use udelay -- this very
much includes its own copy.

Why is udelay() required? Can't the thing simply poll its own register
state to wait for completion?

This all sounds like xdbc cruft is still unreliably garbage..

[toc] | [prev] | [next] | [standalone]

#1566554

Hi,

On 01/25/2017 05:57 PM, Peter Zijlstra wrote:
> On Wed, Jan 25, 2017 at 10:23:55AM +0100, Ingo Molnar wrote:
>> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>>>> Hiding essentially an early udelay() implementation in an early-printk driver is 
>>>> ugly and counterproductive.
>> Yeah - so could we do this in a more generic fashion, not in the early-printk 
>> driver but in core x86 code?
> So ideally early_printk() would not depend on udelay() being setup.
>
> In fact, ideally early_printk() wouldn't even use udelay -- this very
> much includes its own copy.
>
> Why is udelay() required? Can't the thing simply poll its own register
> state to wait for completion?

In my driver, udelay() is mostly used to handle time out.

Xdbc hides most USB things in its firmware. Early printk driver only needs
to setup the registers/data structures and wait until link ready or time out.
Without udelay(), I have no means to convert the polling times into waiting
time.

Best regards,
Lu Baolu

>
> This all sounds like xdbc cruft is still unreliably garbage..
>

[toc] | [prev] | [next] | [standalone]

#1566649

On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote:
> In my driver, udelay() is mostly used to handle time out.
> 
> Xdbc hides most USB things in its firmware. Early printk driver only needs
> to setup the registers/data structures and wait until link ready or time out.
> Without udelay(), I have no means to convert the polling times into waiting
> time.

What is timeout and why? If there is an error other than !ready, I would
expect the hardware to inform you of this through another status bit,
no?

So why can't you poll indefinitely for either ready or error?

[toc] | [prev] | [next] | [standalone]

#1566707

Hi,

On 01/25/2017 10:38 PM, Peter Zijlstra wrote:
> On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote:
>> In my driver, udelay() is mostly used to handle time out.
>>
>> Xdbc hides most USB things in its firmware. Early printk driver only needs
>> to setup the registers/data structures and wait until link ready or time out.
>> Without udelay(), I have no means to convert the polling times into waiting
>> time.
> What is timeout and why?

Put it in simple:

The driver sets the RUN bit in control register and polls READY
bit in status register for the successful USB device enumeration.
As the USB device enumeration might fail and the READY bit will
never be set, the driver must have a timeout logic to avoid
endless loop.

More details:

The operational model is that driver sets up all necessary registers
and data structures, and then starts the debug engine by setting
the RUN/STOP bit in the control register.

The debug engine then brings up itself as a ready-for-enumeration
USB device. The USB link between host and device starts link training
and then host will detect the connected device. The hub driver in
host will then starts the USB device enumeration processes (as defined
in USB spec). If everything goes smoothly, the device gets enumerated
and host can talk with the debug device.

After that, xdbc firmware will set the READY bit in status register. And
the driver can go ahead with data transfer over USB.

>  If there is an error other than !ready, I would
> expect the hardware to inform you of this through another status bit,
> no?

Yeah, this might be another choice of hardware design. But it's not a
topic for this driver.

>
> So why can't you poll indefinitely for either ready or error?
>
>

Even if the hardware has both ready and error status bits, it's still
nice to have a time out watch dog. Buggy hardware or firmware
might not set any of these bits. Polling indefinitely might result in
a endless loop.

Best regards,
Lu Baolu

[toc] | [prev] | [next] | [standalone]

#1566715

On Wed, Jan 25, 2017 at 11:51:34PM +0800, Lu Baolu wrote:

> > What is timeout and why?
> 
> Put it in simple:
> 
> The driver sets the RUN bit in control register and polls READY
> bit in status register for the successful USB device enumeration.
> As the USB device enumeration might fail and the READY bit will
> never be set, the driver must have a timeout logic to avoid
> endless loop.
> 
> More details:
> 
> The operational model is that driver sets up all necessary registers
> and data structures, and then starts the debug engine by setting
> the RUN/STOP bit in the control register.
> 
> The debug engine then brings up itself as a ready-for-enumeration
> USB device. The USB link between host and device starts link training
> and then host will detect the connected device. The hub driver in
> host will then starts the USB device enumeration processes (as defined
> in USB spec). If everything goes smoothly, the device gets enumerated
> and host can talk with the debug device.
> 
> After that, xdbc firmware will set the READY bit in status register. And
> the driver can go ahead with data transfer over USB.

I have vague memories from a prior discussion where you said this READY
state can be lost at any time (cable unplug or whatnot) and at that
point the driver should re-start the setup, right?

> >  If there is an error other than !ready, I would
> > expect the hardware to inform you of this through another status bit,
> > no?
> 
> Yeah, this might be another choice of hardware design. But it's not a
> topic for this driver.

So is there really no way to way to distinguish between "I did setup and
am waiting for READY", "I did setup, am waiting for READY, but things
got hosed" and "I was READY, things be hosed" ?

I suppose the first and last can be distinguished by remembering if you
ever saw READY, but the first and second are the interesting case I
think.

> > So why can't you poll indefinitely for either ready or error?
> >
> 
> Even if the hardware has both ready and error status bits, it's still
> nice to have a time out watch dog. Buggy hardware or firmware
> might not set any of these bits. Polling indefinitely might result in
> a endless loop.

Loosing output, esp. without indication, is very _very_ annoying when
you're debugging things. Its just about on par with a stuck system, at
least then you know something bad happened.

[toc] | [prev] | [next] | [standalone]

#1567099

Hi,

On 01/26/2017 12:16 AM, Peter Zijlstra wrote:
> On Wed, Jan 25, 2017 at 11:51:34PM +0800, Lu Baolu wrote:
>
>>> What is timeout and why?
>> Put it in simple:
>>
>> The driver sets the RUN bit in control register and polls READY
>> bit in status register for the successful USB device enumeration.
>> As the USB device enumeration might fail and the READY bit will
>> never be set, the driver must have a timeout logic to avoid
>> endless loop.
>>
>> More details:
>>
>> The operational model is that driver sets up all necessary registers
>> and data structures, and then starts the debug engine by setting
>> the RUN/STOP bit in the control register.
>>
>> The debug engine then brings up itself as a ready-for-enumeration
>> USB device. The USB link between host and device starts link training
>> and then host will detect the connected device. The hub driver in
>> host will then starts the USB device enumeration processes (as defined
>> in USB spec). If everything goes smoothly, the device gets enumerated
>> and host can talk with the debug device.
>>
>> After that, xdbc firmware will set the READY bit in status register. And
>> the driver can go ahead with data transfer over USB.
> I have vague memories from a prior discussion where you said this READY
> state can be lost at any time (cable unplug or whatnot) and at that
> point the driver should re-start the setup, right?

Yes. So the documentation requires users not to unplug the usb
cable during debugging. This rule applies to other debug methods
as well.

>
>>>  If there is an error other than !ready, I would
>>> expect the hardware to inform you of this through another status bit,
>>> no?
>> Yeah, this might be another choice of hardware design. But it's not a
>> topic for this driver.
> So is there really no way to way to distinguish between "I did setup and
> am waiting for READY", "I did setup, am waiting for READY, but things
> got hosed" and "I was READY, things be hosed" ?
>
> I suppose the first and last can be distinguished by remembering if you
> ever saw READY, but the first and second are the interesting case I
> think.
>
>>> So why can't you poll indefinitely for either ready or error?
>>>
>> Even if the hardware has both ready and error status bits, it's still
>> nice to have a time out watch dog. Buggy hardware or firmware
>> might not set any of these bits. Polling indefinitely might result in
>> a endless loop.
> Loosing output, esp. without indication, is very _very_ annoying when
> you're debugging things. Its just about on par with a stuck system, at
> least then you know something bad happened.

Fair enough.

USB connection is stable enough, unless the user unplugs the
USB cable during debugging.

Best regards,
Lu Baolu

[toc] | [prev] | [next] | [standalone]

#1567136

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Fair enough.
> 
> USB connection is stable enough, unless the user unplugs the
> USB cable during debugging.

What does the hardware do in this case? The XHCI registers are in the host 
hardware, so they won't disappear, right? Is there some cable connection status 
bit we can extract without interrupts?

I.e. if there's any polling component then it would be reasonable to add an error 
component: poll the status and if it goes 'disconnected' then disable early-printk 
altogether in this case and trigger an emergency printk() so that there's chance 
that the user notices [if the system does not misbehave otherwise].

I.e. try to be as robust and informative as lockdep - yet don't lock up the host 
kernel: lockdep too is called from very deep internals, there are various 
conditions where it sees corrupt data structures (i.e. a 'disconnect' - a system 
environment outside the normal bounds of operation), yet of the kernel and over 
the last 10+ years of lockdep's existence we had very, very few cases of lockdep 
itself locking up and behaving unpredictably.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1567149

Hi Ingo,

On 01/26/2017 03:19 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Fair enough.
>>
>> USB connection is stable enough, unless the user unplugs the
>> USB cable during debugging.
> What does the hardware do in this case? The XHCI registers are in the host 
> hardware, so they won't disappear, right? Is there some cable connection status 
> bit we can extract without interrupts?

Yes, there are register bits for us to know the cable status. I will go
through the spec again and give you more accurate answer later.

I'm sorry. I will be off during the next 7 days for Chinese New Year
holiday. My email access will be very limited during this time. I will
revisit this thread after I am back from holiday.

Sorry for the inconvenience.

Best regards,
Lu Baolu

> I.e. if there's any polling component then it would be reasonable to add an error 
> component: poll the status and if it goes 'disconnected' then disable early-printk 
> altogether in this case and trigger an emergency printk() so that there's chance 
> that the user notices [if the system does not misbehave otherwise].
>
> I.e. try to be as robust and informative as lockdep - yet don't lock up the host 
> kernel: lockdep too is called from very deep internals, there are various 
> conditions where it sees corrupt data structures (i.e. a 'disconnect' - a system 
> environment outside the normal bounds of operation), yet of the kernel and over 
> the last 10+ years of lockdep's existence we had very, very few cases of lockdep 
> itself locking up and behaving unpredictably.
>
> Thanks,
>
> 	Ingo
>

[toc] | [prev] | [next] | [standalone]

#1567163

* Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Ingo,
> 
> On 01/26/2017 03:19 PM, Ingo Molnar wrote:
> > * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >
> >> Fair enough.
> >>
> >> USB connection is stable enough, unless the user unplugs the
> >> USB cable during debugging.
> > What does the hardware do in this case? The XHCI registers are in the host 
> > hardware, so they won't disappear, right? Is there some cable connection status 
> > bit we can extract without interrupts?
> 
> Yes, there are register bits for us to know the cable status. I will go
> through the spec again and give you more accurate answer later.

Ok, that's good news - so we don't really have to time out and we don't have to 
rely on the user holding the phone right either.

> I'm sorry. I will be off during the next 7 days for Chinese New Year
> holiday. My email access will be very limited during this time. I will
> revisit this thread after I am back from holiday.
> 
> Sorry for the inconvenience.

No problem, have fun!

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1567292

On Thu, Jan 26, 2017 at 08:19:37AM +0100, Ingo Molnar wrote:
> 
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> 
> > Fair enough.
> > 
> > USB connection is stable enough, unless the user unplugs the
> > USB cable during debugging.
> 
> What does the hardware do in this case? The XHCI registers are in the host 
> hardware, so they won't disappear, right? Is there some cable connection status 
> bit we can extract without interrupts?
> 
> I.e. if there's any polling component then it would be reasonable to add an error 
> component: poll the status and if it goes 'disconnected' then disable early-printk 
> altogether in this case and trigger an emergency printk() so that there's chance 
> that the user notices [if the system does not misbehave otherwise].

That'll be fun when printk() == early_printk() :-)

I myself wouldn't mind the system getting stuck until the link is
re-established. My own damn fault for taking that cable out etc.

[toc] | [prev] | [next] | [standalone]

#1567558

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jan 26, 2017 at 08:19:37AM +0100, Ingo Molnar wrote:
> > 
> > * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> > 
> > > Fair enough.
> > > 
> > > USB connection is stable enough, unless the user unplugs the
> > > USB cable during debugging.
> > 
> > What does the hardware do in this case? The XHCI registers are in the host 
> > hardware, so they won't disappear, right? Is there some cable connection status 
> > bit we can extract without interrupts?
> > 
> > I.e. if there's any polling component then it would be reasonable to add an error 
> > component: poll the status and if it goes 'disconnected' then disable early-printk 
> > altogether in this case and trigger an emergency printk() so that there's chance 
> > that the user notices [if the system does not misbehave otherwise].
> 
> That'll be fun when printk() == early_printk() :-)

My suggestion would be to just print into the printk buffer directly in this case, 
without console output - the developer will notice it in 'dmesg'.

> I myself wouldn't mind the system getting stuck until the link is
> re-established. My own damn fault for taking that cable out etc.

That's fine too, although beyond the obvious "yanked the cable without realizing 
it" case there are corner cases where usability is increased massively if the 
kernel is more proactive about error conditions: for example there are 
sub-standard USB cables and there are too long USB pathways from overloaded USB 
hubs which can result in intermittent behavior, etc.

A clear diagnostic message in 'dmesg' that the USB host controller is unhappy 
about the USB-debug dongle device is a _lot_ more useful when troubleshooting such 
problems than the occasional weird, non-deterministic hang...

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1567630

On Thu, Jan 26, 2017 at 05:01:05PM +0100, Ingo Molnar wrote:
> > > 
> > > I.e. if there's any polling component then it would be reasonable to add an error 
> > > component: poll the status and if it goes 'disconnected' then disable early-printk 
> > > altogether in this case and trigger an emergency printk() so that there's chance 
> > > that the user notices [if the system does not misbehave otherwise].
> > 
> > That'll be fun when printk() == early_printk() :-)
> 
> My suggestion would be to just print into the printk buffer directly in this case, 
> without console output - the developer will notice it in 'dmesg'.

When you map printk() onto early_printk() dmesg will be empty, there
will be nothing there, and therefore no reason what so ever to look
there. I certainly don't ever look there.

Note that the printk buffer itself is a major part of why printk sucks
donkey balls.  Not to mention that you really cannot have an
early_printk() implementation that depends on printk().

> > I myself wouldn't mind the system getting stuck until the link is
> > re-established. My own damn fault for taking that cable out etc.
> 
> That's fine too, although beyond the obvious "yanked the cable without realizing 
> it" case there are corner cases where usability is increased massively if the 
> kernel is more proactive about error conditions: for example there are 
> sub-standard USB cables and there are too long USB pathways from overloaded USB 
> hubs which can result in intermittent behavior, etc.
> 
> A clear diagnostic message in 'dmesg' that the USB host controller is unhappy 
> about the USB-debug dongle device is a _lot_ more useful when troubleshooting such 
> problems than the occasional weird, non-deterministic hang...

Sure, I'm just not sure what or where makes sense.

If your serial cable is bad you notice because you don't receive the
right amount of characters and or stuff gets mangled. You chuck the
cable and get a new one.

I think the most important part is re-establishing the link when the
cable gets re-inserted. Maybe we should just drop all characters written
when there's no link and leave it at that, same as serial.

[toc] | [prev] | [next] | [standalone]

#1567908

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Jan 26, 2017 at 05:01:05PM +0100, Ingo Molnar wrote:
> > > > 
> > > > I.e. if there's any polling component then it would be reasonable to add an error 
> > > > component: poll the status and if it goes 'disconnected' then disable early-printk 
> > > > altogether in this case and trigger an emergency printk() so that there's chance 
> > > > that the user notices [if the system does not misbehave otherwise].
> > > 
> > > That'll be fun when printk() == early_printk() :-)
> > 
> > My suggestion would be to just print into the printk buffer directly in this case, 
> > without console output - the developer will notice it in 'dmesg'.
> 
> When you map printk() onto early_printk() dmesg will be empty, there
> will be nothing there, and therefore no reason what so ever to look
> there.

Unless you want a third layer of a console driver putting the debug message into 
dmesg isn't all that bad of a solution.

Let's admit it: something like USB that involves external pieces of hardware 
_does_ have failure modes, and troubleshooting messages instead of indefinite 
hangs are obviously more robust.

> I certainly don't ever look there.

You'll have to teach yourself that if the box boots up fine but there are no 
messages whatsoever from the early-printk console that you'll need to look at 
dmesg output or the syslog for more clues.

This should not be a common occurrance in any case - but when it happens it's very 
useful to have diagnostic messages. I don't think this is a controversial point in 
any fashion.

> Note that the printk buffer itself is a major part of why printk sucks donkey 
> balls.  Not to mention that you really cannot have an early_printk() 
> implementation that depends on printk().

There are several easy solutions to do that, my favorite would be to put it into 
the printk buffer totally unlocked. When your early-printk is active it's unused 
and in the end it's a known data structure after all:

/*
 * Just zap whatever's in the printk buffer and put your emergency message into 
 * it, prominently. No locking, no worries - don't generate emergency messages
 * while printk is active and syslogd is running - this facility is a poor man's 
 * fallback printk() when early-printk has taken over all kernel logging:
 */
void printk_emergency_puts(const char *str)
{
	struct printk_log *msg, *msg_end;

	msg = log_buf;

	memset(msg, 0, sizeof(*msg));	
	msg.text_len = strlen(str);

	msg_end = (void *)msg + sizeof(*msg) + msg->text_len;

	/* Zero ->len denotes end of log buffer: */
	memset(msg_end, 0, sizeof(*msg_end));	

	snprintf(ptr, str);
}

...
	printk_emergency_puts"earlyprintk emergency: Hardware timed out, shutting down. Fix your debug cable?\n");
...

(Or so - totally untested, some details might be wrong.)

But yes, I agree with your wider point, I just looked at kernel/printk/printk.c 
and puked. Why did we merge that crappy piece of binary logging code, when we 
already have two other binary logging facilities in the kernel already, both of 
them better and cleaner than this?? Why did we mess up our nicely readable, 
simple, reliable ASCII log buffer printk code? :-(

> > > I myself wouldn't mind the system getting stuck until the link is 
> > > re-established. My own damn fault for taking that cable out etc.
> > 
> > That's fine too, although beyond the obvious "yanked the cable without 
> > realizing it" case there are corner cases where usability is increased 
> > massively if the kernel is more proactive about error conditions: for example 
> > there are sub-standard USB cables and there are too long USB pathways from 
> > overloaded USB hubs which can result in intermittent behavior, etc.
> > 
> > A clear diagnostic message in 'dmesg' that the USB host controller is unhappy 
> > about the USB-debug dongle device is a _lot_ more useful when troubleshooting 
> > such problems than the occasional weird, non-deterministic hang...
> 
> Sure, I'm just not sure what or where makes sense.
> 
> If your serial cable is bad you notice because you don't receive the right 
> amount of characters and or stuff gets mangled. You chuck the cable and get a 
> new one.
> 
> I think the most important part is re-establishing the link when the cable gets 
> re-inserted. Maybe we should just drop all characters written when there's no 
> link and leave it at that, same as serial.

That would be fine with me too - but even in this case there should be a stat 
counter somewhere (in /proc or /debug) that counts the number of characters 
dropped. Maybe that file could also display an emergency string - avoiding the 
interaction with the printk buffer.

We can do better than passive-aggressive logging behavior...

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web