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
