[PATCH] usb: xhci: dbc: get rid of global pointer

[PATCH] usb: xhci: dbc: get rid of global pointer

[Felipe Balbi]

If we happen to have two XHCI controllers with DbC capability, then
there's no hope this will ever work as the global pointer will be
overwritten by the controller that probes last.

Avoid this problem by keeping the tty_driver struct pointer inside
struct xhci_dbc.

Fixes: dfba2174dc42 usb: xhci: Add DbC support in xHCI driver
Cc: <stable@vger.kernel.org> # v4.16+
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/host/xhci-dbgcap.c |  4 +--
 drivers/usb/host/xhci-dbgcap.h |  3 +-
 drivers/usb/host/xhci-dbgtty.c | 54 +++++++++++++++++-----------------
 3 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index 52e32644a4b2..5f56b650c0ea 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -948,7 +948,7 @@ int xhci_dbc_init(struct xhci_hcd *xhci)
 	return 0;
 
 init_err1:
-	xhci_dbc_tty_unregister_driver();
+	xhci_dbc_tty_unregister_driver(xhci);
 init_err2:
 	xhci_do_dbc_exit(xhci);
 init_err3:
@@ -963,7 +963,7 @@ void xhci_dbc_exit(struct xhci_hcd *xhci)
 		return;
 
 	device_remove_file(dev, &dev_attr_dbc);
-	xhci_dbc_tty_unregister_driver();
+	xhci_dbc_tty_unregister_driver(xhci);
 	xhci_dbc_stop(xhci);
 	xhci_do_dbc_exit(xhci);
 }
diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
index ce0c6072bd48..30dedf36c566 100644
--- a/drivers/usb/host/xhci-dbgcap.h
+++ b/drivers/usb/host/xhci-dbgcap.h
@@ -151,6 +151,7 @@ struct xhci_dbc {
 	struct dbc_ep			eps[2];
 
 	struct dbc_port			port;
+	struct tty_driver		*tty_driver;
 };
 
 #define dbc_bulkout_ctx(d)		\
@@ -196,7 +197,7 @@ static inline struct dbc_ep *get_out_ep(struct xhci_hcd *xhci)
 int xhci_dbc_init(struct xhci_hcd *xhci);
 void xhci_dbc_exit(struct xhci_hcd *xhci);
 int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci);
-void xhci_dbc_tty_unregister_driver(void);
+void xhci_dbc_tty_unregister_driver(struct xhci_hcd *xhci);
 int xhci_dbc_tty_register_device(struct xhci_hcd *xhci);
 void xhci_dbc_tty_unregister_device(struct xhci_hcd *xhci);
 struct dbc_request *dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags);
diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
index aff79ff5aba4..300fc770a0d5 100644
--- a/drivers/usb/host/xhci-dbgtty.c
+++ b/drivers/usb/host/xhci-dbgtty.c
@@ -279,52 +279,52 @@ static const struct tty_operations dbc_tty_ops = {
 	.unthrottle		= dbc_tty_unthrottle,
 };
 
-static struct tty_driver *dbc_tty_driver;
-
 int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
 {
 	int			status;
 	struct xhci_dbc		*dbc = xhci->dbc;
 
-	dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
+	dbc->tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
 					  TTY_DRIVER_DYNAMIC_DEV);
