| Message ID | 20190318211713.3462-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert USB subsystem to XArray | expand |
On Mon, Mar 18, 2019 at 02:17:11PM -0700, Matthew Wilcox wrote: > Signed-off-by: Matthew Wilcox <willy@infradead.org> Also something here saying acm_minors was an idr structure that is being converted to xarray. > --- > drivers/usb/class/cdc-acm.c | 33 ++++++++++++++------------------- > 1 file changed, 14 insertions(+), 19 deletions(-) > > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c > index 739f8960811a..28eb9a898b4a 100644 > --- a/drivers/usb/class/cdc-acm.c > +++ b/drivers/usb/class/cdc-acm.c > @@ -47,7 +47,7 @@ > static struct usb_driver acm_driver; > static struct tty_driver *acm_tty_driver; > > -static DEFINE_IDR(acm_minors); > +static DEFINE_XARRAY_ALLOC(acm_minors); > static DEFINE_MUTEX(acm_minors_lock); > > static void acm_tty_set_termios(struct tty_struct *tty, > @@ -66,7 +66,7 @@ static struct acm *acm_get_by_minor(unsigned int minor) > struct acm *acm; > > mutex_lock(&acm_minors_lock); > - acm = idr_find(&acm_minors, minor); > + acm = xa_load(&acm_minors, minor); > if (acm) { > mutex_lock(&acm->mutex); > if (acm->disconnected) { > @@ -86,20 +86,15 @@ static struct acm *acm_get_by_minor(unsigned int minor) > */ > static int acm_alloc_minor(struct acm *acm) > { > - int minor; > - > - mutex_lock(&acm_minors_lock); > - minor = idr_alloc(&acm_minors, acm, 0, ACM_TTY_MINORS, GFP_KERNEL); > - mutex_unlock(&acm_minors_lock); > - > - return minor; > + return xa_alloc(&acm_minors, &acm->minor, acm, > + XA_LIMIT(0, ACM_TTY_MINORS - 1), GFP_KERNEL); So, the only thing "better" here is that you don't need a lock anymore? Why not just replace the idr api "underneath" with xarray and then drop all of the locks over time? idr was just a wrapper on top of ida, what's one more :) Anyway, no objection from me, I like tree-wide changes, and appreciate the effort that goes into it. Thanks for doing it. If you fix up the changelog entries for the ones that have none, I'll be glad to queue these up. thanks, greg k-h
On Mo, 2019-03-18 at 14:17 -0700, Matthew Wilcox wrote:
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
Straight replacement. Looks fine to me. But without
a commit message Greg would dismember both of us.
Regards
Oliver
On Di, 2019-03-19 at 13:14 +0100, Greg KH wrote: > On Mon, Mar 18, 2019 at 02:17:11PM -0700, Matthew Wilcox wrote: > > Signed-off-by: Matthew Wilcox <willy@infradead.org> [..] > > static int acm_alloc_minor(struct acm *acm) > > { > > - int minor; > > - > > - mutex_lock(&acm_minors_lock); > > - minor = idr_alloc(&acm_minors, acm, 0, ACM_TTY_MINORS, GFP_KERNEL); > > - mutex_unlock(&acm_minors_lock); > > - > > - return minor; > > + return xa_alloc(&acm_minors, &acm->minor, acm, > > + XA_LIMIT(0, ACM_TTY_MINORS - 1), GFP_KERNEL); > > So, the only thing "better" here is that you don't need a lock anymore? Once this is accepted I can reduce the locking to a per instance spinlock. But to start with a straight conversion is best. Regards Oliver
On Tue, Mar 19, 2019 at 01:14:20PM +0100, Greg KH wrote: > On Mon, Mar 18, 2019 at 02:17:11PM -0700, Matthew Wilcox wrote: > > Signed-off-by: Matthew Wilcox <willy@infradead.org> > > Also something here saying acm_minors was an idr structure that is being > converted to xarray. ACK. > > @@ -86,20 +86,15 @@ static struct acm *acm_get_by_minor(unsigned int minor) > > */ > > static int acm_alloc_minor(struct acm *acm) > > { > > - int minor; > > - > > - mutex_lock(&acm_minors_lock); > > - minor = idr_alloc(&acm_minors, acm, 0, ACM_TTY_MINORS, GFP_KERNEL); > > - mutex_unlock(&acm_minors_lock); > > - > > - return minor; > > + return xa_alloc(&acm_minors, &acm->minor, acm, > > + XA_LIMIT(0, ACM_TTY_MINORS - 1), GFP_KERNEL); > > So, the only thing "better" here is that you don't need a lock anymore? As far as this driver is concerned, I think that's the only advantage. Other drivers see more advantages. > Why not just replace the idr api "underneath" with xarray and then drop > all of the locks over time? > > idr was just a wrapper on top of ida, what's one more :) umm, IDR was never a wrapper over IDA. Originally, it was its own data structure, then IDA was grafted on top of IDR, then I merged the IDR and radix tree, now I'm replacing both the IDR and the radix tree. > Anyway, no objection from me, I like tree-wide changes, and appreciate > the effort that goes into it. Thanks for doing it. If you fix up the > changelog entries for the ones that have none, I'll be glad to queue > these up. Thanks.
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index 739f8960811a..28eb9a898b4a 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -47,7 +47,7 @@ static struct usb_driver acm_driver; static struct tty_driver *acm_tty_driver; -static DEFINE_IDR(acm_minors); +static DEFINE_XARRAY_ALLOC(acm_minors); static DEFINE_MUTEX(acm_minors_lock); static void acm_tty_set_termios(struct tty_struct *tty, @@ -66,7 +66,7 @@ static struct acm *acm_get_by_minor(unsigned int minor) struct acm *acm; mutex_lock(&acm_minors_lock); - acm = idr_find(&acm_minors, minor); + acm = xa_load(&acm_minors, minor); if (acm) { mutex_lock(&acm->mutex); if (acm->disconnected) { @@ -86,20 +86,15 @@ static struct acm *acm_get_by_minor(unsigned int minor) */ static int acm_alloc_minor(struct acm *acm) { - int minor; - - mutex_lock(&acm_minors_lock); - minor = idr_alloc(&acm_minors, acm, 0, ACM_TTY_MINORS, GFP_KERNEL); - mutex_unlock(&acm_minors_lock); - - return minor; + return xa_alloc(&acm_minors, &acm->minor, acm, + XA_LIMIT(0, ACM_TTY_MINORS - 1), GFP_KERNEL); } /* Release the minor number associated with 'acm'. */ static void acm_release_minor(struct acm *acm) { mutex_lock(&acm_minors_lock); - idr_remove(&acm_minors, acm->minor); + xa_erase(&acm_minors, acm->minor); mutex_unlock(&acm_minors_lock); } @@ -1130,7 +1125,6 @@ static int acm_probe(struct usb_interface *intf, struct usb_device *usb_dev = interface_to_usbdev(intf); struct usb_cdc_parsed_header h; struct acm *acm; - int minor; int ctrlsize, readsize; u8 *buf; int call_intf_num = -1; @@ -1302,9 +1296,10 @@ static int acm_probe(struct usb_interface *intf, tty_port_init(&acm->port); acm->port.ops = &acm_port_ops; - minor = acm_alloc_minor(acm); - if (minor < 0) - goto alloc_fail1; + rv = acm_alloc_minor(acm); + if (rv < 0) + goto alloc_fail0; + rv = -ENOMEM; ctrlsize = usb_endpoint_maxp(epctrl); readsize = usb_endpoint_maxp(epread) * @@ -1313,7 +1308,6 @@ static int acm_probe(struct usb_interface *intf, acm->writesize = usb_endpoint_maxp(epwrite) * 20; acm->control = control_interface; acm->data = data_interface; - acm->minor = minor; acm->dev = usb_dev; if (h.usb_cdc_acm_descriptor) acm->ctrl_caps = h.usb_cdc_acm_descriptor->bmCapabilities; @@ -1450,7 +1444,7 @@ static int acm_probe(struct usb_interface *intf, acm->nb_index = 0; acm->nb_size = 0; - dev_info(&intf->dev, "ttyACM%d: USB ACM device\n", minor); + dev_info(&intf->dev, "ttyACM%d: USB ACM device\n", acm->minor); acm->line.dwDTERate = cpu_to_le32(9600); acm->line.bDataBits = 8; @@ -1460,8 +1454,8 @@ static int acm_probe(struct usb_interface *intf, usb_set_intfdata(data_interface, acm); usb_get_intf(control_interface); - tty_dev = tty_port_register_device(&acm->port, acm_tty_driver, minor, - &control_interface->dev); + tty_dev = tty_port_register_device(&acm->port, acm_tty_driver, + acm->minor, &control_interface->dev); if (IS_ERR(tty_dev)) { rv = PTR_ERR(tty_dev); goto alloc_fail6; @@ -1496,6 +1490,8 @@ static int acm_probe(struct usb_interface *intf, alloc_fail2: usb_free_coherent(usb_dev, ctrlsize, acm->ctrl_buffer, acm->ctrl_dma); alloc_fail1: + acm_release_minor(acm); +alloc_fail0: tty_port_put(&acm->port); alloc_fail: return rv; @@ -1986,7 +1982,6 @@ static void __exit acm_exit(void) usb_deregister(&acm_driver); tty_unregister_driver(acm_tty_driver); put_tty_driver(acm_tty_driver); - idr_destroy(&acm_minors); } module_init(acm_init);
Signed-off-by: Matthew Wilcox <willy@infradead.org> --- drivers/usb/class/cdc-acm.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-)