[PATCH] powerpc/powernv : Add support to enable sensor groups

[PATCH] powerpc/powernv : Add support to enable sensor groups

[Shilpasri G Bhat]

Adds support to enable/disable a sensor group. This can be used to
select the sensor groups that needs to be copied to main memory by
OCC. Sensor groups like power, temperature, current, voltage,
frequency, utilization can be enabled/disabled at runtime.

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
The skiboot patch for the opal call is posted below:
https://lists.ozlabs.org/pipermail/skiboot/2017-November/009713.html

 arch/powerpc/include/asm/opal-api.h                |  3 +-
 arch/powerpc/include/asm/opal.h                    |  1 +
 .../powerpc/platforms/powernv/opal-sensor-groups.c | 97 +++++++++++++++-------
 arch/powerpc/platforms/powernv/opal-wrappers.S     |  1 +
 4 files changed, 73 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index 233c750..bdca15a 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -201,7 +201,8 @@
 #define OPAL_SET_POWER_SHIFT_RATIO		155
 #define OPAL_SENSOR_GROUP_CLEAR			156
 #define OPAL_PCI_SET_P2P			157
-#define OPAL_LAST				157
+#define OPAL_SENSOR_GROUP_ENABLE		158
+#define OPAL_LAST				158
 
 /* Device tree flags */
 
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 0c545f7..c3c7e77 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -280,6 +280,7 @@ int64_t opal_imc_counters_init(uint32_t type, uint64_t address,
 int opal_get_power_shift_ratio(u32 handle, int token, u32 *psr);
 int opal_set_power_shift_ratio(u32 handle, int token, u32 psr);
 int opal_sensor_group_clear(u32 group_hndl, int token);
+int opal_sensor_group_enable(u32 group_hndl, int token, bool enable);
 
 s64 opal_signal_system_reset(s32 cpu);
 
diff --git a/arch/powerpc/platforms/powernv/opal-sensor-groups.c b/arch/powerpc/platforms/powernv/opal-sensor-groups.c
index 7e5a235..2f95380 100644
--- a/arch/powerpc/platforms/powernv/opal-sensor-groups.c
+++ b/arch/powerpc/platforms/powernv/opal-sensor-groups.c
@@ -23,6 +23,7 @@
 
 struct sg_attr {
 	u32 handle;
+	u32 opal_no;
 	struct kobj_attribute attr;
 };
 
@@ -32,34 +33,44 @@ struct sg_attr {
 	struct sg_attr *sgattrs;
 } *sgs;
 
-static ssize_t sg_store(struct kobject *kobj, struct kobj_attribute *attr,
-			const char *buf, size_t count)
+static int sensor_group_clear(u32 handle)
 {
-	struct sg_attr *sattr = container_of(attr, struct sg_attr, attr);
 	struct opal_msg msg;
-	u32 data;
-	int ret, token;
-
-	ret = kstrtoint(buf, 0, &data);
-	if (ret)
-		return ret;
-
-	if (data != 1)
-		return -EINVAL;
+	int token, ret;
 
 	token = opal_async_get_token_interruptible();
-	if (token < 0) {
-		pr_devel("Failed to get token\n");
+	if (token < 0)
 		return token;
+
+	ret = opal_sensor_group_clear(handle, token);
+	if (ret == OPAL_ASYNC_COMPLETION) {
+		ret = opal_async_wait_response(token, &msg);
+		if (ret) {
+			pr_devel("Failed to wait for the async response\n");
+			ret = -EIO;
+			goto out;
+		}
+		ret = opal_error_code(opal_get_async_rc(msg));
+	} else {
+		ret = opal_error_code(ret);
 	}
 
-	ret = mutex_lock_interruptible(&sg_mutex);
-	if (ret)
-		goto out_token;
+out:
+	opal_async_release_token(token);
+	return ret;
+}
+
+static int sensor_group_enable(u32 handle, int enable)
+{
+	struct opal_msg msg;
+	int token, ret;
+
+	token = opal_async_get_token_interruptible();
+	if (token < 0)
+		return token;
 
-	ret = opal_sensor_group_clear(sattr->handle, token);
-	switch (ret) {
-	case OPAL_ASYNC_COMPLETION:
+	ret = opal_sensor_group_enable(handle, token, enable);
+	if (ret == OPAL_ASYNC_COMPLETION) {
 		ret = opal_async_wait_response(token, &msg);
 		if (ret) {
 			pr_devel("Failed to wait for the async response\n");
@@ -67,20 +78,48 @@ static ssize_t sg_store(struct kobject *kobj, struct kobj_attribute *attr,
 			goto out;
 		}
 		ret = opal_error_code(opal_get_async_rc(msg));
-		if (!ret)
-			ret = count;
+	} else {
+		ret = opal_error_code(ret);
+	}
+
+out:
+	opal_async_release_token(token);
+	return ret;
+}
+
+static ssize_t sg_store(struct kobject *kobj, struct kobj_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct sg_attr *sattr = container_of(attr, struct sg_attr, attr);
+	u32 data;
+	int ret;
+
+	ret = kstrtoint(buf, 0, &data);
+	if (ret)
+		return ret;
+
+	ret = mutex_lock_interruptible(&sg_mutex);
+	if (ret)
+		return ret;
+
+	ret = -EINVAL;
+	switch (sattr->opal_no) {
+	case OPAL_SENSOR_GROUP_CLEAR:
+		if (data == 1)
+			ret = sensor_group_clear(sattr->handle);
 		break;
-	case OPAL_SUCCESS:
-		ret = count;
+	case OPAL_SENSOR_GROUP_ENABLE:
+		if (data == 0 || data == 1)
+			ret = sensor_group_enable(sattr->handle, data);
 		break;
 	default:
-		ret = opal_error_code(ret);
+		break;
 	}
 
-out:
+	if (!ret)
+		ret = count;
+
 	mutex_unlock(&sg_mutex);
-out_token:
-	opal_async_release_token(token);
 	return ret;
 }
 
@@ -91,11 +130,13 @@ static ssize_t sg_store(struct kobject *kobj, struct kobj_attribute *attr,
 			const char *buf, size_t count);
 } ops_info[] = {
 	{ OPAL_SENSOR_GROUP_CLEAR, "clear", sg_store },
+	{ OPAL_SENSOR_GROUP_ENABLE, "enable", sg_store },
 };
 
 static void add_attr(int handle, struct sg_attr *attr, int index)
 {
 	attr->handle = handle;
+	attr->opal_no = ops_info[index].opal_no;
 	sysfs_attr_init(&attr->attr.attr);
 	attr->attr.attr.name = ops_info[index].attr_name;
 	attr->attr.attr.mode = 0220;
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
index 6f4b00a2..ee9205f 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -320,3 +320,4 @@ OPAL_CALL(opal_set_powercap,			OPAL_SET_POWERCAP);
 OPAL_CALL(opal_get_power_shift_ratio,		OPAL_GET_POWER_SHIFT_RATIO);
 OPAL_CALL(opal_set_power_shift_ratio,		OPAL_SET_POWER_SHIFT_RATIO);
 OPAL_CALL(opal_sensor_group_clear,		OPAL_SENSOR_GROUP_CLEAR);
+OPAL_CALL(opal_sensor_group_enable,		OPAL_SENSOR_GROUP_ENABLE);
-- 
1.8.3.1

Re: [PATCH] powerpc/powernv : Add support to enable sensor groups

[Michael Ellerman]

Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:

> Adds support to enable/disable a sensor group. This can be used to
> select the sensor groups that needs to be copied to main memory by
> OCC. Sensor groups like power, temperature, current, voltage,
> frequency, utilization can be enabled/disabled at runtime.
>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
> The skiboot patch for the opal call is posted below:
> https://lists.ozlabs.org/pipermail/skiboot/2017-November/009713.html

Can you remind me why we're doing this with a completely bespoke sysfs
API, rather than using some generic sensors API?

And if we must do it that way, please add documentation for the sysfs
file(s) in Documentation/ABI/.

cheers

Re: [PATCH] powerpc/powernv : Add support to enable sensor groups

[Shilpasri G Bhat]

Hi,

On 11/28/2017 05:07 PM, Michael Ellerman wrote:
> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> 
>> Adds support to enable/disable a sensor group. This can be used to
>> select the sensor groups that needs to be copied to main memory by
>> OCC. Sensor groups like power, temperature, current, voltage,
>> frequency, utilization can be enabled/disabled at runtime.
>>
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>> The skiboot patch for the opal call is posted below:
>> https://lists.ozlabs.org/pipermail/skiboot/2017-November/009713.html
> 
> Can you remind me why we're doing this with a completely bespoke sysfs
> API, rather than using some generic sensors API?
> 

Disabling/Enabling sensor groups is not supported in the current generic sensors
API. And also we dont export all type of sensors in HWMON as not all of them are
environment sensors (like performance).

> And if we must do it that way, please add documentation for the sysfs
> file(s) in Documentation/ABI/.
> 

Will do.

Thanks and Regards,
Shilpa

> cheers
> 

Re: [PATCH] powerpc/powernv : Add support to enable sensor groups

[Stewart Smith]

Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> On 11/28/2017 05:07 PM, Michael Ellerman wrote:
>> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
>> 
>>> Adds support to enable/disable a sensor group. This can be used to
>>> select the sensor groups that needs to be copied to main memory by
>>> OCC. Sensor groups like power, temperature, current, voltage,
>>> frequency, utilization can be enabled/disabled at runtime.
>>>
>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>>> ---
>>> The skiboot patch for the opal call is posted below:
>>> https://lists.ozlabs.org/pipermail/skiboot/2017-November/009713.html
>> 
>> Can you remind me why we're doing this with a completely bespoke sysfs
>> API, rather than using some generic sensors API?
>> 
>
> Disabling/Enabling sensor groups is not supported in the current generic sensors
> API. And also we dont export all type of sensors in HWMON as not all of them are
> environment sensors (like performance).

Are there barriers to adding such concepts to the generic sensors API?

-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCH] powerpc/powernv : Add support to enable sensor groups

[Shilpasri G Bhat]

Hi,

On 12/04/2017 10:11 AM, Stewart Smith wrote:
> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
>> On 11/28/2017 05:07 PM, Michael Ellerman wrote:
>>> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
>>>
>>>> Adds support to enable/disable a sensor group. This can be used to
>>>> select the sensor groups that needs to be copied to main memory by
>>>> OCC. Sensor groups like power, temperature, current, voltage,
>>>> frequency, utilization can be enabled/disabled at runtime.
>>>>
>>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>>>> ---
>>>> The skiboot patch for the opal call is posted below:
>>>> https://lists.ozlabs.org/pipermail/skiboot/2017-November/009713.html
>>>
>>> Can you remind me why we're doing this with a completely bespoke sysfs
>>> API, rather than using some generic sensors API?
>>>
>>
>> Disabling/Enabling sensor groups is not supported in the current generic sensors
>> API. And also we dont export all type of sensors in HWMON as not all of them are
>> environment sensors (like performance).
> 
> Are there barriers to adding such concepts to the generic sensors API?
> 

Yes.

HWMON does not support attributes for a sensor-group. If we are to extend HWMON
to add new per-sensor attributes to disable/enable, then we need to do either of
the below:

1) If any one of the sensor is disabled then all the sensors belonging to that
group will be disabled. OR

2) To disable a sensor group we need to disable all the sensors belonging to
that group.

Another problem is hwmon categorizes the sensor-groups based on the type of
sensors like power, temp. If OCC allows multiple groups of the same type then
this approach adds some more complexity to the user to identify the sensors
belonging to correct group.

And lastly HWMON does not allow platform specific non-standard sensor groups
like CSM, job-scheduler, profiler.

Thanks and Regards,
Shilpa

Re: [PATCH] powerpc/powernv : Add support to enable sensor groups

[Michael Ellerman]

Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> On 12/04/2017 10:11 AM, Stewart Smith wrote:
>> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
>>> On 11/28/2017 05:07 PM, Michael Ellerman wrote:
>>>> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
>>>>
>>>>> Adds support to enable/disable a sensor group. This can be used to
>>>>> select the sensor groups that needs to be copied to main memory by
>>>>> OCC. Sensor groups like power, temperature, current, voltage,
>>>>> frequency, utilization can be enabled/disabled at runtime.
>>>>>
>>>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>>>>> ---
>>>>> The skiboot patch for the opal call is posted below:
>>>>> https://lists.ozlabs.org/pipermail/skiboot/2017-November/009713.html
>>>>
>>>> Can you remind me why we're doing this with a completely bespoke sysfs
>>>> API, rather than using some generic sensors API?
>>>
>>> Disabling/Enabling sensor groups is not supported in the current generic sensors
>>> API. And also we dont export all type of sensors in HWMON as not all of them are
>>> environment sensors (like performance).
>> 
>> Are there barriers to adding such concepts to the generic sensors API?
>
> Yes.
>
> HWMON does not support attributes for a sensor-group. If we are to extend HWMON
> to add new per-sensor attributes to disable/enable, then we need to do either of
> the below:
>
> 1) If any one of the sensor is disabled then all the sensors belonging to that
> group will be disabled. OR
>
> 2) To disable a sensor group we need to disable all the sensors belonging to
> that group.