-	if (IS_ERR(dbc_tty_driver)) {
-		status = PTR_ERR(dbc_tty_driver);
-		dbc_tty_driver = NULL;
+	if (IS_ERR(dbc->tty_driver)) {
+		status = PTR_ERR(dbc->tty_driver);
+		dbc->tty_driver = NULL;
 		return status;
 	}
 
-	dbc_tty_driver->driver_name = "dbc_serial";
-	dbc_tty_driver->name = "ttyDBC";
+	dbc->tty_driver->driver_name = "dbc_serial";
+	dbc->tty_driver->name = "ttyDBC";
 
-	dbc_tty_driver->type = TTY_DRIVER_TYPE_SERIAL;
-	dbc_tty_driver->subtype = SERIAL_TYPE_NORMAL;
-	dbc_tty_driver->init_termios = tty_std_termios;
-	dbc_tty_driver->init_termios.c_cflag =
+	dbc->tty_driver->type = TTY_DRIVER_TYPE_SERIAL;
+	dbc->tty_driver->subtype = SERIAL_TYPE_NORMAL;
+	dbc->tty_driver->init_termios = tty_std_termios;
+	dbc->tty_driver->init_termios.c_cflag =
 			B9600 | CS8 | CREAD | HUPCL | CLOCAL;
-	dbc_tty_driver->init_termios.c_ispeed = 9600;
-	dbc_tty_driver->init_termios.c_ospeed = 9600;
-	dbc_tty_driver->driver_state = &dbc->port;
+	dbc->tty_driver->init_termios.c_ispeed = 9600;
+	dbc->tty_driver->init_termios.c_ospeed = 9600;
+	dbc->tty_driver->driver_state = &dbc->port;
 
-	tty_set_operations(dbc_tty_driver, &dbc_tty_ops);
+	tty_set_operations(dbc->tty_driver, &dbc_tty_ops);
 
-	status = tty_register_driver(dbc_tty_driver);
+	status = tty_register_driver(dbc->tty_driver);
 	if (status) {
 		xhci_err(xhci,
 			 "can't register dbc tty driver, err %d\n", status);
-		put_tty_driver(dbc_tty_driver);
-		dbc_tty_driver = NULL;
+		put_tty_driver(dbc->tty_driver);
+		dbc->tty_driver = NULL;
 	}
 
 	return status;
 }
 
