* [PATCH skeleton v5] skeleton: fixed hard-coding of master OCC hwmon sysfs path for PowerCap object
@ 2016-03-04 9:50 OpenBMC Patches
2016-03-04 9:50 ` OpenBMC Patches
0 siblings, 1 reply; 3+ messages in thread
From: OpenBMC Patches @ 2016-03-04 9:50 UTC (permalink / raw)
To: openbmc
Currently master OCC hwmon sysfs path is hardcoded to '/sys/class/hwmon/hwmon3/'.
This make the '/org/openbmc/sensors/host/PowerCap' interface cannot work when
master OCC is initilized as hwmon device other than 'hwmon3'.
'usr_powercap' attribute should be searched when OCC is active.
When skeleton creates 'PowerCap' objects, OCC may not be active.
So do the search each time setting power cap. Raise an exception when
there is no 'user_powercap' attribute.
This would fix the issue: https://github.com/openbmc/skeleton/issues/42
Also raise an exception when setting power cap fails. The exception
will be returned to REST API caller. This would fix the issue:
https://github.com/openbmc/skeleton/issues/43
Signed-off-by: Yi Li <adamliyi@msn.com>
https://github.com/openbmc/skeleton/pull/50
Yi Li (1):
skeleton: fixed hard-coding of master OCC hwmon sysfs path for
PowerCap object
bin/Sensors.py | 84 ++++++++++++++++++++++++++++++++++++++++++++------
bin/sensor_manager2.py | 3 --
2 files changed, 75 insertions(+), 12 deletions(-)
--
2.7.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH skeleton v5] skeleton: fixed hard-coding of master OCC hwmon sysfs path for PowerCap object
2016-03-04 9:50 [PATCH skeleton v5] skeleton: fixed hard-coding of master OCC hwmon sysfs path for PowerCap object OpenBMC Patches
@ 2016-03-04 9:50 ` OpenBMC Patches
2016-03-07 1:41 ` Andrew Jeffery
0 siblings, 1 reply; 3+ messages in thread
From: OpenBMC Patches @ 2016-03-04 9:50 UTC (permalink / raw)
To: openbmc
From: Yi Li <adamliyi@msn.com>
Currently master OCC hwmon sysfs path is hardcoded to '/sys/class/hwmon/hwmon3/'.
This make the '/org/openbmc/sensors/host/PowerCap' interface cannot work when
master OCC is initilized as hwmon device other than 'hwmon3'.
'usr_powercap' attribute should be searched when OCC is active.
When skeleton creates 'PowerCap' objects, OCC may not be active.
So do the search each time setting power cap. Raise an exception when
there is no 'user_powercap' attribute.
Fixes: openbmc/skeleton#42
Also raise an exception when setting power cap fails. The exception
will be returned to REST API caller. Need to specify the exception
name to trigger a '400' return code.
Fixes: openbmc/skeleton#43
Add code to validate input user powercap value.
Changed the logic of "reading powercap" using PowerCap.getValue().
Previously PowerCap.getValue() returns current user set powercap value.
Now PowerCap.getValue() returns actual system powercap reading from OCC.
Signed-off-by: Yi Li <adamliyi@msn.com>
---
bin/Sensors.py | 84 ++++++++++++++++++++++++++++++++++++++++++++------
bin/sensor_manager2.py | 3 --
2 files changed, 75 insertions(+), 12 deletions(-)
diff --git a/bin/Sensors.py b/bin/Sensors.py
index f951eeb..6f5bc33 100755
--- a/bin/Sensors.py
+++ b/bin/Sensors.py
@@ -1,13 +1,13 @@
#!/usr/bin/python -u
import sys
-import subprocess
#from gi.repository import GObject
import gobject
import dbus
import dbus.service
import dbus.mainloop.glib
import os
+import glob
import Openbmc
## Abstract class, must subclass
@@ -137,21 +137,87 @@ class PowerCap(VirtualSensor):
def __init__(self, bus, name):
VirtualSensor.__init__(self, bus, name)
SensorValue.setValue(self, 0)
- self.sysfs_attr = "/sys/class/hwmon/hwmon3/user_powercap"
##override setValue method
@dbus.service.method(SensorValue.IFACE_NAME,
in_signature='v', out_signature='')
def setValue(self, value):
try:
- cmd_str = "echo "+str(value)+" > "+self.sysfs_attr
- ret = subprocess.check_output(cmd_str, shell=True)
- except subprocess.CalledProcessError as powerexc:
- print "Set PowerCap Error", powerexc.returncode,
- powerexc.output
- return
+ value = int(value)
+ except ValueError:
+ raise dbus.exceptions.DBusException(
+ "Set an integer value",
+ name='org.freedesktop.DBus.Python.TypeError')
+
+ # master occ hwmon path is created dymanically
+ # when occ is active scan hwmon sysfs directory for update
+ for pcap_file in glob.glob('/sys/class/hwmon/*/user_powercap'):
+ occ_i2c_dev = os.path.dirname(pcap_file)+'/device'
+ occ_i2c_dev = os.path.realpath(
+ occ_i2c_dev).split('/').pop()
+ if occ_i2c_dev == '3-0050':
+ setcap_file = pcap_file
+ break
+ else:
+ raise dbus.exceptions.DBusException(
+ "Cannot find user_powercap hwmon attribute, "+
+ "check occ status.",
+ name='org.freedesktop.DBus.Python.TypeError')
+
+ # verify the user set value is in the range [min_cap, max_cap]
+ try:
+ caps_file = open(os.path.dirname(setcap_file) +
+ '/caps_min_powercap', 'r')
+ min_cap = caps_file.readline()
+ caps_file = open(os.path.dirname(setcap_file) +
+ '/caps_max_powercap', 'r')
+ max_cap = caps_file.readline()
+ except IOError:
+ raise dbus.exceptions.DBusException(
+ "Cannot get caps_min or caps_max",
+ name='org.freedesktop.DBus.Error.InvalidArgs')
+
+ if value != 0 and (value < int(min_cap)
+ or value > int(max_cap)):
+ raise dbus.exceptions.DBusException(
+ "Set a value in the range [%d, %d]"
+ % (int(min_cap), int(max_cap)),
+ name='org.freedesktop.DBus.Error.InvalidArgs')
+
+ try:
+ open(setcap_file, 'w').write(str(value));
+ except IOError:
+ raise dbus.exceptions.DBusException(
+ "Cannot set PowerCap",
+ name='org.freedesktop.DBus.Python.TypeError')
print "Set PowerCap: ", value
SensorValue.setValue(self, value)
-
+ @dbus.service.method(SensorValue.IFACE_NAME,
+ in_signature='', out_signature='v')
+ def getValue(self):
+ # Read current powercap sensor. OCC will set system powercap
+ # based on its algorithm
+ for pcap_file in glob.glob(
+ '/sys/class/hwmon/*/caps_curr_powercap'):
+ occ_i2c_dev = os.path.dirname(pcap_file)+'/device'
+ occ_i2c_dev = os.path.realpath(
+ occ_i2c_dev).split('/').pop()
+ if occ_i2c_dev == '3-0050':
+ curr_cap_file = pcap_file
+ break
+ else:
+ raise dbus.exceptions.DBusException(
+ "Cannot find caps_curr_powercap" +
+ "hwmon attribute, " + "check occ status.",
+ name='org.freedesktop.DBus.Python.TypeError')
+
+ try:
+ cap_val = open(curr_cap_file, 'r').read()
+ except IOError:
+ raise dbus.exceptions.DBusException(
+ "Cannot read current powercap",
+ name='org.freedesktop.DBus.Python.TypeError')
+ return cap_val
+
class BootProgressSensor(VirtualSensor):
def __init__(self,bus,name):
VirtualSensor.__init__(self,bus,name)
diff --git a/bin/sensor_manager2.py b/bin/sensor_manager2.py
index b5aac53..926bfea 100755
--- a/bin/sensor_manager2.py
+++ b/bin/sensor_manager2.py
@@ -54,9 +54,6 @@ if __name__ == '__main__':
obj_path = OBJ_PATH+"/host/PowerCap"
sensor_obj = Sensors.PowerCap(bus,obj_path)
- ## hwmon3 is default for master OCC on Barreleye.
- ## should rewrite sensor_manager to remove hardcode
- sensor_obj.sysfs_attr = "/sys/class/hwmon/hwmon3/user_powercap"
root_sensor.add(obj_path,sensor_obj)
obj_path = OBJ_PATH+"/host/BootProgress"
--
2.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH skeleton v5] skeleton: fixed hard-coding of master OCC hwmon sysfs path for PowerCap object
2016-03-04 9:50 ` OpenBMC Patches
@ 2016-03-07 1:41 ` Andrew Jeffery
0 siblings, 0 replies; 3+ messages in thread
From: Andrew Jeffery @ 2016-03-07 1:41 UTC (permalink / raw)
To: OpenBMC Patches, openbmc
[-- Attachment #1: Type: text/plain, Size: 9799 bytes --]
Hi Yi Li,
On Fri, 2016-03-04 at 03:50 -0600, OpenBMC Patches wrote:
> From: Yi Li <adamliyi@msn.com>
>
> Currently master OCC hwmon sysfs path is hardcoded to '/sys/class/hwmon/hwmon3/'.
> This make the '/org/openbmc/sensors/host/PowerCap' interface cannot work when
> master OCC is initilized as hwmon device other than 'hwmon3'.
>
> 'usr_powercap' attribute should be searched when OCC is active.
> When skeleton creates 'PowerCap' objects, OCC may not be active.
> So do the search each time setting power cap. Raise an exception when
> there is no 'user_powercap' attribute.
> Fixes: openbmc/skeleton#42
>
> Also raise an exception when setting power cap fails. The exception
> will be returned to REST API caller. Need to specify the exception
> name to trigger a '400' return code.
> Fixes: openbmc/skeleton#43
>
> Add code to validate input user powercap value.
>
> Changed the logic of "reading powercap" using PowerCap.getValue().
> Previously PowerCap.getValue() returns current user set powercap value.
> Now PowerCap.getValue() returns actual system powercap reading from OCC.
Taking the commit message at face value it sounds like this should be
split into multiple commits.
>
> Signed-off-by: Yi Li <adamliyi@msn.com>
> ---
> bin/Sensors.py | 84 ++++++++++++++++++++++++++++++++++++++++++++------
> bin/sensor_manager2.py | 3 --
> 2 files changed, 75 insertions(+), 12 deletions(-)
>
> diff --git a/bin/Sensors.py b/bin/Sensors.py
> index f951eeb..6f5bc33 100755
> --- a/bin/Sensors.py
> +++ b/bin/Sensors.py
> @@ -1,13 +1,13 @@
> #!/usr/bin/python -u
>
> import sys
> -import subprocess
> #from gi.repository import GObject
> import gobject
> import dbus
> import dbus.service
> import dbus.mainloop.glib
> import os
> +import glob
> import Openbmc
>
> ## Abstract class, must subclass
> @@ -137,21 +137,87 @@ class PowerCap(VirtualSensor):
> > > def __init__(self, bus, name):
> > > > VirtualSensor.__init__(self, bus, name)
> > > > SensorValue.setValue(self, 0)
> -> > > self.sysfs_attr = "/sys/class/hwmon/hwmon3/user_powercap"
> > > ##override setValue method
> > > @dbus.service.method(SensorValue.IFACE_NAME,
> > > > in_signature='v', out_signature='')
> > > def setValue(self, value):
> > > > try:
> -> > > > cmd_str = "echo "+str(value)+" > "+self.sysfs_attr
> -> > > > ret = subprocess.check_output(cmd_str, shell=True)
> -> > > except subprocess.CalledProcessError as powerexc:
> -> > > > print "Set PowerCap Error", powerexc.returncode,
> -> > > > powerexc.output
> -> > > > return
> +> > > > value = int(value)
> +> > > except ValueError:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Set an integer value",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> +
> +> > > # master occ hwmon path is created dymanically
> +> > > # when occ is active scan hwmon sysfs directory for update
> +> > > for pcap_file in glob.glob('/sys/class/hwmon/*/user_powercap'):
> +> > > > occ_i2c_dev = os.path.dirname(pcap_file)+'/device'
Apologies for not raising some of these issues when I responded to your
initial patch. But:
os.path.join() gives us a way to avoid using string concatenation for
path building. Maybe we could do something like:
for pcap_file in glob.glob('/sys/class/hwmon/*/user_powercap'):
pcap_dir = os.path.dirname(pcap_file)
occ_i2c_dev = os.path.join(pcap_dir, "device")
> + occ_i2c_dev = os.path.realpath(
> +> > > > > > occ_i2c_dev).split('/').pop()
Again I think we could be taking better advantage of the os.path
methods - it has a basename() which I believe will provide what you
want:
occ_i2c_dev = os.path.basename(os.path.realpath(occ_i2c_dev))
> + if occ_i2c_dev == '3-0050':
> +> > > > > setcap_file = pcap_file
> +> > > > > break
> +> > > else:
So I learnt something new here - loops can have else cases[1]. It hurt
my brain a little but again [1] documents this approach as a good use
-case for the feature. Without knowing of the feature I probably
would've done something like:
from os import path as p # reduce verbosity for purpose of example
def upc_to_dev(user_powercap):
return p.realpath(p.join(p.dirname(user_powercap), 'device'))
def setValue(self, value):
...
upc_pattern = '/sys/class/hwmon/*/user_powercap'
setcap_files = [ x for x in glob.glob(upc_pattern)
if p.basename(upc_to_dev(x)) == '3-0050' ]
if not setcap_files:
raise dbusexceptions.DBusException(...)
setcap_file = setcap_files[0]
Does that improve the readability? Reduce it (e.g. use of list
comprehension)? Maybe I'm just bikeshedding here, but the for/else
concept was quite a surprise to me.
[1] http://python-notes.curiousefficiency.org/en/latest/python_concepts/break_else.html
> + raise dbus.exceptions.DBusException(
> +> > > > > "Cannot find user_powercap hwmon attribute, "+
> +> > > > > "check occ status.",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> +
> +> > > # verify the user set value is in the range [min_cap, max_cap]
Can you please extract this out to a helper? Something like
def get_powercap_range(cap_file):
...
return min_cap, max_cap
> + try:
> +> > > > caps_file = open(os.path.dirname(setcap_file) +
> +> > > > > > '/caps_min_powercap', 'r')
Searching for 'close' gives no hits - we're leaking this resource?
Maybe we should be using contexts:
try:
with open(.../caps_min_powercap) as f_min:
with open(.../caps_max_powercap) as f_max:
return int(f_min.readline()), int(f_max.readline())
except IOError:
raise dbus.exceptions.DBusException(...)
Separately, os.path.join() again here, instead of string concat.
> + min_cap = caps_file.readline()
> +> > > > caps_file = open(os.path.dirname(setcap_file) +
> +> > > > > > '/caps_max_powercap', 'r')
> +> > > > max_cap = caps_file.readline()
> +> > > except IOError:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Cannot get caps_min or caps_max",
> +> > > > > name='org.freedesktop.DBus.Error.InvalidArgs')
> +
> + if value != 0 and (value < int(min_cap)
> +> > > > > or value > int(max_cap)):
This can just be:
min_cap, max_cap = get_powercap_range(setcaps_file)
if min_cap < value < max_cap:
for the condition, with
try:
with open(setcap_file, 'w') as f:
f.write(str(value))
except IOError:
raise dbus.exceptions.DBusException(...)
else:
print "Set PowerCap: ", value
SensorValue.setValue(self, value)
else:
raise dbus.exceptions.DBusException(...)
as the if/else bodies.
> + raise dbus.exceptions.DBusException(
> +> > > > > "Set a value in the range [%d, %d]"
> +> > > > > % (int(min_cap), int(max_cap)),
> +> > > > > name='org.freedesktop.DBus.Error.InvalidArgs')
> +
> +> > > try:
> +> > > > open(setcap_file, 'w').write(str(value));
Using the context as above prevents leaking the open file resource.
> + except IOError:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Cannot set PowerCap",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> > > > print "Set PowerCap: ", value
> > > > SensorValue.setValue(self, value)
> -> >
> +> > @dbus.service.method(SensorValue.IFACE_NAME,
> +> > > in_signature='', out_signature='v')
> +> > def getValue(self):
> +> > > # Read current powercap sensor. OCC will set system powercap
> +> > > # based on its algorithm
This is a copy/paste of the 'find-the-device' logic. Can we please
extract it to a helper method? This is partly resolved in my
alternative above.
> + for pcap_file in glob.glob(
> +> > > > > '/sys/class/hwmon/*/caps_curr_powercap'):
> +> > > > occ_i2c_dev = os.path.dirname(pcap_file)+'/device'
> +> > > > occ_i2c_dev = os.path.realpath(
> +> > > > > > occ_i2c_dev).split('/').pop()
> +> > > > if occ_i2c_dev == '3-0050':
> +> > > > > curr_cap_file = pcap_file
> +> > > > > break
> +> > > else:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Cannot find caps_curr_powercap" +
> +> > > > > "hwmon attribute, " + "check occ status.",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> +
> +> > > try:
> +> > > > cap_val = open(curr_cap_file, 'r').read()
Again, file isn't closed, probably best use contexts?
> + except IOError:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Cannot read current powercap",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> +> > > return cap_val
This is fine as a str? Should it be an int?
> +
> class BootProgressSensor(VirtualSensor):
> > > def __init__(self,bus,name):
> > > > VirtualSensor.__init__(self,bus,name)
> diff --git a/bin/sensor_manager2.py b/bin/sensor_manager2.py
> index b5aac53..926bfea 100755
> --- a/bin/sensor_manager2.py
> +++ b/bin/sensor_manager2.py
> @@ -54,9 +54,6 @@ if __name__ == '__main__':
>
> > > obj_path = OBJ_PATH+"/host/PowerCap"
> > > sensor_obj = Sensors.PowerCap(bus,obj_path)
> -> > ## hwmon3 is default for master OCC on Barreleye.
> -> > ## should rewrite sensor_manager to remove hardcode
> -> > sensor_obj.sysfs_attr = "/sys/class/hwmon/hwmon3/user_powercap"
> > > root_sensor.add(obj_path,sensor_obj)
>
> > > obj_path = OBJ_PATH+"/host/BootProgress"
Cheers,
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-07 1:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-04 9:50 [PATCH skeleton v5] skeleton: fixed hard-coding of master OCC hwmon sysfs path for PowerCap object OpenBMC Patches
2016-03-04 9:50 ` OpenBMC Patches
2016-03-07 1:41 ` Andrew Jeffery
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.