Messages in this thread | | | From | "Gangurde, Abhijit" <> | Subject | RE: [PATCH v3 3/5] cdx: add support for bus enable and disable | Date | Thu, 24 Aug 2023 12:11:11 +0000 |
| |
> On Mon, Aug 14, 2023 at 03:52:21PM +0530, Abhijit Gangurde wrote: > > CDX bus needs to be disabled before updating/writing devices > > in the FPGA. Once the devices are written, the bus shall be > > rescanned. This change provides sysfs entry to enable/disable the > > CDX bus. > > > > Co-developed-by: Nipun Gupta <nipun.gupta@amd.com> > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com> > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van- > vuuren@amd.com> > > Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com> > > --- > > Documentation/ABI/testing/sysfs-bus-cdx | 27 +++++++ > > drivers/cdx/cdx.c | 94 +++++++++++++++++++++++++ > > drivers/cdx/controller/cdx_controller.c | 50 +++++++++++++ > > drivers/cdx/controller/mc_cdx_pcol.h | 54 ++++++++++++++ > > drivers/cdx/controller/mcdi_functions.c | 24 +++++++ > > drivers/cdx/controller/mcdi_functions.h | 18 +++++ > > include/linux/cdx/cdx_bus.h | 12 ++++ > > 7 files changed, 279 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx > b/Documentation/ABI/testing/sysfs-bus-cdx > > index 7af477f49998..04c8dfe7e201 100644 > > --- a/Documentation/ABI/testing/sysfs-bus-cdx > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx > > @@ -11,6 +11,33 @@ Description: > > > > # echo 1 > /sys/bus/cdx/rescan > > > > +What: /sys/bus/cdx/enable > > +Date: July 2023 > > +Contact: nipun.gupta@amd.com > > +Description: > > + Writing bus number in hex to this file will attempt to enable > > + the CDX bus. The bus number for the cdx devices can be found > > + at /sys/bus/cdx/devices/cdx-BB:DD, where BB denotes the > bus > > + number for the respective device. > > + > > + For example :: > > + > > + # echo 00 > /sys/bus/cdx/enable > > + > > +What: /sys/bus/cdx/disable > > +Date: July 2023 > > +Contact: nipun.gupta@amd.com > > +Description: > > + Writing bus number in hex to this file will attempt to disable > > + the CDX bus. CDX bus should be disabled before updating the > > + devices in FPGA. The bus number for the cdx devices can be > > + found at /sys/bus/cdx/devices/cdx-BB:DD, where BB denotes > the > > + bus number for the respective device. > > + > > + For example :: > > + > > + # echo 00 > /sys/bus/cdx/disable > > + > > What: /sys/bus/cdx/devices/.../vendor > > Date: March 2023 > > Contact: nipun.gupta@amd.com > > diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c > > index 50df061f4537..411beb035fdd 100644 > > --- a/drivers/cdx/cdx.c > > +++ b/drivers/cdx/cdx.c > > @@ -70,6 +70,10 @@ > > #define CDX_DEFAULT_DMA_MASK (~0ULL) > > #define MAX_CDX_CONTROLLERS 16 > > > > +#define CONTROLLER_ID(X) \ > > + (((X) & CDX_CONTROLLER_ID_MASK) >> > CDX_CONTROLLER_ID_SHIFT) > > +#define BUS_ID(X) ((X) & CDX_BUS_NUM_MASK) > > + > > /* IDA for CDX controllers registered with the CDX bus */ > > DEFINE_IDA(cdx_controller_ida); > > /* Lock to protect controller ops */ > > @@ -384,6 +388,94 @@ static struct attribute *cdx_dev_attrs[] = { > > }; > > ATTRIBUTE_GROUPS(cdx_dev); > > > > +static struct cdx_controller *cdx_find_controller(u8 controller_id) > > +{ > > + struct cdx_controller *cdx = NULL; > > + struct platform_device *pd; > > + struct device_node *np; > > + > > + for_each_compatible_node(np, NULL, compat_node_name) { > > + if (!np) > > + return NULL; > > + > > + pd = of_find_device_by_node(np); > > + if (!pd) > > + return NULL; > > + > > + cdx = platform_get_drvdata(pd); > > + if (cdx && cdx->controller_registered && cdx->id == > controller_id) { > > + put_device(&pd->dev); > > + break; > > + } > > + > > + put_device(&pd->dev); > > Where is the locking if a platform device is removed?
If platform device is removed, it is expected that controller would be unregistered in its exit process. And controller unregistering is already under the lock.
> > > + } > > + > > + return cdx; > > +} > > + > > +static ssize_t enable_store(const struct bus_type *bus, > > + const char *buf, size_t count) > > +{ > > + unsigned long controller_id; > > + struct cdx_controller *cdx; > > + u8 bus_id; > > + int ret; > > + > > + if (kstrtou8(buf, 16, &bus_id)) > > + return -EINVAL; > > + > > + controller_id = CONTROLLER_ID(bus_id); > > + bus_id = BUS_ID(bus_id); > > + > > + mutex_lock(&cdx_controller_lock); > > + > > + cdx = cdx_find_controller(controller_id); > > You lock before you call this function? Is the lock required before you > call it? If so, please document that and enforce it with the proper > sparse marking.
Yes. Lock would be requiring for cdx_find_controller() to keep it in sync with register and unregister. Will document it and add the sparse marking in next version.
Thanks, Abhijit.
| |