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

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

On Tue, Sep 06, 2022 at 05:07:27PM +0200, Jan Beulich wrote:
> 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.

Right.

> > --- 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).

I've changed "dbc" to "xhci", as "dbc" isn't really surfaced to the user
anywhere else. As in - it requires some deeper knowledge to draw a
connection between console=dbc and dbgp=xhci. And yes, when going this
way, "ehci" alias would make sense.

> 
> > --- 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?

Not really, sbdf == 0 means "find Nth xhci", where N=xhc_num+1 (and
xhc_num can be zero too). See the "if" at the very top of
dbc_init_xhc().

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

Attachment: signature.asc
Description: PGP signature