| Message ID | 20220216095153.1303105-6-mathias.nyman@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e1ec140f273e1e30cea7e6d5f50934d877232121 |
| Headers | show |
| Series | xhci and hub features for usb-next | expand |
On Wed, Feb 16, 2022 at 11:51:49AM +0200, Mathias Nyman wrote: > To support systems with several xhci controllers with active > dbc on each xhci we need to use IDR to identify and give > an index to each port. > > Avoid using global struct tty_driver.driver_state for storing > dbc port pointer as it won't work with several dbc ports > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > --- > drivers/usb/host/xhci-dbgcap.h | 1 + > drivers/usb/host/xhci-dbgtty.c | 46 ++++++++++++++++++++++++++++------ > 2 files changed, 40 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h > index 5f3304a06591..ca04192fdab1 100644 > --- a/drivers/usb/host/xhci-dbgcap.h > +++ b/drivers/usb/host/xhci-dbgcap.h > @@ -100,6 +100,7 @@ struct dbc_ep { > struct dbc_port { > struct tty_port port; > spinlock_t port_lock; /* port access */ > + int minor; > > struct list_head read_pool; > struct list_head read_queue; > diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c > index 059b58f48e3a..d3acc0829ee5 100644 > --- a/drivers/usb/host/xhci-dbgtty.c > +++ b/drivers/usb/host/xhci-dbgtty.c > @@ -10,11 +10,14 @@ > #include <linux/slab.h> > #include <linux/tty.h> > #include <linux/tty_flip.h> > +#include <linux/idr.h> > > #include "xhci.h" > #include "xhci-dbgcap.h" > > static struct tty_driver *dbc_tty_driver; > +static struct idr dbc_tty_minors; > +static DEFINE_MUTEX(dbc_tty_minors_lock); Why not just use an ida instead? That way you do not need another lock and it becomes a tiny bit simpler overall. I'll take this now, but in the future it might be worth to change this. thanks, greg k-h
On 17.2.2022 17.16, Greg KH wrote: > On Wed, Feb 16, 2022 at 11:51:49AM +0200, Mathias Nyman wrote: >> To support systems with several xhci controllers with active >> dbc on each xhci we need to use IDR to identify and give >> an index to each port. >> >> Avoid using global struct tty_driver.driver_state for storing >> dbc port pointer as it won't work with several dbc ports >> >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> >> --- >> drivers/usb/host/xhci-dbgcap.h | 1 + >> drivers/usb/host/xhci-dbgtty.c | 46 ++++++++++++++++++++++++++++------ >> 2 files changed, 40 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h >> index 5f3304a06591..ca04192fdab1 100644 >> --- a/drivers/usb/host/xhci-dbgcap.h >> +++ b/drivers/usb/host/xhci-dbgcap.h >> @@ -100,6 +100,7 @@ struct dbc_ep { >> struct dbc_port { >> struct tty_port port; >> spinlock_t port_lock; /* port access */ >> + int minor; >> >> struct list_head read_pool; >> struct list_head read_queue; >> diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c >> index 059b58f48e3a..d3acc0829ee5 100644 >> --- a/drivers/usb/host/xhci-dbgtty.c >> +++ b/drivers/usb/host/xhci-dbgtty.c >> @@ -10,11 +10,14 @@ >> #include <linux/slab.h> >> #include <linux/tty.h> >> #include <linux/tty_flip.h> >> +#include <linux/idr.h> >> >> #include "xhci.h" >> #include "xhci-dbgcap.h" >> >> static struct tty_driver *dbc_tty_driver; >> +static struct idr dbc_tty_minors; >> +static DEFINE_MUTEX(dbc_tty_minors_lock); > > Why not just use an ida instead? That way you do not need another lock > and it becomes a tiny bit simpler overall. idr seemed like a good way to tie together the minor number (in tty_struct index) to the right dbc_port structure, which contains a pointer to that specific xhci device This way I could find the right dbc_port and store it in tty_struct driver_data in the tty_operations install callback, allowing later tty read and and write calls to find the correct xhci controller from tty_struct driver_data. Looking at it now it might be possible to use just ida instead, and set the tty_struct driver_data right after calling tty_port_register_device(). We should know everything we need there, id (minor), dbc_port, and can probably get the tty_struct from the newly created tty port device somehow. Should dig into tty a bit more for this. > > I'll take this now, but in the future it might be worth to change this. Thanks -Mathias
diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h index 5f3304a06591..ca04192fdab1 100644 --- a/drivers/usb/host/xhci-dbgcap.h +++ b/drivers/usb/host/xhci-dbgcap.h @@ -100,6 +100,7 @@ struct dbc_ep { struct dbc_port { struct tty_port port; spinlock_t port_lock; /* port access */ + int minor; struct list_head read_pool; struct list_head read_queue; diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c index 059b58f48e3a..d3acc0829ee5 100644 --- a/drivers/usb/host/xhci-dbgtty.c +++ b/drivers/usb/host/xhci-dbgtty.c @@ -10,11 +10,14 @@ #include <linux/slab.h> #include <linux/tty.h> #include <linux/tty_flip.h> +#include <linux/idr.h> #include "xhci.h" #include "xhci-dbgcap.h" static struct tty_driver *dbc_tty_driver; +static struct idr dbc_tty_minors; +static DEFINE_MUTEX(dbc_tty_minors_lock); static inline struct dbc_port *dbc_to_port(struct xhci_dbc *dbc) { @@ -177,7 +180,14 @@ xhci_dbc_free_requests(struct list_head *head) static int dbc_tty_install(struct tty_driver *driver, struct tty_struct *tty) { - struct dbc_port *port = driver->driver_state; + struct dbc_port *port; + + mutex_lock(&dbc_tty_minors_lock); + port = idr_find(&dbc_tty_minors, tty->index); + mutex_unlock(&dbc_tty_minors_lock); + + if (!port) + return -ENXIO; tty->driver_data = port; @@ -406,6 +416,15 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc) xhci_dbc_tty_init_port(dbc, port); + mutex_lock(&dbc_tty_minors_lock); + port->minor = idr_alloc(&dbc_tty_minors, port, 0, 64, GFP_KERNEL); + mutex_unlock(&dbc_tty_minors_lock); + + if (port->minor < 0) { + ret = port->minor; + goto err_idr; + } + ret = kfifo_alloc(&port->write_fifo, DBC_WRITE_BUF_SIZE, GFP_KERNEL); if (ret) goto err_exit_port; @@ -421,7 +440,7 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc) goto err_free_requests; tty_dev = tty_port_register_device(&port->port, - dbc_tty_driver, 0, NULL); + dbc_tty_driver, port->minor, NULL); if (IS_ERR(tty_dev)) { ret = PTR_ERR(tty_dev); goto err_free_requests; @@ -437,6 +456,8 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc) err_free_fifo: kfifo_free(&port->write_fifo); err_exit_port: + idr_remove(&dbc_tty_minors, port->minor); +err_idr: xhci_dbc_tty_exit_port(port); dev_err(dbc->dev, "can't register tty port, err %d\n", ret); @@ -450,10 +471,14 @@ static void xhci_dbc_tty_unregister_device(struct xhci_dbc *dbc) if (!port->registered) return; - tty_unregister_device(dbc_tty_driver, 0); + tty_unregister_device(dbc_tty_driver, port->minor); xhci_dbc_tty_exit_port(port); port->registered = false; + mutex_lock(&dbc_tty_minors_lock); + idr_remove(&dbc_tty_minors, port->minor); + mutex_unlock(&dbc_tty_minors_lock); + kfifo_free(&port->write_fifo); xhci_dbc_free_requests(&port->read_pool); xhci_dbc_free_requests(&port->read_queue); @@ -478,9 +503,8 @@ int xhci_dbc_tty_probe(struct device *dev, void __iomem *base, struct xhci_hcd * if (!port) return -ENOMEM; - dbc_tty_driver->driver_state = port; - dbc = xhci_alloc_dbc(dev, base, &dbc_driver); + if (!dbc) { status = -ENOMEM; goto out2; @@ -514,10 +538,14 @@ int dbc_tty_init(void) { int ret; - dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW | + idr_init(&dbc_tty_minors); + + dbc_tty_driver = tty_alloc_driver(64, TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV); - if (IS_ERR(dbc_tty_driver)) + if (IS_ERR(dbc_tty_driver)) { + idr_destroy(&dbc_tty_minors); return PTR_ERR(dbc_tty_driver); + } dbc_tty_driver->driver_name = "dbc_serial"; dbc_tty_driver->name = "ttyDBC"; @@ -536,7 +564,9 @@ int dbc_tty_init(void) if (ret) { pr_err("Can't register dbc tty driver\n"); tty_driver_kref_put(dbc_tty_driver); + idr_destroy(&dbc_tty_minors); } + return ret; } @@ -547,4 +577,6 @@ void dbc_tty_exit(void) tty_driver_kref_put(dbc_tty_driver); dbc_tty_driver = NULL; } + + idr_destroy(&dbc_tty_minors); }
To support systems with several xhci controllers with active dbc on each xhci we need to use IDR to identify and give an index to each port. Avoid using global struct tty_driver.driver_state for storing dbc port pointer as it won't work with several dbc ports Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> --- drivers/usb/host/xhci-dbgcap.h | 1 + drivers/usb/host/xhci-dbgtty.c | 46 ++++++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 7 deletions(-)