Re: [PATCH v6 01/10] drivers/char: allow using both dbgp=xhci and dbgp=ehci

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2022 15:17, Marek Marczykowski-Górecki wrote:
> This allows configuring EHCI and XHCI consoles separately,
> simultaneously.
> 
> This changes string_param() to custom_param() in both ehci and xhci
> drivers. Both drivers parse only values applicable to them.
> 
> While at it, drop unnecessary memset() of a static variable.

Are you sure of this? What if there are two "dbgp=xhci,..." options
on the command line, the latter intended to override the earlier but
malformed. Then ->enabled would be left set from parsing the first
instance, afaict.

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -409,7 +409,7 @@ The following are examples of correct specifications:
>  Specify the size of the console ring buffer.
>  
>  ### console
> -> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | none ]`
> +> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | xhci | none ]`

Personally I consider "dbc" more in line with "dbgp", but I'm okay
with "xhci". We may want to allow for "ehci" then as an alias of
"dbgp", though (in a separate, later patch).

> --- a/xen/drivers/char/ehci-dbgp.c
> +++ b/xen/drivers/char/ehci-dbgp.c
> @@ -1464,7 +1464,18 @@ static struct uart_driver __read_mostly 
> ehci_dbgp_driver = {
>  static struct ehci_dbgp ehci_dbgp = { .state = dbgp_unsafe, .phys_port = 1 };
>  
>  static char __initdata opt_dbgp[30];
> -string_param("dbgp", opt_dbgp);
> +
> +static int __init parse_ehci_dbgp(const char *opt)
> +{
> +    if ( strncmp(opt, "ehci", 4) )
> +        return 0;
> +
> +    strlcpy(opt_dbgp, opt, sizeof(opt_dbgp));
> +
> +    return 0;
> +}
> +
> +custom_param("dbgp", parse_ehci_dbgp);

We commonly don't put a blank line between the function and this
construct. (Same again further down then.)

> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -245,6 +245,7 @@ struct dbc {
>      uint64_t xhc_dbc_offset;
>      void __iomem *xhc_mmio;
>  
> +    bool enable; /* whether dbgp=xhci was set at all */

In dbc_init_xhc() there's an assumption that the "sbdf" field is
always non-zero. Do you really need this separate flag then?

Jan