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

[Felipe Balbi <felipe.balbi@linux.intel.com>]

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