Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] usb: xhci: dbc: get rid of global pointer
@ 2019-06-11 17:24 Felipe Balbi
  2019-06-14 12:26 ` Mathias Nyman
  2019-06-14 14:52 ` Johan Hovold
  0 siblings, 2 replies; 8+ messages in thread
From: Felipe Balbi @ 2019-06-11 17:24 UTC (permalink / raw)
  To: Mathias Nyman; +Cc: linux-usb, Felipe Balbi, stable

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: xhci: dbc: get rid of global pointer
  2019-06-11 17:24 [PATCH] usb: xhci: dbc: get rid of global pointer Felipe Balbi
@ 2019-06-14 12:26 ` Mathias Nyman
  2019-06-14 14:52 ` Johan Hovold
  1 sibling, 0 replies; 8+ messages in thread
From: Mathias Nyman @ 2019-06-14 12:26 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: linux-usb, stable

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: xhci: dbc: get rid of global pointer
  2019-06-11 17:24 [PATCH] usb: xhci: dbc: get rid of global pointer Felipe Balbi
  2019-06-14 12:26 ` Mathias Nyman
@ 2019-06-14 14:52 ` Johan Hovold
  2019-06-17  6:43   ` Felipe Balbi
  1 sibling, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2019-06-14 14:52 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Mathias Nyman, linux-usb, stable

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: xhci: dbc: get rid of global pointer
  2019-06-14 14:52 ` Johan Hovold
@ 2019-06-17  6:43   ` Felipe Balbi
  2019-06-18 14:31     ` Johan Hovold
  0 siblings, 1 reply; 8+ messages in thread
From: Felipe Balbi @ 2019-06-17  6:43 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Mathias Nyman, linux-usb, stable

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: xhci: dbc: get rid of global pointer
  2019-06-17  6:43   ` Felipe Balbi
@ 2019-06-18 14:31     ` Johan Hovold
  2019-06-19  6:33       ` Felipe Balbi
  0 siblings, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2019-06-18 14:31 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Johan Hovold, Mathias Nyman, linux-usb, stable

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: xhci: dbc: get rid of global pointer
  2019-06-18 14:31     ` Johan Hovold
@ 2019-06-19  6:33       ` Felipe Balbi
  2019-06-19  7:27         ` Mathias Nyman
  2019-06-19 12:14         ` Johan Hovold
  0 siblings, 2 replies; 8+ messages in thread
From: Felipe Balbi @ 2019-06-19  6:33 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Johan Hovold, Mathias Nyman, linux-usb, stable

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: xhci: dbc: get rid of global pointer
  2019-06-19  6:33       ` Felipe Balbi
@ 2019-06-19  7:27         ` Mathias Nyman
  2019-06-19 12:14         ` Johan Hovold
  1 sibling, 0 replies; 8+ messages in thread
From: Mathias Nyman @ 2019-06-19  7:27 UTC (permalink / raw)
  To: Felipe Balbi, Johan Hovold; +Cc: linux-usb, stable

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: xhci: dbc: get rid of global pointer
  2019-06-19  6:33       ` Felipe Balbi
  2019-06-19  7:27         ` Mathias Nyman
@ 2019-06-19 12:14         ` Johan Hovold
  1 sibling, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2019-06-19 12:14 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Johan Hovold, Mathias Nyman, linux-usb, stable

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-11 17:24 [PATCH] usb: xhci: dbc: get rid of global pointer Felipe Balbi
2019-06-14 12:26 ` Mathias Nyman
2019-06-14 14:52 ` Johan Hovold
2019-06-17  6:43   ` Felipe Balbi
2019-06-18 14:31     ` Johan Hovold
2019-06-19  6:33       ` Felipe Balbi
2019-06-19  7:27         ` Mathias Nyman
2019-06-19 12:14         ` Johan Hovold

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox