dbus-sensors

dbus-sensors

[Matt Spinler]

Hi James,

We're looking into using dbus-sensors(HwmonTemp and PSU) in the future,
but would need to make a few changes to fit our requirements.  Was wondering
what you'd think of the following:

1. Check if a sensor has a _fault sysfs file, and if it does and it
   is nonzero, set the Functional property on the OperationalStatus
   interface to false (and/or maybe 6 below).

2. After the 10 failed reads, instead of just setting the sensor to 0
   also make a D-Bus call to create a phosphor-logging event log and set
   the OperationalStatus sensor to false.

3. After creating this event log, make sure not to do it again until
   main power is cycled.

4. If not already supported (was unsure), be able to find an
   _input file based on a value it has in the corresponding _label file.

5. We have a case where a driver isn't loaded with power off, so somehow
   we still need the sensors to stay on D-Bus when off (and show them
   as not available).

6. Maybe add a new property to Sensor.Value on the validity
   of the value property, for when driver is unloaded or there is an
   error or the sensor reading is otherwise not valid.  We could add
  this to phosphor-hwmon at the same time.
  (I think this was mentioned on the list before).

We would definitely of course work with you on the best way to
accomplish these, and I know #6 needs more discussion on if
this is something we want to do in OpenBMC, though I thought
I remembered an earlier discussion where it was popular.

Thanks,
Matt

Re: dbus-sensors

[James Feist]

On 4/21/2020 12:35 PM, Matt Spinler wrote:
> Hi James,
> 
> We're looking into using dbus-sensors(HwmonTemp and PSU) in the future,
> but would need to make a few changes to fit our requirements.  Was 
> wondering
> what you'd think of the following:
> 
> 1. Check if a sensor has a _fault sysfs file, and if it does and it
>    is nonzero, set the Functional property on the OperationalStatus
>    interface to false (and/or maybe 6 below)
Sounds ok.

> 
> 2. After the 10 failed reads, instead of just setting the sensor to 0
>    also make a D-Bus call to create a phosphor-logging event log and set
>    the OperationalStatus sensor to false.

Sounds ok.

> 
> 3. After creating this event log, make sure not to do it again until
>    main power is cycled.

I'd rather this be until the status goes OK again.

> 
> 4. If not already supported (was unsure), be able to find an
>    _input file based on a value it has in the corresponding _label file.

PSU sensor does this, hwmontemp does it by index.
> 
> 5. We have a case where a driver isn't loaded with power off, so somehow
>    we still need the sensors to stay on D-Bus when off (and show them
>    as not available).

All sensors are on d-bus all the time, its based on the EM config.

> 
> 6. Maybe add a new property to Sensor.Value on the validity
>    of the value property, for when driver is unloaded or there is an
>    error or the sensor reading is otherwise not valid.  We could add
>   this to phosphor-hwmon at the same time.
>   (I think this was mentioned on the list before).

Yes, this is where we've used std::nan, I'm not sure if that made it to 
all sensors as it's not tested very much. I know the fans do this.

> 
> We would definitely of course work with you on the best way to
> accomplish these, and I know #6 needs more discussion on if
> this is something we want to do in OpenBMC, though I thought
> I remembered an earlier discussion where it was popular.
> 
> Thanks,
> Matt

Re: dbus-sensors

[Brad Bishop]

at 5:54 PM, James Feist <james.feist@linux.intel.com> wrote:

> On 4/21/2020 12:35 PM, Matt Spinler wrote:
>> Hi James,
>> We're looking into using dbus-sensors(HwmonTemp and PSU) in the future,
>> but would need to make a few changes to fit our requirements.  Was  
>> wondering
>> what you'd think of the following:
>> 1. Check if a sensor has a _fault sysfs file, and if it does and it
>>    is nonzero, set the Functional property on the OperationalStatus
>>    interface to false (and/or maybe 6 below)
> Sounds ok.
>
>> 2. After the 10 failed reads, instead of just setting the sensor to 0
>>    also make a D-Bus call to create a phosphor-logging event log and set
>>    the OperationalStatus sensor to false.
>
> Sounds ok.
>
>> 3. After creating this event log, make sure not to do it again until
>>    main power is cycled.
>
> I'd rather this be until the status goes OK again.

We have user-experience requirements that the server administrator must be  
“nagged” in this fashion when something is broken like this.  Could the  
behavior be selectable via build switch?  Any other ideas on how to  
accommodate both behaviors?

>
>> 4. If not already supported (was unsure), be able to find an
>>    _input file based on a value it has in the corresponding _label file.
>
> PSU sensor does this, hwmontemp does it by index.

