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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> > > Cc: Herve Codina <herve.codina@xxxxxxxxxxx> > > Cc: Rob Herring <robh@xxxxxxxxxx> > > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Cc: Grant Grundler <grundler@xxxxxxxxxxxx> > > Cc: Oliver Neukum <oneukum@xxxxxxxx> > > Cc: Yajun Deng <yajun.deng@xxxxxxxxx> > > Cc: Douglas Anderson <dianders@xxxxxxxxxxxx> > > Cc: linux-usb@xxxxxxxxxxxxxxx > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxx> > THanks for the reviews!
