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;
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
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;
- 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
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;
- 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
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;
- 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
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;
- 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
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
linux-stable-mirror@lists.linaro.org
-
Felipe Balbi -
Johan Hovold -
Mathias Nyman