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