Re: [PATCH v2 1/2] USB: make single lock for all usb dynamic id lists

[Greg Kroah-Hartman]

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!