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

[Guenter Roeck <linux@roeck-us.net>]

On 07/15/2016 07:36 AM, Quentin Schulz wrote:
> On 15/07/2016 16:03, Guenter Roeck wrote:
>> On 07/15/2016 02:59 AM, Quentin Schulz wrote:
> [...]
>>> +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.
>
> It cannot, the extend_name field is const char* in the struct:
> http://lxr.free-electrons.com/source/include/linux/iio/iio.h#L247
>

Then why "if(label)" ?

> [...]
>>> @@ -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 ?
>
> I don't get your question. I've followed the exact same pattern as for
> "a" variable's initialization.
> The initialization is before the switch case because the name of the
> exposed sysfs file depends on the type of the IIO channel. If I move the
> initialization in the switch case, I'll have duplicated code.
>

Yes, but that didn't require all those if statements.

>>> +        }
>>> +
>>>            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.
>>
>
> I don't know if we have to do something to revert the effects of
> sysfs_attr_init but you sure are right that the a and b's attribute's
> name is never freed. This case would be handled with devm_kasprintf, I
> guess? Thanks.
>
>>>            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) {

sysfs_attr_init() might be better here to keep the initialization of '*b'
as close together as possible.

Guenetr

>>> +            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;
>>>
>>
>
> Quentin
>