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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y8atGJV6yY4mXD+W@kroah.com>
Date:   Tue, 17 Jan 2023 15:13:44 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Nipun Gupta <nipun.gupta@....com>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        rafael@...nel.org, eric.auger@...hat.com,
        alex.williamson@...hat.com, cohuck@...hat.com,
        song.bao.hua@...ilicon.com, mchehab+huawei@...nel.org,
        maz@...nel.org, f.fainelli@...il.com, jeffrey.l.hugo@...il.com,
        saravanak@...gle.com, Michael.Srba@...nam.cz, mani@...nel.org,
        yishaih@...dia.com, jgg@...pe.ca, jgg@...dia.com,
        robin.murphy@....com, will@...nel.org, joro@...tes.org,
        masahiroy@...nel.org, ndesaulniers@...gle.com,
        linux-arm-kernel@...ts.infradead.org, linux-kbuild@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        okaya@...nel.org, harpreet.anand@....com, nikhil.agarwal@....com,
        michal.simek@....com, git@....com
Subject: Re: [PATCH 07/19] bus/cdx: add device attributes

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@....com>
> Signed-off-by: Nipun Gupta <nipun.gupta@....com>
> Signed-off-by: Tarak Reddy <tarak.reddy@....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@....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@....com
> +Description:
> +		Device ID for this CDX device

Same here, be specific.

> +
> +What:		/sys/bus/cdx/devices/.../reset
> +Date:		January 2023
> +Contact:	nipun.gupta@....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@....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