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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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? But then IMO having "console=dbgp console=dbc" would
be confusing, as "dbc" has no obvious relation to neither side of
"dbgp=xhci". Maybe use "console=xhci" then?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature