lkml.org 
[lkml]   [2016]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon
From
Date
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
>

\
 
 \ /
  Last update: 2016-07-16 05:41    [W:0.195 / U:0.652 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site