RE: [PATCH v3 3/5] cdx: add support for bus enable and disable

[Gangurde, Abhijit]

> 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@xxxxxxx>
> > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@xxxxxxx>
> > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> vuuren@xxxxxxx>
> > Tested-by: Nikhil Agarwal <nikhil.agarwal@xxxxxxx>
> > ---
> >  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@xxxxxxx
> > +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@xxxxxxx
> > +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@xxxxxxx
> > 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.