Re: [PATCH v2 1/4] cdx: Introduce lock to protect controller ops and controller list

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Aug 01, 2023 at 08:34:00AM +0000, Gangurde, Abhijit wrote:
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Monday, July 31, 2023 5:55 PM
> > To: Gangurde, Abhijit <abhijit.gangurde@amd.com>
> > Cc: masahiroy@kernel.org; linux-kernel@vger.kernel.org; Simek, Michal
> > <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com>; Agarwal, Nikhil
> > <nikhil.agarwal@amd.com>; Gupta, Nipun <Nipun.Gupta@amd.com>
> > Subject: Re: [PATCH v2 1/4] cdx: Introduce lock to protect controller ops and
> > controller list
> > 
> > On Mon, Jul 31, 2023 at 05:38:10PM +0530, Abhijit Gangurde wrote:
> > > Add a mutex lock to prevent race between controller ops initiated by
> > > the bus subsystem and the controller registration/unregistration.
> > >
> > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > > ---
> > >  drivers/cdx/cdx.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> > > index d2cad4c670a0..66797c8fe400 100644
> > > --- a/drivers/cdx/cdx.c
> > > +++ b/drivers/cdx/cdx.c
> > > @@ -72,6 +72,8 @@
> > >
> > >  /* CDX controllers registered with the CDX bus */
> > >  static DEFINE_XARRAY_ALLOC(cdx_controllers);
> > > +/* Lock to protect controller ops and controller list */
> > > +static DEFINE_MUTEX(cdx_controller_lock);
> > 
> > Wait, why do you have a local list and not just rely on the list the
> > driver core has for you already?  Isn't this a duplicate list where you
> > have objects on two different lists with a lifespan controlled only by
> > one of them?
> 
> cdx_controllers list is holding just the controllers registered on the cdx bus system.

Which are devices on the bus, so why do you need a separate list?

> CDX devices are still maintained by driver core list. Controller list is used by rescan
> which triggers rescan on all the controllers.

Again, why a separate list?  The driver core already tracks these,
right?

thanks,

greg k-h