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

[Greg KH]

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?

> +	}
> +
> +	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.

thanks,

greg k-h