I think the question here was can we change the temp sensor implementation  
to do that also?

>> 5. We have a case where a driver isn't loaded with power off, so somehow
>>    we still need the sensors to stay on D-Bus when off (and show them
>>    as not available).
>
> All sensors are on d-bus all the time, its based on the EM config.
>
>> 6. Maybe add a new property to Sensor.Value on the validity
>>    of the value property, for when driver is unloaded or there is an
>>    error or the sensor reading is otherwise not valid.  We could add
>>   this to phosphor-hwmon at the same time.
>>   (I think this was mentioned on the list before).
>
> Yes, this is where we've used std::nan, I'm not sure if that made it to  
> all sensors as it's not tested very much. I know the fans do this.
>
>> We would definitely of course work with you on the best way to
>> accomplish these, and I know #6 needs more discussion on if
>> this is something we want to do in OpenBMC, though I thought
>> I remembered an earlier discussion where it was popular.
>> Thanks,
>> Matt

Re: dbus-sensors

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

On Wed, Apr 22, 2020 at 07:56:14AM -0400, Brad Bishop wrote:
> at 5:54 PM, James Feist <james.feist@linux.intel.com> wrote:
> >> 3. After creating this event log, make sure not to do it again until
> >>    main power is cycled.
> >
> > I'd rather this be until the status goes OK again.
> 
> We have user-experience requirements that the server administrator must be  
> “nagged” in this fashion when something is broken like this.  Could the  
> behavior be selectable via build switch?  Any other ideas on how to  
> accommodate both behaviors?

This sounds like a form of error filtering.  Shouldn't that decision be
done at a much higher level in the stack than down in the entity that
reads the hardware sensor?
-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: dbus-sensors

[Brad Bishop]

at 8:11 AM, Patrick Williams <patrick@stwcx.xyz> wrote:

> On Wed, Apr 22, 2020 at 07:56:14AM -0400, Brad Bishop wrote:
>> at 5:54 PM, James Feist <james.feist@linux.intel.com> wrote:
>>>> 3. After creating this event log, make sure not to do it again until
>>>>    main power is cycled.
>>>
>>> I'd rather this be until the status goes OK again.
>>
>> We have user-experience requirements that the server administrator must be
>> “nagged” in this fashion when something is broken like this.  Could the
>> behavior be selectable via build switch?  Any other ideas on how to
>> accommodate both behaviors?
>
> This sounds like a form of error filtering.  Shouldn't that decision be
> done at a much higher level in the stack than down in the entity that
> reads the hardware sensor?

Thats an interesting thought.  When the error reporting code sees the error  
for the first time, it could maintain a list of errors that it needs to  
“replay” at different system events, like when the server powers on.

This is certainly more flexible and I like the idea - but one down side  
though is the logging code becomes stateful and the complexity is slightly  
higher.

Re: dbus-sensors

[Matt Spinler]

On 4/22/2020 7:24 AM, Brad Bishop wrote:
> at 8:11 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
>
>> On Wed, Apr 22, 2020 at 07:56:14AM -0400, Brad Bishop wrote:
>>> at 5:54 PM, James Feist <james.feist@linux.intel.com> wrote:
>>>>> 3. After creating this event log, make sure not to do it again until
>>>>>    main power is cycled.
>>>>
>>>> I'd rather this be until the status goes OK again.
>>>
>>> We have user-experience requirements that the server administrator 
>>> must be
>>> “nagged” in this fashion when something is broken like this. Could the
>>> behavior be selectable via build switch?  Any other ideas on how to
>>> accommodate both behaviors?
>>
>> This sounds like a form of error filtering.  Shouldn't that decision be
>> done at a much higher level in the stack than down in the entity that
>> reads the hardware sensor?
>
> Thats an interesting thought.  When the error reporting code sees the 
> error for the first time, it could maintain a list of errors that it 
> needs to “replay” at different system events, like when the server 
> powers on.

It isn't really nagging, it's more like error throttling.  At most, only 
log one error per power cycle.
I have to check still, but we may also need to still log the other 
errors, just with a
different severity (for debug purposes).

I kinda like this filtering idea too.  It is flexible and we would only 
have to do it in one place as
opposed to in all the sensor applications we end up using, and could 
also be used to change the
event log severities as mentioned above.  We will have to make sure when 
creating the event log
that it contains enough information to recognize the device that is 
failing so that we can filter
appropriately.


>
> This is certainly more flexible and I like the idea - but one down 
> side though is the logging code becomes stateful and the complexity is 
> slightly higher.