Either of those sound doable, the first is probably simpler, as long as
there's some way for userspace to understand that it is modifying the
state of the whole group.

> Another problem is hwmon categorizes the sensor-groups based on the type of
> sensors like power, temp. If OCC allows multiple groups of the same type then
> this approach adds some more complexity to the user to identify the sensors
> belonging to correct group.

I don't really understand this one, what do you mean by "If OCC allows"?

Also do we really expect users to be using this API? Or rather tools?

> And lastly HWMON does not allow platform specific non-standard sensor groups
> like CSM, job-scheduler, profiler.

Have we actually made specific proposals to the hwmon maintainers on
adding/changing any of the above? Have they rejected those proposals and
told us to go away?

cheers

Re: [PATCH] powerpc/powernv : Add support to enable sensor groups

[Guenter Roeck]

On Tue, Mar 13, 2018 at 10:02:09PM +1100, Michael Ellerman wrote:
> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> > On 12/04/2017 10:11 AM, Stewart Smith wrote:
> >> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> >>> On 11/28/2017 05:07 PM, Michael Ellerman wrote:
> >>>> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> >>>>
> >>>>> Adds support to enable/disable a sensor group. This can be used to
> >>>>> select the sensor groups that needs to be copied to main memory by
> >>>>> OCC. Sensor groups like power, temperature, current, voltage,
> >>>>> frequency, utilization can be enabled/disabled at runtime.
> >>>>>
> >>>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> >>>>> ---
> >>>>> The skiboot patch for the opal call is posted below:
> >>>>> https://lists.ozlabs.org/pipermail/skiboot/2017-November/009713.html
> >>>>
> >>>> Can you remind me why we're doing this with a completely bespoke sysfs
> >>>> API, rather than using some generic sensors API?
> >>>
> >>> Disabling/Enabling sensor groups is not supported in the current generic sensors
> >>> API. And also we dont export all type of sensors in HWMON as not all of them are
> >>> environment sensors (like performance).
> >> 
> >> Are there barriers to adding such concepts to the generic sensors API?
> >
> > Yes.
> >
> > HWMON does not support attributes for a sensor-group. If we are to extend HWMON
> > to add new per-sensor attributes to disable/enable, then we need to do either of
> > the below:
> >
> > 1) If any one of the sensor is disabled then all the sensors belonging to that
> > group will be disabled. OR
> >
> > 2) To disable a sensor group we need to disable all the sensors belonging to
> > that group.
> 
> Either of those sound doable, the first is probably simpler, as long as
> there's some way for userspace to understand that it is modifying the
> state of the whole group.
> 
> > Another problem is hwmon categorizes the sensor-groups based on the type of
> > sensors like power, temp. If OCC allows multiple groups of the same type then
> > this approach adds some more complexity to the user to identify the sensors
> > belonging to correct group.
> 
> I don't really understand this one, what do you mean by "If OCC allows"?
> 
> Also do we really expect users to be using this API? Or rather tools?
> 
> > And lastly HWMON does not allow platform specific non-standard sensor groups
> > like CSM, job-scheduler, profiler.
> 
> Have we actually made specific proposals to the hwmon maintainers on
> adding/changing any of the above? Have they rejected those proposals and
> told us to go away?
> 
Those don't really sound like sensor groups at all. What does "job-scheduler"
or "profiler" have to do with hardware monitoring ? We do allow additional
attributes if it makes sense, but those should be hardware monitoring related.
We also allow registration with other subsystems (such as gpio) if a hardware
monitoring device also has gpio pins and it seems to cumbersome to request
that an mfd driver is written. However, I am not convinced that completely
unrelated attributes should be handled through the hwmon subsystem; if this
is deemed necessary, it rather seems that hardware monitoring is one of many
functionalities of a given chip, and such functionality should be handled
elsewhere.

For the rest (enabling or disabling sensors dynamically), I am not specifically
opposed to improving the hwmon core to add such support, but someone would have
to make a specific proposal. One key problem is that the hwmon API assumes
'static' sensor allocation. The behavior of sensors appearng or disappearing
at runtime (even though it happens) is not well defined. Any proposal along
that line will need to ensure that userspace behavior is well documented.

Thanks,
Guenter