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

From: Johan Hovold <johan@kernel.org>
To: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Mathias Nyman <mathias.nyman@linux.intel.com>,
	linux-usb@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] usb: xhci: dbc: get rid of global pointer
Date: Fri, 14 Jun 2019 16:52:36 +0200	[thread overview]
Message-ID: <20190614145236.GB3849@localhost> (raw)
In-Reply-To: <20190611172416.12473-1-felipe.balbi@linux.intel.com>

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