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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2023082207-humped-unclasp-6058@gregkh>
Date:   Tue, 22 Aug 2023 16:06:04 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Abhijit Gangurde <abhijit.gangurde@....com>
Cc:     masahiroy@...nel.org, linux-kernel@...r.kernel.org,
        Nipun.Gupta@....com, nikhil.agarwal@....com, puneet.gupta@....com,
        git@....com, michal.simek@....com
Subject: Re: [PATCH v3 1/5] cdx: Remove cdx controller list from cdx bus
 system

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