dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)

dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)

[Max Lai/WYHQ/Wiwynn]

[-- Attachment #1.1: Type: text/plain, Size: 1993 bytes --]

Hi James,
Our validation team met a problem in dbus-sensor recently. The bug is about that if we set the upper non-critical (unc) threshold value smaller than reading value, we should get only one assert log but we got 3 logs ( assert log, de-assert log and then assert log) .
We traced the code of Ipmbsensor. The picture below is the code flow of Ipmbsensor.

[cid:image001.png@01D5E707.664E4310]

We found that when we set the threshold , the function match (configMatch) which is registered in Ipmbsensor would catch the signal like the below

[cid:image002.png@01D5E707.664E4310]

When it trigger the handler (eventhandler) and then enter the function of createsensor() and later enter the function of setInitialProperties(). In the setInitialProperties(), It would initial the property (setting the threshold of alarm to default (false) is the root cause). This would trigger Ipmbsensor to send the deassert log. We was wondering that why setting the threshold would initial the property. Is the code flow we draw is correct? Is there any misunderstanding in our thought ? What is the purpose of registering match this signal ?

Our source revision : fb64f45d3399b182ceadffb8fa86ee68c0aa0a11

Note: We found that this issue didn't happen in CPUSensor and HwmonTempSensor.

Regards,
Max Lai


---------------------------------------------------------------------------------------------------------------------------------------------------------------
This email contains confidential or legally privileged information and is for the sole use of its intended recipient. 
Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
---------------------------------------------------------------------------------------------------------------------------------------------------------------

[-- Attachment #1.2: Type: text/html, Size: 7729 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 34211 bytes --]

[-- Attachment #3: image002.png --]
[-- Type: image/png, Size: 109041 bytes --]

Re: dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)

[James Feist]

On 2/18/20 5:32 PM, Max Lai/WYHQ/Wiwynn wrote:
> *Hi James,*
> 
> *Our validation team met a problem in dbus-sensor recently. The bug is 
> about that if we set the upper non-critical (unc) threshold value 
> smaller than reading value, we should get only one assert log but we got 
> 3 logs ( assert log, de-assert log and then assert log) .*
> 
> *We traced the code of Ipmbsensor. The picture below is the code flow of 
> Ipmbsensor.*
> 
> **
> 
> **
> 
> **
> 
> *We found that when we set the threshold , the function match 
> (configMatch) which is registered in Ipmbsensor would catch the signal 
> like the below*
> 
> **
> 
> **
> 
> **
> 
> *When it trigger the handler (eventhandler) and then enter the function 
> of createsensor() and later enter the function of 
> setInitialProperties(). In the setInitialProperties(), It would initial 
> the property (setting the threshold of alarm to default (false) is the 
> root cause). This would trigger Ipmbsensor to send the deassert log. We 
> was wondering that why setting the threshold would initial the property. 
> Is the code flow we draw is correct? Is there any misunderstanding in 
> our thought ? What is the purpose of registering match this signal ?*

Because configurations can change at runtime. For instance if you change 
a threshold, or you add/remove a card. This gets persisted back to 
entity-manager 
https://github.com/openbmc/dbus-sensors/blob/a97f1343379bbb52371195bc613a689cbfb374f3/src/Thresholds.cpp#L118 
and that will trigger an update.

I'm guessing this todo is what is needed to fix your issue:
https://github.com/openbmc/dbus-sensors/blob/a97f1343379bbb52371195bc613a689cbfb374f3/include/sensor.hpp#L117

I was wondering when I wrote this if the config can just update the 
threshold, and updating it locally was not needed. Deleting lines 117 
and 118 might fix your issue.


> 
> **
> 
> *Our source revision :**fb64f45d3399b182ceadffb8fa86ee68c0aa0a11*
> 
> **
> 
> *Note: We found that this issue didn’t happen in CPUSensor and 
> HwmonTempSensor.*
> 
> **
> 
> *Regards,*
> 
> *Max Lai*
> 
> *---------------------------------------------------------------------------------------------------------------------------------------------------------------*
> 
> *This email contains confidential or legally privileged information and 
> is for the sole use of its intended recipient. *
> 
> *Any unauthorized review, use, copying or distribution of this email or 
> the content of this email is strictly prohibited.*
> 
> *If you are not the intended recipient, you may reply to the sender and 
> should delete this e-mail immediately.*
> 
> *---------------------------------------------------------------------------------------------------------------------------------------------------------------*
> 

RE: dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)

[Max Lai/WYHQ/Wiwynn]

[-- Attachment #1.1: Type: text/plain, Size: 3302 bytes --]

Hi James,



We had tried your fix solution (Deleting lines 117 and 118). Deleting the lines 117 and 118 would stop sending the PropertiesChanged signal and even stop updating threshold value on Dbus. The result we want is we can change threshold value on Dbus and get assert sel log when we trigger the threshold mechanism. And we also tried the latest source revision on upstream dbus-sensor repository. We found that latest source revision in IpmbSensor.cpp, struct sensor's "objectType" member which was set "xyz.openbmc_project.Configuration.ExitAirTemp" was different than our "xyz.openbmc_project.EntityManager". So this issue doesn't happen.

[cid:image001.png@01D5EB48.36B03D30]

What's the purpose of this changing?

Engineer
Storage Firmware
Development Dept.
Firmware Development Div.

Wiwynn Corporation

Tel: +886-2-6614-7549
E-mail: Max_Lai@wiwynn.com<mailto:Max_Lai@wiwynn.com>



-----Original Message-----
From: James Feist <james.feist@linux.intel.com>
Sent: Thursday, February 20, 2020 1:11 AM
To: Max Lai/WYHQ/Wiwynn <Max_Lai@wiwynn.com>
Cc: openbmc@lists.ozlabs.org; Delphine Chiu/WYHQ/Wiwynn <DELPHINE_CHIU@wiwynn.com>; Bonnie Lo/WYHQ/Wiwynn <Bonnie_Lo@wiwynn.com>
Subject: Re: dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)



Because configurations can change at runtime. For instance if you change a threshold, or you add/remove a card. This gets persisted back to entity-manager

https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenbmc%2Fdbus-sensors%2Fblob%2Fa97f1343379bbb52371195bc613a689cbfb374f3%2Fsrc%2FThresholds.cpp%23L118&amp;data=02%7C01%7CMax_Lai%40wiwynn.com%7Ca8d9411b7f9647a0174108d7b55eccb5%7Cde0795e0d7c04eebb9bbbc94d8980d3b%7C1%7C0%7C637177291088060436&amp;sdata=QrbRo%2BqQjKQelorhW4zxp9DUcwsyxBZl5b%2BKXCnTC98%3D&amp;reserved=0

and that will trigger an update.



I'm guessing this todo is what is needed to fix your issue:

https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenbmc%2Fdbus-sensors%2Fblob%2Fa97f1343379bbb52371195bc613a689cbfb374f3%2Finclude%2Fsensor.hpp%23L117&amp;data=02%7C01%7CMax_Lai%40wiwynn.com%7Ca8d9411b7f9647a0174108d7b55eccb5%7Cde0795e0d7c04eebb9bbbc94d8980d3b%7C1%7C0%7C637177291088060436&amp;sdata=dl3PecBITgFAmtRJSNsCitl5e8dkqd%2BVWFSmHysStJM%3D&amp;reserved=0



I was wondering when I wrote this if the config can just update the threshold, and updating it locally was not needed. Deleting lines 117 and 118 might fix your issue.

---------------------------------------------------------------------------------------------------------------------------------------------------------------
This email contains confidential or legally privileged information and is for the sole use of its intended recipient. 
Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
---------------------------------------------------------------------------------------------------------------------------------------------------------------

[-- Attachment #1.2: Type: text/html, Size: 9410 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 30368 bytes --]

Re: dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)

[James Feist]

On 2/24/20 3:31 AM, Max Lai/WYHQ/Wiwynn wrote:
> Hi James,
> 
> We had tried your fix solution (Deleting lines 117 and 118). Deleting 
> the lines 117 and 118 would stop sending the PropertiesChanged signal 
> and even stop updating threshold value on Dbus. The result we want is we 
> can change threshold value on Dbus and get assert sel log when we 
> trigger the threshold mechanism. And we also tried the latest source 
> revision on upstream dbus-sensor repository. We found that latest source 
> revision in IpmbSensor.cpp, struct sensor's "objectType" member which 
> was set "xyz.openbmc_project.Configuration.ExitAirTemp" was different 
> than our "xyz.openbmc_project.EntityManager". So this issue doesn’t happen.
> 
> What's the purpose of this changing?

Can you send a diff? I'm not exactly sure the line you're mentioning.

RE: dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)

