Hi Ingo,
On 03/02/2017 02:40 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
>
>> 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?
==============================
>
> 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.
-----------------------------------------------------------------
>> 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’.
"
-----------------------------------------------------------------
> +
> + val64 = val & PCI_BASE_ADDRESS_MEM_MASK;
> + sz64 = sz & PCI_BASE_ADDRESS_MEM_MASK;
> + mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;
Is this cast necessary?
-----------------------------------------------------------------
> + if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, ~0);
> + sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, val);
> +
> + val64 |= (u64)val << 32;
> + sz64 |= (u64)sz << 32;
Could these type casts be avoided by declaring 'val' and 'sz' as u64 to begin
with?
==============================
I posted all feedbacks I received by now at the tail of this email to
make sure all feedbacks were communicated and addressed.
>
> Plus please go beyond the feedback given - for example the Kconfig text 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?
-----------------------------------------------------------------
> + if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> + PCI_BASE_ADDRESS_MEM_TYPE_64) {
Please don't break the line here.
-----------------------------------------------------------------
> + val64 |= ((u64)val << 32);
> + sz64 |= ((u64)sz << 32);
> + mask64 |= ((u64)~0 << 32);
Unnecessary parentheses.
-----------------------------------------------------------------
> + 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?
-----------------------------------------------------------------
> + 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_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.
-----------------------------------------------------------------
> + /*
> + * 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
?
-----------------------------------------------------------------
> + /* 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.
-----------------------------------------------------------------
>
> 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.
-----------------------------------------------------------------
>> 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’.
"
-----------------------------------------------------------------
> between the debug host and target. The DbC functionality is
> independent of xHCI host. There isn't any precondition from
> xHCI host side for DbC to work.
s/xHCI host/the xHCI host
-----------------------------------------------------------------
> This patch also includes bulk out and bulk in interfaces.
> These interfaces could be used to implement early printk
> bootconsole or hook to various system debuggers.
s/out/output
s/in/input
-----------------------------------------------------------------
> +config EARLY_PRINTK_USB_XDBC
> + bool "Early printk via xHCI debug port"
s/xHCI/the xHCI
I remarked on this in my first review as well - mind checking the whole series for
uses of 'xHCI'?
-----------------------------------------------------------------
> +
> + val64 = val & PCI_BASE_ADDRESS_MEM_MASK;
> + sz64 = sz & PCI_BASE_ADDRESS_MEM_MASK;
> + mask64 = (u32)PCI_BASE_ADDRESS_MEM_MASK;
Is this cast necessary?
-----------------------------------------------------------------
> + if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, ~0);
> + sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, val);
> +
> + val64 |= (u64)val << 32;
> + sz64 |= (u64)sz << 32;
Could these type casts be avoided by declaring 'val' and 'sz' as u64 to begin
with?
-----------------------------------------------------------------
> + mask64 |= (u64)~0 << 32;
Type cast could be avoided by using 0L.
-----------------------------------------------------------------
> + for (bus = 0; bus < XDBC_PCI_MAX_BUSES; bus++) {
> + for (dev = 0; dev < XDBC_PCI_MAX_DEVICES; dev++) {
> + for (func = 0; func < XDBC_PCI_MAX_FUNCTION; func++) {
> + class = read_pci_config(bus, dev, func, PCI_CLASS_REVISION);
> + if ((class >> 8) != PCI_CLASS_SERIAL_USB_XHCI)
> + continue;
> +
> + if (xdbc_num-- != 0)
> + continue;
> +
> + *b = bus;
> + *d = dev;
> + *f = func;
> +
> + return 0;
> + }
> + }
> + }
Nit: to me it would look more balanced visually if the body of the innermost loop
started with an empty newline.
-----------------------------------------------------------------
> + ext_cap_offset = xhci_find_next_ext_cap(xdbc.xhci_base,
> + 0, XHCI_EXT_CAPS_LEGACY);
Please don't break this line. You can shorten the variable name if you want to
save line length.
-----------------------------------------------------------------
> + /* populate the contexts */
> + context = (struct xdbc_context *)xdbc.dbcc_base;
> + context->info.string0 = cpu_to_le64(xdbc.string_dma);
> + context->info.manufacturer = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
> + context->info.product = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
> + context->info.serial = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
> + context->info.length = cpu_to_le32(string_length);
Wouldn't this (and some of the other initializations) look nicer this way:
/* Populate the contexts: */
context = (struct xdbc_context *)xdbc.dbcc_base;
context->info.string0 = cpu_to_le64(xdbc.string_dma);
context->info.manufacturer = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH);
context->info.product = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 2);
context->info.serial = cpu_to_le64(xdbc.string_dma + XDBC_MAX_STRING_LENGTH * 3);
context->info.length = cpu_to_le32(string_length);
?
-----------------------------------------------------------------
> + /* Check completion of the previous request. */
Could you please standardize the capitalization and punctuation of all the
comments in the patches? Some are lower case, some are upper case, some have full
stops, some don't.
One good style would be to use this form when a comment refers to what the next
statement does:
/* Check completion of the previous request: */
-----------------------------------------------------------------
> +#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
=================================================================
> +#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
=================================================================
> This patch add dbc debug device support in usb_debug driver.
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.]
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
