Re: [PATCH 07/19] bus/cdx: add device attributes

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jan 17, 2023 at 07:11:39PM +0530, Nipun Gupta wrote:
> Create sysfs entry for CDX devices.
> 
> Sysfs entries provided in each of the CDX device detected by
> the CDX controller
>  - vendor id
>  - device id
>  - remove
>  - reset of the device.
>  - driver override
> 
> Signed-off-by: Puneet Gupta <puneet.gupta@amd.com>
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> Signed-off-by: Tarak Reddy <tarak.reddy@amd.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cdx     |  34 +++++
>  drivers/bus/cdx/cdx.c                       | 144 ++++++++++++++++++++
>  drivers/bus/cdx/controller/cdx_controller.c |  19 +++
>  drivers/bus/cdx/controller/mcdi_functions.c |  14 ++
>  drivers/bus/cdx/controller/mcdi_functions.h |  11 ++
>  include/linux/cdx/cdx_bus.h                 |  23 ++++
>  6 files changed, 245 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx
> index 8c2425fdb6d9..1e0fdce18cde 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cdx
> +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> @@ -10,3 +10,37 @@ Description:
>                  For example::
>  
>  		  # echo 1 > /sys/bus/cdx/rescan
> +
> +What:		/sys/bus/cdx/devices/.../vendor
> +Date:		January 2023
> +Contact:	nipun.gupta@amd.com
> +Description:
> +		Vendor ID for this CDX device

What format is this in?  How big is it?  Examples?

> +
> +What:		/sys/bus/cdx/devices/.../device
> +Date:		January 2023
> +Contact:	nipun.gupta@amd.com
> +Description:
> +		Device ID for this CDX device

Same here, be specific.

> +
> +What:		/sys/bus/cdx/devices/.../reset
> +Date:		January 2023
> +Contact:	nipun.gupta@amd.com
> +Description:
> +		Writing a non-zero value to this file would reset the
> +		CDX device
> +
> +                For example::
> +
> +		  # echo 1 > /sys/bus/cdx/.../reset

Will that remove the device from the driver too?

Again, more documentation please.

> +
> +What:		/sys/bus/cdx/devices/.../remove
> +Date:		January 2023
> +Contact:	tarak.reddy@amd.com
> +Description:
> +		Writing a non-zero value to this file would remove the
> +		corrosponding device from the CDX bus
> +
> +		For example::
> +
> +		# echo 1 > /sys/bus/cdx/devices/.../remove

Why would you want to remove a device from the bus like this?

> diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c
> index 2737470f08d3..1372d8dcaa4c 100644
> --- a/drivers/bus/cdx/cdx.c
> +++ b/drivers/bus/cdx/cdx.c
> @@ -72,6 +72,38 @@ static DECLARE_BITMAP(cdx_controller_id_map, MAX_CDX_CONTROLLERS);
>  /* List of CDX controllers registered with the CDX bus */
>  static LIST_HEAD(cdx_controllers);
>  
> +/**
> + * reset_cdx_device - Reset a CDX device
> + * @dev: CDX device
> + * @data: This is always passed as NULL, and is not used in this API,
> + *	  but is required here as the bus_for_each_dev() API expects
> + *	  the passed function (reset_cdx_device) to have this
> + *	  as an argument.
> + *
> + * @return: -errno on failure, 0 on success.

I recommend this function actually be the one without the data pointer,
that way you don't get confused here.

> + */
> +static int reset_cdx_device(struct device *dev, void *data)
> +{
> +	struct cdx_device *cdx_dev = to_cdx_device(dev);
> +	struct cdx_controller *cdx = cdx_dev->cdx;
> +	struct cdx_device_config dev_config;
> +	int ret;
> +
> +	dev_config.type = CDX_DEV_RESET_CONF;
> +	ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num,
> +				      cdx_dev->dev_num, &dev_config);
> +	if (ret)
> +		dev_err(dev, "cdx device reset failed\n");
> +
> +	return ret;
> +}
> +
> +int cdx_dev_reset(struct device *dev)
> +{
> +	return reset_cdx_device(dev, NULL);
> +}
> +EXPORT_SYMBOL_GPL(cdx_dev_reset);

Remove the indirection as mentioned above please.

Otherwise, very minor comments, nice work.

greg k-h