[Max Lai/WYHQ/Wiwynn]

[-- Attachment #1.1: Type: text/plain, Size: 3127 bytes --]

Hi James,

Sorry for I offered the wrong information. The last mail this sentence "struct sensor's "objectType" member which was set "xyz.openbmc_project.Configuration.ExitAirTemp" was different than our "xyz.openbmc_project.EntityManager"." is wrong. The correct information is "struct sensor's "objectType" member which was set "xyz.openbmc_project.Configuration.ExitAirTemp" was different than our "xyz.openbmc_project.Configuration.IpmbSensor". The different is between "xyz.openbmc_project.Configuration.ExitAirTemp" and "xyz.openbmc_project.Configuration.IpmbSensor".



Upstream Source Revision : 241356e3dcff3e91393c858256ac29d003e6179e

[cid:image004.png@01D5EBBE.164F0A50][cid:image002.png@01D5EBB7.BF058450]



Our Source Revision : fb64f45d3399b182ceadffb8fa86ee68c0aa0a11

[cid:image004.png@01D5EBBE.164F0A50][cid:image001.png@01D5EBB7.48C83300]


Please let us know if you have any questions.

Thanks for your reply!

Engineer
Storage Firmware
Development Dept.
Firmware Development Div.

Wiwynn Corporation

Tel: +886-2-6614-7549
E-mail: Max_Lai@wiwynn.com<mailto:Max_Lai@wiwynn.com>



-----Original Message-----
From: James Feist <james.feist@linux.intel.com>
Sent: Tuesday, February 25, 2020 4:38 AM
To: Max Lai/WYHQ/Wiwynn <Max_Lai@wiwynn.com>
Cc: openbmc@lists.ozlabs.org; LF_OpenBMC.WYHQ.Wiwynn <LF_OpenBMC.WYHQ.Wiwynn@Wiwynn.com>
Subject: Re: dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)



