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

[Gangurde, Abhijit]

> On Wed, Aug 02, 2023 at 11:20:17AM +0000, Gangurde, Abhijit wrote:
> > > > > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > > Sent: Monday, July 31, 2023 5:55 PM
> > > > > To: Gangurde, Abhijit <abhijit.gangurde@xxxxxxx>
> > > > > Cc: masahiroy@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Simek, Michal
> > > > > <michal.simek@xxxxxxx>; git (AMD-Xilinx) <git@xxxxxxx>; Agarwal,
> > > Nikhil
> > > > > <nikhil.agarwal@xxxxxxx>; Gupta, Nipun <Nipun.Gupta@xxxxxxx>
> > > > > 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@xxxxxxx>
> > > > > > ---
> > > > > >  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?
> >
> > As of now, cdx controllers are platform devices and maintained on
> cdx_controllers list.
> 
> Oh, that's not ok.  Please do NOT abuse platform devices for things that
> are not actually platform devices.  Make them real devices on a real
> bus.
> 
> > CDX controller devices are not added on cdx bus. IIUC, you mean to use
> driver core
> > list to find out different cdx controllers, in that case cdx bus would need to
> scan
> > platform bus and access the private data of platform device to get a
> cdx_controller ops.
> > IMHO, that would not be a right approach.
> 
> If these are actually real patform devices, then yes, that's the correct
> thing to do.
> 
> Or you can create a cdx controller device and add that to the device
> tree, that's usually the way "controller" devices work (look at USB host
> controllers as one example.)

Thank you for the suggestion. CDX controller is already in the device tree as
a platform device. We are weighing both the options right now and would
send the changes accordingly in next spin.

Thanks,
Abhijit

> 
> > Or as an alternative cdx controller could be added on cdx bus as well. And we
> can then
> > get these controllers from driver core list.
> 
> Yes, that can work too, but don't keep them outside of the driver model,
> that will not work well over time, as you can see here already.
> 
> thanks,
> 
> greg k-h