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

[Gangurde, Abhijit]

> 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@xxxxxxx>
> > ---
> >  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