Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port

[Lu Baolu]

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