phosphor-hwmon funcitonal interface

phosphor-hwmon funcitonal interface

[Patrick Venture]

Right now, if a sensor has an explicit hwmon fault interface, then we
have the notion of a status:

https://github.com/openbmc/phosphor-hwmon/blob/master/sensor.cpp#L205

However, I propose that if there isn't a fault file, we still add the
interface, and set to functional=true.  This would allow us to change
it to false in the following condition:

https://github.com/openbmc/phosphor-hwmon/blob/master/hwmonio.cpp#L127

So basically, instead of returning -rc on failure (optionally), it
could optionally set functional to false, and phosphor-host-ipmid
would need to know to check this additional interface.

Thoughts?  (IIRC< I've mentioned this idea before, but now i'm setting
up some cycles to hammer it out, as I continue to implement unit-tests
in phosphor-hwmon)

Patrick

Re: phosphor-hwmon funcitonal interface

[Lei YU]

> Thoughts?  (IIRC< I've mentioned this idea before, but now i'm setting
> up some cycles to hammer it out, as I continue to implement unit-tests
> in phosphor-hwmon)

I second this proposal.

Be noted that this "functional" property on sensor is not the same one on
inventory, and from my understanding, this functional property is not being
used by other services (yet), it's just used to check if hwmon should read the
value for the sensor or not.

So we could re-use this property to indicate if a sensor is functional or not,
especially useful for fans that will get reading error when it's turned off,
e.g. the fans using Aspeed fan/pwm driver.

@Matt What's your idea?

Re: phosphor-hwmon funcitonal interface

[Matthew Barth]

On 1/14/19 2:31 PM, Patrick Venture wrote:
> Right now, if a sensor has an explicit hwmon fault interface, then we
> have the notion of a status:
> 
> https://github.com/openbmc/phosphor-hwmon/blob/master/sensor.cpp#L205
> 
> However, I propose that if there isn't a fault file, we still add the
> interface, and set to functional=true.  This would allow us to change
> it to false in the following condition:
> 
Would this be optional so this interface/property doesnt show on sensors 
that dont want it?
> https://github.com/openbmc/phosphor-hwmon/blob/master/hwmonio.cpp#L127
> 
> So basically, instead of returning -rc on failure (optionally), it
> could optionally set functional to false, and phosphor-host-ipmid
> would need to know to check this additional interface.
> 
I'm ok with this. Where and how would this optionally be configured for 
a device and its sensors?

> Thoughts?  (IIRC< I've mentioned this idea before, but now i'm setting
> up some cycles to hammer it out, as I continue to implement unit-tests
> in phosphor-hwmon)
> 
> Patrick
> 

Re: phosphor-hwmon funcitonal interface

[Matthew Barth]

On 1/14/19 9:33 PM, Lei YU wrote:
>> Thoughts?  (IIRC< I've mentioned this idea before, but now i'm setting
>> up some cycles to hammer it out, as I continue to implement unit-tests
>> in phosphor-hwmon)
> 
> I second this proposal.
> 
> Be noted that this "functional" property on sensor is not the same one on
> inventory, and from my understanding, this functional property is not being
> used by other services (yet), it's just used to check if hwmon should read the
> value for the sensor or not.
> 
Actually, phosphor-fan-presence/control is using the functional property 
that's currently being created where the 'fault' file exists in sysfs 
for the OCCs.
> So we could re-use this property to indicate if a sensor is functional or not,
> especially useful for fans that will get reading error when it's turned off,
> e.g. the fans using Aspeed fan/pwm driver.
> 
This case seems to not follow whether a fan is functional or not when 
its turned off. The fan is still functional, just not powered.
> @Matt What's your idea?
> 

Re: phosphor-hwmon funcitonal interface

[Lei YU]

> Actually, phosphor-fan-presence/control is using the functional property
> that's currently being created where the 'fault' file exists in sysfs
> for the OCCs.

Good to know. Thanks for the info.

> > So we could re-use this property to indicate if a sensor is functional or not,
> > especially useful for fans that will get reading error when it's turned off,
> > e.g. the fans using Aspeed fan/pwm driver.
> >
> This case seems to not follow whether a fan is functional or not when
> its turned off. The fan is still functional, just not powered.

Yeah, but there is no way to tell the difference between power off and
non-functional for Aspeed pwm fans, that the driver gives errors on reading
fan speed.
So the current solution is to use NEGATIVE_ERRNO_ON_FAIL config to tell
phosphor-hwmon to return the -errno instead of throwing.
That way, the reading of the sensor becomes a negative value.

I think a "functional" property is more appropriate for such case than a
negative reading of sensor value.
The code to check this property is free to check the power status as well, so
it knows if a fan is *really* non-functional or not based on the power status.

Re: phosphor-hwmon funcitonal interface

[Patrick Venture]

On Tue, Jan 15, 2019 at 5:51 PM Lei YU <mine260309@gmail.com> wrote:
>
> > Actually, phosphor-fan-presence/control is using the functional property
> > that's currently being created where the 'fault' file exists in sysfs
> > for the OCCs.
>
> Good to know. Thanks for the info.
>
> > > So we could re-use this property to indicate if a sensor is functional or not,
> > > especially useful for fans that will get reading error when it's turned off,
> > > e.g. the fans using Aspeed fan/pwm driver.
> > >
> > This case seems to not follow whether a fan is functional or not when
> > its turned off. The fan is still functional, just not powered.

There's also the notion in phosphor-hwmon where it'll remove a sensor
from dbus on some failures... - just throwing that out there.

>
> Yeah, but there is no way to tell the difference between power off and
> non-functional for Aspeed pwm fans, that the driver gives errors on reading
> fan speed.
> So the current solution is to use NEGATIVE_ERRNO_ON_FAIL config to tell
> phosphor-hwmon to return the -errno instead of throwing.
> That way, the reading of the sensor becomes a negative value.
>
> I think a "functional" property is more appropriate for such case than a
> negative reading of sensor value.

Yes. that's something I'm hoping to resolve.

> The code to check this property is free to check the power status as well, so
> it knows if a fan is *really* non-functional or not based on the power status.

Agreed.