Message-Id: <6e0f4a3c-0f49-f779-8d2c-e6831d6bd0d8@linux.vnet.ibm.com>
Date:   Wed, 18 Jul 2018 12:27:59 +0530
From:   Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>
To:     Guenter Roeck <linux@...ck-us.net>, mpe@...erman.id.au
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org, ego@...ux.vnet.ibm.com
Subject: Re: [PATCH v5 2/2] hwmon: ibmpowernv: Add attributes to
 enable/disable sensor groups

Hi,

On 07/17/2018 07:17 PM, Guenter Roeck wrote:
> On 07/14/2018 11:54 PM, Shilpasri G Bhat wrote:
>> On-Chip-Controller(OCC) is an embedded micro-processor in POWER9 chip
>> which measures various system and chip level sensors. These sensors
>> comprises of environmental sensors (like power, temperature, current
>> and voltage) and performance sensors (like utilization, frequency).
>> All these sensors are copied to main memory at a regular interval of
>> 100ms. OCC provides a way to select a group of sensors that is copied
>> to the main memory to increase the update frequency of selected sensor
>> groups. When a sensor-group is disabled, OCC will not copy it to main
>> memory and those sensors read 0 values.
>>
>> This patch provides support for enabling/disabling the sensor groups
>> like power, temperature, current and voltage. This patch adds new
>> per-senor sysfs attribute to disable and enable them.
>>
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>
>> ---
>> Changes from v4:
>> - As per Mpe's suggestion store device_node instead of phandles and
>>    clean it after init
>> - s/sg_data/sgrp_data
>>
>>   Documentation/hwmon/ibmpowernv |  43 ++++++-
>>   drivers/hwmon/ibmpowernv.c     | 256 +++++++++++++++++++++++++++++++++++------
>>   2 files changed, 265 insertions(+), 34 deletions(-)
>>
>> diff --git a/Documentation/hwmon/ibmpowernv b/Documentation/hwmon/ibmpowernv
>> index 8826ba2..5646825 100644
>> --- a/Documentation/hwmon/ibmpowernv
>> +++ b/Documentation/hwmon/ibmpowernv
>> @@ -33,9 +33,48 @@ fanX_input        Measured RPM value.
>>   fanX_min        Threshold RPM for alert generation.
>>   fanX_fault        0: No fail condition
>>               1: Failing fan
>> +
>>   tempX_input        Measured ambient temperature.
>>   tempX_max        Threshold ambient temperature for alert generation.
>> -inX_input        Measured power supply voltage
>> +tempX_highest        Historical maximum temperature
>> +tempX_lowest        Historical minimum temperature
>> +tempX_enable        Enable/disable all temperature sensors belonging to the
>> +            sub-group. In POWER9, this attribute corresponds to
>> +            each OCC. Using this attribute each OCC can be asked to
>> +            disable/enable all of its temperature sensors.
>> +            1: Enable
>> +            0: Disable
>> +
>> +inX_input        Measured power supply voltage (millivolt)
>>   inX_fault        0: No fail condition.
>>               1: Failing power supply.
>> -power1_input        System power consumption (microWatt)
>> +inX_highest        Historical maximum voltage
>> +inX_lowest        Historical minimum voltage
>> +inX_enable        Enable/disable all voltage sensors belonging to the
>> +            sub-group. In POWER9, this attribute corresponds to
>> +            each OCC. Using this attribute each OCC can be asked to
>> +            disable/enable all of its voltage sensors.
>> +            1: Enable
>> +            0: Disable
>> +
>> +powerX_input        Power consumption (microWatt)
>> +powerX_input_highest    Historical maximum power
>> +powerX_input_lowest    Historical minimum power
>> +powerX_enable        Enable/disable all power sensors belonging to the
>> +            sub-group. In POWER9, this attribute corresponds to
>> +            each OCC. Using this attribute each OCC can be asked to
>> +            disable/enable all of its power sensors.
>> +            1: Enable
>> +            0: Disable
>> +
>> +currX_input        Measured current (milliampere)
>> +currX_highest        Historical maximum current
>> +currX_lowest        Historical minimum current
>> +currX_enable        Enable/disable all current sensors belonging to the
>> +            sub-group. In POWER9, this attribute corresponds to
>> +            each OCC. Using this attribute each OCC can be asked to
>> +            disable/enable all of its current sensors.
>> +            1: Enable
>> +            0: Disable
>> +
>> +energyX_input        Cumulative energy (microJoule)
>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>> index f829dad..a509b9b 100644
>> --- a/drivers/hwmon/ibmpowernv.c
>> +++ b/drivers/hwmon/ibmpowernv.c
>> @@ -90,11 +90,23 @@ struct sensor_data {
>>       char label[MAX_LABEL_LEN];
>>       char name[MAX_ATTR_LEN];
>>       struct device_attribute dev_attr;
>> +    struct sensor_group_data *sgrp_data;
>> +};
>> +
>> +struct sensor_group_data {
>> +    struct mutex mutex;
>> +    struct device_node **of_nodes;
>> +    u32 gid;
>> +    u32 nr_nodes;
>> +    enum sensors type;
>> +    bool enable;
>>   };
>>     struct platform_data {
>>       const struct attribute_group *attr_groups[MAX_SENSOR_TYPE + 1];
>> +    struct sensor_group_data *sgrp_data;
>>       u32 sensors_count; /* Total count of sensors from each group */
>> +    u32 nr_sensor_groups; /* Total number of sensor groups */
>>   };
>>     static ssize_t show_sensor(struct device *dev, struct device_attribute
>> *devattr,
>> @@ -105,6 +117,9 @@ static ssize_t show_sensor(struct device *dev, struct
>> device_attribute *devattr,
>>       ssize_t ret;
>>       u64 x;
>>   +    if (sdata->sgrp_data && !sdata->sgrp_data->enable)
>> +        return -ENODATA;
>> +
>>       ret =  opal_get_sensor_data_u64(sdata->id, &x);
>>         if (ret)
>> @@ -120,6 +135,46 @@ static ssize_t show_sensor(struct device *dev, struct
>> device_attribute *devattr,
>>       return sprintf(buf, "%llu\n", x);
>>   }
>>   +static ssize_t show_enable(struct device *dev,
>> +               struct device_attribute *devattr, char *buf)
>> +{
>> +    struct sensor_data *sdata = container_of(devattr, struct sensor_data,
>> +                         dev_attr);
>> +
>> +    return sprintf(buf, "%u\n", sdata->sgrp_data->enable);
>> +}
>> +
>> +static ssize_t store_enable(struct device *dev,
>> +                struct device_attribute *devattr,
>> +                const char *buf, size_t count)
>> +{
>> +    struct sensor_data *sdata = container_of(devattr, struct sensor_data,
>> +                         dev_attr);
>> +    struct sensor_group_data *sgrp_data = sdata->sgrp_data;
>> +    bool data;
>> +    int ret;
>> +
>> +    ret = kstrtobool(buf, &data);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = mutex_lock_interruptible(&sgrp_data->mutex);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (data != sgrp_data->enable) {
>> +        ret =  sensor_group_enable(sgrp_data->gid, data);
>> +        if (!ret)
>> +            sgrp_data->enable = data;
>> +    }
>> +
>> +    if (!ret)
>> +        ret = count;
>> +
>> +    mutex_unlock(&sgrp_data->mutex);
>> +    return ret;
>> +}
>> +
>>   static ssize_t show_label(struct device *dev, struct device_attribute *devattr,
>>                 char *buf)
>>   {
>> @@ -292,12 +347,129 @@ static u32 get_sensor_hwmon_index(struct sensor_data
>> *sdata,
>>       return ++sensor_groups[sdata->type].hwmon_index;
>>   }
>>   +static int init_sensor_group_data(struct platform_device *pdev,
>> +                  struct platform_data *pdata)
>> +{
>> +    struct sensor_group_data *sgrp_data;
>> +    struct device_node *groups, *sgrp;
>> +    enum sensors type;
>> +    int count = 0, ret = 0;
>> +
>> +    groups = of_find_node_by_path("/ibm,opal/sensor-groups");
>> +    if (!groups)
>> +        return ret;
>> +
>> +    for_each_child_of_node(groups, sgrp) {
>> +        type = get_sensor_type(sgrp);
>> +        if (type != MAX_SENSOR_TYPE)
>> +            pdata->nr_sensor_groups++;
>> +    }
>> +
>> +    if (!pdata->nr_sensor_groups)
>> +        goto out;
>> +
>> +    sgrp_data = devm_kcalloc(&pdev->dev, pdata->nr_sensor_groups,
>> +                 sizeof(*sgrp_data), GFP_KERNEL);
>> +    if (!sgrp_data) {
>> +        ret = -ENOMEM;
>> +        goto out;
>> +    }
>> +
>> +    for_each_child_of_node(groups, sgrp) {
>> +        const __be32 *phandles;
>> +        int len, gid, i, k = 0;
>> +
>> +        type = get_sensor_type(sgrp);
>> +        if (type == MAX_SENSOR_TYPE)
>> +            continue;
>> +
>> +        if (of_property_read_u32(sgrp, "sensor-group-id", &gid))
>> +            continue;
>> +
>> +        phandles = of_get_property(sgrp, "sensors", &len);
>> +        if (!phandles)
>> +            continue;
>> +
>> +        len /= sizeof(u32);
>> +        if (!len)
>> +            continue;
>> +
>> +        sgrp_data[count].of_nodes = devm_kcalloc(&pdev->dev,
>> +                        sizeof(struct device_node *),
>> +                        len, GFP_KERNEL);
>> +        if (!sgrp_data[count].of_nodes) {
>> +            ret = -ENOMEM;
>> +            of_node_put(sgrp);
>> +            goto out;
>> +        }
>> +
>> +        for (i = 0; i < len; i++) {
>> +            struct device_node *node;
>> +
>> +            node = of_parse_phandle(sgrp, "sensors", i);
>> +            if (!node)
>> +                continue;
>> +            sgrp_data[count].of_nodes[k++] = node;
> 
> I don't immediately see where "node" is used later. As mentioned below,
> I'll need to have a much closer look into the code to understand what
> is going on here.

sgrp_data[count].of_nodes is used to store the pointers to sensor nodes
belonging to sensor-group 'sgrp_data[count]' . This is being used later in
to find the sensor-group of each sensor in create_device_attrs()

> 
>> +        }
>> +
>> +        sensor_groups[type].attr_count++;
>> +        sgrp_data[count].gid = gid;
>> +        sgrp_data[count].type = type;
>> +        sgrp_data[count].nr_nodes = len;
>> +        mutex_init(&sgrp_data[count].mutex);
>> +        sgrp_data[count++].enable = false;
>> +    }
>> +    pdata->sgrp_data = sgrp_data;
>> +out:
>> +    of_node_put(groups);
>> +    return ret;
>> +}
>> +
>> +static struct sensor_group_data *get_sensor_group(struct platform_data *pdata,
>> +                          struct device_node *node,
>> +                          enum sensors type)
>> +{
>> +    struct sensor_group_data *sgrp_data = pdata->sgrp_data;
>> +    int i, j;
>> +
>> +    for (i = 0; i < pdata->nr_sensor_groups; i++) {
>> +        if (type != sgrp_data[i].type)
>> +            continue;
>> +
>> +        for (j = 0; j < sgrp_data[i].nr_nodes; j++)
>> +            if (sgrp_data[i].of_nodes[j] == node)
>> +                return &sgrp_data[i];
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static void clean_sensor_group_of_node(struct platform_device *pdev)
>> +{
>> +    struct platform_data *pdata = platform_get_drvdata(pdev);
>> +    struct sensor_group_data *sgrp_data = pdata->sgrp_data;
>> +    int i, j;
>> +
>> +    for (i = 0; i < pdata->nr_sensor_groups; i++) {
>> +        for (j = 0; j < sgrp_data[i].nr_nodes; j++)
>> +            of_node_put(sgrp_data[i].of_nodes[j]);
>> +
>> +        devm_kfree(&pdev->dev, sgrp_data[i].of_nodes);
> 
> The whole point of calling devm_kzalloc() is that calling devm_kfree()
> is not necessary. Any call to devm_kfree() is a strong indication
> that something is wrong.
> 
>> +        sgrp_data[i].of_nodes = NULL;
>> +    }
>> +}
>> +
> 
> Ok, this will require a detailed review. I don't understand what this
> function is about. It seems that sgrp_data[].of_nodes is only allocated
> to be  freed at the end of create_dev_attrs(), suggesting that the cleared
> data isn't needed for runtime and thus should not be stored in an
> array in the first place. I'll have to understand this much better
> than I do right now before approving it.
> 

Yeah sgrp_data[].of_nodes is not required at runtime. This can be defined locally.


Thanks and Regards,
Shilpa

> Guenter
> 
>>   static int populate_attr_groups(struct platform_device *pdev)
>>   {
>>       struct platform_data *pdata = platform_get_drvdata(pdev);
>>       const struct attribute_group **pgroups = pdata->attr_groups;
>>       struct device_node *opal, *np;
>>       enum sensors type;
>> +    int ret;
>> +
>> +    ret = init_sensor_group_data(pdev, pdata);
>> +    if (ret)
>> +        return ret;
>>         opal = of_find_node_by_path("/ibm,opal/sensors");
>>       for_each_child_of_node(opal, np) {
>> @@ -344,7 +516,10 @@ static int populate_attr_groups(struct platform_device
>> *pdev)
>>   static void create_hwmon_attr(struct sensor_data *sdata, const char *attr_name,
>>                     ssize_t (*show)(struct device *dev,
>>                             struct device_attribute *attr,
>> -                          char *buf))
>> +                          char *buf),
>> +                ssize_t (*store)(struct device *dev,
>> +                         struct device_attribute *attr,
>> +                         const char *buf, size_t count))
>>   {
>>       snprintf(sdata->name, MAX_ATTR_LEN, "%s%d_%s",
>>            sensor_groups[sdata->type].name, sdata->hwmon_index,
>> @@ -352,23 +527,33 @@ static void create_hwmon_attr(struct sensor_data *sdata,
>> const char *attr_name,
>>         sysfs_attr_init(&sdata->dev_attr.attr);
>>       sdata->dev_attr.attr.name = sdata->name;
>> -    sdata->dev_attr.attr.mode = S_IRUGO;
>>       sdata->dev_attr.show = show;
>> +    if (store) {
>> +        sdata->dev_attr.store = store;
>> +        sdata->dev_attr.attr.mode = 0664;
>> +    } else {
>> +        sdata->dev_attr.attr.mode = 0444;
>> +    }
>>   }
>>     static void populate_sensor(struct sensor_data *sdata, int od, int hd, int
>> sid,
>>                   const char *attr_name, enum sensors type,
>>                   const struct attribute_group *pgroup,
>> +                struct sensor_group_data *sgrp_data,
>>                   ssize_t (*show)(struct device *dev,
>>                           struct device_attribute *attr,
>> -                        char *buf))
>> +                        char *buf),
>> +                ssize_t (*store)(struct device *dev,
>> +                         struct device_attribute *attr,
>> +                         const char *buf, size_t count))
>>   {
>>       sdata->id = sid;
>>       sdata->type = type;
>>       sdata->opal_index = od;
>>       sdata->hwmon_index = hd;
>> -    create_hwmon_attr(sdata, attr_name, show);
>> +    create_hwmon_attr(sdata, attr_name, show, store);
>>       pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr;
>> +    sdata->sgrp_data = sgrp_data;
>>   }
>>     static char *get_max_attr(enum sensors type)
>> @@ -403,24 +588,23 @@ static int create_device_attrs(struct platform_device
>> *pdev)
>>       const struct attribute_group **pgroups = pdata->attr_groups;
>>       struct device_node *opal, *np;
>>       struct sensor_data *sdata;
>> -    u32 sensor_id;
>> -    enum sensors type;
>>       u32 count = 0;
>> -    int err = 0;
>> +    u32 group_attr_id[MAX_SENSOR_TYPE] = {0};
>>   -    opal = of_find_node_by_path("/ibm,opal/sensors");
>>       sdata = devm_kcalloc(&pdev->dev,
>>                    pdata->sensors_count, sizeof(*sdata),
>>                    GFP_KERNEL);
>> -    if (!sdata) {
>> -        err = -ENOMEM;
>> -        goto exit_put_node;
>> -    }
>> +    if (!sdata)
>> +        return -ENOMEM;
>>   +    opal = of_find_node_by_path("/ibm,opal/sensors");
>>       for_each_child_of_node(opal, np) {
>> +        struct sensor_group_data *sgrp_data;
>>           const char *attr_name;
>> -        u32 opal_index;
>> +        u32 opal_index, hw_id;
>> +        u32 sensor_id;
>>           const char *label;
>> +        enum sensors type;
>>             if (np->name == NULL)
>>               continue;
>> @@ -456,14 +640,12 @@ static int create_device_attrs(struct platform_device
>> *pdev)
>>               opal_index = INVALID_INDEX;
>>           }
>>   -        sdata[count].opal_index = opal_index;
>> -        sdata[count].hwmon_index =
>> -            get_sensor_hwmon_index(&sdata[count], sdata, count);
>> -
>> -        create_hwmon_attr(&sdata[count], attr_name, show_sensor);
>> -
>> -        pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>> -                &sdata[count++].dev_attr.attr;
>> +        hw_id = get_sensor_hwmon_index(&sdata[count], sdata, count);
>> +        sgrp_data = get_sensor_group(pdata, np, type);
>> +        populate_sensor(&sdata[count], opal_index, hw_id, sensor_id,
>> +                attr_name, type, pgroups[type], sgrp_data,
>> +                show_sensor, NULL);
>> +        count++;
>>             if (!of_property_read_string(np, "label", &label)) {
>>               /*
>> @@ -474,35 +656,44 @@ static int create_device_attrs(struct platform_device
>> *pdev)
>>                */
>>                 make_sensor_label(np, &sdata[count], label);
>> -            populate_sensor(&sdata[count], opal_index,
>> -                    sdata[count - 1].hwmon_index,
>> +            populate_sensor(&sdata[count], opal_index, hw_id,
>>                       sensor_id, "label", type, pgroups[type],
>> -                    show_label);
>> +                    NULL, show_label, NULL);
>>               count++;
>>           }
>>             if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
>>               attr_name = get_max_attr(type);
>> -            populate_sensor(&sdata[count], opal_index,
>> -                    sdata[count - 1].hwmon_index,
>> +            populate_sensor(&sdata[count], opal_index, hw_id,
>>                       sensor_id, attr_name, type,
>> -                    pgroups[type], show_sensor);
>> +                    pgroups[type], sgrp_data, show_sensor,
>> +                    NULL);
>>               count++;
>>           }
>>             if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
>>               attr_name = get_min_attr(type);
>> -            populate_sensor(&sdata[count], opal_index,
>> -                    sdata[count - 1].hwmon_index,
>> +            populate_sensor(&sdata[count], opal_index, hw_id,
>>                       sensor_id, attr_name, type,
>> -                    pgroups[type], show_sensor);
>> +                    pgroups[type], sgrp_data, show_sensor,
>> +                    NULL);
>> +            count++;
>> +        }
>> +
>> +        if (sgrp_data && !sgrp_data->enable) {
>> +            sgrp_data->enable = true;
>> +            hw_id = ++group_attr_id[type];
>> +            populate_sensor(&sdata[count], opal_index, hw_id,
>> +                    sgrp_data->gid, "enable", type,
>> +                    pgroups[type], sgrp_data, show_enable,
>> +                    store_enable);
>>               count++;
>>           }
>>       }
>>   -exit_put_node:
>>       of_node_put(opal);
>> -    return err;
>> +    clean_sensor_group_of_node(pdev);
>> +    return 0;
>>   }
>>     static int ibmpowernv_probe(struct platform_device *pdev)
>> @@ -517,6 +708,7 @@ static int ibmpowernv_probe(struct platform_device *pdev)
>>         platform_set_drvdata(pdev, pdata);
>>       pdata->sensors_count = 0;
>> +    pdata->nr_sensor_groups = 0;
>>       err = populate_attr_groups(pdev);
>>       if (err)
>>           return err;
>>
> 

Powered by blists - more mailing lists