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 <[email protected]>
---
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);
/**
* cdx_dev_reset - Reset a CDX device
@@ -393,6 +395,8 @@ static ssize_t rescan_store(const struct bus_type *bus,
if (!val)
return -EINVAL;
+ mutex_lock(&cdx_controller_lock);
+
/* Unregister all the devices on the bus */
cdx_unregister_devices(&cdx_bus_type);
@@ -405,6 +409,8 @@ static ssize_t rescan_store(const struct bus_type *bus,
dev_err(cdx->dev, "cdx bus scanning failed\n");
}
+ mutex_unlock(&cdx_controller_lock);
+
return count;
}
static BUS_ATTR_WO(rescan);
@@ -520,9 +526,12 @@ int cdx_register_controller(struct cdx_controller *cdx)
{
int ret;
+ mutex_lock(&cdx_controller_lock);
+
ret = xa_alloc(&cdx_controllers, &cdx->id, cdx,
XA_LIMIT(0, MAX_CDX_CONTROLLERS - 1), GFP_KERNEL);
if (ret) {
+ mutex_unlock(&cdx_controller_lock);
dev_err(cdx->dev,
"No free index available. Maximum controllers already registered\n");
cdx->id = (u8)MAX_CDX_CONTROLLERS;
@@ -531,6 +540,7 @@ int cdx_register_controller(struct cdx_controller *cdx)
/* Scan all the devices */
cdx->ops->scan(cdx);
+ mutex_unlock(&cdx_controller_lock);
return 0;
}
@@ -541,8 +551,12 @@ void cdx_unregister_controller(struct cdx_controller *cdx)
if (cdx->id >= MAX_CDX_CONTROLLERS)
return;
+ mutex_lock(&cdx_controller_lock);
+
device_for_each_child(cdx->dev, NULL, cdx_unregister_device);
xa_erase(&cdx_controllers, cdx->id);
+
+ mutex_unlock(&cdx_controller_lock);
}
EXPORT_SYMBOL_GPL(cdx_unregister_controller);
--
2.25.1
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 <[email protected]>
> ---
> 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?
thanks,
greg k-h
On Tue, Aug 01, 2023 at 08:34:00AM +0000, Gangurde, Abhijit wrote:
> > From: Greg KH <[email protected]>
> > Sent: Monday, July 31, 2023 5:55 PM
> > To: Gangurde, Abhijit <[email protected]>
> > Cc: [email protected]; [email protected]; Simek, Michal
> > <[email protected]>; git (AMD-Xilinx) <[email protected]>; Agarwal, Nikhil
> > <[email protected]>; Gupta, Nipun <[email protected]>
> > 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 <[email protected]>
> > > ---
> > > 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
> From: Greg KH <[email protected]>
> Sent: Monday, July 31, 2023 5:55 PM
> To: Gangurde, Abhijit <[email protected]>
> Cc: [email protected]; [email protected]; Simek, Michal
> <[email protected]>; git (AMD-Xilinx) <[email protected]>; Agarwal, Nikhil
> <[email protected]>; Gupta, Nipun <[email protected]>
> 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 <[email protected]>
> > ---
> > 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.
CDX devices are still maintained by driver core list. Controller list is used by rescan
which triggers rescan on all the controllers.
Thanks,
Abhijit
> > > From: Greg KH <[email protected]>
> > > Sent: Monday, July 31, 2023 5:55 PM
> > > To: Gangurde, Abhijit <[email protected]>
> > > Cc: [email protected]; [email protected]; Simek, Michal
> > > <[email protected]>; git (AMD-Xilinx) <[email protected]>; Agarwal,
> Nikhil
> > > <[email protected]>; Gupta, Nipun <[email protected]>
> > > 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 <[email protected]>
> > > > ---
> > > > 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.
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.
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.
Thanks,
Abhijit
On Wed, Aug 02, 2023 at 11:20:17AM +0000, Gangurde, Abhijit wrote:
> > > > From: Greg KH <[email protected]>
> > > > Sent: Monday, July 31, 2023 5:55 PM
> > > > To: Gangurde, Abhijit <[email protected]>
> > > > Cc: [email protected]; [email protected]; Simek, Michal
> > > > <[email protected]>; git (AMD-Xilinx) <[email protected]>; Agarwal,
> > Nikhil
> > > > <[email protected]>; Gupta, Nipun <[email protected]>
> > > > 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 <[email protected]>
> > > > > ---
> > > > > 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.)
> 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
> On Wed, Aug 02, 2023 at 11:20:17AM +0000, Gangurde, Abhijit wrote:
> > > > > From: Greg KH <[email protected]>
> > > > > Sent: Monday, July 31, 2023 5:55 PM
> > > > > To: Gangurde, Abhijit <[email protected]>
> > > > > Cc: [email protected]; [email protected]; Simek, Michal
> > > > > <[email protected]>; git (AMD-Xilinx) <[email protected]>; Agarwal,
> > > Nikhil
> > > > > <[email protected]>; Gupta, Nipun <[email protected]>
> > > > > 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 <[email protected]>
> > > > > > ---
> > > > > > 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