Re: [PATCH v1 01/10] drivers/char: Add support for Xue USB3 debugger

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.06.2022 17:47, Marek Marczykowski-Górecki wrote:
> On Wed, Jun 15, 2022 at 04:25:54PM +0200, Jan Beulich wrote:
>> On 07.06.2022 16:30, Marek Marczykowski-Górecki wrote:
>>> +    /* ...we found it, so parse the BAR and map the registers */
>>> +    bar0 = pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_0);
>>> +    bar1 = pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_1);
>>
>> What if there are multiple?
> 
> You mean two 32-bit BARs? I check for that below (refusing to use them).
> Anyway, I don't think that's a thing in USB 3.0 controllers.

No, I mean multiple controllers. When making the remark I didn't know
yet that you'll deal with that in patch 3. A sentence making the
restriction (and its intended resolution) explicit in the description
would help.

>>> +    memset(xue, 0, sizeof(*xue));
>>> +
>>> +    xue->dbc_ctx = &ctx;
>>> +    xue->dbc_erst = &erst;
>>> +    xue->dbc_ering.trb = evt_trb;
>>> +    xue->dbc_oring.trb = out_trb;
>>> +    xue->dbc_iring.trb = in_trb;
>>> +    xue->dbc_owork.buf = wrk_buf;
>>> +    xue->dbc_str = str_buf;
>>
>> Especially the page-sized entities want allocating dynamically here, as
>> they won't be needed without the command line option requesting the use
>> of this driver.
> 
> Are you okay with changing this only in patch 9, where I restructure those
> buffers anyway?

I'm afraid I'll need to make it to patch 9 to answer this question. If
suitable dealt with later, I don't see a fundamental problem, as long
as it's clear then that I will request that this patch be committed in
a batch with that later one, not in isolation.

>>> +    serial_register_uart(SERHND_DBGP, &xue_uart_driver, &xue_uart);
>>> +}
>>> +
>>> +void xue_uart_dump(void)
>>> +{
>>> +    struct xue_uart *uart = &xue_uart;
>>> +    struct xue *xue = &uart->xue;
>>> +
>>> +    xue_dump(xue);
>>> +}
>>
>> This function looks to be unused (and lacks a declaration).
> 
> It is unused, same as xue_dump(), but is extremely useful when
> debugging. Should I put it behind something like #ifdef XUE_DEBUG,
> accompanied with a comment about its purpose?

Yes, please (or any other suitable means to make the functions
disappear from the final binary). The function here then also ought
to be static, I suppose - you're not adding a declaration anywhere
for it to be usable outside of this source file.

Jan