-void xhci_dbc_tty_unregister_driver(void)
+void xhci_dbc_tty_unregister_driver(struct xhci_hcd *xhci)
 {
-	if (dbc_tty_driver) {
-		tty_unregister_driver(dbc_tty_driver);
-		put_tty_driver(dbc_tty_driver);
-		dbc_tty_driver = NULL;
+	struct xhci_dbc		*dbc = xhci->dbc;
+
+	if (dbc->tty_driver) {
+		tty_unregister_driver(dbc->tty_driver);
+		put_tty_driver(dbc->tty_driver);
+		dbc->tty_driver = NULL;
 	}
 }
 
@@ -449,7 +449,7 @@ int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
 
 	xhci_dbc_tty_init_port(xhci, port);
 	tty_dev = tty_port_register_device(&port->port,
-					   dbc_tty_driver, 0, NULL);
+					   dbc->tty_driver, 0, NULL);
 	if (IS_ERR(tty_dev)) {
 		ret = PTR_ERR(tty_dev);
 		goto register_fail;
@@ -479,7 +479,7 @@ int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
 	kfifo_free(&port->write_fifo);
 
 buf_alloc_fail:
-	tty_unregister_device(dbc_tty_driver, 0);
+	tty_unregister_device(dbc->tty_driver, 0);
 
 register_fail:
 	xhci_dbc_tty_exit_port(port);
@@ -494,7 +494,7 @@ void xhci_dbc_tty_unregister_device(struct xhci_hcd *xhci)
 	struct xhci_dbc		*dbc = xhci->dbc;
 	struct dbc_port		*port = &dbc->port;
 
-	tty_unregister_device(dbc_tty_driver, 0);
+	tty_unregister_device(dbc->tty_driver, 0);
 	xhci_dbc_tty_exit_port(port);
 	port->registered = false;
 
-- 
2.22.0

Re: [PATCH] usb: xhci: dbc: get rid of global pointer

[Mathias Nyman]

On 11.6.2019 20.24, Felipe Balbi wrote:
> If we happen to have two XHCI controllers with DbC capability, then
> there's no hope this will ever work as the global pointer will be
> overwritten by the controller that probes last.
> 
> Avoid this problem by keeping the tty_driver struct pointer inside
> struct xhci_dbc.
> 
> Fixes: dfba2174dc42 usb: xhci: Add DbC support in xHCI driver
> Cc: <stable@vger.kernel.org> # v4.16+
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---

Thanks, adding to queue

-Mathias

Re: [PATCH] usb: xhci: dbc: get rid of global pointer

[Johan Hovold]

On Tue, Jun 11, 2019 at 08:24:16PM +0300, Felipe Balbi wrote:
> If we happen to have two XHCI controllers with DbC capability, then
> there's no hope this will ever work as the global pointer will be
> overwritten by the controller that probes last.
> 
> Avoid this problem by keeping the tty_driver struct pointer inside
> struct xhci_dbc.

How did you test this patch?

> Fixes: dfba2174dc42 usb: xhci: Add DbC support in xHCI driver
> Cc: <stable@vger.kernel.org> # v4.16+
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>  drivers/usb/host/xhci-dbgcap.c |  4 +--
>  drivers/usb/host/xhci-dbgcap.h |  3 +-
>  drivers/usb/host/xhci-dbgtty.c | 54 +++++++++++++++++-----------------
>  3 files changed, 31 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> index 52e32644a4b2..5f56b650c0ea 100644
> --- a/drivers/usb/host/xhci-dbgcap.c
> +++ b/drivers/usb/host/xhci-dbgcap.c
> @@ -948,7 +948,7 @@ int xhci_dbc_init(struct xhci_hcd *xhci)
>  	return 0;
>  
>  init_err1:
> -	xhci_dbc_tty_unregister_driver();
> +	xhci_dbc_tty_unregister_driver(xhci);
>  init_err2:
>  	xhci_do_dbc_exit(xhci);
>  init_err3:
> @@ -963,7 +963,7 @@ void xhci_dbc_exit(struct xhci_hcd *xhci)
>  		return;
>  
>  	device_remove_file(dev, &dev_attr_dbc);
> -	xhci_dbc_tty_unregister_driver();
> +	xhci_dbc_tty_unregister_driver(xhci);
>  	xhci_dbc_stop(xhci);
>  	xhci_do_dbc_exit(xhci);
>  }
> diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
> index ce0c6072bd48..30dedf36c566 100644
> --- a/drivers/usb/host/xhci-dbgcap.h
> +++ b/drivers/usb/host/xhci-dbgcap.h
> @@ -151,6 +151,7 @@ struct xhci_dbc {
>  	struct dbc_ep			eps[2];
>  
>  	struct dbc_port			port;
> +	struct tty_driver		*tty_driver;
>  };
>  
>  #define dbc_bulkout_ctx(d)		\
> @@ -196,7 +197,7 @@ static inline struct dbc_ep *get_out_ep(struct xhci_hcd *xhci)
>  int xhci_dbc_init(struct xhci_hcd *xhci);
>  void xhci_dbc_exit(struct xhci_hcd *xhci);
>  int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci);
> -void xhci_dbc_tty_unregister_driver(void);
> +void xhci_dbc_tty_unregister_driver(struct xhci_hcd *xhci);
>  int xhci_dbc_tty_register_device(struct xhci_hcd *xhci);
>  void xhci_dbc_tty_unregister_device(struct xhci_hcd *xhci);
>  struct dbc_request *dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags);
> diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
> index aff79ff5aba4..300fc770a0d5 100644
> --- a/drivers/usb/host/xhci-dbgtty.c
> +++ b/drivers/usb/host/xhci-dbgtty.c
> @@ -279,52 +279,52 @@ static const struct tty_operations dbc_tty_ops = {
>  	.unthrottle		= dbc_tty_unthrottle,
>  };
>  
> -static struct tty_driver *dbc_tty_driver;
> -
>  int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
>  {
>  	int			status;
>  	struct xhci_dbc		*dbc = xhci->dbc;
>  
> -	dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
> +	dbc->tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
>  					  TTY_DRIVER_DYNAMIC_DEV);
> -	if (IS_ERR(dbc_tty_driver)) {
> -		status = PTR_ERR(dbc_tty_driver);
> -		dbc_tty_driver = NULL;
> +	if (IS_ERR(dbc->tty_driver)) {
> +		status = PTR_ERR(dbc->tty_driver);
> +		dbc->tty_driver = NULL;
>  		return status;
>  	}
>  
> -	dbc_tty_driver->driver_name = "dbc_serial";
> -	dbc_tty_driver->name = "ttyDBC";
> +	dbc->tty_driver->driver_name = "dbc_serial";
> +	dbc->tty_driver->name = "ttyDBC";

You're now registering multiple drivers for the same thing (and wasting
a major number for each) and specifically using the same name, which
should lead to name clashes when registering the second port.

Possibly better than the current situation, but why not fix this
properly instead? Register the driver once, and just pick a new minor
number for each controller.

