Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> +/* Defines the size in bytes of TRB rings as 2^DBC_TRB_RING_ORDER * 4096 */
> +#ifndef DBC_TRB_RING_ORDER
> +#define DBC_TRB_RING_ORDER 4
> +#endif
> +#define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE * (1 << DBC_TRB_RING_ORDER))

I have to admit that I'm always puzzled when seeing such - why not

#define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE << DBC_TRB_RING_ORDER)

?

> +struct dbc {
> +    struct dbc_reg __iomem *dbc_reg;
> +    struct xhci_dbc_ctx *dbc_ctx;
> +    struct xhci_erst_segment *dbc_erst;
> +    struct xhci_trb_ring dbc_ering;
> +    struct xhci_trb_ring dbc_oring;
> +    struct xhci_trb_ring dbc_iring;
> +    struct dbc_work_ring dbc_owork;
> +    struct xhci_string_descriptor *dbc_str;

I'm afraid I still don't see why the static page this field is being
initialized with is necessary. Can't you have a static variable (of
some struct type) all pre-filled at build time, which you then apply
virt_to_maddr() to in order to fill the respective dbc_ctx fields?
That struct will be quite a bit less than a page's worth in size.

If you build the file with -fshort-wchar, you may even be able to
use easy to read string literals for the initializer.

> +static void *dbc_sys_map_xhc(uint64_t phys, size_t size)
> +{
> +    size_t i;
> +
> +    if ( size != MAX_XHCI_PAGES * DBC_PAGE_SIZE )
> +        return NULL;
> +
> +    for ( i = FIX_XHCI_END; i >= FIX_XHCI_BEGIN; i-- )
> +    {
> +        set_fixmap_nocache(i, phys);
> +        phys += DBC_PAGE_SIZE;

While there may be an assumption of DBC_PAGE_SIZE == PAGE_SIZE, the
constant used here clearly needs to be PAGE_SIZE, as that's the unit
set_fixmap_nocache() deals with.

> +static bool __init dbc_init_xhc(struct dbc *dbc)
> +{
> +    uint32_t bar0;
> +    uint64_t bar1;
> +    uint64_t bar_size;
> +    uint64_t devfn;
> +    uint16_t cmd;
> +    size_t xhc_mmio_size;
> +
> +    /*
> +     * Search PCI bus 0 for the xHC. All the host controllers supported so 
> far
> +     * are part of the chipset and are on bus 0.
> +     */
> +    for ( devfn = 0; devfn < 256; devfn++ )
> +    {
> +        pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
> +        uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
> +
> +        if ( hdr == 0 || hdr == 0x80 )
> +        {
> +            if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == 
> DBC_XHC_CLASSC )
> +            {
> +                dbc->sbdf = sbdf;
> +                break;
> +            }
> +        }
> +    }
> +
> +    if ( !dbc->sbdf.sbdf )
> +    {
> +        dbc_error("Compatible xHC not found on bus 0\n");
> +        return false;
> +    }
> +
> +    /* ...we found it, so parse the BAR and map the registers */
> +    bar0 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
> +    bar1 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1);
> +
> +    /* IO BARs not allowed; BAR must be 64-bit */
> +    if ( (bar0 & PCI_BASE_ADDRESS_SPACE) != PCI_BASE_ADDRESS_SPACE_MEMORY ||
> +         (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_MASK) != 
> PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +        return false;
> +
> +    cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
> +    pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);
> +
> +    pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
> +    pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, 0xFFFFFFFF);
> +    bar_size = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
> +    bar_size |= (uint64_t)pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1) << 
> 32;
> +    xhc_mmio_size = ~(bar_size & PCI_BASE_ADDRESS_MEM_MASK) + 1;
> +    pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, bar0);
> +    pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, bar1);
> +
> +    pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
> +
> +    dbc->xhc_mmio_phys = (bar0 & PCI_BASE_ADDRESS_MEM_MASK) | (bar1 << 32);
> +    dbc->xhc_mmio = dbc_sys_map_xhc(dbc->xhc_mmio_phys, xhc_mmio_size);

Before actually using the address to map the MMIO you will want to make
somewhat sure firmware did initialize it: The EHCI driver checks for
all zeroes or all ones in the writable bits.

> +/**
> + * The first register of the debug capability is found by traversing the
> + * host controller's capability list (xcap) until a capability
> + * with ID = 0xA is found. The xHCI capability list begins at address
> + * mmio + (HCCPARAMS1[31:16] << 2).
> + */
> +static struct dbc_reg __iomem *xhci_find_dbc(struct dbc *dbc)
> +{
> +    uint32_t *xcap;

const?

> +    uint32_t xcap_val;
> +    uint32_t next;
> +    uint32_t id = 0;
> +    uint8_t *mmio = (uint8_t *)dbc->xhc_mmio;

Can't this be const void * (and probably wants to also use __iomem),
avoiding the cast here, ...

> +    uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);

... here, ...

> +    const uint32_t DBC_ID = 0xA;
> +    int ttl = 48;
> +
> +    xcap = (uint32_t *)dbc->xhc_mmio;

... and here (if actually using the local variable you've got).

> +/*
> + * Note that if IN transfer support is added, then this
> + * will need to be changed; it assumes an OUT transfer ring only
> + */

Hmm, is this comment telling me that this driver is an output-only one?
Or is it simply that input doesn't use this code path?

> +static void dbc_init_string_single(struct xhci_string_descriptor *string,
> +                                   char *ascii_str,

If this function has to survive, then const please here and ...

> +                                   uint64_t *str_ptr,
> +                                   uint8_t *str_size_ptr)
> +{
> +    size_t i, len = strlen(ascii_str);
> +
> +    string->size = offsetof(typeof(*string), string) + len * 2;
> +    string->type = XHCI_DT_STRING;
> +    /* ASCII to UTF16 conversion */
> +    for (i = 0; i < len; i++)

... this missing blanks added here.

> +static struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
> +static struct xhci_trb out_trb[DBC_TRB_RING_CAP];
> +static struct xhci_trb in_trb[DBC_TRB_RING_CAP];
> +static struct xhci_erst_segment erst __aligned(64);
> +static struct xhci_dbc_ctx ctx __aligned(64);
> +static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
> +static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
> +static char __initdata opt_dbgp[30];
> +
> +string_param("dbgp", opt_dbgp);

This duplicates what ehci-dbgp.c already has. I guess we can live with
it for now and de-duplicate later, but it's still a little odd. In any
even please move the blank line up be a line, so that string_param()
and its referenced array are kept together.

> +void __init xhci_dbc_uart_init(void)
> +{
> +    struct dbc_uart *uart = &dbc_uart;
> +    struct dbc *dbc = &uart->dbc;
> +
> +    if ( strncmp(opt_dbgp, "xhci", 4) )
> +        return;
> +
> +    memset(dbc, 0, sizeof(*dbc));

Why? dbc_uart is a static variable, and hence already zero-initialized.

Jan