Messages in this thread | | | From | "Gangurde, Abhijit" <> | Subject | RE: [PATCH v2 1/4] cdx: Introduce lock to protect controller ops and controller list | Date | Fri, 4 Aug 2023 11:11:39 +0000 |
| |
> On Wed, Aug 02, 2023 at 11:20:17AM +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? > > > > 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
| |