> -	dbc_tty_driver->type = TTY_DRIVER_TYPE_SERIAL;
> -	dbc_tty_driver->subtype = SERIAL_TYPE_NORMAL;
> -	dbc_tty_driver->init_termios = tty_std_termios;
> -	dbc_tty_driver->init_termios.c_cflag =
> +	dbc->tty_driver->type = TTY_DRIVER_TYPE_SERIAL;
> +	dbc->tty_driver->subtype = SERIAL_TYPE_NORMAL;
> +	dbc->tty_driver->init_termios = tty_std_termios;
> +	dbc->tty_driver->init_termios.c_cflag =
>  			B9600 | CS8 | CREAD | HUPCL | CLOCAL;
> -	dbc_tty_driver->init_termios.c_ispeed = 9600;
> -	dbc_tty_driver->init_termios.c_ospeed = 9600;
> -	dbc_tty_driver->driver_state = &dbc->port;
> +	dbc->tty_driver->init_termios.c_ispeed = 9600;
> +	dbc->tty_driver->init_termios.c_ospeed = 9600;
> +	dbc->tty_driver->driver_state = &dbc->port;
>  
> -	tty_set_operations(dbc_tty_driver, &dbc_tty_ops);
> +	tty_set_operations(dbc->tty_driver, &dbc_tty_ops);
>  
> -	status = tty_register_driver(dbc_tty_driver);
> +	status = tty_register_driver(dbc->tty_driver);
>  	if (status) {
>  		xhci_err(xhci,
>  			 "can't register dbc tty driver, err %d\n", status);
> -		put_tty_driver(dbc_tty_driver);
> -		dbc_tty_driver = NULL;
> +		put_tty_driver(dbc->tty_driver);
> +		dbc->tty_driver = NULL;
>  	}
>  
>  	return status;
>  }

Johan

Re: [PATCH] usb: xhci: dbc: get rid of global pointer

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]


Hi,

Johan Hovold <johan@kernel.org> writes:
> On Tue, Jun 11, 2019 at 08:24:16PM +0300, Felipe Balbi wrote:
>> If we happen to have two XHCI controllers with DbC capability, then
>> there's no hope this will ever work as the global pointer will be
>> overwritten by the controller that probes last.
>> 
>> Avoid this problem by keeping the tty_driver struct pointer inside
>> struct xhci_dbc.
>
> How did you test this patch?

by running it on a machine that actually has two DbCs

>> @@ -279,52 +279,52 @@ static const struct tty_operations dbc_tty_ops = {
>>  	.unthrottle		= dbc_tty_unthrottle,
>>  };
>>  
>> -static struct tty_driver *dbc_tty_driver;
>> -
>>  int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
>>  {
>>  	int			status;
>>  	struct xhci_dbc		*dbc = xhci->dbc;
>>  
>> -	dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
>> +	dbc->tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
>>  					  TTY_DRIVER_DYNAMIC_DEV);
>> -	if (IS_ERR(dbc_tty_driver)) {
>> -		status = PTR_ERR(dbc_tty_driver);
>> -		dbc_tty_driver = NULL;
>> +	if (IS_ERR(dbc->tty_driver)) {
>> +		status = PTR_ERR(dbc->tty_driver);
>> +		dbc->tty_driver = NULL;
>>  		return status;
>>  	}
>>  
>> -	dbc_tty_driver->driver_name = "dbc_serial";
>> -	dbc_tty_driver->name = "ttyDBC";
>> +	dbc->tty_driver->driver_name = "dbc_serial";
>> +	dbc->tty_driver->name = "ttyDBC";
>
> You're now registering multiple drivers for the same thing (and wasting
> a major number for each) and specifically using the same name, which
> should lead to name clashes when registering the second port.

No warnings were printed while running this, actually. Odd

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: xhci: dbc: get rid of global pointer

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

