Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

[Guenter Roeck]

On 07/15/2016 02:59 AM, Quentin Schulz wrote:
> Currently, iio_hwmon only exposes values of the IIO channels it can read
> but no label by channel is exposed.
>
> This adds exposition of sysfs files containing label for IIO channels it
> can read based on extended_name field of the iio_chan_spec of the channel.
> If the extended_name field is empty, the sysfs file is not created by
> iio_hwmon.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxx>
> ---
>
> patch added in v2
>
>   drivers/hwmon/iio_hwmon.c | 77 +++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 68 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index c0da4d9..28d15b2 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -16,6 +16,7 @@
>   #include <linux/of.h>
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
>   #include <linux/iio/types.h>
>
>   /**
> @@ -57,12 +58,26 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
>   	return sprintf(buf, "%d\n", result);
>   }
>
> +static ssize_t iio_hwmon_read_label(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
> +	const char *label = state->channels[sattr->index].channel->extend_name;
> +
> +	if (label)
> +		return sprintf(buf, "%s\n", label);
> +
Can the name disappear on the fly, or be changed on the fly ?
Then this is unusable. We should and can only provide labels
if a name exists and is permanent. Otherwise all we do is
to confuse user space.

> +	return 0;
> +}
> +
>   static int iio_hwmon_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct iio_hwmon_state *st;
> -	struct sensor_device_attribute *a;
> -	int ret, i;
> +	struct sensor_device_attribute *a, *b;
> +	int ret, i, j = 0;
>   	int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1;
>   	enum iio_chan_type type;
>   	struct iio_channel *channels;
> @@ -92,7 +107,8 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   		st->num_channels++;
>
>   	st->attrs = devm_kzalloc(dev,
> -				 sizeof(*st->attrs) * (st->num_channels + 1),
> +				 sizeof(*st->attrs) *
> +				 (2 * st->num_channels + 1),
>   				 GFP_KERNEL);
>   	if (st->attrs == NULL) {
>   		ret = -ENOMEM;
> @@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   		}
>
>   		sysfs_attr_init(&a->dev_attr.attr);
> +
> +		b = NULL;
> +		if (st->channels[i].channel->extend_name) {
> +			b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
> +			if (b == NULL) {
> +				ret = -ENOMEM;
> +				goto error_release_channels;
> +			}
> +
> +			sysfs_attr_init(&b->dev_attr.attr);

Why is this initialization here and not with the rest of the initialization
of this attribute ?

> +		}
> +
>   		ret = iio_get_channel_type(&st->channels[i], &type);
>   		if (ret < 0)
>   			goto error_release_channels;
> @@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   		case IIO_VOLTAGE:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "in%d_input",
> -							  in_i++);
> +							  in_i);
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "in%d_label",
> +								  in_i);
> +			in_i++;
>   			break;
>   		case IIO_TEMP:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "temp%d_input",
> -							  temp_i++);
> +							  temp_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "temp%d_label",
> +								  temp_i);
> +			temp_i++;
>   			break;
>   		case IIO_CURRENT:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "curr%d_input",
> -							  curr_i++);
> +							  curr_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "curr%d_label",
> +								  curr_i);
> +			curr_i++;
>   			break;
>   		case IIO_HUMIDITYRELATIVE:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "humidity%d_input",
> -							  humidity_i++);
> +							  humidity_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "humidity%d_label",
> +								  humidity_i);
> +			humidity_i++;
>   			break;
>   		default:
>   			ret = -EINVAL;
>   			goto error_release_channels;
>   		}
> -		if (a->dev_attr.attr.name == NULL) {
> +		if (a->dev_attr.attr.name == NULL ||
> +		    (b && b->dev_attr.attr.name == NULL)) {
>   			ret = -ENOMEM;
>   			goto error_release_channels;
>   		}

Just realized that we have a memory leak here. The 'name' memory is never released.

>   		a->dev_attr.show = iio_hwmon_read_val;
>   		a->dev_attr.attr.mode = S_IRUGO;
>   		a->index = i;
> -		st->attrs[i] = &a->dev_attr.attr;
> +		st->attrs[j++] = &a->dev_attr.attr;
> +
> +		if (b) {
> +			b->dev_attr.show = iio_hwmon_read_label;
> +			b->dev_attr.attr.mode = S_IRUGO;
> +			b->index = i;
> +			st->attrs[j++] = &b->dev_attr.attr;
> +		}
>   	}
>
>   	st->attr_group.attrs = st->attrs;