| Message ID | 2024111322-kindly-finalist-d247@gregkh (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0b3144da31f855fce652303f588416a60991bdef |
| Headers | show |
| Series | [v2,1/2] USB: make single lock for all usb dynamic id lists | expand |
Hi, On Tue, Nov 12, 2024 at 10:49 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > There are a number of places where we accidentally pass in a constant > structure to later cast it off to a dynamic one, and then attempt to > grab a lock on it, which is not a good idea. To help resolve this, move > the dynamic id lock out of the dynamic id structure for the driver and > into one single lock for all USB dynamic ids. As this lock should never > have any real contention (it's only every accessed when a device is nit: s/every/ever/ > added or removed, which is always serialized) there should not be any > difference except for some memory savings. > > Note, this just converts the existing use of the dynamic id lock to the > new static lock, there is one place that is accessing the dynamic id > list without grabbing the lock, that will be fixed up in a follow-on > change. > > Cc: Johan Hovold <johan@kernel.org> > Cc: Herve Codina <herve.codina@bootlin.com> > Cc: Rob Herring <robh@kernel.org> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Grant Grundler <grundler@chromium.org> > Cc: Oliver Neukum <oneukum@suse.com> > Cc: Yajun Deng <yajun.deng@linux.dev> > Cc: Douglas Anderson <dianders@chromium.org> > Cc: linux-usb@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > v2: - change to a mutex > - strip out of larger series > > drivers/usb/common/common.c | 3 +++ > drivers/usb/core/driver.c | 15 +++++---------- > drivers/usb/serial/bus.c | 4 +--- > drivers/usb/serial/usb-serial.c | 4 +--- > include/linux/usb.h | 2 +- > 5 files changed, 11 insertions(+), 17 deletions(-) I'm not familiar enough with the code to confirm with 100% certainty your assertions that there won't be any contention problems with this lock. However, your argument sounds reasonable to me and, since you are much more familiar with the subsystem, I'll believe you. :-) I would have a slight concern that you are changing a "spin_lock" to a "mutex", which doesn't seem to be talked about in the patch description. This is likely to be fine given that all of the users are "spin_lock" and not "spin_lock_irq" or "spin_lock_irqsave", but it still makes me worried that there's some random bit of code somewhere that calls one of these functions while sleeping is disabled. I guess that's not likely. In any case, this seems OK to me assuming it tests well. Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Thu, Nov 14, 2024 at 02:37:10PM -0800, Doug Anderson wrote: > Hi, > > On Tue, Nov 12, 2024 at 10:49 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > There are a number of places where we accidentally pass in a constant > > structure to later cast it off to a dynamic one, and then attempt to > > grab a lock on it, which is not a good idea. To help resolve this, move > > the dynamic id lock out of the dynamic id structure for the driver and > > into one single lock for all USB dynamic ids. As this lock should never > > have any real contention (it's only every accessed when a device is > > nit: s/every/ever/ > > > > added or removed, which is always serialized) there should not be any > > difference except for some memory savings. > > > > Note, this just converts the existing use of the dynamic id lock to the > > new static lock, there is one place that is accessing the dynamic id > > list without grabbing the lock, that will be fixed up in a follow-on > > change. > > > > Cc: Johan Hovold <johan@kernel.org> > > Cc: Herve Codina <herve.codina@bootlin.com> > > Cc: Rob Herring <robh@kernel.org> > > Cc: Alan Stern <stern@rowland.harvard.edu> > > Cc: Grant Grundler <grundler@chromium.org> > > Cc: Oliver Neukum <oneukum@suse.com> > > Cc: Yajun Deng <yajun.deng@linux.dev> > > Cc: Douglas Anderson <dianders@chromium.org> > > Cc: linux-usb@vger.kernel.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > v2: - change to a mutex > > - strip out of larger series > > > > drivers/usb/common/common.c | 3 +++ > > drivers/usb/core/driver.c | 15 +++++---------- > > drivers/usb/serial/bus.c | 4 +--- > > drivers/usb/serial/usb-serial.c | 4 +--- > > include/linux/usb.h | 2 +- > > 5 files changed, 11 insertions(+), 17 deletions(-) > > I'm not familiar enough with the code to confirm with 100% certainty > your assertions that there won't be any contention problems with this > lock. However, your argument sounds reasonable to me and, since you > are much more familiar with the subsystem, I'll believe you. :-) > > I would have a slight concern that you are changing a "spin_lock" to a > "mutex", which doesn't seem to be talked about in the patch > description. This is likely to be fine given that all of the users are > "spin_lock" and not "spin_lock_irq" or "spin_lock_irqsave", but it > still makes me worried that there's some random bit of code somewhere > that calls one of these functions while sleeping is disabled. I guess > that's not likely. > > In any case, this seems OK to me assuming it tests well. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > THanks for the reviews!
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c index b7bea1015d7c..871cf199b6bf 100644 --- a/drivers/usb/common/common.c +++ b/drivers/usb/common/common.c @@ -415,6 +415,9 @@ EXPORT_SYMBOL_GPL(usb_of_get_companion_dev); struct dentry *usb_debug_root; EXPORT_SYMBOL_GPL(usb_debug_root); +DEFINE_MUTEX(usb_dynids_lock); +EXPORT_SYMBOL_GPL(usb_dynids_lock); + static int __init usb_common_init(void) { usb_debug_root = debugfs_create_dir("usb", NULL); diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index 0c3f12daac79..bc3c00580238 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -95,9 +95,9 @@ ssize_t usb_store_new_id(struct usb_dynids *dynids, } } - spin_lock(&dynids->lock); + mutex_lock(&usb_dynids_lock); list_add_tail(&dynid->node, &dynids->list); - spin_unlock(&dynids->lock); + mutex_unlock(&usb_dynids_lock); retval = driver_attach(driver); @@ -160,7 +160,7 @@ static ssize_t remove_id_store(struct device_driver *driver, const char *buf, if (fields < 2) return -EINVAL; - spin_lock(&usb_driver->dynids.lock); + guard(mutex)(&usb_dynids_lock); list_for_each_entry_safe(dynid, n, &usb_driver->dynids.list, node) { struct usb_device_id *id = &dynid->id; @@ -171,7 +171,6 @@ static ssize_t remove_id_store(struct device_driver *driver, const char *buf, break; } } - spin_unlock(&usb_driver->dynids.lock); return count; } @@ -220,12 +219,11 @@ static void usb_free_dynids(struct usb_driver *usb_drv) { struct usb_dynid *dynid, *n; - spin_lock(&usb_drv->dynids.lock); + guard(mutex)(&usb_dynids_lock); list_for_each_entry_safe(dynid, n, &usb_drv->dynids.list, node) { list_del(&dynid->node); kfree(dynid); } - spin_unlock(&usb_drv->dynids.lock); } static const struct usb_device_id *usb_match_dynamic_id(struct usb_interface *intf, @@ -233,14 +231,12 @@ static const struct usb_device_id *usb_match_dynamic_id(struct usb_interface *in { struct usb_dynid *dynid; - spin_lock(&drv->dynids.lock); + guard(mutex)(&usb_dynids_lock); list_for_each_entry(dynid, &drv->dynids.list, node) { if (usb_match_one_id(intf, &dynid->id)) { - spin_unlock(&drv->dynids.lock); return &dynid->id; } } - spin_unlock(&drv->dynids.lock); return NULL; } @@ -1076,7 +1072,6 @@ int usb_register_driver(struct usb_driver *new_driver, struct module *owner, new_driver->driver.owner = owner; new_driver->driver.mod_name = mod_name; new_driver->driver.dev_groups = new_driver->dev_groups; - spin_lock_init(&new_driver->dynids.lock); INIT_LIST_HEAD(&new_driver->dynids.list); retval = driver_register(&new_driver->driver); diff --git a/drivers/usb/serial/bus.c b/drivers/usb/serial/bus.c index d200e2c29a8f..2fea1b1db4a2 100644 --- a/drivers/usb/serial/bus.c +++ b/drivers/usb/serial/bus.c @@ -136,12 +136,11 @@ static void free_dynids(struct usb_serial_driver *drv) { struct usb_dynid *dynid, *n; - spin_lock(&drv->dynids.lock); + guard(mutex)(&usb_dynids_lock); list_for_each_entry_safe(dynid, n, &drv->dynids.list, node) { list_del(&dynid->node); kfree(dynid); } - spin_unlock(&drv->dynids.lock); } const struct bus_type usb_serial_bus_type = { @@ -157,7 +156,6 @@ int usb_serial_bus_register(struct usb_serial_driver *driver) int retval; driver->driver.bus = &usb_serial_bus_type; - spin_lock_init(&driver->dynids.lock); INIT_LIST_HEAD(&driver->dynids.list); retval = driver_register(&driver->driver); diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c index df6a2ae0bf42..7266558d823a 100644 --- a/drivers/usb/serial/usb-serial.c +++ b/drivers/usb/serial/usb-serial.c @@ -706,14 +706,12 @@ static const struct usb_device_id *match_dynamic_id(struct usb_interface *intf, { struct usb_dynid *dynid; - spin_lock(&drv->dynids.lock); + guard(mutex)(&usb_dynids_lock); list_for_each_entry(dynid, &drv->dynids.list, node) { if (usb_match_one_id(intf, &dynid->id)) { - spin_unlock(&drv->dynids.lock); return &dynid->id; } } - spin_unlock(&drv->dynids.lock); return NULL; } diff --git a/include/linux/usb.h b/include/linux/usb.h index 672d8fc2abdb..b66b1af3e439 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1129,8 +1129,8 @@ static inline int usb_make_path(struct usb_device *dev, char *buf, size_t size) /* ----------------------------------------------------------------------- */ /* Stuff for dynamic usb ids */ +extern struct mutex usb_dynids_lock; struct usb_dynids { - spinlock_t lock; struct list_head list; };
There are a number of places where we accidentally pass in a constant structure to later cast it off to a dynamic one, and then attempt to grab a lock on it, which is not a good idea. To help resolve this, move the dynamic id lock out of the dynamic id structure for the driver and into one single lock for all USB dynamic ids. As this lock should never have any real contention (it's only every accessed when a device is added or removed, which is always serialized) there should not be any difference except for some memory savings. Note, this just converts the existing use of the dynamic id lock to the new static lock, there is one place that is accessing the dynamic id list without grabbing the lock, that will be fixed up in a follow-on change. Cc: Johan Hovold <johan@kernel.org> Cc: Herve Codina <herve.codina@bootlin.com> Cc: Rob Herring <robh@kernel.org> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Grant Grundler <grundler@chromium.org> Cc: Oliver Neukum <oneukum@suse.com> Cc: Yajun Deng <yajun.deng@linux.dev> Cc: Douglas Anderson <dianders@chromium.org> Cc: linux-usb@vger.kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- v2: - change to a mutex - strip out of larger series drivers/usb/common/common.c | 3 +++ drivers/usb/core/driver.c | 15 +++++---------- drivers/usb/serial/bus.c | 4 +--- drivers/usb/serial/usb-serial.c | 4 +--- include/linux/usb.h | 2 +- 5 files changed, 11 insertions(+), 17 deletions(-)