On Mon, Jun 17, 2019 at 09:43:19AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Johan Hovold <johan@kernel.org> writes:
> > On Tue, Jun 11, 2019 at 08:24:16PM +0300, Felipe Balbi wrote:
> >> If we happen to have two XHCI controllers with DbC capability, then
> >> there's no hope this will ever work as the global pointer will be
> >> overwritten by the controller that probes last.
> >> 
> >> Avoid this problem by keeping the tty_driver struct pointer inside
> >> struct xhci_dbc.
> >
> > How did you test this patch?
> 
> by running it on a machine that actually has two DbCs
> 
> >> @@ -279,52 +279,52 @@ static const struct tty_operations dbc_tty_ops = {
> >>  	.unthrottle		= dbc_tty_unthrottle,
> >>  };
> >>  
> >> -static struct tty_driver *dbc_tty_driver;
> >> -
> >>  int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
> >>  {
> >>  	int			status;
> >>  	struct xhci_dbc		*dbc = xhci->dbc;
> >>  
> >> -	dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
> >> +	dbc->tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
> >>  					  TTY_DRIVER_DYNAMIC_DEV);
> >> -	if (IS_ERR(dbc_tty_driver)) {
> >> -		status = PTR_ERR(dbc_tty_driver);
> >> -		dbc_tty_driver = NULL;
> >> +	if (IS_ERR(dbc->tty_driver)) {
> >> +		status = PTR_ERR(dbc->tty_driver);
> >> +		dbc->tty_driver = NULL;
> >>  		return status;
> >>  	}
> >>  
> >> -	dbc_tty_driver->driver_name = "dbc_serial";
> >> -	dbc_tty_driver->name = "ttyDBC";
> >> +	dbc->tty_driver->driver_name = "dbc_serial";
> >> +	dbc->tty_driver->name = "ttyDBC";
> >
> > You're now registering multiple drivers for the same thing (and wasting
> > a major number for each) and specifically using the same name, which
> > should lead to name clashes when registering the second port.
> 
> No warnings were printed while running this, actually. Odd

Odd indeed. I get the expected warning from sysfs when trying to
register a second tty using an already registered name:

[  643.360555] sysfs: cannot create duplicate filename '/class/tty/ttyS0'
[  643.360637] CPU: 1 PID: 2383 Comm: modprobe Not tainted 5.2.0-rc1 #2
[  643.360702] Hardware name:  /D34010WYK, BIOS WYLPT10H.86A.0051.2019.0322.1320 03/22/2019
[  643.360784] Call Trace:
[  643.360823]  dump_stack+0x46/0x60
[  643.360865]  sysfs_warn_dup.cold.3+0x17/0x2f
[  643.360914]  sysfs_do_create_link_sd.isra.2+0xa6/0xc0
[  643.360961]  device_add+0x30d/0x660
[  643.360987]  tty_register_device_attr+0xdd/0x1d0
[  643.361018]  ? sysfs_create_file_ns+0x5d/0x90
[  643.361049]  usb_serial_device_probe+0x72/0xf0 [usbserial]
...

Are you sure you actually did register two xhci debug ttys?

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] usb: xhci: dbc: get rid of global pointer

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4661 bytes --]


Hi,

Johan Hovold <johan@kernel.org> writes:
>> Johan Hovold <johan@kernel.org> writes:
>> > On Tue, Jun 11, 2019 at 08:24:16PM +0300, Felipe Balbi wrote:
>> >> If we happen to have two XHCI controllers with DbC capability, then
>> >> there's no hope this will ever work as the global pointer will be
>> >> overwritten by the controller that probes last.
>> >> 
>> >> Avoid this problem by keeping the tty_driver struct pointer inside
>> >> struct xhci_dbc.
>> >
>> > How did you test this patch?
>> 
>> by running it on a machine that actually has two DbCs
>> 
>> >> @@ -279,52 +279,52 @@ static const struct tty_operations dbc_tty_ops = {
>> >>  	.unthrottle		= dbc_tty_unthrottle,
>> >>  };
>> >>  
>> >> -static struct tty_driver *dbc_tty_driver;
>> >> -
>> >>  int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
>> >>  {
>> >>  	int			status;
>> >>  	struct xhci_dbc		*dbc = xhci->dbc;
>> >>  
>> >> -	dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
>> >> +	dbc->tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
>> >>  					  TTY_DRIVER_DYNAMIC_DEV);
>> >> -	if (IS_ERR(dbc_tty_driver)) {
>> >> -		status = PTR_ERR(dbc_tty_driver);
>> >> -		dbc_tty_driver = NULL;
>> >> +	if (IS_ERR(dbc->tty_driver)) {
>> >> +		status = PTR_ERR(dbc->tty_driver);
>> >> +		dbc->tty_driver = NULL;
>> >>  		return status;
>> >>  	}
>> >>  
>> >> -	dbc_tty_driver->driver_name = "dbc_serial";
>> >> -	dbc_tty_driver->name = "ttyDBC";
>> >> +	dbc->tty_driver->driver_name = "dbc_serial";
>> >> +	dbc->tty_driver->name = "ttyDBC";
>> >
>> > You're now registering multiple drivers for the same thing (and wasting
>> > a major number for each) and specifically using the same name, which
>> > should lead to name clashes when registering the second port.
>> 
>> No warnings were printed while running this, actually. Odd
>
> Odd indeed. I get the expected warning from sysfs when trying to
> register a second tty using an already registered name:
>
> [  643.360555] sysfs: cannot create duplicate filename '/class/tty/ttyS0'
> [  643.360637] CPU: 1 PID: 2383 Comm: modprobe Not tainted 5.2.0-rc1 #2
> [  643.360702] Hardware name:  /D34010WYK, BIOS WYLPT10H.86A.0051.2019.0322.1320 03/22/2019
> [  643.360784] Call Trace:
> [  643.360823]  dump_stack+0x46/0x60
> [  643.360865]  sysfs_warn_dup.cold.3+0x17/0x2f
> [  643.360914]  sysfs_do_create_link_sd.isra.2+0xa6/0xc0
> [  643.360961]  device_add+0x30d/0x660
> [  643.360987]  tty_register_device_attr+0xdd/0x1d0
> [  643.361018]  ? sysfs_create_file_ns+0x5d/0x90
> [  643.361049]  usb_serial_device_probe+0x72/0xf0 [usbserial]
> ...
>
> Are you sure you actually did register two xhci debug ttys?

