Re: [PATCH v2 2/2] hwmon: ibmpowernv: Add attributes to enable/disable sensor groups
On 07/04/2018 09:53 AM, Shilpasri G Bhat wrote:
Hi Guenter,
Thanks for reviewing the patch.
On 07/04/2018 08:16 PM, Guenter Roeck wrote:
+/* Disable if last sensor in the group */
+send_command = true;
+for (i = 0; i < sg->nr_sensor; i++) {
+struct sensor_data *sd = sg->sensors[i];
+
+if (sd->enable) {
+send_command = false;
+break;
+}
This is weird. So there are situations where a request to disable
a sensor is accepted, but effectively ignored ? Shouldn't that
return, say, -EBUSY ?
This is because we do not support per-sensor enable/disable. We can only
enable/disable at a sensor-group level.
This patch follows the semantic to disable a sensor group iff all the sensors
belonging to that group have been disabled. Otherwise the sensor alone is marked
to be disabled and returns -ENODATA on reading it.
And a sensor group will be enabled if any of the sensor in that group is
enabled.
In similar situations, where setting one attribute affects others, a common
solution
is to make only the first attribute writable and have it affect all the others.
I think that would make sense here as well, and it would be much simpler to
implement.
Guenter
I will make changes to the remaining code according to your suggestion.
Thanks and Regards,
Shilpa
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] hwmon: ibmpowernv: Add attributes to enable/disable sensor groups
Hi Guenter,
Thanks for reviewing the patch.
On 07/04/2018 08:16 PM, Guenter Roeck wrote:
>> +/* Disable if last sensor in the group */
>> +send_command = true;
>> +for (i = 0; i < sg->nr_sensor; i++) {
>> +struct sensor_data *sd = sg->sensors[i];
>> +
>> +if (sd->enable) {
>> +send_command = false;
>> +break;
>> +}
>
> This is weird. So there are situations where a request to disable
> a sensor is accepted, but effectively ignored ? Shouldn't that
> return, say, -EBUSY ?
This is because we do not support per-sensor enable/disable. We can only
enable/disable at a sensor-group level.
This patch follows the semantic to disable a sensor group iff all the sensors
belonging to that group have been disabled. Otherwise the sensor alone is marked
to be disabled and returns -ENODATA on reading it.
And a sensor group will be enabled if any of the sensor in that group is
enabled.
I will make changes to the remaining code according to your suggestion.
Thanks and Regards,
Shilpa
Re: [PATCH v2 2/2] hwmon: ibmpowernv: Add attributes to enable/disable sensor groups
On 07/04/2018 02:16 AM, 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
---
Changes from v1:
- Add per-sensor 'enable' attribute
- Return -ENODATA when sensor is disabled
Documentation/hwmon/sysfs-interface | 22 +++
drivers/hwmon/ibmpowernv.c | 281 +++-
2 files changed, 264 insertions(+), 39 deletions(-)
diff --git a/Documentation/hwmon/sysfs-interface
b/Documentation/hwmon/sysfs-interface
index fc337c3..38ab05c 100644
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
Separate patch please.
@@ -184,6 +184,11 @@ vrmVoltage Regulator Module version number.
Affects the way the driver calculates the CPU core reference
voltage from the vid pins.
+in[0-*]_enable Enable or disable the sensor
+ 1 : Enable
+ 0 : Disable
+ RW
+
Also see the Alarms section for status flags associated with voltages.
@@ -409,6 +414,12 @@ temp_reset_history
Reset temp_lowest and temp_highest for all sensors
WO
+temp[1-*]_enable
+ Enable or disable the sensor
+ 1 : Enable
+ 0 : Disable > + RW
+
Some chips measure temperature using external thermistors and an ADC, and
report the temperature measurement as a voltage. Converting this voltage
back to a temperature (or the other way around for limits) requires
@@ -468,6 +479,12 @@ curr_reset_history
Reset currX_lowest and currX_highest for all sensors
WO
+curr[1-*]_enable
+ Enable or disable the sensor
+ 1 : Enable
+ 0 : Disable
+ RW
+
Also see the Alarms section for status flags associated with currents.
*
@@ -566,6 +583,11 @@ power[1-*]_critCritical maximum power.
Unit: microWatt
RW
+power[1-*]_enable Enable or disable the sensor
+ 1 : Enable
+ 0 : Disable
+ RW
+
Also see the Alarms section for status flags associated with power readings.
Any reason for excluding fan, energy, humidity ?
**
diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index f829dad..61e04cf 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -90,8 +90,28 @@ struct sensor_data {
char label[MAX_LABEL_LEN];
char name[MAX_ATTR_LEN];
struct device_attribute dev_attr;
+ struct sensor_group_data *sgdata;
+ struct sensor_data *sdata[3];
+ bool enable;
};
+static struct sensor_group_data {
+ u32 gid;
+ u32 nr_phandle;
+ u32 nr_sensor;
+ enum sensors type;
+ const __be32 *phandles;
+ struct sensor_data **sensors;
+ bool enable;
+} *sg_data;
+
+/*
+ * To synchronise writes to struct sensor_data.enable and
+ * struct sensor_group_data.enable
+ */
+DEFINE_MUTEX(sensor_groups_mutex);
Not as global variable, please.
+static int nr_sensor_groups;
+
Do those have to be static variables ? Why not in struct platform_data ?
struct platform_data {
const struct attribute_group *attr_groups[MAX_SENSOR_TYPE + 1];
u32 sensors_count; /* Total count of sensors from each group */
@@ -105,6 +125,9 @@ static ssize_t show_sensor(struct device *dev, struct
device_attribute *devattr,
ssize_t ret;
u64 x;
+ if (sdata->sgdata && !sdata->enable)
+ return -ENODATA;
+
This return code should be documented in the ABI.
ret = opal_get_sensor_data_u64(sdata->id, );
if (ret)
@@ -120,6 +143,74 @@ 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);
+
+
[PATCH v2 2/2] hwmon: ibmpowernv: Add attributes to enable/disable sensor groups
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
---
Changes from v1:
- Add per-sensor 'enable' attribute
- Return -ENODATA when sensor is disabled
Documentation/hwmon/sysfs-interface | 22 +++
drivers/hwmon/ibmpowernv.c | 281 +++-
2 files changed, 264 insertions(+), 39 deletions(-)
diff --git a/Documentation/hwmon/sysfs-interface
b/Documentation/hwmon/sysfs-interface
index fc337c3..38ab05c 100644
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
@@ -184,6 +184,11 @@ vrmVoltage Regulator Module version number.
Affects the way the driver calculates the CPU core reference
voltage from the vid pins.
+in[0-*]_enable Enable or disable the sensor
+ 1 : Enable
+ 0 : Disable
+ RW
+
Also see the Alarms section for status flags associated with voltages.
@@ -409,6 +414,12 @@ temp_reset_history
Reset temp_lowest and temp_highest for all sensors
WO
+temp[1-*]_enable
+ Enable or disable the sensor
+ 1 : Enable
+ 0 : Disable
+ RW
+
Some chips measure temperature using external thermistors and an ADC, and
report the temperature measurement as a voltage. Converting this voltage
back to a temperature (or the other way around for limits) requires
@@ -468,6 +479,12 @@ curr_reset_history
Reset currX_lowest and currX_highest for all sensors
WO
+curr[1-*]_enable
+ Enable or disable the sensor
+ 1 : Enable
+ 0 : Disable
+ RW
+
Also see the Alarms section for status flags associated with currents.
*
@@ -566,6 +583,11 @@ power[1-*]_critCritical maximum power.
Unit: microWatt
RW
+power[1-*]_enable Enable or disable the sensor
+ 1 : Enable
+ 0 : Disable
+ RW
+
Also see the Alarms section for status flags associated with power readings.
**
diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index f829dad..61e04cf 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -90,8 +90,28 @@ struct sensor_data {
char label[MAX_LABEL_LEN];
char name[MAX_ATTR_LEN];
struct device_attribute dev_attr;
+ struct sensor_group_data *sgdata;
+ struct sensor_data *sdata[3];
+ bool enable;
};
+static struct sensor_group_data {
+ u32 gid;
+ u32 nr_phandle;
+ u32 nr_sensor;
+ enum sensors type;
+ const __be32 *phandles;
+ struct sensor_data **sensors;
+ bool enable;
+} *sg_data;
+
+/*
+ * To synchronise writes to struct sensor_data.enable and
+ * struct sensor_group_data.enable
+ */
+DEFINE_MUTEX(sensor_groups_mutex);
+static int nr_sensor_groups;
+
struct platform_data {
const struct attribute_group *attr_groups[MAX_SENSOR_TYPE + 1];
u32 sensors_count; /* Total count of sensors from each group */
@@ -105,6 +125,9 @@ static ssize_t show_sensor(struct device *dev, struct
device_attribute *devattr,
ssize_t ret;
u64 x;
+ if (sdata->sgdata && !sdata->enable)
+ return -ENODATA;
+
ret = opal_get_sensor_data_u64(sdata->id, );
if (ret)
@@ -120,6 +143,74 @@ 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->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,
+
