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

[Greg KH]

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

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