On 2/24/20 3:31 AM, Max Lai/WYHQ/Wiwynn wrote:

> Hi James,

>

> We had tried your fix solution (Deleting lines 117 and 118). Deleting

> the lines 117 and 118 would stop sending the PropertiesChanged signal

> and even stop updating threshold value on Dbus. The result we want is

> we can change threshold value on Dbus and get assert sel log when we

> trigger the threshold mechanism. And we also tried the latest source

> revision on upstream dbus-sensor repository. We found that latest

> source revision in IpmbSensor.cpp, struct sensor's "objectType" member

> which was set "xyz.openbmc_project.Configuration.ExitAirTemp" was

> different than our "xyz.openbmc_project.EntityManager". So this issue doesn't happen.

>

> What's the purpose of this changing?



Can you send a diff? I'm not exactly sure the line you're mentioning.





---------------------------------------------------------------------------------------------------------------------------------------------------------------
This email contains confidential or legally privileged information and is for the sole use of its intended recipient. 
Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
---------------------------------------------------------------------------------------------------------------------------------------------------------------

[-- Attachment #1.2: Type: text/html, Size: 12451 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 31699 bytes --]

[-- Attachment #3: image002.png --]
[-- Type: image/png, Size: 30368 bytes --]

[-- Attachment #4: image004.png --]
[-- Type: image/png, Size: 255 bytes --]

Re: dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)

[James Feist]

On 2/24/20 5:29 PM, Max Lai/WYHQ/Wiwynn wrote:
> Hi James,
> 
> Sorry for I offered the wrong information. The last mail this sentence 
> "struct sensor's "objectType" member which was set 
> "xyz.openbmc_project.Configuration.ExitAirTemp" was different than our 
> "xyz.openbmc_project.EntityManager"." is *wrong*. The 
> *correct*information is "struct sensor's "objectType" member which was 
> set "xyz.openbmc_project.Configuration.ExitAirTemp" was different than 
> our "*xyz.openbmc_project.Configuration.IpmbSensor*". The different is 
> between "xyz.openbmc_project.Configuration.*ExitAirTemp*" and 
> "xyz.openbmc_project.Configuration.*IpmbSensor*".

That looks like a bug.. I'll look into it. I think it's a copy paste error.

RE: dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)

[Max Lai/WYHQ/Wiwynn]

[-- Attachment #1.1: Type: text/plain, Size: 3318 bytes --]

Hi James,

  We offer a proposal to fix this bug. The bug is about that if we set the upper non-critical (unc) threshold value smaller than reading value, we should get only one assert log but we got 3 logs (assert log, de-assert log and then assert log). We expect the result is that we can change threshold value property on Dbus (xyz.openbmc_project.EntityManager service and xyz.openbmc_project.IpmbSensor service) and get assert/deassert sel log when we trigger/untrigger the threshold mechanism.

  Our proposal is about that modify struct sensor's "objectType" member define from "xyz.openbmc_project.Configuration.ExitAirTemp" to "xyz.openbmc_project.Configuration.IpmbSensor". Add a new member (bool alarmvalue) in struct Threshold in threshold.hpp. Keeping update alarmvalue when process (ipmbsensor) in function of checkThresholds(). When modifying the threshold value and then process going into function of creatsensor() again. Rewrite the sensor value and alarmvalue into global struct sensor after global struct sensor initialization. What do you think about our proposal ? Would you accept our proposal ? Please let us know if you have any questions. Thanks for your reply!



[cid:image003.png@01D5ECCA.3ACBEE90]



Engineer

Storage Firmware

Development Dept.

Firmware Development Div.



Wiwynn Corporation



Tel: +886-2-6614-7549

E-mail: Max_Lai@wiwynn.com





-----Original Message-----

From: James Feist <james.feist@linux.intel.com>

Sent: Wednesday, February 26, 2020 1:33 AM

To: Max Lai/WYHQ/Wiwynn <Max_Lai@wiwynn.com>

Cc: openbmc@lists.ozlabs.org; LF_OpenBMC.WYHQ.Wiwynn <LF_OpenBMC.WYHQ.Wiwynn@Wiwynn.com>

Subject: Re: dbus-sensor: setting the upper non-critical(unc) threshold value smaller than reading value would get 3 logs ( assert log, de-assert log and then assert log)



On 2/24/20 5:29 PM, Max Lai/WYHQ/Wiwynn wrote:

> Hi James,

>

> Sorry for I offered the wrong information. The last mail this sentence

> "struct sensor's "objectType" member which was set

> "xyz.openbmc_project.Configuration.ExitAirTemp" was different than our

> "xyz.openbmc_project.EntityManager"." is *wrong*. The

> *correct*information is "struct sensor's "objectType" member which was

> set "xyz.openbmc_project.Configuration.ExitAirTemp" was different than

> our "*xyz.openbmc_project.Configuration.IpmbSensor*". The different is

> between "xyz.openbmc_project.Configuration.*ExitAirTemp*" and

> "xyz.openbmc_project.Configuration.*IpmbSensor*".



That looks like a bug.. I'll look into it. I think it's a copy paste error.





---------------------------------------------------------------------------------------------------------------------------------------------------------------
This email contains confidential or legally privileged information and is for the sole use of its intended recipient. 
Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
---------------------------------------------------------------------------------------------------------------------------------------------------------------

[-- Attachment #1.2: Type: text/html, Size: 8976 bytes --]

[-- Attachment #2: image003.png --]
[-- Type: image/png, Size: 71864 bytes --]