hmm, let me check:

int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
{
	int			ret;
	struct device		*tty_dev;
	struct xhci_dbc		*dbc = xhci->dbc;
	struct dbc_port		*port = &dbc->port;

	xhci_dbc_tty_init_port(xhci, port);
	tty_dev = tty_port_register_device(&port->port,
					   dbc_tty_driver, 0, NULL);

	[...]
}

static void xhci_dbc_handle_events(struct work_struct *work)
{
	int			ret;
	enum evtreturn		evtr;
	struct xhci_dbc		*dbc;
	unsigned long		flags;
	struct xhci_hcd		*xhci;

	dbc = container_of(to_delayed_work(work), struct xhci_dbc, event_work);
	xhci = dbc->xhci;

	spin_lock_irqsave(&dbc->lock, flags);
	evtr = xhci_dbc_do_handle_events(dbc);
	spin_unlock_irqrestore(&dbc->lock, flags);

	switch (evtr) {
	case EVT_GSER:
		ret = xhci_dbc_tty_register_device(xhci);

	[...]
}

static int xhci_dbc_start(struct xhci_hcd *xhci)
{
	int			ret;
	unsigned long		flags;
	struct xhci_dbc		*dbc = xhci->dbc;

	WARN_ON(!dbc);

	pm_runtime_get_sync(xhci_to_hcd(xhci)->self.controller);

	spin_lock_irqsave(&dbc->lock, flags);
	ret = xhci_do_dbc_start(xhci);
	spin_unlock_irqrestore(&dbc->lock, flags);

	if (ret) {
		pm_runtime_put(xhci_to_hcd(xhci)->self.controller);
		return ret;
	}

	return mod_delayed_work(system_wq, &dbc->event_work, 1);
}

static ssize_t dbc_store(struct device *dev,
			 struct device_attribute *attr,
			 const char *buf, size_t count)
{
	struct xhci_hcd		*xhci;

	xhci = hcd_to_xhci(dev_get_drvdata(dev));

	if (!strncmp(buf, "enable", 6))
		xhci_dbc_start(xhci);
	else if (!strncmp(buf, "disable", 7))
		xhci_dbc_stop(xhci);
	else
		return -EINVAL;

	return count;
}

Hmm, so it only really registers after writing to sysfs file. Man, this
is an odd driver :-)

@Mathias, can you drop the previous fix? I'll try to come up with a
better version of this.

@Johan, thanks for the review.

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] usb: xhci: dbc: get rid of global pointer

[Mathias Nyman]

On 19.6.2019 9.33, Felipe Balbi wrote:
> 
> 
> @Mathias, can you drop the previous fix? I'll try to come up with a
> better version of this.

Dropped

-Mathias

Re: [PATCH] usb: xhci: dbc: get rid of global pointer

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

On Wed, Jun 19, 2019 at 09:33:40AM +0300, Felipe Balbi wrote:
> Johan Hovold <johan@kernel.org> writes:

> > Are you sure you actually did register two xhci debug ttys?
> 
> hmm, let me check:

...

> Hmm, so it only really registers after writing to sysfs file. Man, this
> is an odd driver :-)

I hear you. :)

> @Mathias, can you drop the previous fix? I'll try to come up with a
> better version of this.
> 
> @Johan, thanks for the review.

You're welcome.

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]