Re: [PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 26/08/2022 15:20, Jan Beulich wrote:
> On 26.08.2022 13:46, Marek Marczykowski-Górecki wrote:
>> On Thu, Aug 25, 2022 at 05:44:54PM +0200, Jan Beulich wrote:
>>> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
>>>> This allows configuring EHCI and XHCI consoles separately,
>>>> simultaneously.
>>>>
>>>> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> But was I maybe confused, and much less of a change would suffice? After
>>> all ...
>>>
>>>> --- a/xen/drivers/char/xhci-dbc.c
>>>> +++ b/xen/drivers/char/xhci-dbc.c
>>>> @@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
>>>>  static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
>>>>  static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
>>>>  
>>>> -static char __initdata opt_dbgp[30];
>>>> +static char __initdata opt_dbc[30];
>>>>  
>>>> -string_param("dbgp", opt_dbgp);
>>>> +string_param("dbc", opt_dbc);
>>>>  
>>>>  void __init xhci_dbc_uart_init(void)
>>>>  {
>>>> @@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
>>>>      struct dbc *dbc = &uart->dbc;
>>>>      const char *e;
>>>>  
>>>> -    if ( strncmp(opt_dbgp, "xhci", 4) )
>>>> +    if ( strncmp(opt_dbc, "xhci", 4) )
>>>>          return;
>>> ... this already avoids mixing up who's going to parse what. So right
>>> now I think that ...
>>>
>>>> @@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
>>>>      dbc->dbc_str = str_buf;
>>>>  
>>>>      if ( dbc_open(dbc) )
>>>> -        serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
>>>> +        serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
>>>>  }
>>> ... this and other SERHND_* related changes are enough, and there's no
>>> need for a separate "dbc=" option.
>> But then you wouldn't be able to configure "dbgp=ehci dbgp=xhci" as
>> one would override the other, no?
> Not as long as both use string_param(), true. They'd need to both become
> custom_param(), doing at least some basic parsing right away.

I've looked through our string params, and none of them look like they
should be string params.

I have half a mind to transform them all, one at a time, and remove
string_param() to prevent problems like this in the future.

~Andrew