linux-kernel - Re: [PATCH 2/2] hwmon: Add "label" attribute

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <78620358-737e-368e-e5f5-82a673adf26a@roeck-us.net>
Date:   Tue, 21 Dec 2021 10:17:03 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Paul Cercueil <paul@...pouillou.net>,
        Jean Delvare <jdelvare@...e.com>,
        Rob Herring <robh+dt@...nel.org>
Cc:     linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Cosmin Tanislav <cosmin.tanislav@...log.com>
Subject: Re: [PATCH 2/2] hwmon: Add "label" attribute

On 12/21/21 9:50 AM, Paul Cercueil wrote:
> If a label is defined in the device tree for this device add that
> to the device specific attributes. This is useful for userspace to
> be able to identify an individual device when multiple identical
> chips are present in the system.
> 

This is an ABI change which needs to be documented in
Documentation/hwmon/sysfs-interface.rst.

> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
> Tested-by: Cosmin Tanislav <cosmin.tanislav@...log.com>
> ---
>   drivers/hwmon/hwmon.c | 22 +++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 3501a3ead4ba..15826260a463 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -71,8 +71,23 @@ name_show(struct device *dev, struct device_attribute *attr, char *buf)
>   }
>   static DEVICE_ATTR_RO(name);
>   
> +static ssize_t
> +label_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	const char *label;
> +	int ret;
> +
> +	ret = device_property_read_string(dev, "label", &label);

Requires "#include <linux/property.h>". Also, reading and verifying the label
each time it is read is excessive. More on that see below.

> +	if (ret < 0)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%s\n", label);
> +}
> +static DEVICE_ATTR_RO(label);
> +
>   static struct attribute *hwmon_dev_attrs[] = {
>   	&dev_attr_name.attr,
> +	&dev_attr_label.attr,
>   	NULL
>   };
>   
> @@ -81,7 +96,12 @@ static umode_t hwmon_dev_name_is_visible(struct kobject *kobj,

That function name is no longer appropriate and should be changed to
something like hwmon_dev_attr_is_visible.

>   {
>   	struct device *dev = kobj_to_dev(kobj);
>   
> -	if (to_hwmon_device(dev)->name == NULL)
> +	if (attr == &dev_attr_name.attr &&
> +	    to_hwmon_device(dev)->name == NULL)

Unnecessary continuation line.

> +		return 0;
> +
> +	if (attr == &dev_attr_label.attr &&
> +	    !device_property_present(dev, "label"))

If the property is present but not a string, each read of "label" would
return a runtime error. I don't like that. I would suggest to store
a pointer to the label in struct hwmon_device, set it during registration
(eg by using devm_strdup() if it is defined), and use

	if (attr == &dev_attr_label.attr && to_hwmon_device(dev)->label == NULL)
		return 0;

to check if it is present.

>   		return 0;
>   
>   	return attr->mode;
>