Re: dbus-sensors

[Matt Spinler]

Great!  Sounds like we should be able to make thing work.
A few comments below.

On 4/21/2020 4:54 PM, James Feist wrote:
> On 4/21/2020 12:35 PM, Matt Spinler wrote:
>> Hi James,
>>
>> We're looking into using dbus-sensors(HwmonTemp and PSU) in the future,
>> but would need to make a few changes to fit our requirements. Was 
>> wondering
>> what you'd think of the following:
>>
>> 1. Check if a sensor has a _fault sysfs file, and if it does and it
>>    is nonzero, set the Functional property on the OperationalStatus
>>    interface to false (and/or maybe 6 below)
> Sounds ok.
>
>>
>> 2. After the 10 failed reads, instead of just setting the sensor to 0
>>    also make a D-Bus call to create a phosphor-logging event log and set
>>    the OperationalStatus sensor to false.
>
> Sounds ok.
>
>>
>> 3. After creating this event log, make sure not to do it again until
>>    main power is cycled.
>
> I'd rather this be until the status goes OK again.

As suggested by Patrick, I agree the throttling can be done elsewhere, so we
would just create the logs as you state here.

>
>>
>> 4. If not already supported (was unsure), be able to find an
>>    _input file based on a value it has in the corresponding _label file.
>
> PSU sensor does this, hwmontemp does it by index.

Would you be OK with us also adding this to PSUSensor?

>>
>> 5. We have a case where a driver isn't loaded with power off, so somehow
>>    we still need the sensors to stay on D-Bus when off (and show them
>>    as not available).
>
> All sensors are on d-bus all the time, its based on the EM config.

Perfect!

>
>>
>> 6. Maybe add a new property to Sensor.Value on the validity
>>    of the value property, for when driver is unloaded or there is an
>>    error or the sensor reading is otherwise not valid.  We could add
>>   this to phosphor-hwmon at the same time.
>>   (I think this was mentioned on the list before).
>
> Yes, this is where we've used std::nan, I'm not sure if that made it 
> to all sensors as it's not tested very much. I know the fans do this.
>
>>
>> We would definitely of course work with you on the best way to
>> accomplish these, and I know #6 needs more discussion on if
>> this is something we want to do in OpenBMC, though I thought
>> I remembered an earlier discussion where it was popular.
>>
>> Thanks,
>> Matt

Re: dbus-sensors

[Bills, Jason M]

On 4/22/2020 9:19 AM, Matt Spinler wrote:
> Great!  Sounds like we should be able to make thing work.
> A few comments below.
> 
> On 4/21/2020 4:54 PM, James Feist wrote:
>> On 4/21/2020 12:35 PM, Matt Spinler wrote:
>>> Hi James,
>>>
>>> We're looking into using dbus-sensors(HwmonTemp and PSU) in the future,
>>> but would need to make a few changes to fit our requirements. Was 
>>> wondering
>>> what you'd think of the following:
>>>
>>> 1. Check if a sensor has a _fault sysfs file, and if it does and it
>>>    is nonzero, set the Functional property on the OperationalStatus
>>>    interface to false (and/or maybe 6 below)
>> Sounds ok.
>>
>>>
>>> 2. After the 10 failed reads, instead of just setting the sensor to 0
>>>    also make a D-Bus call to create a phosphor-logging event log and set
>>>    the OperationalStatus sensor to false.
>>
>> Sounds ok.
>>
>>>
>>> 3. After creating this event log, make sure not to do it again until
>>>    main power is cycled.
>>
>> I'd rather this be until the status goes OK again.
> 
> As suggested by Patrick, I agree the throttling can be done elsewhere, 
> so we
> would just create the logs as you state here.
> 
I'm not familiar with the sensors, but for this specific case, would it 
work to base the log on OperationalStatus?  It seems logical to not log 
events for sensors that are not operational, and since it will be set to 
false after the 10 failures, it would stop the log from nagging.

>>
>>>
>>> 4. If not already supported (was unsure), be able to find an
>>>    _input file based on a value it has in the corresponding _label file.
>>
>> PSU sensor does this, hwmontemp does it by index.
> 
> Would you be OK with us also adding this to PSUSensor?
> 
>>>
>>> 5. We have a case where a driver isn't loaded with power off, so somehow
>>>    we still need the sensors to stay on D-Bus when off (and show them
>>>    as not available).
>>
>> All sensors are on d-bus all the time, its based on the EM config.
> 
> Perfect!
> 
>>
>>>
>>> 6. Maybe add a new property to Sensor.Value on the validity
>>>    of the value property, for when driver is unloaded or there is an
>>>    error or the sensor reading is otherwise not valid.  We could add
>>>   this to phosphor-hwmon at the same time.
>>>   (I think this was mentioned on the list before).
>>
>> Yes, this is where we've used std::nan, I'm not sure if that made it 
>> to all sensors as it's not tested very much. I know the fans do this.
>>
>>>
>>> We would definitely of course work with you on the best way to
>>> accomplish these, and I know #6 needs more discussion on if
>>> this is something we want to do in OpenBMC, though I thought
>>> I remembered an earlier discussion where it was popular.
>>>
>>> Thanks,
>>> Matt
> 

