usb: early: add support for early printk through USB3 debug port
Subsystems (4)
v7 cover usb: early: add support for early printk through USB3 debug port
xHCI debug capability (DbC) is an optional but standalone functionality provided by an xHCI host controller. With DbC hardware initialized, the system will present a debug device through the USB3 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 set adds support for early printk functionality through a USB3 debug port by 1) initializing and enabling the DbC hardware during early boot; 2) registering a boot console to the system so that early printk messages can go through the USB3 debug port. It also includes some lines of changes in usb_debug driver so that it can be bound when a USB3 debug device is enumerated.
---
Change log:v6->v7:
- add a new patch "[PATCH 1/5] x86: add simple udelay calibration" to make udelay() work for early drivers.
- [PATCH 2/5] usb: dbc: early driver for xhci debug capability
- add a kernel thread to handle error cases, such as cable unplugging.
- Fixed several code styles pointed out by Ingo.
v5->v6:
- rebase the patches on top of the latest 4.10-rc4
- run successfully in a 32-bit kernel
- [PATCH 1/4]
- remove ugly linebreaks to make code more readable
- rename config names to make them consistency
- move sleep-able ioremap() out of the lock area
- rename reserved fields of register structures
- make the vertical tabulation of struct fields consistent
- [PATCH 2/4]
- remove "#ifdef" in the generic code by creating proper wrappers in header file
- [PATCH 3/4]
- refine the title and commit message
- [PATCH 4/4]
- fix several typos and grammar errors in the document
v4->v5:
- add raw_spin_lock to make xdbc_bulk_write() reentrant.
v3->v4:
- Rename the document with .dst suffix.
- Add the list of hardware that has been succesfuly tested on in the document.
v2->v3:
- Removed spinlock usage.
- Removed work queue usage.
- Refined the user guide document.
v1->v2:
- Refactor the duplicate code in xdbc_early_start() and xdbc_handle_external_reset().
- Free resources when hardware not used any more.
- Refine the user guide document.
Lu Baolu (5):
x86: add simple udelay calibration
usb: dbc: early driver for xhci debug capability
x86: add support for earlyprintk via USB3 debug port
usb: serial: add dbc debug device support to usb_debug
usb: doc: add document for USB3 debug port usage
Documentation/admin-guide/kernel-parameters.txt | 1 + Documentation/usb/usb3-debug-port.rst | 98 +++ arch/x86/Kconfig.debug | 17 + arch/x86/kernel/early_printk.c | 5 + arch/x86/kernel/setup.c | 26 + drivers/usb/Kconfig | 3 + drivers/usb/Makefile | 2 +- drivers/usb/early/Makefile | 1 + drivers/usb/early/xhci-dbc.c | 1026 +++++++++++++++++++++++ drivers/usb/early/xhci-dbc.h | 210 +++++ drivers/usb/serial/usb_debug.c | 28 +- include/linux/usb/xhci-dbgp.h | 29 + 12 files changed, 1442 insertions(+), 4 deletions(-) create mode 100644 Documentation/usb/usb3-debug-port.rst 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
Signature · 1 line
2.1.4
PATCH v7 1/5 x86: add simple udelay calibration
Add a simple udelay calibration in x86 architecture-specific boot-time initializations. This will get a workable estimate for loops_per_jiffy. Hence, udelay() could be used after this initialization.
--- arch/x86/kernel/setup.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
arch/x86/kernel/setup.c +22 -0
@@ -835,6 +835,26 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)return 0 ; } +static void __init simple_udelay_calibration (void ) +{ +unsigned int tsc_khz, cpu_khz; +unsigned long lpj; + +if (!boot_cpu_has (X86_FEATURE_TSC )) +return ; + + cpu_khz = x86_platform.calibrate_cpu (); + tsc_khz = x86_platform.calibrate_tsc (); + + tsc_khz = tsc_khz ? : cpu_khz; +if (!tsc_khz) +return ; + + lpj = tsc_khz *1000 ; +do_div (lpj,HZ ); + loops_per_jiffy = lpj; +} +/* * Determine if we were loaded by an EFI loader. If so, then we have also been * passed the efi memmap, systab, etc., so we should use these data structures
@@ -983,6 +1003,8 @@ void __init setup_arch(char **cmdline_p)*/ x86_configure_nx (); +simple_udelay_calibration (); +parse_early_param ();#ifdef CONFIG_MEMORY_HOTPLUG
Signature · 1 line
2.1.4
Re: [PATCH v7 1/5] x86: add simple udelay calibration
Hello!
Quoted 22 lines · Lu Baolu
Add a simple udelay calibration in x86 architecture-specific boot-time initializations. This will get a workable estimate for
loops_per_jiffy. Hence,udelay()could be used after this initialization.--- arch/x86/kernel/setup.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)arch/x86/kernel/setup.c +2
-0@@ -835,6 +835,26 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)return 0 ; } +static void __init simple_udelay_calibration (void ) +{
arch/x86/kernel/setup.c dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) · line 849
Quoted 10 lines
arch/x86/kernel/setup.c +10
-0+unsigned int tsc_khz, cpu_khz; +unsigned long lpj; + +if (!boot_cpu_has (X86_FEATURE_TSC )) +return ; + + cpu_khz = x86_platform.calibrate_cpu (); + tsc_khz = x86_platform.calibrate_tsc (); + + tsc_khz = tsc_khz ? : cpu_khz;
Why not:
if (!tsc_khz) tsc_khz = cpu_khz;
It's more clear IMHO.
arch/x86/kernel/setup.c dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) · line 860
Quoted 11 lines
arch/x86/kernel/setup.c +8
-0+if (!tsc_khz) +return ; + + lpj = tsc_khz *1000 ; +do_div (lpj,HZ ); + loops_per_jiffy = lpj; +} +/* * Determine if we were loaded by an EFI loader. If so, then we have also been * passed the efi memmap, systab, etc., so we should use these data structures
[...]
MBR, Sergei
Re: [PATCH v7 1/5] x86: add simple udelay calibration
Hi,
Quoted 35 lines · Sergei Shtylyov
Hello!
Quoted 31 lines · Lu Baolu
Add a simple udelay calibration in x86 architecture-specific boot-time initializations. This will get a workable estimate for
loops_per_jiffy. Hence,udelay()could be used after this initialization.--- arch/x86/kernel/setup.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)arch/x86/kernel/setup.c +11
-0@@ -835,6 +835,26 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)return 0 ; } +static void __init simple_udelay_calibration (void ) +{ +unsigned int tsc_khz, cpu_khz; +unsigned long lpj; + +if (!boot_cpu_has (X86_FEATURE_TSC )) +return ; + + cpu_khz = x86_platform.calibrate_cpu (); + tsc_khz = x86_platform.calibrate_tsc (); +
Quoted 8 lines
arch/x86/kernel/setup.c dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) · line 849
Quoted 1 line
arch/x86/kernel/setup.c +1
-0+ tsc_khz = tsc_khz ? : cpu_khz;Why not: if (!tsc_khz) tsc_khz = cpu_khz; It's more clear IMHO.
Sure.
Best regards,
Lu Baolu
Quoted 17 lines
arch/x86/kernel/setup.c dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) · line 860
Quoted 12 lines
arch/x86/kernel/setup.c +8
-0+if (!tsc_khz) +return ; + + lpj = tsc_khz *1000 ; +do_div (lpj,HZ ); + loops_per_jiffy = lpj; +} +/* * Determine if we were loaded by an EFI loader. If so, then we have also been * passed the efi memmap, systab, etc., so we should use these data structures [...]
MBR, Sergei
PATCH v7 2/5 usb: dbc: early driver for xhci debug capability
In reply to Lu BaoluxHCI 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 the xHCI host. There isn't any precondition from the xHCI host side for the DbC to work.
This patch also includes bulk output and bulk input interfaces. These interfaces could be used to implement early printk bootconsole or hook to various system debuggers.
--- arch/x86/Kconfig.debug | 17 + drivers/usb/Kconfig | 3 + drivers/usb/Makefile | 2 +- drivers/usb/early/Makefile | 1 + drivers/usb/early/xhci-dbc.c | 1026 +++++++++++++++++++++++++++++++++++++++++ drivers/usb/early/xhci-dbc.h | 210 +++++++++ include/linux/usb/xhci-dbgp.h | 29 ++ 7 files changed, 1287 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
arch/x86/Kconfig.debug +17 -0
@@ -5,6 +5,9 @@ config TRACE_IRQFLAGS_SUPPORT source "lib/Kconfig.debug" +config EARLY_PRINTK_USB + bool + config X86_VERBOSE_BOOTUP bool "Enable verbose x86 bootup info messages" default y
@@ -29,6 +32,7 @@ config EARLY_PRINTK config EARLY_PRINTK_DBGP bool "Early printk via EHCI debug port" depends on EARLY_PRINTK && PCI + select EARLY_PRINTK_USB ---help--- Write kernel log output directly into the EHCI debug port.
@@ -48,6 +52,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_USB_XDBC + bool "Early printk via the xHCI debug port" + 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
drivers/usb/Kconfig +3 -0
@@ -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
drivers/usb/Makefile +1 -1
@@ -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/
drivers/usb/early/Makefile +1 -0
@@ -3,3 +3,4 @@ # obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o +obj-$(CONFIG_EARLY_PRINTK_USB_XDBC) += xhci-dbc.o
drivers/usb/early/xhci-dbc.c new file +1026 -0
1,026 lines not on this page · raw message
drivers/usb/early/xhci-dbc.h new file +210 -0
210 lines not on this page · raw message
include/linux/usb/xhci-dbgp.h new file +29 -0
@@ -0,0 +1,29 @@ +/* +* Standalone xHCI debug capability driver +* +* Copyright (C) 2016 Intel Corporation +* +* Author: Lu Baolu <[email protected]> +* +* 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. +*/ + +#ifndef __LINUX_XHCI_DBGP_H +#define __LINUX_XHCI_DBGP_H + +#ifdef CONFIG_EARLY_PRINTK_USB_XDBC +int __init early_xdbc_parse_parameter (char *s); +int __init early_xdbc_setup_hardware (void ); +void __init early_xdbc_register_console (void ); +#else +static inline int __init early_xdbc_setup_hardware (void ) +{ +return -ENODEV ; +} +static inline void __init early_xdbc_register_console (void ) +{ +} +#endif /* CONFIG_EARLY_PRINTK_USB_XDBC */ +#endif /* __LINUX_XHCI_DBGP_H */
Signature · 1 line
2.1.4
PATCH v7 3/5 x86: add support for earlyprintk via USB3 debug port
In reply to Lu BaoluAdd support for early printk by writing debug messages to the
USB3 debug port. Users can use this type of early printk by
specifying kernel parameter of "earlyprintk=xdbc". This gives users a chance of providing debug output.
The hardware for USB3 debug port requires DMA memory blocks. This requires to delay setting up debugging hardware and registering boot console until the memblocks are filled.
--- Documentation/admin-guide/kernel-parameters.txt | 1 + arch/x86/kernel/early_printk.c | 5 +++++ arch/x86/kernel/setup.c | 4 ++++ 3 files changed, 10 insertions(+)
Documentation/admin-guide/kernel-parameters.txt +1 -0
@@ -988,6 +988,7 @@ earlyprintk=ttySn[,baudrate] earlyprintk=dbgp[debugController#] earlyprintk=pciserial,bus:device.function[,baudrate] + earlyprintk=xdbc[xhciController#] earlyprintk is useful when the kernel crashes before the normal console is initialized. It is not enabled by
arch/x86/kernel/early_printk.c +5 -0
@@ -17,6 +17,7 @@#include <asm/intel-mid.h> #include <asm/pgtable.h> #include <linux/usb/ehci_def.h> +#include <linux/usb/xhci-dbgp.h> #include <linux/efi.h> #include <asm/efi.h> #include <asm/pci_x86.h>
@@ -381,6 +382,10 @@ static int __init setup_early_printk(char *buf)if (!strncmp (buf,"efi" ,3 ))early_console_register (&early_efi_console, keep);#endif +#ifdef CONFIG_EARLY_PRINTK_USB_XDBC +if (!strncmp (buf,"xdbc" ,4 )) +early_xdbc_parse_parameter (buf +4 ); +#endif buf++; }
arch/x86/kernel/setup.c +4 -0
@@ -70,6 +70,7 @@#include <linux/tboot.h> #include <linux/jiffies.h> +#include <linux/usb/xhci-dbgp.h> #include <video/edid.h> #include <asm/mtrr.h>
@@ -1142,6 +1143,9 @@ void __init setup_arch(char **cmdline_p)memblock_set_current_limit (ISA_END_ADDRESS );memblock_x86_fill (); +if (!early_xdbc_setup_hardware ()) +early_xdbc_register_console (); +reserve_bios_regions ();if (efi_enabled (EFI_MEMMAP )) {
Signature · 1 line
2.1.4
PATCH v7 4/5 usb: serial: add dbc debug device support to usb_debug
In reply to Lu BaoluThis patch adds dbc debug device support to the usb_debug driver.
--- drivers/usb/serial/usb_debug.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)
drivers/usb/serial/usb_debug.c +25 -3
@@ -32,7 +32,18 @@ static const struct usb_device_id id_table[] = { { USB_DEVICE(0x0525, 0x127a) }, { }, }; -MODULE_DEVICE_TABLE (usb, id_table); + +static const struct usb_device_id dbc_id_table[] = { + {USB_DEVICE (0x1d6b ,0x0004 ) }, + { }, +}; + +static const struct usb_device_id id_table_combined[] = { + {USB_DEVICE (0x0525 ,0x127a ) }, + {USB_DEVICE (0x1d6b ,0x0004 ) }, + { }, +}; +MODULE_DEVICE_TABLE (usb, id_table_combined);/* This HW really does not support a serial break, so one will be * emulated when ever the break state is set to true.
@@ -71,9 +82,20 @@ static struct usb_serial_driver debug_device = { .process_read_urb = usb_debug_process_read_urb, }; +static struct usb_serial_driver dbc_device = { + .driver = { + .owner =THIS_MODULE , + .name ="xhci_dbc" , + }, + .id_table = dbc_id_table, + .num_ports =1 , + .break_ctl = usb_debug_break_ctl, + .process_read_urb = usb_debug_process_read_urb, +}; +static struct usb_serial_driver *const serial_drivers[] = { - &debug_device,NULL + &debug_device, &dbc_device,NULL }; -module_usb_serial_driver (serial_drivers, id_table); +module_usb_serial_driver (serial_drivers, id_table_combined);MODULE_LICENSE ("GPL" );
Signature · 1 line
2.1.4
PATCH v7 5/5 usb: doc: add document for USB3 debug port usage
In reply to Lu BaoluAdd Documentation/usb/usb3-debug-port.rst. This document includes the user guide for USB3 debug port.
--- Documentation/usb/usb3-debug-port.rst | 98 +++++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/usb/usb3-debug-port.rst
Documentation/usb/usb3-debug-port.rst new file +98 -0
@@ -0,0 +1,98 @@ +=============== +USB3 debug port +=============== + +:Author: Lu Baolu <[email protected]> +:Date: January 2017 + +GENERAL +======= + +This is a HOWTO for using USB3 debug port on x86 systems. + +Before using any kernel debugging functionality based on USB3 +debug port, you need to check 1) whether debug port is supported +by the xHCI host; 2) which port is used for debugging purposes +(normally the first USB3 root port). You must have a USB 3.0 +super-speed A-to-A debugging cable to connect the debug target +with a debug host. In this document, "debug target" stands for +the system under debugging, and "debug host" stands for a +stand-alone system that is able to talk to the debugging target +through the USB3 debug port. + +EARLY PRINTK +============ + +On the debug target system, you need to customize a debugging +kernel with CONFIG_EARLY_PRINTK_USB_XDBC enabled. And, add +below kernel boot parameter:: + + "earlyprintk=xdbc" + +If there are multiple xHCI controllers in the system, you can +append a host contoller index to this kernel parameter. This +index starts from 0. + +If you are going to use the "keep" option defined by the +early printk framework to keep the boot console alive after +early boot, you'd better add below kernel boot parameter:: + + "usbcore.autosuspend=-1" + +On the debug host side, you don't need to customize the kernel, +but you'd better disable usb subsystem runtime power management +by adding below kernel boot parameter:: + + "usbcore.autosuspend=-1" + +Before starting the debug target, you should connect the debug +port on the debug target with a root port or port of any external +hub on the debug host. The cable used to connect these two ports +should be a USB 3.0 super-speed A-to-A debugging cable. + +During early boot of the debug target, DbC (xHCI debug engine for +USB3 debug port) hardware will be detected and initialized. After +initialization, the debug host should be able to enumerate the +debug target as a debug device. The debug host will then bind the +debug device with the usb_debug driver module and create the +/dev/ttyUSB0 device. + +If the debug device enumeration goes smoothly, you should be able +to see below kernel messages on the debug host:: + + # tail -f /var/log/kern.log + [ 1815.983374] usb 4-3: new SuperSpeed USB device number 4 using xhci_hcd + [ 1815.999595] usb 4-3: LPM exit latency is zeroed, disabling LPM. + [ 1815.999899] usb 4-3: New USB device found, idVendor=1d6b, idProduct=0004 + [ 1815.999902] usb 4-3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 + [ 1815.999903] usb 4-3: Product: Remote GDB + [ 1815.999904] usb 4-3: Manufacturer: Linux + [ 1815.999905] usb 4-3: SerialNumber: 0001 + [ 1816.000240] usb_debug 4-3:1.0: xhci_dbc converter detected + [ 1816.000360] usb 4-3: xhci_dbc converter now attached to ttyUSB0 + +You can run below bash scripts on the debug host to read the kernel +log sent from debug target. + +.. code-block:: sh + + ===== start of bash scripts ============= + #!/bin/bash + + while true ; do + while [ ! -d /sys/class/tty/ttyUSB0 ] ; do + : + done + cat /dev/ttyUSB0 >> xdbc.log + done + ===== end of bash scripts =============== + +You should be able to see the early boot message in xdbc.log. + +If it doesn't work, please ask it on the <[email protected]> +mailing list. + +Below USB hosts have been verified to work:: + + Intel Corporation Sunrise Point-H USB 3.0 xHCI Controller + Intel Corporation Wildcat Point-LP USB xHCI Controller
Signature · 1 line
2.1.4
A message that is not in the archive (sent off-list or not yet received): CAL411-o5xaA+awYi9zEZog1zCZvMCvNQ0i0R7yh24_zrTuu4gQ@mail.gmail.com
Re: [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage
Hi,
Quoted 61 lines · Peter Chen
Quoted 59 lines · Lu Baolu
Add
Documentation/usb/usb3-debug-port.rst. This document includes the user guide for USB3 debug port.Signed-off-by Lu Baolu <[email protected]>--- Documentation/usb/usb3-debug-port.rst | 98 ++++++++++++++++++++++++++++++ +++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/usb/usb3-debug-port.rstDocumentation/usb/usb3-debug-port.rst new file +41
-0@@ -0,0 +1,98 @@ +=============== +USB3 debug port +=============== + +:Author: Lu Baolu <[email protected]> +:Date: January 2017 + +GENERAL +======= + +This is a HOWTO for using USB3 debug port on x86 systems. + +Before using any kernel debugging functionality based on USB3 +debug port, you need to check 1) whether debug port is supported +by the xHCI host; 2) which port is used for debugging purposes +(normally the first USB3 root port). You must have a USB 3.0 +super-speed A-to-A debugging cable to connect the debug target +with a debug host. In this document, "debug target" stands for +the system under debugging, and "debug host" stands for a +stand-alone system that is able to talk to the debugging target +through the USB3 debug port. + +EARLY PRINTK +============ + +On the debug target system, you need to customize a debugging +kernel with CONFIG_EARLY_PRINTK_USB_XDBC enabled. And, add +below kernel boot parameter:: + + "earlyprintk=xdbc" + +If there are multiple xHCI controllers in the system, you can +append a host contoller index to this kernel parameter. This +index starts from 0. + +If you are going to use the "keep" option defined by the +early printk framework to keep the boot console alive after +early boot, you'd better add below kernel boot parameter:: + + "usbcore.autosuspend=-1" +
Quoted 8 lines
Documentation/usb/usb3-debug-port.rst line 47
Quoted 7 lines
Documentation/usb/usb3-debug-port.rst new file +6
-0+On the debug host side, you don't need to customize the kernel, +but you'd better disable usb subsystem runtime power management +by adding below kernel boot parameter:: + + "usbcore.autosuspend=-1" +Just curious, why autosuspend needs to be disabled for this function?
This implementation doesn't support suspend/resume yet.
Best regards,
Lu Baolu
Quoted 4 lines
BR,
Peter Chen
RE: [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage
Quoted 73 lines · Peter Chen
Quoted 61 lines · Lu Baolu
Quoted 60 lines
Add
Documentation/usb/usb3-debug-port.rst. This document includes the user guide for USB3 debug port.Signed-off-by Lu Baolu <[email protected]>--- Documentation/usb/usb3-debug-port.rst | 98 ++++++++++++++++++++++++++++++ +++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/usb/usb3-debug-port.rstDocumentation/usb/usb3-debug-port.rst new file +40
-0@@ -0,0 +1,98 @@ +=============== +USB3 debug port +=============== + +:Author: Lu Baolu <[email protected]> +:Date: January 2017 + +GENERAL +======= + +This is a HOWTO for using USB3 debug port on x86 systems. + +Before using any kernel debugging functionality based on USB3 debug +port, you need to check 1) whether debug port is supported by the +xHCI host; 2) which port is used for debugging purposes (normally +the first USB3 root port). You must have a USB 3.0 super-speed +A-to-A debugging cable to connect the debug target with a debug +host. In this document, "debug target" stands for the system under +debugging, and "debug host" stands for a stand-alone system that is +able to talk to the debugging target through the USB3 debug port. + +EARLY PRINTK +============ + +On the debug target system, you need to customize a debugging kernel +with CONFIG_EARLY_PRINTK_USB_XDBC enabled. And, add below kernel +boot parameter:: + + "earlyprintk=xdbc" + +If there are multiple xHCI controllers in the system, you can append +a host contoller index to this kernel parameter. This index starts +from 0. + +If you are going to use the "keep" option defined by the early +printk framework to keep the boot console alive after early boot, +you'd better add below kernel boot parameter:: + + "usbcore.autosuspend=-1" +Quoted 8 lines
Documentation/usb/usb3-debug-port.rst line 41
Quoted 7 lines
Documentation/usb/usb3-debug-port.rst new file +6
-0+On the debug host side, you don't need to customize the kernel, but +you'd better disable usb subsystem runtime power management by +adding below kernel boot parameter:: + + "usbcore.autosuspend=-1" +Just curious, why autosuspend needs to be disabled for this function?
Quoted 1 line
This implementation doesn't support suspend/resume yet.
Why host side needs to disable it too?
Peter
Re: [PATCH v7 5/5] usb: doc: add document for USB3 debug port usage
Hi,
Quoted 71 lines · Peter Chen
Quoted 71 lines · Peter Chen
Quoted 69 lines · Lu Baolu
Quoted 57 lines
Add
Documentation/usb/usb3-debug-port.rst. This document includes the user guide for USB3 debug port.Signed-off-by Lu Baolu <[email protected]>--- Documentation/usb/usb3-debug-port.rst | 98 ++++++++++++++++++++++++++++++ +++++ 1 file changed, 98 insertions(+) create mode 100644 Documentation/usb/usb3-debug-port.rstDocumentation/usb/usb3-debug-port.rst new file +36
-0@@ -0,0 +1,98 @@ +=============== +USB3 debug port +=============== + +:Author: Lu Baolu <[email protected]> +:Date: January 2017 + +GENERAL +======= + +This is a HOWTO for using USB3 debug port on x86 systems. + +Before using any kernel debugging functionality based on USB3 debug +port, you need to check 1) whether debug port is supported by the +xHCI host; 2) which port is used for debugging purposes (normally +the first USB3 root port). You must have a USB 3.0 super-speed +A-to-A debugging cable to connect the debug target with a debug +host. In this document, "debug target" stands for the system under +debugging, and "debug host" stands for a stand-alone system that is +able to talk to the debugging target through the USB3 debug port. + +EARLY PRINTK +============ + +On the debug target system, you need to customize a debugging kernel +with CONFIG_EARLY_PRINTK_USB_XDBC enabled. And, add below kernel +boot parameter:: + + "earlyprintk=xdbc" + +If there are multiple xHCI controllers in the system, you can append +a host contoller index to this kernel parameter. This index starts +from 0. + +If you are going to use the "keep" option defined by the early +printk framework to keep the boot console alive after early boot,Documentation/usb/usb3-debug-port.rst line 46
Quoted 10 lines
Documentation/usb/usb3-debug-port.rst new file +10
-0+you'd better add below kernel boot parameter:: + + "usbcore.autosuspend=-1" + +On the debug host side, you don't need to customize the kernel, but +you'd better disable usb subsystem runtime power management by +adding below kernel boot parameter:: + + "usbcore.autosuspend=-1" +Just curious, why autosuspend needs to be disabled for this function?
Quoted 3 lines
Quoted 1 line
This implementation doesn't support suspend/resume yet.
Why host side needs to disable it too?
If host side runtime suspend/resume is enabled, it might ask the debug device to suspend. This will cause the debug device dead.
The suspend/resume of the debug device depends on xhci driver. I will make it happen in separated patches with more tests. It's in my TODO list.
Best regards,
Lu Baolu
Hi Ingo,
How about this version? Any further comments?
Best regards,
Lu Baolu
Quoted 89 lines · Lu Baolu
xHCI debug capability (DbC) is an optional but standalone functionality provided by an xHCI host controller. With DbC hardware initialized, the system will present a debug device through the USB3 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 set adds support for early printk functionality through a USB3 debug port by 1) initializing and enabling the DbC hardware during early boot; 2) registering a boot console to the system so that early printk messages can go through the USB3 debug port. It also includes some lines of changes in
usb_debugdriver so that it can be bound when a USB3 debug device is enumerated.---
Change log:v6->v7:
- add a new patch "[PATCH 1/5] x86: add simple udelay calibration" to makeudelay()work for early drivers.
- [PATCH 2/5] usb: dbc: early driver for xhci debug capability
- add a kernel thread to handle error cases, such as cable unplugging.
- Fixed several code styles pointed out by Ingo.
v5->v6:
- rebase the patches on top of the latest 4.10-rc4
- run successfully in a 32-bit kernel
- [PATCH 1/4]
- remove ugly linebreaks to make code more readable
- rename config names to make them consistency
- move sleep-ableioremap()out of the lock area
- rename reserved fields of register structures
- make the vertical tabulation of struct fields consistent
- [PATCH 2/4]
- remove "#ifdef" in the generic code by creating proper wrappers in header file
- [PATCH 3/4]
- refine the title and commit message
- [PATCH 4/4]
- fix several typos and grammar errors in the document
v4->v5:
- addraw_spin_lockto makexdbc_bulk_write()reentrant.
v3->v4:
- Rename the document with .dst suffix.
- Add the list of hardware that has been succesfuly tested on in the document.
v2->v3:
- Removed spinlock usage.
- Removed work queue usage.
- Refined the user guide document.
v1->v2:
- Refactor the duplicate code inxdbc_early_start()andxdbc_handle_external_reset().
- Free resources when hardware not used any more.
- Refine the user guide document.Lu Baolu (5):
x86: add simple udelay calibration
usb: dbc: early driver for xhci debug capability
x86: add support for earlyprintk via USB3 debug port
usb: serial: add dbc debug device support tousb_debug
usb: doc: add document for USB3 debug port usageDocumentation/admin-guide/kernel-parameters.txt | 1 + Documentation/usb/usb3-debug-port.rst | 98 +++ arch/x86/Kconfig.debug | 17 + arch/x86/kernel/early_printk.c | 5 + arch/x86/kernel/setup.c | 26 + drivers/usb/Kconfig | 3 + drivers/usb/Makefile | 2 +- drivers/usb/early/Makefile | 1 + drivers/usb/early/xhci-dbc.c | 1026 +++++++++++++++++++++++ drivers/usb/early/xhci-dbc.h | 210 +++++ drivers/usb/serial/usb_debug.c | 28 +- include/linux/usb/xhci-dbgp.h | 29 + 12 files changed, 1442 insertions(+), 4 deletions(-) create mode 100644 Documentation/usb/usb3-debug-port.rst 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
Quoted 3 lines · Lu Baolu
Hi Ingo,
How about this version? Any further comments?
So I have re-read the review feedback I gave on Jan 19 and found at least one thing I pointed out that you didn't address in the latest patches ...
Plus please go beyond the feedback given - for example the Kconfig text contains at least one typo.
Thanks,
Ingo
Hi Ingo,
Quoted 7 lines · Ingo Molnar
Quoted 3 lines · Lu Baolu
Hi Ingo,
How about this version? Any further comments?
So I have re-read the review feedback I gave on Jan 19 and found at least one thing I pointed out that you didn't address in the latest patches ...
Do you mind telling me which one is not addressed? Is it one of below feedbacks?
==============================
Quoted 3 lines
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.
-----------------------------------------------------------------
Quoted 9 lines
Quoted 7 lines
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.
Is there any error status available in the host registers anywhere that tells us that enumeration did not succeed?
No, there isn't. The xhci spec requires software to impose a timeout.
Page 425, xhci specification:
"
Software shall impose a timeout between the detection of the Debug Host connection and the DbC Run transition to ‘1’.
"
-----------------------------------------------------------------
Review
Quoted 4 lines
(quoted diff) +4
-0+ + 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?
-----------------------------------------------------------------
Review
Quoted 8 lines
(quoted diff) +8
-0+ 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?
==============================
I posted all feedbacks I received by now at the tail of this email to make sure all feedbacks were communicated and addressed.
Quoted 5 lines
Plus please go beyond the feedback given - for example the
Kconfigtext contains at least one typo.
I am sorry about this.
I will try my best to make typos and code style clean.
Best regards,
Lu Baolu
[Below are all feedbacks.]
=================================================================
[PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
=================================================================
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.
-----------------------------------------------------------------
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?
-----------------------------------------------------------------
Review
Quoted 2 lines
(quoted diff) +2
-0+ if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == + PCI_BASE_ADDRESS_MEM_TYPE_64) {
Please don't break the line here.
-----------------------------------------------------------------
Review
Quoted 3 lines
(quoted diff) +3
-0+ val64 |= ((u64)val << 32); + sz64 |= ((u64)sz << 32); + mask64 |= ((u64)~0 << 32);
Unnecessary parentheses.
-----------------------------------------------------------------
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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?
-----------------------------------------------------------------
Review
Quoted 5 lines
(quoted diff) +5
-0+ 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.
-----------------------------------------------------------------
Quoted 1 line
+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.
-----------------------------------------------------------------
Review
Quoted 2 lines
(quoted diff) +2
-0+static int handshake(void __iomem *ptr, u32 mask, u32 done, + int wait, int delay)
Please break lines more intelligently:
static inthandshake(void __iomem *ptr, u32 mask, u32 done, int wait, int delay)
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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:
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
-----------------------------------------------------------------
Review
Quoted 3 lines
(quoted diff) +3
-0+ /* + * 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
?
-----------------------------------------------------------------
Review
Quoted 4 lines
(quoted diff) +4
-0+ */ + if (early_xdbc_console.index == -1 || + (early_xdbc_console.flags & CON_BOOT)) { + xdbc_trace("hardware not used any more\n");
s/any more
anymore
-----------------------------------------------------------------
Review
Quoted 2 lines
(quoted diff) +2
-0+ raw_spin_lock_irqsave(&xdbc.lock, flags); + base = ioremap_nocache(xdbc.xhci_start, xdbc.xhci_length);
Ugh, ioremap() can sleep ...
-----------------------------------------------------------------
Review
Quoted 8 lines
(quoted diff) +8
-0+/** + * 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.
Review
Quoted 5 lines
(quoted diff) +3
-0+ __le32 rsvd; + __le32 rsvdz[7]; + __le32 rsvd0[11];
ditto.
-----------------------------------------------------------------
Review
Quoted 7 lines
(quoted diff) +7
-0+#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.
-----------------------------------------------------------------
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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
?
-----------------------------------------------------------------
Review
Quoted 11 lines
(quoted diff) +11
-0+ /* 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.
-----------------------------------------------------------------
Quoted 3 lines
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.
-----------------------------------------------------------------
Quoted 9 lines
Quoted 7 lines
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.
Is there any error status available in the host registers anywhere that tells us that enumeration did not succeed?
No, there isn't. The xhci spec requires software to impose a timeout.
Page 425, xhci specification:
"
Software shall impose a timeout between the detection of the Debug Host connection and the DbC Run transition to ‘1’.
"
-----------------------------------------------------------------
Quoted 3 lines
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
-----------------------------------------------------------------
Quoted 3 lines
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
-----------------------------------------------------------------
Review
Quoted 2 lines
(quoted diff) +2
-0+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'?
-----------------------------------------------------------------
Review
Quoted 4 lines
(quoted diff) +4
-0+ + 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?
-----------------------------------------------------------------
Review
Quoted 8 lines
(quoted diff) +8
-0+ 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?
-----------------------------------------------------------------
Quoted 1 line
+ mask64 |= (u64)~0 << 32;
Type cast could be avoided by using 0L.
-----------------------------------------------------------------
Quoted 8 lines
(quoted diff) +8
-0+ 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)
Review
Quoted 10 lines
(quoted diff) +10
-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.
-----------------------------------------------------------------
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
-----------------------------------------------------------------
Review
Quoted 7 lines
(quoted diff) +7
-0+ /* 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);?
-----------------------------------------------------------------
Quoted 1 line
+ /* 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: */
-----------------------------------------------------------------
Review
Quoted 10 lines
(quoted diff) +10
-0+#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.
=================================================================
[PATCH v5 2/4] x86: add support for earlyprintk via USB3 debug port
=================================================================
Quoted 10 lines
(quoted diff) +5
-1+#ifdef CONFIG_EARLY_PRINTK_XDBC + if (!early_xdbc_setup_hardware()) + early_xdbc_register_console(); +#endif + reserve_bios_regions(); if (efi_enabled(EFI_MEMMAP)) { --2.1.4
Please create proper wrappers in xhci-dbgp.h to not litter generic code with these #ifdefs.
-----------------------------------------------------------------
=================================================================
[PATCH v5 3/4] usb: serial: usb_debug: add support for dbc debug device
=================================================================
Quoted 1 line
This patch add dbc debug device support in
usb_debugdriver.
s/add dbc debug device support in usb_debug driver
/adds dbc debug device support to the usb_debug driver
Please fix the title as well.
=================================================================
[PATCH v5 4/4] usb: doc: add document for USB3 debug port usage
=================================================================
-----------------------------------------------------------------
s/debugging functionalities
/debugging functionality
-----------------------------------------------------------------
s/debugging purpose
/debugging purposes
-----------------------------------------------------------------
s/On debug target system
On the debug target system
[End of feedback list.]
Quoted 5 lines · Lu Baolu
Hi Ingo,
Quoted 2 lines · Ingo Molnar
* Lu Baolu <[email protected]> wrote:
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Hi Ingo,
How about this version? Any further comments?
So I have re-read the review feedback I gave on Jan 19 and found at least one thing I pointed out that you didn't address in the latest patches ...
Do you mind telling me which one is not addressed? Is it one of below feedbacks?
So one piece of feedback I gave was:
| BTW., just a side note, some kernel developers (like PeterZ - and I do it | sometimes too) remap early_printk to printk permanently and use it as their main | printk facility - because printk() reliability has suffered over the last couple | of years. | | So it's more than just early boot debugging - it's a very simple state-less | logging facility to an external computer.
But the latest Kconfig help text still says this:
+config EARLY_PRINTK_USB_XDBC
+ bool "Early printk via the xHCI debug port" + 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.
... while in reality it's an alternative lockless logging facility that goes way beyond debugging early boot crashes!
Granted, I qualified that with 'just a side note'. I guess something like this would work:
+ One use for this feature is kernel debugging, for example when your
+ machine crashes very early before the regular console code is + initialized. Other uses include simpler, lockless logging instead of a + full-blown printk console driver + klogd. + + For normal production environments this is normally not recommended, + because it doesn't feed events into klogd/syslogd and doesn't try to + print anything on the screen.
+
+ You should normally N here, unless you want to debug early crashes or
+ need a very simple printk logging facility.
Another piece of feedback I gave was:
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+config USB_EARLY_PRINTK + boolAlso, could we standardize the nomencalture to not be a mixture of prefixes and postfixes - i.e. standardize on postfixes (as commonly done in the
Kconfigspace) and rename this one toEARLY_PRINTK_USBor so?
yet your latest submission still includes the very same postfixed config switch name that collides with the prefixed names such as EARLY_PRINTK_USB:
drivers/usb/Kconfig +3 -0
@@ -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
The problem I tried to point out with my review feedback is that we thus have both:
CONFIG_EARLY_PRINTK_USB=y
CONFIG_USB_EARLY_PRINTK=y
... which is confusing at the very least.
On a second look, this config switch appears to be unused - is it a leftover from earlier patches?
The patches don't look too bad otherwise, so we are not far from having something acceptable, IMHO.
Thanks,
Ingo
Hi Ingo,
Quoted 46 lines · Ingo Molnar
Quoted 5 lines · Lu Baolu
Hi Ingo,
Quoted 2 lines · Ingo Molnar
* Lu Baolu <[email protected]> wrote:
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Hi Ingo,
How about this version? Any further comments?
So I have re-read the review feedback I gave on Jan 19 and found at least one thing I pointed out that you didn't address in the latest patches ...
Do you mind telling me which one is not addressed? Is it one of below feedbacks?
So one piece of feedback I gave was:
| BTW., just a side note, some kernel developers (like PeterZ - and I do it | sometimes too) remap early_printk to printk permanently and use it as their main | printk facility - because printk() reliability has suffered over the last couple | of years. | | So it's more than just early boot debugging - it's a very simple state-less | logging facility to an external computer.But the latest
Kconfighelp text still says this:(quoted diff) +12
-0+config EARLY_PRINTK_USB_XDBC + bool "Early printk via the xHCI debug port" + 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.... while in reality it's an alternative lockless logging facility that goes way beyond debugging early boot crashes!
Granted, I qualified that with 'just a side note'. I guess something like this would work:
(quoted diff) +1
-0+ One use for this feature is kernel debugging, for example when your
Review
Quoted 10 lines
(quoted diff) +10
-0+ machine crashes very early before the regular console code is + initialized. Other uses include simpler, lockless logging instead of a + full-blown printk console driver + klogd. + + For normal production environments this is normally not recommended, + because it doesn't feed events into klogd/syslogd and doesn't try to + print anything on the screen. + + You should normally N here, unless you want to debug early crashes or + need a very simple printk logging facility.
Very appreciated for pointing this out. I will replace it.
Quoted 27 lines
Another piece of feedback I gave was:
Quoted 5 lines
drivers/usb/Kconfig config USB_EHCI_BIG_ENDIAN_MMIO · line 23
Quoted 2 lines
drivers/usb/Kconfig +2
-0+config USB_EARLY_PRINTK + boolAlso, could we standardize the nomencalture to not be a mixture of prefixes and postfixes - i.e. standardize on postfixes (as commonly done in the
Kconfigspace) and rename this one toEARLY_PRINTK_USBor so?yet your latest submission still includes the very same postfixed config switch name that collides with the prefixed names such as
EARLY_PRINTK_USB:drivers/usb/Kconfig +3
-0@@ -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_IOMEMThe problem I tried to point out with my review feedback is that we thus have both:
Quoted 7 lines
CONFIG_EARLY_PRINTK_USB=y
CONFIG_USB_EARLY_PRINTK=y... which is confusing at the very least.
On a second look, this config switch appears to be unused - is it a leftover from earlier patches?
Yes, it is a leftover from the earlier patches. I should remove it. Sorry about it.
Quoted 4 lines
The patches don't look too bad otherwise, so we are not far from having something acceptable, IMHO.
Thanks.
For the typo and grammar issues, I will recheck the patches and ask some English speakers for review.
Best regards,
Lu Baolu