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]

Date:   Thu, 24 Aug 2023 12:10:55 +0000
From:   "Gangurde, Abhijit" <abhijit.gangurde@....com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "masahiroy@...nel.org" <masahiroy@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Gupta, Nipun" <Nipun.Gupta@....com>,
        "Agarwal, Nikhil" <nikhil.agarwal@....com>,
        "Gupta, Puneet (DCG-ENG)" <puneet.gupta@....com>,
        "git (AMD-Xilinx)" <git@....com>,
        "Simek, Michal" <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?

Yes. Will add it in the description.

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

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