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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2022 13:57, Marek Marczykowski-Górecki wrote:
> On Mon, Aug 29, 2022 at 01:49:30PM +0200, Marek Marczykowski-Górecki wrote:
>> On Fri, Aug 26, 2022 at 04:20:52PM +0200, 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.
>>>
>>> But using two such options at the same time isn't of interest anyway
>>> without your multiple-serial-consoles change, so possibly not of
>>> immediate need (unless someone comes forward expressing interest and
>>> actually approving that change of yours).
>>
>> Then why change at all? Since you can configure only one (dbgp=ehci _or_
>> dbgp=xhci), then there is not ambiguity what "console=dbgp" means.
>> Separating SERHND_DBC from SERHND_DBGP would IMO make sense only if you
>> can actually use them both (even if not both for console, but for
>> example one for debugger).
> 
> Or do you mean to use custom_param() to actually make "dbgp=xhci
> dbgp=ehci" working?

Yes.

> But then IMO having "console=dbgp console=dbc" would
> be confusing, as "dbc" has no obvious relation to neither side of
> "dbgp=xhci".

Well, there was never any idea of using multiple serial consoles, so
the present "console=dbgp" doesn't provide room for telling apart the
two. Just like there's no way to tell apart two EHCI controllers'
debug ports both (intended to be) used at the same time.

JanMaybe use "console=xhci" then?
>