Hi,
Johan Hovold <johan@xxxxxxxxxx> writes:
>> Johan Hovold <johan@xxxxxxxxxx> 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:
signature.asc
Description: PGP signature
