1. Patchew
  2. linux
  3. [PATCH v3 0/5] cdx: provide sysfs interface for cdx device resources
  4. View patch

[PATCH v3 1/5] cdx: Remove cdx controller list from cdx bus system

Abhijit Gangurde posted 5 patches 2 years, 4 months ago
There is a newer version of this series
[PATCH v3 1/5] cdx: Remove cdx controller list from cdx bus system
Posted by Abhijit Gangurde 2 years, 4 months ago 
Remove xarray list of cdx controller. Instead, use platform bus
to locate the cdx controller using compat string used by cdx
controller platform driver.

Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
---
 drivers/cdx/cdx.c           | 39 ++++++++++++++++++++++++-------------
 include/linux/cdx/cdx_bus.h |  2 ++
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index d2cad4c670a0..1c0f76cf4f15 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -60,7 +60,7 @@
 #include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/mm.h>
-#include <linux/xarray.h>
+#include <linux/idr.h>
 #include <linux/cdx/cdx_bus.h>
 #include <linux/iommu.h>
 #include <linux/dma-map-ops.h>
@@ -70,8 +70,10 @@
 #define CDX_DEFAULT_DMA_MASK	(~0ULL)
 #define MAX_CDX_CONTROLLERS 16
 
-/* CDX controllers registered with the CDX bus */
-static DEFINE_XARRAY_ALLOC(cdx_controllers);
+/* IDA for CDX controllers registered with the CDX bus */
+DEFINE_IDA(cdx_controller_ida);
+
+static char *compat_node_name = "xlnx,versal-net-cdx";
 
 /**
  * cdx_dev_reset - Reset a CDX device
@@ -384,7 +386,8 @@ static ssize_t rescan_store(const struct bus_type *bus,
 			    const char *buf, size_t count)
 {
 	struct cdx_controller *cdx;
-	unsigned long index;
+	struct platform_device *pd;
+	struct device_node *np;
 	bool val;
 
 	if (kstrtobool(buf, &val) < 0)
@@ -397,12 +400,19 @@ static ssize_t rescan_store(const struct bus_type *bus,
 	cdx_unregister_devices(&cdx_bus_type);
 
 	/* Rescan all the devices */
