| 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
| ||
|
Message-ID: <406e0838-2401-2a0b-fc97-0d2443c8535f@roeck-us.net>
Date: Mon, 1 May 2017 22:04:22 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>,
jdelvare@...e.com, paulus@...ba.org, mpe@...erman.id.au
Cc: linux-hwmon@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
linux-kernel@...r.kernel.org, svaidy@...ux.vnet.ibm.com,
ego@...ux.vnet.ibm.com, akshay.adiga@...ux.vnet.ibm.com,
andrew@...id.au, clg@...d.org
Subject: Re: [PATCH V2] hwmon: (ibmpowernv) Add min/max attributes and current
sensors
On 05/01/2017 09:35 PM, Shilpasri G Bhat wrote:
> Hi Guenter,
>
> On 04/28/2017 06:59 PM, Guenter Roeck wrote:
>> On 04/27/2017 10:59 PM, Shilpasri G Bhat wrote:
>>> Add support for adding min/max values for the inband sensors copied by
>>> OCC to main memory. And also add current(mA) sensors to the list.
>>>
>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>
>>> ---
>>> Changes from V1:
>>> - Add functions to get min and max attribute strings
>>> - Add function 'populate_sensor' to fill in the 'struct sensor_data'
>>> for each sensor.
>>>
>>> drivers/hwmon/ibmpowernv.c | 96 +++++++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 77 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>>> index 6d2e660..d59262c 100644
>>> --- a/drivers/hwmon/ibmpowernv.c
>>> +++ b/drivers/hwmon/ibmpowernv.c
>>> @@ -50,6 +50,7 @@ enum sensors {
>>> TEMP,
>>> POWER_SUPPLY,
>>> POWER_INPUT,
>>> + CURRENT,
>>> MAX_SENSOR_TYPE,
>>> };
>>>
>>> @@ -65,7 +66,8 @@ enum sensors {
>>> {"fan", "ibm,opal-sensor-cooling-fan"},
>>> {"temp", "ibm,opal-sensor-amb-temp"},
>>> {"in", "ibm,opal-sensor-power-supply"},
>>> - {"power", "ibm,opal-sensor-power"}
>>> + {"power", "ibm,opal-sensor-power"},
>>> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
>>
>> Following up on a previous e-mail, this really _is_ odd. Any chance to fix it
>> in the firmware and have current sensors return "ibm,opal-sensor-current" ?
>
> Okay will drop "curr' sensor till we resolve this in the firmware.
>
>>
>>> };
>>>
>>> struct sensor_data {
>>> @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev)
>>> opal = of_find_node_by_path("/ibm,opal/sensors");
>>> for_each_child_of_node(opal, np) {
>>> const char *label;
>>> + int len;
>>>
>>> if (np->name == NULL)
>>> continue;
>>> @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device
>>> *pdev)
>>> sensor_groups[type].attr_count++;
>>>
>>> /*
>>> - * add a new attribute for labels
>>> + * add attributes for labels, min and max
>>> */
>>> if (!of_property_read_string(np, "label", &label))
>>> sensor_groups[type].attr_count++;
>>> + if (of_find_property(np, "sensor-data-min", &len))
>>> + sensor_groups[type].attr_count++;
>>> + if (of_find_property(np, "sensor-data-max", &len))
>>> + sensor_groups[type].attr_count++;
>>> }
>>>
>>> of_node_put(opal);
>>> @@ -337,6 +344,49 @@ static void create_hwmon_attr(struct sensor_data *sdata,
>>> const char *attr_name,
>>> sdata->dev_attr.show = show;
>>> }
>>>
>>> +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,
>>> + ssize_t (*show)(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf))
>>> +{
>>> + sdata->id = sid;
>>> + sdata->type = type;
>>> + sdata->opal_index = od;
>>> + sdata->hwmon_index = hd;
>>> + create_hwmon_attr(sdata, attr_name, show);
>>> + pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr;
>>> +}
>>> +
>>> +static char *get_max_attr(enum sensors type)
>>> +{
>>> + switch (type) {
>>> + case POWER_INPUT:
>>> + return "input_highest";
>>> + case TEMP:
>>> + return "max";
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return "highest";
>>
>> This is a bit confusing. Why not 'return "highest";' in the default case above ?
>>
>> Also, is this correct for type == POWER_SUPPLY, ie is it "highest" vs. "max" ?
>>
>> Kind of odd that the firmware reports "highest/lowest" in some cases
>> and "max/min" in others. Guess there is nothing we can do about that,
>> just a note.
>
> Firmware has min/max values for all sensors, but I had mapped them as
> highest/lowest to add reset_history attribute in the later patches.
>
Not sure I am parsing that correctly. Does the firmware report highest/lowest
or min/max ? reset_history only makes sense if the chip supports highest/lowest.
In case you plan to implement highest/lowest in software ... please don't.
Highest/lowest attributes must only be provided if the history is delivered
by the chip. If the chip does not support highest/lowest values, user
space can implement it, but we definitely don't want any workers or
similar in the kernel to do it.
Thanks,
Guenter
> Will change "TEMP" to highest/lowest to keep consistency.
>
> Thanks and Regards,
> Shilpa
>
>>
>>> +}
>>> +
>>> +static char *get_min_attr(enum sensors type)
>>> +{
>>> + switch (type) {
>>> + case POWER_INPUT:
>>> + return "input_lowest";
>>> + case TEMP:
>>> + return "min";
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return "lowest";
>>
>> Same here.
>>
>>> +}
>>> +
>>> /*
>>> * Iterate through the device tree for each child of 'sensors' node, create
>>> * a sysfs attribute file, the file is named by translating the DT node name
>>> @@ -365,6 +415,7 @@ static int create_device_attrs(struct platform_device *pdev)
>>> for_each_child_of_node(opal, np) {
>>> const char *attr_name;
>>> u32 opal_index;
>>> + u32 hwmon_index;
>>> const char *label;
>>>
>>> if (np->name == NULL)
>>> @@ -386,9 +437,6 @@ static int create_device_attrs(struct platform_device *pdev)
>>> continue;
>>> }
>>>
>>> - sdata[count].id = sensor_id;
>>> - sdata[count].type = type;
>>> -
>>> /*
>>> * If we can not parse the node name, it means we are
>>> * running on a newer device tree. We can just forget
>>> @@ -401,14 +449,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;
>>> + hwmon_index = get_sensor_hwmon_index(&sdata[count], sdata,
>>> + count);
>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>> + sensor_id, attr_name, type, pgroups[type],
>>> + show_sensor);
>>> + count++;
>>>
>>> if (!of_property_read_string(np, "label", &label)) {
>>> /*
>>> @@ -417,16 +463,28 @@ static int create_device_attrs(struct platform_device
>>> *pdev)
>>> * attribute. They are related to the same
>>> * sensor.
>>> */
>>> - sdata[count].type = type;
>>> - sdata[count].opal_index = sdata[count - 1].opal_index;
>>> - sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
>>>
>>> make_sensor_label(np, &sdata[count], label);
>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>> + sensor_id, "label", type, pgroups[type],
>>> + show_label);
>>> + count++;
>>> + }
>>>
>>> - create_hwmon_attr(&sdata[count], "label", show_label);
>>> + if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
>>> + attr_name = get_max_attr(type);
>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>> + sensor_id, attr_name, type,
>>> + pgroups[type], show_sensor);
>>> + count++;
>>> + }
>>>
>>> - pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>>> - &sdata[count++].dev_attr.attr;
>>> + if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
>>> + attr_name = get_min_attr(type);
>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>> + sensor_id, attr_name, type,
>>> + pgroups[type], show_sensor);
>>> + count++;
>>> }
>>> }
>>>
>>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Powered by blists - more mailing lists