Messages in this thread | | | From | "Gangurde, Abhijit" <> | Subject | RE: [PATCH v3 1/5] cdx: Remove cdx controller list from cdx bus system | Date | Thu, 24 Aug 2023 12:10:55 +0000 |
| |
> 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
| |