Re: [PATCH v6 01/10] drivers/char: allow using both dbgp=xhci and dbgp=ehci
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
signature.asc
Description: PGP signature
Re: [PATCH v6 01/10] drivers/char: allow using both dbgp=xhci and dbgp=ehci
On 06.09.2022 17:46, Marek Marczykowski-Górecki wrote:
> On Tue, Sep 06, 2022 at 05:07:27PM +0200, Jan Beulich wrote:
>> On 02.09.2022 15:17, Marek Marczykowski-Górecki wrote:
>>> --- 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().
Oh, I see. I'm sorry for mis-reading that code.
Jan
Re: [PATCH v6 01/10] drivers/char: allow using both dbgp=xhci and dbgp=ehci
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
[PATCH v6 01/10] drivers/char: allow using both dbgp=xhci and dbgp=ehci
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.
Suggested-by: Jan Beulich
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v6:
- keep dbgp=xhci, but use custom_param() to parse multiple values
separately
- modify ehci-dbgp to use custom_param() too
- change console=dbc to console=xhci, as 'dbc' doesn't appear in any
other option anymore
- update comment in serial.h
new in v5
---
docs/misc/xen-command-line.pandoc | 6 --
xen/drivers/char/ehci-dbgp.c | 15 +--
xen/drivers/char/serial.c | 6 ++
xen/drivers/char/xhci-dbc.c | 30 --
xen/include/xen/serial.h | 3 ++-
5 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index 9a79385a3712..c8b07042f58e 100644
--- 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 ]`
> Default: `console=com1,vga`
@@ -428,7 +428,9 @@ cleared. This allows a single port to be shared by two
subsystems
`pv` indicates that Xen should use Xen's PV console. This option is
only available when used together with `pv-in-pvh`.
-`dbgp` indicates that Xen should use a USB debug port.
+`dbgp` indicates that Xen should use a USB2 debug port.
+
+`xhci` indicates that Xen should use a USB3 debug port.
`none` indicates that Xen should not use a console. This option only
makes sense on its own.
diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
index 92c588ec0aa3..b1794ed64c7b 100644
--- 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);
void __init ehci_dbgp_init(void)
{
@@ -1472,7 +1483,7 @@ void __init ehci_dbgp_init(void)
u32 debug_port, offset, bar_val;
const char *e;
-if ( strncmp(opt_dbgp, "ehci", 4) )
+if ( !opt_dbgp[0] )
return;
if ( isdigit(opt_dbgp[4]) || !opt_dbgp[4] )
diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index 47899222cef8..9d9445039232 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -311,6 +311,12 @@ int __init serial_parse_handle(const char *conf)
goto common;
}
+if ( !strncmp(conf, "xhci", 4) && (!conf[4] || conf[4] == ',') )
+{
+handle = SERHND_XHCI;
+goto common;
+}
+
if ( !strncmp(conf, "dtuart", 6) )
{
handle = SERHND_DTUART;
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index ca7d4a62139e..8da76282259a 100644
--- 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 */
bool open;
unsigned int xhc_num; /* look for n-th xhc */
};
@@ -1058,20 +1059,14 @@ 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];
-
-string_param("dbgp", opt_dbgp);
-
-void __init xhci_dbc_uart_init(void)
+static int __init xhci_parse_dbgp(const char *opt_dbgp)
{
struct dbc_uart *uart = _uart;
struct dbc *dbc = >dbc;
const char *e;
if ( strncmp(opt_dbgp, "xhci", 4) )
-return;
-
-memset(dbc, 0, sizeof(*dbc));
+return 0;
if ( isdigit(opt_dbgp[4]) )
{
@@ -1087,12 +1082,27 @@ void __init xhci_dbc_uart_init(void)
printk(XENLOG_ERR
"Invalid dbgp= PCI device spec: '%s'\n",
opt_dbgp + 8);
-return;
+return -EINVAL;
}
dbc->sbdf = PCI_SBDF(0, bus, slot, func);
}
+dbc->enable = true;
+
+return 0;
+}
+
+custom_param("dbgp", xhci_parse_dbgp);
+
+void __init xhci_dbc_uart_init(void)
+{
+struct dbc_uart *uart = _uart;
+struct dbc *dbc = >dbc;
+
+if ( !dbc->enable )
+return;
+
dbc->dbc_ctx =
dbc->dbc_erst =
dbc->dbc_ering.trb = evt_trb;
@@