Re: dbus-sensors

[James Feist]

On 4/22/2020 9:01 AM, Matt Spinler wrote:
> 
> 
> On 4/22/2020 7:24 AM, Brad Bishop wrote:
>> at 8:11 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
>>
>>> On Wed, Apr 22, 2020 at 07:56:14AM -0400, Brad Bishop wrote:
>>>> at 5:54 PM, James Feist <james.feist@linux.intel.com> wrote:
>>>>>> 3. After creating this event log, make sure not to do it again until
>>>>>>    main power is cycled.
>>>>>
>>>>> I'd rather this be until the status goes OK again.
>>>>
>>>> We have user-experience requirements that the server administrator 
>>>> must be
>>>> “nagged” in this fashion when something is broken like this. Could the
>>>> behavior be selectable via build switch?  Any other ideas on how to
>>>> accommodate both behaviors?

As long as it's configurable somehow, fine by me.

>>>
>>> This sounds like a form of error filtering.  Shouldn't that decision be
>>> done at a much higher level in the stack than down in the entity that
>>> reads the hardware sensor?
>>
>> Thats an interesting thought.  When the error reporting code sees the 
>> error for the first time, it could maintain a list of errors that it 
>> needs to “replay” at different system events, like when the server 
>> powers on.
> 
> It isn't really nagging, it's more like error throttling.  At most, only 
> log one error per power cycle.
> I have to check still, but we may also need to still log the other 
> errors, just with a
> different severity (for debug purposes).
> 
> I kinda like this filtering idea too.  It is flexible and we would only 
> have to do it in one place as
> opposed to in all the sensor applications we end up using, and could 
> also be used to change the
> event log severities as mentioned above.  We will have to make sure when 
> creating the event log
> that it contains enough information to recognize the device that is 
> failing so that we can filter
> appropriately.
> 
> 
>>
>> This is certainly more flexible and I like the idea - but one down 
>> side though is the logging code becomes stateful and the complexity is 
>> slightly higher.
> 

Re: dbus-sensors

[James Feist]

On 4/22/2020 9:19 AM, Matt Spinler wrote:
>>>
>>> 4. If not already supported (was unsure), be able to find an
>>>    _input file based on a value it has in the corresponding _label file.
>>
>> PSU sensor does this, hwmontemp does it by index.
> 
> Would you be OK with us also adding this to PSUSensor?

PSUSensor already does this? Do you mean hwmontemp sensor? I'd be fine 
with converting the index scheme to a label scheme.

Re: dbus-sensors

[Deng Tyler]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

Hi Jame:
    I am investigating and porting psusensor to my platform. I have a
question item 5.
>> 5. We have a case where a driver isn't loaded with power off, so somehow
>>    we still need the sensors to stay on D-Bus when off (and show them
>>    as not available).
>
> All sensors are on d-bus all the time, its based on the EM config.

I create 2 psu config, psu1 and psu2, in entityManager and booting my
system with 1 psu. There is only psu1 sensor object created in psusensor
service.
After inserting psu2 and the driver binding hwmon for psu2, psusensor
service don't create psu2 sensor object.
Does it work as design? or I need update dbus-sensor?
My dbus-sensor revision is "c140e2008b038b5fbf07aca0795b8030224eddd4".

Tyler

James Feist <james.feist@linux.intel.com> 於 2020年4月23日 週四 上午1:27寫道：

> On 4/22/2020 9:19 AM, Matt Spinler wrote:
> >>>
> >>> 4. If not already supported (was unsure), be able to find an
> >>>    _input file based on a value it has in the corresponding _label
> file.
> >>
> >> PSU sensor does this, hwmontemp does it by index.
> >
> > Would you be OK with us also adding this to PSUSensor?
>
> PSUSensor already does this? Do you mean hwmontemp sensor? I'd be fine
> with converting the index scheme to a label scheme.
>
>

[-- Attachment #2: Type: text/html, Size: 2129 bytes --]