-	xa_for_each(&cdx_controllers, index, cdx) {
-		int ret;
+	for_each_compatible_node(np, NULL, compat_node_name) {
+		if (!np)
+			return -EINVAL;
 
-		ret = cdx->ops->scan(cdx);
-		if (ret)
-			dev_err(cdx->dev, "cdx bus scanning failed\n");
+		pd = of_find_device_by_node(np);
+		if (!pd)
+			return -EINVAL;
+
+		cdx = platform_get_drvdata(pd);
+		if (cdx && cdx->controller_registered && cdx->ops->scan)
+			cdx->ops->scan(cdx);
+
+		put_device(&pd->dev);
 	}
 
 	return count;
@@ -520,17 +530,19 @@ int cdx_register_controller(struct cdx_controller *cdx)
 {
 	int ret;
 
-	ret = xa_alloc(&cdx_controllers, &cdx->id, cdx,
-		       XA_LIMIT(0, MAX_CDX_CONTROLLERS - 1), GFP_KERNEL);
-	if (ret) {
+	ret = ida_alloc_range(&cdx_controller_ida, 0,  MAX_CDX_CONTROLLERS - 1, GFP_KERNEL);
+	if (ret < 0) {
 		dev_err(cdx->dev,
 			"No free index available. Maximum controllers already registered\n");
 		cdx->id = (u8)MAX_CDX_CONTROLLERS;
 		return ret;
 	}
 
+	cdx->id = ret;
+
 	/* Scan all the devices */
 	cdx->ops->scan(cdx);
+	cdx->controller_registered = true;
 
 	return 0;
 }
@@ -541,8 +553,9 @@ void cdx_unregister_controller(struct cdx_controller *cdx)
 	if (cdx->id >= MAX_CDX_CONTROLLERS)
 		return;
 
+	cdx->controller_registered = false;
 	device_for_each_child(cdx->dev, NULL, cdx_unregister_device);
-	xa_erase(&cdx_controllers, cdx->id);
+	ida_free(&cdx_controller_ida, cdx->id);
 }
 EXPORT_SYMBOL_GPL(cdx_unregister_controller);
 
diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h
index bead71b7bc73..82c27b8c94e1 100644
--- a/include/linux/cdx/cdx_bus.h
+++ b/include/linux/cdx/cdx_bus.h
@@ -63,12 +63,14 @@ struct cdx_ops {
  * @dev: Linux device associated with the CDX controller.
  * @priv: private data
  * @id: Controller ID
+ * @controller_registered: controller registered with bus
  * @ops: CDX controller ops
  */
 struct cdx_controller {
 	struct device *dev;
 	void *priv;
 	u32 id;
+	bool controller_registered;
 	struct cdx_ops *ops;
 };
 
-- 
2.25.1
Re: [PATCH v3 1/5] cdx: Remove cdx controller list from cdx bus system
Posted by Greg KH 2 years, 3 months ago 
On Mon, Aug 14, 2023 at 03:52:19PM +0530, Abhijit Gangurde wrote:
> Remove xarray list of cdx controller. Instead, use platform bus
> to locate the cdx controller using compat string used by cdx
> controller platform driver.

You also now use an ida instead, right?  Do you need to mention that
here?

> Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> ---
>  drivers/cdx/cdx.c           | 39 ++++++++++++++++++++++++-------------
>  include/linux/cdx/cdx_bus.h |  2 ++
>  2 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> index d2cad4c670a0..1c0f76cf4f15 100644
> --- a/drivers/cdx/cdx.c
> +++ b/drivers/cdx/cdx.c
> @@ -60,7 +60,7 @@
>  #include <linux/of_device.h>
>  #include <linux/slab.h>
>  #include <linux/mm.h>
> -#include <linux/xarray.h>
> +#include <linux/idr.h>
>  #include <linux/cdx/cdx_bus.h>
>  #include <linux/iommu.h>
>  #include <linux/dma-map-ops.h>
> @@ -70,8 +70,10 @@
>  #define CDX_DEFAULT_DMA_MASK	(~0ULL)
>  #define MAX_CDX_CONTROLLERS 16
>  
> -/* CDX controllers registered with the CDX bus */
> -static DEFINE_XARRAY_ALLOC(cdx_controllers);
> +/* IDA for CDX controllers registered with the CDX bus */
> +DEFINE_IDA(cdx_controller_ida);

Why is this now a global variable?

thanks,

> +
> +static char *compat_node_name = "xlnx,versal-net-cdx";
>  
>  /**
>   * cdx_dev_reset - Reset a CDX device
> @@ -384,7 +386,8 @@ static ssize_t rescan_store(const struct bus_type *bus,
>  			    const char *buf, size_t count)
>  {
>  	struct cdx_controller *cdx;
> -	unsigned long index;
> +	struct platform_device *pd;
> +	struct device_node *np;
>  	bool val;
>  
>  	if (kstrtobool(buf, &val) < 0)
> @@ -397,12 +400,19 @@ static ssize_t rescan_store(const struct bus_type *bus,
>  	cdx_unregister_devices(&cdx_bus_type);
>  
>  	/* Rescan all the devices */
> -	xa_for_each(&cdx_controllers, index, cdx) {
> -		int ret;
> +	for_each_compatible_node(np, NULL, compat_node_name) {
> +		if (!np)
> +			return -EINVAL;
>  
> -		ret = cdx->ops->scan(cdx);
> -		if (ret)
> -			dev_err(cdx->dev, "cdx bus scanning failed\n");
> +		pd = of_find_device_by_node(np);
> +		if (!pd)
> +			return -EINVAL;
> +
> +		cdx = platform_get_drvdata(pd);
> +		if (cdx && cdx->controller_registered && cdx->ops->scan)
> +			cdx->ops->scan(cdx);
> +
> +		put_device(&pd->dev);

What handles the locking here?  What if a new device is added , or
removed, while you iterate this?

And what about racing with the controller_registered flag?  Are you sure
that's needed?

thanks,

greg k-h
RE: [PATCH v3 1/5] cdx: Remove cdx controller list from cdx bus system
Posted by Gangurde, Abhijit 2 years, 3 months ago 
> On Mon, Aug 14, 2023 at 03:52:19PM +0530, Abhijit Gangurde wrote:
> > Remove xarray list of cdx controller. Instead, use platform bus
> > to locate the cdx controller using compat string used by cdx
> > controller platform driver.
> 
> You also now use an ida instead, right?  Do you need to mention that
> here?

Yes. Will add it in the description.

> 
> > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com>
> > ---
> >  drivers/cdx/cdx.c           | 39 ++++++++++++++++++++++++-------------
> >  include/linux/cdx/cdx_bus.h |  2 ++
> >  2 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> > index d2cad4c670a0..1c0f76cf4f15 100644
> > --- a/drivers/cdx/cdx.c
> > +++ b/drivers/cdx/cdx.c
> > @@ -60,7 +60,7 @@
> >  #include <linux/of_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/mm.h>
> > -#include <linux/xarray.h>
> > +#include <linux/idr.h>
> >  #include <linux/cdx/cdx_bus.h>
> >  #include <linux/iommu.h>
> >  #include <linux/dma-map-ops.h>
> > @@ -70,8 +70,10 @@
> >  #define CDX_DEFAULT_DMA_MASK	(~0ULL)
> >  #define MAX_CDX_CONTROLLERS 16
> >
> > -/* CDX controllers registered with the CDX bus */
> > -static DEFINE_XARRAY_ALLOC(cdx_controllers);
> > +/* IDA for CDX controllers registered with the CDX bus */
> > +DEFINE_IDA(cdx_controller_ida);
> 
> Why is this now a global variable?

This should have been static. Will correct this in next version.

> 
> thanks,
> 
> > +
> > +static char *compat_node_name = "xlnx,versal-net-cdx";
> >
> >  /**
> >   * cdx_dev_reset - Reset a CDX device
> > @@ -384,7 +386,8 @@ static ssize_t rescan_store(const struct bus_type
> *bus,
> >  			    const char *buf, size_t count)
> >  {
> >  	struct cdx_controller *cdx;
> > -	unsigned long index;
> > +	struct platform_device *pd;
> > +	struct device_node *np;
> >  	bool val;
> >
> >  	if (kstrtobool(buf, &val) < 0)
> > @@ -397,12 +400,19 @@ static ssize_t rescan_store(const struct bus_type
> *bus,
> >  	cdx_unregister_devices(&cdx_bus_type);
> >
> >  	/* Rescan all the devices */
> > -	xa_for_each(&cdx_controllers, index, cdx) {
> > -		int ret;
> > +	for_each_compatible_node(np, NULL, compat_node_name) {
> > +		if (!np)
> > +			return -EINVAL;
> >
> > -		ret = cdx->ops->scan(cdx);
> > -		if (ret)
> > -			dev_err(cdx->dev, "cdx bus scanning failed\n");
> > +		pd = of_find_device_by_node(np);
> > +		if (!pd)
> > +			return -EINVAL;
> > +
> > +		cdx = platform_get_drvdata(pd);
> > +		if (cdx && cdx->controller_registered && cdx->ops->scan)
> > +			cdx->ops->scan(cdx);
> > +
> > +		put_device(&pd->dev);
> 
> What handles the locking here?  What if a new device is added , or
> removed, while you iterate this?
> 
> And what about racing with the controller_registered flag?  Are you sure
> that's needed?
> 

As you highlighted, locking is introduced in patch 2 of this series.

Thanks,
Abhijit

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.