* Fan PWM settings via Redfish
@ 2021-03-12 6:37 Bruce Lee (李昀峻)
2021-03-12 17:40 ` Ed Tanous
0 siblings, 1 reply; 11+ messages in thread
From: Bruce Lee (李昀峻) @ 2021-03-12 6:37 UTC (permalink / raw)
To: Nan Zhou, edtanous, rhanley; +Cc: openbmc
[-- Attachment #1: Type: text/plain, Size: 5760 bytes --]
Hi All,
We are designing and implementing the Fan PWM settings via Redfish. The goal is that clients can set sensor value to bmc via Redfish.
We divide the work into three phases.
Phase 1 is to remove the definition “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was added by Intel group, please refer to https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/30000,
The Intel solution has 4 conditions needs to match one of them and that can be work to override sensor but actually not all project needs those conditions, so we want to propose to remove the insecure definition and use new definition to include the intel solution and execute when compile. It would be no compile time with option for common project. And the insecure issue we will discuss in phase 2.
Example below:
-----------------------------------------------------------------------------------------------------
[Before modified]
#ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
// Proceed with sensor override
setSensorsOverride(sensorAsyncResp, allCollections);
return;
#endif
doIntelSpecialModeManager code …
-----------------------------------------------------------------------------------------------------
[After modified]
#ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
doIntelSpecialModeManager code …
return;
#endif
//Proceed with sensor override
setSensorsOverride(sensorAsyncResp, allCollections);
-----------------------------------------------------------------------------------------------------
Phase 2 is to add a condition to check the sensor name’s Mutable value of EM if the value is true do the sensor override function else not do.
The Mutable value can be set in the sensor configuration of Entity-Manage, when using the patch command to override the sensor, it needs to check the EntityManager subtree’s sensor name and its interface “xyz.openbmc_project.Configuration.I2CFan.Connector” to check the corresponding property name’s mutable value to decide whether executing the override function.
This achieves feature parity with the ipmi::sensor::Mutability parameter of the old hardcoded YAML configuration files
Execute steps:
1. Patch command to override sensor.
2. Check the EM of sensor’s Mutable value
3. If Mutable value is true do sensor override action else not do.
Phase 3 is to add a new get command to get the Zone_$id’s "Manual" value and patch command to change the fan mode from auto to manual mode ("Manual":true).
Because the fan control is use package phosphor-pid-control, when we need to set fan pwm, it needs to set the fan mode from auto mode to manual mode, for now, the phosphor-pid-control has already provided ipmi-oem command to achieve this feature, so we need to implement this fan mode change via redfish command.
Example URLs |Method |Example Payload
--------------------------------------- |-------------- |--
/redfish/v1/Managers/bmc |GET |"Oem": {
| | Fan": {
| | "FanZones": {
| | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
| | "@odata.type": "#OemManager.FanZones",
| | "Zone_0": {
| | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
| | "@odata.type": "#OemManager.FanZone",
| | "Chassis": {
| | "@odata.id": "/redfish/v1/Chassis/GSZ_EVT"
| | },
| | "FailSafePercent": 100.0,
| | "MinThermalOutput": 0.0,
| | "ZoneIndex": 0.0,
| | "Manual":false
| | },
| | },
| | },
| |}
--------------------------------------- |-------------- |----
/redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan": { "FanZones": { "Zone_0": { "Manual":true } } }
If any thoughts on this topic, feel free to give your comments. Thanks!
Sincerely,
Bruce
[-- Attachment #2: Type: text/html, Size: 34641 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Fan PWM settings via Redfish
2021-03-12 6:37 Fan PWM settings via Redfish Bruce Lee (李昀峻)
@ 2021-03-12 17:40 ` Ed Tanous
2021-03-16 8:34 ` Bruce Lee (李昀峻)
2021-03-16 9:34 ` Bruce Lee (李昀峻)
0 siblings, 2 replies; 11+ messages in thread
From: Ed Tanous @ 2021-03-12 17:40 UTC (permalink / raw)
To: Bruce Lee (李昀峻); +Cc: openbmc, Nan Zhou, rhanley
On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com> wrote:
>
> Hi All,
>
>
>
> We are designing and implementing the Fan PWM settings via Redfish. The goal is that clients can set sensor value to bmc via Redfish.
>
>
>
> We divide the work into three phases.
>
>
>
> Phase 1 is to remove the definition “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
>
> The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was added by Intel group, please refer to https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/30000,
>
> The Intel solution has 4 conditions needs to match one of them and that can be work to override sensor but actually not all project needs those conditions, so we want to propose to remove the insecure definition and use new definition to include the intel solution and execute when compile. It would be no compile time with option for common project. And the insecure issue we will discuss in phase 2.
>
>
>
> Example below:
>
> -----------------------------------------------------------------------------------------------------
>
> [Before modified]
>
> #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
>
> // Proceed with sensor override
>
> setSensorsOverride(sensorAsyncResp, allCollections);
>
> return;
>
> #endif
>
> doIntelSpecialModeManager code …
>
> -----------------------------------------------------------------------------------------------------
>
> [After modified]
>
> #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
>
> doIntelSpecialModeManager code …
>
> return;
>
> #endif
>
> //Proceed with sensor override
>
> setSensorsOverride(sensorAsyncResp, allCollections);
>
> -----------------------------------------------------------------------------------------------------
>
>
>
>
I suspect this check and option needs to be moved into the individual
sensors, so that we can differentiate between "should be settable in a
test context" and "should be settable in a normal context".
>
> Phase 2 is to add a condition to check the sensor name’s Mutable value of EM if the value is true do the sensor override function else not do.
I suspect this patchset needs to be moved forward if you're hoping to
use the mutable param:
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-dbus-interfaces/+/36333
>
> The Mutable value can be set in the sensor configuration of Entity-Manage, when using the patch command to override the sensor, it needs to check the EntityManager subtree’s sensor name and its interface “xyz.openbmc_project.Configuration.I2CFan.Connector” to check the corresponding property name’s mutable value to decide whether executing the override function.
See above. I suspect that the redfish code doesn't need to check the
mutability of the sensor, the interface should just have the correct
behavior. The only place I would expect to need to know the
mutability of a sensor is in the IPMI sdr, where we will need to set
the modifiable bit appropriately.
>
> This achieves feature parity with the ipmi::sensor::Mutability parameter of the old hardcoded YAML configuration files
Not sure what you're referring to. That may have been something done in a fork.
>
>
>
> Execute steps:
>
> 1. Patch command to override sensor.
>
> 2. Check the EM of sensor’s Mutable value
>
> 3. If Mutable value is true do sensor override action else not do.
>
>
>
>
>
> Phase 3 is to add a new get command to get the Zone_$id’s "Manual" value and patch command to change the fan mode from auto to manual mode ("Manual":true).
>
> Because the fan control is use package phosphor-pid-control, when we need to set fan pwm, it needs to set the fan mode from auto mode to manual mode, for now, the phosphor-pid-control has already provided ipmi-oem command to achieve this feature, so we need to implement this fan mode change via redfish command.
Doesn't this already work today? I thought we had all that sorted a
long time ago. For some reason I thought we intentionally didn't
expose the manual/automatic param, because that only applied to the
PID loops, and PWM sensor didn't expose that interface. I need to go
look at the code at some point.
>
>
>
> Example URLs |Method |Example Payload
>
> --------------------------------------- |-------------- |--
>
> /redfish/v1/Managers/bmc |GET |"Oem": {
>
> | | Fan": {
>
> | | "FanZones": {
>
> | | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
>
> | | "@odata.type": "#OemManager.FanZones",
>
> | | "Zone_0": {
>
> | | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
>
> | | "@odata.type": "#OemManager.FanZone",
>
> | | "Chassis": {
>
> | | "@odata.id": "/redfish/v1/Chassis/GSZ_EVT"
>
> | | },
>
> | | "FailSafePercent": 100.0,
>
> | | "MinThermalOutput": 0.0,
>
> | | "ZoneIndex": 0.0,
>
> | | "Manual":false
>
> | | },
>
> | | },
>
> | | },
>
> | |}
>
> --------------------------------------- |-------------- |----
>
> /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan": { "FanZones": { "Zone_0": { "Manual":true } } }
>
>
It should be noted, this schema needs some serious cleanup to make it
proper resources, paths, and collections, and should version the
schema files appropriately. If you're planning on extending it, I
would expect _some_ effort to be put into cleanup. There's several
github bugs that have more details, and I will leave it up to you to
decide how much you'd like to do as part of this work, but please plan
on some.
>
> If any thoughts on this topic, feel free to give your comments. Thanks!
>
>
>
> Sincerely,
>
> Bruce
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: Fan PWM settings via Redfish
2021-03-12 17:40 ` Ed Tanous
@ 2021-03-16 8:34 ` Bruce Lee (李昀峻)
2021-03-16 9:34 ` Bruce Lee (李昀峻)
1 sibling, 0 replies; 11+ messages in thread
From: Bruce Lee (李昀峻) @ 2021-03-16 8:34 UTC (permalink / raw)
To: Ed Tanous; +Cc: openbmc, Nan Zhou, rhanley
[-- Attachment #1: Type: text/plain, Size: 11045 bytes --]
>I suspect this check and option needs to be moved into the individual sensors, so that we can differentiate between "should be settable in a test context" and "should be settable in a normal context".
1. Does you mean don't change the Intel definition and keep the origin code when compile time?
2. What do you mean this option needs to be moved into the individual sensors so that we can differentiate between "should be settable in a test context" and "should be settable in a normal context".
Please provide more details about your thinking.
>See above. I suspect that the redfish code doesn't need to check the mutability of the sensor, the interface should just have the correct behavior. The only place I would expect to need to know the >mutability of a sensor is in the IPMI sdr, where we will need to set the modifiable bit appropriately.
For now, the function to set sensor in redfish code is to set the d-bus value directly (internally writable), if we don't check the EM mutability in Redfish, follow the Add Mutable property to Sensor Value interface, we still need to check the sensor mutable property to know whether or not to write the d-bus value in redfish or we need other external services to know whether or not to grant write permission to their users like IPMI sensor.
>Doesn't this already work today? I thought we had all that sorted a long time ago. For some reason I thought we intentionally didn't expose the manual/automatic param, because that only applied to >the PID loops, and PWM sensor didn't expose that interface. I need to go look at the code at some point.
Yes, ipmi-oem is work today. I agree it is not properly to show on redfish to let users can easily change the fan mode, the reason to change fan mode to the manual is for debugging. Maybe let users use ipmi-oem to replace show on Redfish URLs.
-----Original Message-----
From: Ed Tanous <edtanous@google.com>
Sent: Saturday, March 13, 2021 1:40 AM
To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com; openbmc@lists.ozlabs.org
Subject: Re: Fan PWM settings via Redfish
On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com<mailto:Bruce_Lee@quantatw.com>> wrote:
>
> Hi All,
>
>
>
> We are designing and implementing the Fan PWM settings via Redfish. The goal is that clients can set sensor value to bmc via Redfish.
>
>
>
> We divide the work into three phases.
>
>
>
> Phase 1 is to remove the definition “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
>
> The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was added by Intel
> group, please refer to
> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgerr
> it.openbmc-project.xyz%2Fc%2Fopenbmc%2Fbmcweb%2F%2B%2F30000&data=0
> 4%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e57df35c%7
> C179b032707fc4973ac738de7313561b2%7C1%7C0%7C637511676404227113%7CUnkno
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C1000&sdata=f604Piz1vDfItDZ3docZOPfryJesavkbOwhKywJ
> oXlU%3D&reserved=0,
>
> The Intel solution has 4 conditions needs to match one of them and that can be work to override sensor but actually not all project needs those conditions, so we want to propose to remove the insecure definition and use new definition to include the intel solution and execute when compile. It would be no compile time with option for common project. And the insecure issue we will discuss in phase 2.
>
>
>
> Example below:
>
> ----------------------------------------------------------------------
> -------------------------------
>
> [Before modified]
>
> #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
>
> // Proceed with sensor override
>
> setSensorsOverride(sensorAsyncResp, allCollections);
>
> return;
>
> #endif
>
> doIntelSpecialModeManager code …
>
> ----------------------------------------------------------------------
> -------------------------------
>
> [After modified]
>
> #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
>
> doIntelSpecialModeManager code …
>
> return;
>
> #endif
>
> //Proceed with sensor override
>
> setSensorsOverride(sensorAsyncResp, allCollections);
>
> ----------------------------------------------------------------------
> -------------------------------
>
>
>
>
I suspect this check and option needs to be moved into the individual sensors, so that we can differentiate between "should be settable in a test context" and "should be settable in a normal context".
>
> Phase 2 is to add a condition to check the sensor name’s Mutable value of EM if the value is true do the sensor override function else not do.
I suspect this patchset needs to be moved forward if you're hoping to use the mutable param:
https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgerrit.openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F%2B%2F36333&data=04%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e57df35c%7C179b032707fc4973ac738de7313561b2%7C1%7C0%7C637511676404227113%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tdExxB%2BY7O1cKb%2FYMdvPGnw7YThW7J55jytDPh4YWYo%3D&reserved=0
>
> The Mutable value can be set in the sensor configuration of Entity-Manage, when using the patch command to override the sensor, it needs to check the EntityManager subtree’s sensor name and its interface “xyz.openbmc_project.Configuration.I2CFan.Connector” to check the corresponding property name’s mutable value to decide whether executing the override function.
See above. I suspect that the redfish code doesn't need to check the mutability of the sensor, the interface should just have the correct behavior. The only place I would expect to need to know the mutability of a sensor is in the IPMI sdr, where we will need to set the modifiable bit appropriately.
>
> This achieves feature parity with the ipmi::sensor::Mutability
> parameter of the old hardcoded YAML configuration files
Not sure what you're referring to. That may have been something done in a fork.
>
>
>
> Execute steps:
>
> 1. Patch command to override sensor.
>
> 2. Check the EM of sensor’s Mutable value
>
> 3. If Mutable value is true do sensor override action else not do.
>
>
>
>
>
> Phase 3 is to add a new get command to get the Zone_$id’s "Manual" value and patch command to change the fan mode from auto to manual mode ("Manual":true).
>
> Because the fan control is use package phosphor-pid-control, when we need to set fan pwm, it needs to set the fan mode from auto mode to manual mode, for now, the phosphor-pid-control has already provided ipmi-oem command to achieve this feature, so we need to implement this fan mode change via redfish command.
Doesn't this already work today? I thought we had all that sorted a long time ago. For some reason I thought we intentionally didn't expose the manual/automatic param, because that only applied to the PID loops, and PWM sensor didn't expose that interface. I need to go look at the code at some point.
>
>
>
> Example URLs |Method |Example Payload
>
> --------------------------------------- |-------------- |--
>
> /redfish/v1/Managers/bmc |GET |"Oem": {
>
> | | Fan": {
>
> | | "FanZones": {
>
> | | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
>
> | | "@odata.type": "#OemManager.FanZones",
>
> | | "Zone_0": {
>
> | | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
>
> | | "@odata.type": "#OemManager.FanZone",
>
> | | "Chassis": {
>
> | | "@odata.id": "/redfish/v1/Chassis/GSZ_EVT"
>
> | | },
>
> | | "FailSafePercent": 100.0,
>
> | | "MinThermalOutput": 0.0,
>
> | | "ZoneIndex": 0.0,
>
> | | "Manual":false
>
> | | },
>
> | | },
>
> | | },
>
> | |}
>
> --------------------------------------- |-------------- |----
>
> /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan": { "FanZones": { "Zone_0": { "Manual":true } } }
>
>
It should be noted, this schema needs some serious cleanup to make it proper resources, paths, and collections, and should version the schema files appropriately. If you're planning on extending it, I would expect _some_ effort to be put into cleanup. There's several github bugs that have more details, and I will leave it up to you to decide how much you'd like to do as part of this work, but please plan on some.
>
> If any thoughts on this topic, feel free to give your comments. Thanks!
>
>
>
> Sincerely,
>
> Bruce
[-- Attachment #2: Type: text/html, Size: 38546 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: Fan PWM settings via Redfish
2021-03-12 17:40 ` Ed Tanous
2021-03-16 8:34 ` Bruce Lee (李昀峻)
@ 2021-03-16 9:34 ` Bruce Lee (李昀峻)
2021-03-16 15:18 ` Ed Tanous
1 sibling, 1 reply; 11+ messages in thread
From: Bruce Lee (李昀峻) @ 2021-03-16 9:34 UTC (permalink / raw)
To: Ed Tanous; +Cc: openbmc, Nan Zhou, rhanley
-----Original Message-----
From: Ed Tanous <edtanous@google.com>
Sent: Saturday, March 13, 2021 1:40 AM
To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com; openbmc@lists.ozlabs.org
Subject: Re: Fan PWM settings via Redfish
On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com> wrote:
>
> Hi All,
>
>
>
> We are designing and implementing the Fan PWM settings via Redfish. The goal is that clients can set sensor value to bmc via Redfish.
>
>
>
> We divide the work into three phases.
>
>
>
> Phase 1 is to remove the definition “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
>
> The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was added by Intel
> group, please refer to
> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgerr
> it.openbmc-project.xyz%2Fc%2Fopenbmc%2Fbmcweb%2F%2B%2F30000&data=0
> 4%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e57df35c%7
> C179b032707fc4973ac738de7313561b2%7C1%7C0%7C637511676404227113%7CUnkno
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C1000&sdata=f604Piz1vDfItDZ3docZOPfryJesavkbOwhKywJ
> oXlU%3D&reserved=0,
>
> The Intel solution has 4 conditions needs to match one of them and that can be work to override sensor but actually not all project needs those conditions, so we want to propose to remove the insecure definition and use new definition to include the intel solution and execute when compile. It would be no compile time with option for common project. And the insecure issue we will discuss in phase 2.
>
>
>
> Example below:
>
> ----------------------------------------------------------------------
> -------------------------------
>
> [Before modified]
>
> #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
>
> // Proceed with sensor override
>
> setSensorsOverride(sensorAsyncResp, allCollections);
>
> return;
>
> #endif
>
> doIntelSpecialModeManager code …
>
> ----------------------------------------------------------------------
> -------------------------------
>
> [After modified]
>
> #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
>
> doIntelSpecialModeManager code …
>
> return;
>
> #endif
>
> //Proceed with sensor override
>
> setSensorsOverride(sensorAsyncResp, allCollections);
>
> ----------------------------------------------------------------------
> -------------------------------
>
>
>
>
>I suspect this check and option needs to be moved into the individual sensors, so that we can differentiate between "should be settable in a test context" and "should be settable in a normal context".
1. Does you mean don't change the Intel definition and keep the origin code when compile time?
2. What do you mean this option needs to be moved into the individual sensors so that we can differentiate between "should be settable in a test context" and "should be settable in a normal context".
Please provide more details about your thinking.
>
> Phase 2 is to add a condition to check the sensor name’s Mutable value of EM if the value is true do the sensor override function else not do.
>I suspect this patchset needs to be moved forward if you're hoping to use the mutable param:
>https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgerrit.openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F%2B%2F36333&data=04%7C01%>7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e57df35c%7C179b032707fc4973ac738de7313561b2%7C1%7C0%7C637511676404227113%7CUnknown%>7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tdExxB%2BY7O1cKb%2FYMdvPGnw7YThW7J55jytDPh4YWYo%3D&reserved=0
>
> The Mutable value can be set in the sensor configuration of Entity-Manage, when using the patch command to override the sensor, it needs to check the EntityManager subtree’s sensor name and its interface “xyz.openbmc_project.Configuration.I2CFan.Connector” to check the corresponding property name’s mutable value to decide whether executing the override function.
>See above. I suspect that the redfish code doesn't need to check the mutability of the sensor, the interface should just have the correct behavior. The only place I would expect to need to know the >mutability of a sensor is in the IPMI sdr, where we will need to set the modifiable bit appropriately.
For now, the function to set sensor in redfish code is to set the d-bus value directly (internally writable), if we don't check the EM mutability in Redfish, follow the Add Mutable property to Sensor Value interface, we still need to check the sensor mutable property to know whether or not to write the d-bus value in redfish or we need other external services to know whether or not to grant write permission to their users like IPMI sensor.
>
> This achieves feature parity with the ipmi::sensor::Mutability
> parameter of the old hardcoded YAML configuration files
>Not sure what you're referring to. That may have been something done in a fork.
>
>
>
> Execute steps:
>
> 1. Patch command to override sensor.
>
> 2. Check the EM of sensor’s Mutable value
>
> 3. If Mutable value is true do sensor override action else not do.
>
>
>
>
>
> Phase 3 is to add a new get command to get the Zone_$id’s "Manual" value and patch command to change the fan mode from auto to manual mode ("Manual":true).
>
> Because the fan control is use package phosphor-pid-control, when we need to set fan pwm, it needs to set the fan mode from auto mode to manual mode, for now, the phosphor-pid-control has already provided ipmi-oem command to achieve this feature, so we need to implement this fan mode change via redfish command.
>Doesn't this already work today? I thought we had all that sorted a long time ago. For some reason I thought we intentionally didn't expose the manual/automatic param, because that only applied to >the PID loops, and PWM sensor didn't expose that interface. I need to go look at the code at some point.
Yes, ipmi-oem is work today. I agree it is not properly to show on redfish to let users can easily change the fan mode, the reason to change fan mode to the manual is for debugging. Maybe let users use ipmi-oem to replace show on Redfish URLs.
>
>
>
> Example URLs |Method |Example Payload
>
> --------------------------------------- |-------------- |--
>
> /redfish/v1/Managers/bmc |GET |"Oem": {
>
> | | Fan": {
>
> | | "FanZones": {
>
> | | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
>
> | | "@odata.type": "#OemManager.FanZones",
>
> | | "Zone_0": {
>
> | | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
>
> | | "@odata.type": "#OemManager.FanZone",
>
> | | "Chassis": {
>
> | | "@odata.id": "/redfish/v1/Chassis/GSZ_EVT"
>
> | | },
>
> | | "FailSafePercent": 100.0,
>
> | | "MinThermalOutput": 0.0,
>
> | | "ZoneIndex": 0.0,
>
> | | "Manual":false
>
> | | },
>
> | | },
>
> | | },
>
> | |}
>
> --------------------------------------- |-------------- |----
>
> /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan": { "FanZones": { "Zone_0": { "Manual":true } } }
>
>
>It should be noted, this schema needs some serious cleanup to make it proper resources, paths, and collections, and should version the schema files appropriately. If you're planning on extending it, I ?>would expect _some_ effort to be put into cleanup. There's several github bugs that have more details, and I will leave it up to you to decide how much you'd like to do as part of this work, but please >plan on some.
>
> If any thoughts on this topic, feel free to give your comments. Thanks!
>
>
>
> Sincerely,
>
> Bruce
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Fan PWM settings via Redfish
2021-03-16 9:34 ` Bruce Lee (李昀峻)
@ 2021-03-16 15:18 ` Ed Tanous
2021-03-17 10:17 ` Bruce Lee (李昀峻)
0 siblings, 1 reply; 11+ messages in thread
From: Ed Tanous @ 2021-03-16 15:18 UTC (permalink / raw)
To: Bruce Lee (李昀峻); +Cc: openbmc, Nan Zhou, rhanley
On Tue, Mar 16, 2021 at 2:35 AM Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com> wrote:
>
>
>
> -----Original Message-----
> From: Ed Tanous <edtanous@google.com>
> Sent: Saturday, March 13, 2021 1:40 AM
> To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com; openbmc@lists.ozlabs.org
> Subject: Re: Fan PWM settings via Redfish
>
> On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com> wrote:
> >
> > Hi All,
> >
> >
> >
> > We are designing and implementing the Fan PWM settings via Redfish. The goal is that clients can set sensor value to bmc via Redfish.
> >
> >
> >
> > We divide the work into three phases.
> >
> >
> >
> > Phase 1 is to remove the definition “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
> >
> > The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was added by Intel
> > group, please refer to
> > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgerr
> > it.openbmc-project.xyz%2Fc%2Fopenbmc%2Fbmcweb%2F%2B%2F30000&data=0
> > 4%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e57df35c%7
> > C179b032707fc4973ac738de7313561b2%7C1%7C0%7C637511676404227113%7CUnkno
> > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> > CJXVCI6Mn0%3D%7C1000&sdata=f604Piz1vDfItDZ3docZOPfryJesavkbOwhKywJ
> > oXlU%3D&reserved=0,
> >
> > The Intel solution has 4 conditions needs to match one of them and that can be work to override sensor but actually not all project needs those conditions, so we want to propose to remove the insecure definition and use new definition to include the intel solution and execute when compile. It would be no compile time with option for common project. And the insecure issue we will discuss in phase 2.
> >
> >
> >
> > Example below:
> >
> > ----------------------------------------------------------------------
> > -------------------------------
> >
> > [Before modified]
> >
> > #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
> >
> > // Proceed with sensor override
> >
> > setSensorsOverride(sensorAsyncResp, allCollections);
> >
> > return;
> >
> > #endif
> >
> > doIntelSpecialModeManager code …
> >
> > ----------------------------------------------------------------------
> > -------------------------------
> >
> > [After modified]
> >
> > #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
> >
> > doIntelSpecialModeManager code …
> >
> > return;
> >
> > #endif
> >
> > //Proceed with sensor override
> >
> > setSensorsOverride(sensorAsyncResp, allCollections);
> >
> > ----------------------------------------------------------------------
> > -------------------------------
> >
> >
> >
> >
>
> >I suspect this check and option needs to be moved into the individual sensors, so that we can differentiate between "should be settable in a test context" and "should be settable in a normal context".
> 1. Does you mean don't change the Intel definition and keep the origin code when compile time?
No, this means that the checking code needs to move from redfish into
dbus-sensors.
> 2. What do you mean this option needs to be moved into the individual sensors so that we can differentiate between "should be settable in a test context" and "should be settable in a normal context".
> Please provide more details about your thinking.
Individual sensors need to provide an appropriate dbus interface.
Part of that is enforcing whether or not they're writable, and
checking for the debug state of the system to do so.
>
>
> >
> > Phase 2 is to add a condition to check the sensor name’s Mutable value of EM if the value is true do the sensor override function else not do.
>
> >I suspect this patchset needs to be moved forward if you're hoping to use the mutable param:
> >https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgerrit.openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F%2B%2F36333&data=04%7C01%>7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e57df35c%7C179b032707fc4973ac738de7313561b2%7C1%7C0%7C637511676404227113%7CUnknown%>7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tdExxB%2BY7O1cKb%2FYMdvPGnw7YThW7J55jytDPh4YWYo%3D&reserved=0
Not quite, but close. I wouldn't expect the configurability to be
directly configurable. External sensor types should be mutable, all
other types should not be mutable (except in a debug context). I
don't think there's any reason to add a separate "IsMutable" parameter
into the EM json, unless it really needs to be configurable per
sensor, which I don't think is the case.
>
> >
> > The Mutable value can be set in the sensor configuration of Entity-Manage, when using the patch command to override the sensor, it needs to check the EntityManager subtree’s sensor name and its interface “xyz.openbmc_project.Configuration.I2CFan.Connector” to check the corresponding property name’s mutable value to decide whether executing the override function.
>
> >See above. I suspect that the redfish code doesn't need to check the mutability of the sensor, the interface should just have the correct behavior. The only place I would expect to need to know the >mutability of a sensor is in the IPMI sdr, where we will need to set the modifiable bit appropriately.
>
> For now, the function to set sensor in redfish code is to set the d-bus value directly (internally writable), if we don't check the EM mutability in Redfish, follow the Add Mutable property to Sensor Value interface, we still need to check the sensor mutable property to know whether or not to write the d-bus value in redfish or we need other external services to know whether or not to grant write permission to their users like IPMI sensor.
I'm not really following this. My point is that the only thing that
really needs to "check" the mutability requirement is dbus-sensors.
They should only allow setting when sensors are mutable, and reject
when they're not.
>
> >
> > This achieves feature parity with the ipmi::sensor::Mutability
> > parameter of the old hardcoded YAML configuration files
>
> >Not sure what you're referring to. That may have been something done in a fork.
>
> >
> >
> >
> > Execute steps:
> >
> > 1. Patch command to override sensor.
> >
> > 2. Check the EM of sensor’s Mutable value
> >
> > 3. If Mutable value is true do sensor override action else not do.
> >
> >
> >
> >
> >
> > Phase 3 is to add a new get command to get the Zone_$id’s "Manual" value and patch command to change the fan mode from auto to manual mode ("Manual":true).
> >
> > Because the fan control is use package phosphor-pid-control, when we need to set fan pwm, it needs to set the fan mode from auto mode to manual mode, for now, the phosphor-pid-control has already provided ipmi-oem command to achieve this feature, so we need to implement this fan mode change via redfish command.
>
> >Doesn't this already work today? I thought we had all that sorted a long time ago. For some reason I thought we intentionally didn't expose the manual/automatic param, because that only applied to >the PID loops, and PWM sensor didn't expose that interface. I need to go look at the code at some point.
>
> Yes, ipmi-oem is work today. I agree it is not properly to show on redfish to let users can easily change the fan mode, the reason to change fan mode to the manual is for debugging. Maybe let users use ipmi-oem to replace show on Redfish URLs.
>
> >
> >
> >
> > Example URLs |Method |Example Payload
> >
> > --------------------------------------- |-------------- |--
> >
> > /redfish/v1/Managers/bmc |GET |"Oem": {
> >
> > | | Fan": {
> >
> > | | "FanZones": {
> >
> > | | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
> >
> > | | "@odata.type": "#OemManager.FanZones",
> >
> > | | "Zone_0": {
> >
> > | | "@odata.id": "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
> >
> > | | "@odata.type": "#OemManager.FanZone",
> >
> > | | "Chassis": {
> >
> > | | "@odata.id": "/redfish/v1/Chassis/GSZ_EVT"
> >
> > | | },
> >
> > | | "FailSafePercent": 100.0,
> >
> > | | "MinThermalOutput": 0.0,
> >
> > | | "ZoneIndex": 0.0,
> >
> > | | "Manual":false
> >
> > | | },
> >
> > | | },
> >
> > | | },
> >
> > | |}
> >
> > --------------------------------------- |-------------- |----
> >
> > /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan": { "FanZones": { "Zone_0": { "Manual":true } } }
> >
> >
>
> >It should be noted, this schema needs some serious cleanup to make it proper resources, paths, and collections, and should version the schema files appropriately. If you're planning on extending it, I ?>would expect _some_ effort to be put into cleanup. There's several github bugs that have more details, and I will leave it up to you to decide how much you'd like to do as part of this work, but please >plan on some.
>
> >
> > If any thoughts on this topic, feel free to give your comments. Thanks!
> >
> >
> >
> > Sincerely,
> >
> > Bruce
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: Fan PWM settings via Redfish
2021-03-16 15:18 ` Ed Tanous
@ 2021-03-17 10:17 ` Bruce Lee (李昀峻)
2021-03-17 15:52 ` Ed Tanous
0 siblings, 1 reply; 11+ messages in thread
From: Bruce Lee (李昀峻) @ 2021-03-17 10:17 UTC (permalink / raw)
To: Ed Tanous; +Cc: openbmc, Nan Zhou, rhanley
> -----Original Message-----
> From: Ed Tanous <edtanous@google.com>
> Sent: Tuesday, March 16, 2021 11:18 PM
> To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> openbmc@lists.ozlabs.org
> Subject: Re: Fan PWM settings via Redfish
>
> On Tue, Mar 16, 2021 at 2:35 AM Bruce Lee (李昀峻)
> <Bruce_Lee@quantatw.com> wrote:
> >
> >
> >
> > -----Original Message-----
> > From: Ed Tanous <edtanous@google.com>
> > Sent: Saturday, March 13, 2021 1:40 AM
> > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > openbmc@lists.ozlabs.org
> > Subject: Re: Fan PWM settings via Redfish
> >
> > On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻)
> <Bruce_Lee@quantatw.com> wrote:
> > >
> > > Hi All,
> > >
> > >
> > >
> > > We are designing and implementing the Fan PWM settings via Redfish. The
> goal is that clients can set sensor value to bmc via Redfish.
> > >
> > >
> > >
> > > We divide the work into three phases.
> > >
> > >
> > >
> > > Phase 1 is to remove the definition
> “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new
> definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
> > >
> > > The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was added
> by
> > > Intel group, please refer to
> > > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fge
> > > rr
> > >
> it.openbmc-project.xyz%2Fc%2Fopenbmc%2Fbmcweb%2F%2B%2F30000&am
> p;data
> > > =0
> > >
> 4%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e5
> 7df35c
> > > %7
> > >
> C179b032707fc4973ac738de7313561b2%7C1%7C0%7C63751167640422711
> 3%7CUnk
> > > no
> > >
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> Ww
> > > iL
> > >
> CJXVCI6Mn0%3D%7C1000&sdata=f604Piz1vDfItDZ3docZOPfryJesavkbOw
> hKy
> > > wJ
> > > oXlU%3D&reserved=0,
> > >
> > > The Intel solution has 4 conditions needs to match one of them and that can
> be work to override sensor but actually not all project needs those conditions, so
> we want to propose to remove the insecure definition and use new definition to
> include the intel solution and execute when compile. It would be no compile time
> with option for common project. And the insecure issue we will discuss in phase
> 2.
> > >
> > >
> > >
> > > Example below:
> > >
> > > --------------------------------------------------------------------
> > > --
> > > -------------------------------
> > >
> > > [Before modified]
> > >
> > > #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
> > >
> > > // Proceed with sensor override
> > >
> > > setSensorsOverride(sensorAsyncResp, allCollections);
> > >
> > > return;
> > >
> > > #endif
> > >
> > > doIntelSpecialModeManager code …
> > >
> > > --------------------------------------------------------------------
> > > --
> > > -------------------------------
> > >
> > > [After modified]
> > >
> > > #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
> > >
> > > doIntelSpecialModeManager code …
> > >
> > > return;
> > >
> > > #endif
> > >
> > > //Proceed with sensor override
> > >
> > > setSensorsOverride(sensorAsyncResp, allCollections);
> > >
> > > --------------------------------------------------------------------
> > > --
> > > -------------------------------
> > >
> > >
> > >
> > >
> >
> > >I suspect this check and option needs to be moved into the individual sensors,
> so that we can differentiate between "should be settable in a test context" and
> "should be settable in a normal context".
> > 1. Does you mean don't change the Intel definition and keep the origin code
> when compile time?
>
> No, this means that the checking code needs to move from redfish into
> dbus-sensors.
>
> > 2. What do you mean this option needs to be moved into the individual sensors
> so that we can differentiate between "should be settable in a test context" and
> "should be settable in a normal context".
> > Please provide more details about your thinking.
>
> Individual sensors need to provide an appropriate dbus interface.
> Part of that is enforcing whether or not they're writable, and checking for the
> debug state of the system to do so.
>
> >
> >
> > >
> > > Phase 2 is to add a condition to check the sensor name’s Mutable value of
> EM if the value is true do the sensor override function else not do.
> >
> > >I suspect this patchset needs to be moved forward if you're hoping to use the
> mutable param:
> > >https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fger
> >
> >rit.openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F%
> 2
> >
> >B%2F36333&data=04%7C01%25>7CBruce_Lee%40quantatw.com%7C64
> a1153cd4
> >
> >5b46eeca4008d8e57df35c%7C179b032707fc4973ac738de7313561b2%7C1
> %7C0%7C6
> >
> >37511676404227113%7CUnknown%>7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIj
> >
> >oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tdExxB%
> 2BY7
> > >O1cKb%2FYMdvPGnw7YThW7J55jytDPh4YWYo%3D&reserved=0
>
> Not quite, but close. I wouldn't expect the configurability to be directly
> configurable. External sensor types should be mutable, all other types should
> not be mutable (except in a debug context). I don't think there's any reason to
> add a separate "IsMutable" parameter into the EM json, unless it really needs to
> be configurable per sensor, which I don't think is the case.
>
> >
> > >
> > > The Mutable value can be set in the sensor configuration of Entity-Manage,
> when using the patch command to override the sensor, it needs to check the
> EntityManager subtree’s sensor name and its interface
> “xyz.openbmc_project.Configuration.I2CFan.Connector” to check the
> corresponding property name’s mutable value to decide whether executing the
> override function.
> >
> > >See above. I suspect that the redfish code doesn't need to check the
> mutability of the sensor, the interface should just have the correct behavior.
> The only place I would expect to need to know the >mutability of a sensor is in
> the IPMI sdr, where we will need to set the modifiable bit appropriately.
> >
> > For now, the function to set sensor in redfish code is to set the d-bus value
> directly (internally writable), if we don't check the EM mutability in Redfish,
> follow the Add Mutable property to Sensor Value interface, we still need to check
> the sensor mutable property to know whether or not to write the d-bus value in
> redfish or we need other external services to know whether or not to grant write
> permission to their users like IPMI sensor.
>
> I'm not really following this. My point is that the only thing that really needs to
> "check" the mutability requirement is dbus-sensors.
> They should only allow setting when sensors are mutable, and reject when
> they're not.
>
IPMI has an external service to check the Mutability and allow setting when it is "Write" and reject when it's "not write".
Today if we add a mutable property in the d-bus sensor, we also need an external-service like IPMI sensor-handler to check the mutable value to grant write access or not.
You stated that "They should only allow setting when sensors are mutable and reject when they're not." are means an external-service as I mention above?
> >
> > >
> > > This achieves feature parity with the ipmi::sensor::Mutability
> > > parameter of the old hardcoded YAML configuration files
> >
> > >Not sure what you're referring to. That may have been something done in a
> fork.
> >
> > >
> > >
> > >
> > > Execute steps:
> > >
> > > 1. Patch command to override sensor.
> > >
> > > 2. Check the EM of sensor’s Mutable value
> > >
> > > 3. If Mutable value is true do sensor override action else not do.
> > >
> > >
> > >
> > >
> > >
> > > Phase 3 is to add a new get command to get the Zone_$id’s "Manual" value
> and patch command to change the fan mode from auto to manual mode
> ("Manual":true).
> > >
> > > Because the fan control is use package phosphor-pid-control, when we need
> to set fan pwm, it needs to set the fan mode from auto mode to manual mode,
> for now, the phosphor-pid-control has already provided ipmi-oem command to
> achieve this feature, so we need to implement this fan mode change via redfish
> command.
> >
> > >Doesn't this already work today? I thought we had all that sorted a long
> time ago. For some reason I thought we intentionally didn't expose the
> manual/automatic param, because that only applied to >the PID loops, and
> PWM sensor didn't expose that interface. I need to go look at the code at some
> point.
> >
> > Yes, ipmi-oem is work today. I agree it is not properly to show on redfish to let
> users can easily change the fan mode, the reason to change fan mode to the
> manual is for debugging. Maybe let users use ipmi-oem to replace show on
> Redfish URLs.
> >
> > >
> > >
> > >
> > > Example URLs |Method |Example
> Payload
> > >
> > > --------------------------------------- |-------------- |--
> > >
> > > /redfish/v1/Managers/bmc |GET |"Oem": {
> > >
> > > |
> | Fan": {
> > >
> > > |
> | "FanZones": {
> > >
> > > |
> | "@odata.id":
> "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
> > >
> > > |
> | "@odata.type": "#OemManager.FanZones",
> > >
> > > |
> | "Zone_0": {
> > >
> > > |
> | "@odata.id":
> "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
> > >
> > > |
> | "@odata.type":
> "#OemManager.FanZone",
> > >
> > > |
> | "Chassis": {
> > >
> > > |
> | "@odata.id":
> "/redfish/v1/Chassis/GSZ_EVT"
> > >
> > > |
> | },
> > >
> > > |
> | "FailSafePercent": 100.0,
> > >
> > > |
> | "MinThermalOutput": 0.0,
> > >
> > > |
> | "ZoneIndex": 0.0,
> > >
> > > |
> | "Manual":false
> > >
> > > |
> | },
> > >
> > > |
> | },
> > >
> > > |
> | },
> > >
> > > |
> |}
> > >
> > > --------------------------------------- |-------------- |----
> > >
> > > /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan":
> { "FanZones": { "Zone_0": { "Manual":true } } }
> > >
> > >
> >
> > >It should be noted, this schema needs some serious cleanup to make it proper
> resources, paths, and collections, and should version the schema files
> appropriately. If you're planning on extending it, I ?>would expect _some_
> effort to be put into cleanup. There's several github bugs that have more details,
> and I will leave it up to you to decide how much you'd like to do as part of this
> work, but please >plan on some.
> >
> > >
> > > If any thoughts on this topic, feel free to give your comments. Thanks!
> > >
> > >
> > >
> > > Sincerely,
> > >
> > > Bruce
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Fan PWM settings via Redfish
2021-03-17 10:17 ` Bruce Lee (李昀峻)
@ 2021-03-17 15:52 ` Ed Tanous
2021-03-18 9:24 ` Bruce Lee (李昀峻)
0 siblings, 1 reply; 11+ messages in thread
From: Ed Tanous @ 2021-03-17 15:52 UTC (permalink / raw)
To: Bruce Lee (李昀峻); +Cc: openbmc, Nan Zhou, rhanley
On Wed, Mar 17, 2021 at 3:17 AM Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ed Tanous <edtanous@google.com>
> > Sent: Tuesday, March 16, 2021 11:18 PM
> > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > openbmc@lists.ozlabs.org
> > Subject: Re: Fan PWM settings via Redfish
> >
> > On Tue, Mar 16, 2021 at 2:35 AM Bruce Lee (李昀峻)
> > <Bruce_Lee@quantatw.com> wrote:
> > >
> > >
> > >
> > > -----Original Message-----
> > > From: Ed Tanous <edtanous@google.com>
> > > Sent: Saturday, March 13, 2021 1:40 AM
> > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > openbmc@lists.ozlabs.org
> > > Subject: Re: Fan PWM settings via Redfish
> > >
> > > On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻)
> > <Bruce_Lee@quantatw.com> wrote:
> > > >
> > > > Hi All,
> > > >
> > > >
> > > >
> > > > We are designing and implementing the Fan PWM settings via Redfish. The
> > goal is that clients can set sensor value to bmc via Redfish.
> > > >
> > > >
> > > >
> > > > We divide the work into three phases.
> > > >
> > > >
> > > >
> > > > Phase 1 is to remove the definition
> > “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new
> > definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
> > > >
> > > > The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was added
> > by
> > > > Intel group, please refer to
> > > > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fge
> > > > rr
> > > >
> > it.openbmc-project.xyz%2Fc%2Fopenbmc%2Fbmcweb%2F%2B%2F30000&am
> > p;data
> > > > =0
> > > >
> > 4%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e5
> > 7df35c
> > > > %7
> > > >
> > C179b032707fc4973ac738de7313561b2%7C1%7C0%7C63751167640422711
> > 3%7CUnk
> > > > no
> > > >
> > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > Ww
> > > > iL
> > > >
> > CJXVCI6Mn0%3D%7C1000&sdata=f604Piz1vDfItDZ3docZOPfryJesavkbOw
> > hKy
> > > > wJ
> > > > oXlU%3D&reserved=0,
> > > >
> > > > The Intel solution has 4 conditions needs to match one of them and that can
> > be work to override sensor but actually not all project needs those conditions, so
> > we want to propose to remove the insecure definition and use new definition to
> > include the intel solution and execute when compile. It would be no compile time
> > with option for common project. And the insecure issue we will discuss in phase
> > 2.
> > > >
> > > >
> > > >
> > > > Example below:
> > > >
> > > > --------------------------------------------------------------------
> > > > --
> > > > -------------------------------
> > > >
> > > > [Before modified]
> > > >
> > > > #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
> > > >
> > > > // Proceed with sensor override
> > > >
> > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > >
> > > > return;
> > > >
> > > > #endif
> > > >
> > > > doIntelSpecialModeManager code …
> > > >
> > > > --------------------------------------------------------------------
> > > > --
> > > > -------------------------------
> > > >
> > > > [After modified]
> > > >
> > > > #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
> > > >
> > > > doIntelSpecialModeManager code …
> > > >
> > > > return;
> > > >
> > > > #endif
> > > >
> > > > //Proceed with sensor override
> > > >
> > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > >
> > > > --------------------------------------------------------------------
> > > > --
> > > > -------------------------------
> > > >
> > > >
> > > >
> > > >
> > >
> > > >I suspect this check and option needs to be moved into the individual sensors,
> > so that we can differentiate between "should be settable in a test context" and
> > "should be settable in a normal context".
> > > 1. Does you mean don't change the Intel definition and keep the origin code
> > when compile time?
> >
> > No, this means that the checking code needs to move from redfish into
> > dbus-sensors.
> >
> > > 2. What do you mean this option needs to be moved into the individual sensors
> > so that we can differentiate between "should be settable in a test context" and
> > "should be settable in a normal context".
> > > Please provide more details about your thinking.
> >
> > Individual sensors need to provide an appropriate dbus interface.
> > Part of that is enforcing whether or not they're writable, and checking for the
> > debug state of the system to do so.
> >
> > >
> > >
> > > >
> > > > Phase 2 is to add a condition to check the sensor name’s Mutable value of
> > EM if the value is true do the sensor override function else not do.
> > >
> > > >I suspect this patchset needs to be moved forward if you're hoping to use the
> > mutable param:
> > > >https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fger
> > >
> > >rit.openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F%
> > 2
> > >
> > >B%2F36333&data=04%7C01%25>7CBruce_Lee%40quantatw.com%7C64
> > a1153cd4
> > >
> > >5b46eeca4008d8e57df35c%7C179b032707fc4973ac738de7313561b2%7C1
> > %7C0%7C6
> > >
> > >37511676404227113%7CUnknown%>7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> > MDAiLCJQIj
> > >
> > >oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tdExxB%
> > 2BY7
> > > >O1cKb%2FYMdvPGnw7YThW7J55jytDPh4YWYo%3D&reserved=0
> >
> > Not quite, but close. I wouldn't expect the configurability to be directly
> > configurable. External sensor types should be mutable, all other types should
> > not be mutable (except in a debug context). I don't think there's any reason to
> > add a separate "IsMutable" parameter into the EM json, unless it really needs to
> > be configurable per sensor, which I don't think is the case.
> >
> > >
> > > >
> > > > The Mutable value can be set in the sensor configuration of Entity-Manage,
> > when using the patch command to override the sensor, it needs to check the
> > EntityManager subtree’s sensor name and its interface
> > “xyz.openbmc_project.Configuration.I2CFan.Connector” to check the
> > corresponding property name’s mutable value to decide whether executing the
> > override function.
> > >
> > > >See above. I suspect that the redfish code doesn't need to check the
> > mutability of the sensor, the interface should just have the correct behavior.
> > The only place I would expect to need to know the >mutability of a sensor is in
> > the IPMI sdr, where we will need to set the modifiable bit appropriately.
> > >
> > > For now, the function to set sensor in redfish code is to set the d-bus value
> > directly (internally writable), if we don't check the EM mutability in Redfish,
> > follow the Add Mutable property to Sensor Value interface, we still need to check
> > the sensor mutable property to know whether or not to write the d-bus value in
> > redfish or we need other external services to know whether or not to grant write
> > permission to their users like IPMI sensor.
> >
> > I'm not really following this. My point is that the only thing that really needs to
> > "check" the mutability requirement is dbus-sensors.
> > They should only allow setting when sensors are mutable, and reject when
> > they're not.
> >
>
> IPMI has an external service to check the Mutability and allow setting when it is "Write" and reject when it's "not write".
I think we're talking past eachother a little. My point is that dbus
should allow setting, and enforce the check, not IPMI. That means
that we don't have to duplicate the is settable logic between IPMI and
Redfish.
> Today if we add a mutable property in the d-bus sensor, we also need an external-service like IPMI sensor-handler to check the mutable value to grant write access or not.
> You stated that "They should only allow setting when sensors are mutable and reject when they're not." are means an external-service as I mention above?
I'm not following what external service would be needed in this case.
Sensors know their specific type, and the only type of sensor that
should be settable at this point is external sensor, so we can just
encode that logic into the sensor system.
>
> > >
> > > >
> > > > This achieves feature parity with the ipmi::sensor::Mutability
> > > > parameter of the old hardcoded YAML configuration files
> > >
> > > >Not sure what you're referring to. That may have been something done in a
> > fork.
> > >
> > > >
> > > >
> > > >
> > > > Execute steps:
> > > >
> > > > 1. Patch command to override sensor.
> > > >
> > > > 2. Check the EM of sensor’s Mutable value
> > > >
> > > > 3. If Mutable value is true do sensor override action else not do.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > Phase 3 is to add a new get command to get the Zone_$id’s "Manual" value
> > and patch command to change the fan mode from auto to manual mode
> > ("Manual":true).
> > > >
> > > > Because the fan control is use package phosphor-pid-control, when we need
> > to set fan pwm, it needs to set the fan mode from auto mode to manual mode,
> > for now, the phosphor-pid-control has already provided ipmi-oem command to
> > achieve this feature, so we need to implement this fan mode change via redfish
> > command.
> > >
> > > >Doesn't this already work today? I thought we had all that sorted a long
> > time ago. For some reason I thought we intentionally didn't expose the
> > manual/automatic param, because that only applied to >the PID loops, and
> > PWM sensor didn't expose that interface. I need to go look at the code at some
> > point.
> > >
> > > Yes, ipmi-oem is work today. I agree it is not properly to show on redfish to let
> > users can easily change the fan mode, the reason to change fan mode to the
> > manual is for debugging. Maybe let users use ipmi-oem to replace show on
> > Redfish URLs.
> > >
> > > >
> > > >
> > > >
> > > > Example URLs |Method |Example
> > Payload
> > > >
> > > > --------------------------------------- |-------------- |--
> > > >
> > > > /redfish/v1/Managers/bmc |GET |"Oem": {
> > > >
> > > > |
> > | Fan": {
> > > >
> > > > |
> > | "FanZones": {
> > > >
> > > > |
> > | "@odata.id":
> > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
> > > >
> > > > |
> > | "@odata.type": "#OemManager.FanZones",
> > > >
> > > > |
> > | "Zone_0": {
> > > >
> > > > |
> > | "@odata.id":
> > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
> > > >
> > > > |
> > | "@odata.type":
> > "#OemManager.FanZone",
> > > >
> > > > |
> > | "Chassis": {
> > > >
> > > > |
> > | "@odata.id":
> > "/redfish/v1/Chassis/GSZ_EVT"
> > > >
> > > > |
> > | },
> > > >
> > > > |
> > | "FailSafePercent": 100.0,
> > > >
> > > > |
> > | "MinThermalOutput": 0.0,
> > > >
> > > > |
> > | "ZoneIndex": 0.0,
> > > >
> > > > |
> > | "Manual":false
> > > >
> > > > |
> > | },
> > > >
> > > > |
> > | },
> > > >
> > > > |
> > | },
> > > >
> > > > |
> > |}
> > > >
> > > > --------------------------------------- |-------------- |----
> > > >
> > > > /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan":
> > { "FanZones": { "Zone_0": { "Manual":true } } }
> > > >
> > > >
> > >
> > > >It should be noted, this schema needs some serious cleanup to make it proper
> > resources, paths, and collections, and should version the schema files
> > appropriately. If you're planning on extending it, I ?>would expect _some_
> > effort to be put into cleanup. There's several github bugs that have more details,
> > and I will leave it up to you to decide how much you'd like to do as part of this
> > work, but please >plan on some.
> > >
> > > >
> > > > If any thoughts on this topic, feel free to give your comments. Thanks!
> > > >
> > > >
> > > >
> > > > Sincerely,
> > > >
> > > > Bruce
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: Fan PWM settings via Redfish
2021-03-17 15:52 ` Ed Tanous
@ 2021-03-18 9:24 ` Bruce Lee (李昀峻)
2021-03-18 16:08 ` Ed Tanous
0 siblings, 1 reply; 11+ messages in thread
From: Bruce Lee (李昀峻) @ 2021-03-18 9:24 UTC (permalink / raw)
To: Ed Tanous; +Cc: openbmc, Nan Zhou, rhanley
> -----Original Message-----
> From: Ed Tanous <edtanous@google.com>
> Sent: Wednesday, March 17, 2021 11:53 PM
> To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> openbmc@lists.ozlabs.org
> Subject: Re: Fan PWM settings via Redfish
>
> On Wed, Mar 17, 2021 at 3:17 AM Bruce Lee (李昀峻)
> <Bruce_Lee@quantatw.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Ed Tanous <edtanous@google.com>
> > > Sent: Tuesday, March 16, 2021 11:18 PM
> > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > openbmc@lists.ozlabs.org
> > > Subject: Re: Fan PWM settings via Redfish
> > >
> > > On Tue, Mar 16, 2021 at 2:35 AM Bruce Lee (李昀峻)
> > > <Bruce_Lee@quantatw.com> wrote:
> > > >
> > > >
> > > >
> > > > -----Original Message-----
> > > > From: Ed Tanous <edtanous@google.com>
> > > > Sent: Saturday, March 13, 2021 1:40 AM
> > > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > > openbmc@lists.ozlabs.org
> > > > Subject: Re: Fan PWM settings via Redfish
> > > >
> > > > On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻)
> > > <Bruce_Lee@quantatw.com> wrote:
> > > > >
> > > > > Hi All,
> > > > >
> > > > >
> > > > >
> > > > > We are designing and implementing the Fan PWM settings via
> > > > > Redfish. The
> > > goal is that clients can set sensor value to bmc via Redfish.
> > > > >
> > > > >
> > > > >
> > > > > We divide the work into three phases.
> > > > >
> > > > >
> > > > >
> > > > > Phase 1 is to remove the definition
> > > “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new
> > > definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
> > > > >
> > > > > The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was
> added
> > > by
> > > > > Intel group, please refer to
> > > > > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > > 2Fge
> > > > > rr
> > > > >
> > >
> it.openbmc-project.xyz%2Fc%2Fopenbmc%2Fbmcweb%2F%2B%2F30000&am
> > > p;data
> > > > > =0
> > > > >
> > >
> 4%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e5
> > > 7df35c
> > > > > %7
> > > > >
> > >
> C179b032707fc4973ac738de7313561b2%7C1%7C0%7C63751167640422711
> > > 3%7CUnk
> > > > > no
> > > > >
> > >
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > > Ww
> > > > > iL
> > > > >
> > >
> CJXVCI6Mn0%3D%7C1000&sdata=f604Piz1vDfItDZ3docZOPfryJesavkbOw
> > > hKy
> > > > > wJ
> > > > > oXlU%3D&reserved=0,
> > > > >
> > > > > The Intel solution has 4 conditions needs to match one of them
> > > > > and that can
> > > be work to override sensor but actually not all project needs those
> > > conditions, so we want to propose to remove the insecure definition
> > > and use new definition to include the intel solution and execute
> > > when compile. It would be no compile time with option for common
> > > project. And the insecure issue we will discuss in phase 2.
> > > > >
> > > > >
> > > > >
> > > > > Example below:
> > > > >
> > > > > ----------------------------------------------------------------
> > > > > ----
> > > > > --
> > > > > -------------------------------
> > > > >
> > > > > [Before modified]
> > > > >
> > > > > #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
> > > > >
> > > > > // Proceed with sensor override
> > > > >
> > > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > > >
> > > > > return;
> > > > >
> > > > > #endif
> > > > >
> > > > > doIntelSpecialModeManager code …
> > > > >
> > > > > ----------------------------------------------------------------
> > > > > ----
> > > > > --
> > > > > -------------------------------
> > > > >
> > > > > [After modified]
> > > > >
> > > > > #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
> > > > >
> > > > > doIntelSpecialModeManager code …
> > > > >
> > > > > return;
> > > > >
> > > > > #endif
> > > > >
> > > > > //Proceed with sensor override
> > > > >
> > > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > > >
> > > > > ----------------------------------------------------------------
> > > > > ----
> > > > > --
> > > > > -------------------------------
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > > >I suspect this check and option needs to be moved into the
> > > > >individual sensors,
> > > so that we can differentiate between "should be settable in a test
> > > context" and "should be settable in a normal context".
> > > > 1. Does you mean don't change the Intel definition and keep the
> > > > origin code
> > > when compile time?
> > >
> > > No, this means that the checking code needs to move from redfish
> > > into dbus-sensors.
> > >
> > > > 2. What do you mean this option needs to be moved into the
> > > > individual sensors
> > > so that we can differentiate between "should be settable in a test
> > > context" and "should be settable in a normal context".
> > > > Please provide more details about your thinking.
> > >
> > > Individual sensors need to provide an appropriate dbus interface.
> > > Part of that is enforcing whether or not they're writable, and
> > > checking for the debug state of the system to do so.
> > >
> > > >
> > > >
> > > > >
> > > > > Phase 2 is to add a condition to check the sensor name’s Mutable
> > > > > value of
> > > EM if the value is true do the sensor override function else not do.
> > > >
> > > > >I suspect this patchset needs to be moved forward if you're
> > > > >hoping to use the
> > > mutable param:
> > > > >https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2
> > > > >Fger
> > > >
> > >
> >rit.openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F
> > > >%
> > > 2
> > > >
> > >
> >B%2F36333&data=04%7C01%25>7CBruce_Lee%40quantatw.com%7C64
> > > a1153cd4
> > > >
> > >
> >5b46eeca4008d8e57df35c%7C179b032707fc4973ac738de7313561b2%7C1
> > > %7C0%7C6
> > > >
> > >
> >37511676404227113%7CUnknown%>7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> > > MDAiLCJQIj
> > > >
> > >
> >oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tdExxB%
> > > 2BY7
> > > > >O1cKb%2FYMdvPGnw7YThW7J55jytDPh4YWYo%3D&reserved=0
> > >
> > > Not quite, but close. I wouldn't expect the configurability to be
> > > directly configurable. External sensor types should be mutable, all
> > > other types should not be mutable (except in a debug context). I
> > > don't think there's any reason to add a separate "IsMutable"
> > > parameter into the EM json, unless it really needs to be configurable per
> sensor, which I don't think is the case.
> > >
> > > >
> > > > >
> > > > > The Mutable value can be set in the sensor configuration of
> > > > > Entity-Manage,
> > > when using the patch command to override the sensor, it needs to
> > > check the EntityManager subtree’s sensor name and its interface
> > > “xyz.openbmc_project.Configuration.I2CFan.Connector” to check the
> > > corresponding property name’s mutable value to decide whether
> > > executing the override function.
> > > >
> > > > >See above. I suspect that the redfish code doesn't need to check
> > > > >the
> > > mutability of the sensor, the interface should just have the correct behavior.
> > > The only place I would expect to need to know the >mutability of a
> > > sensor is in the IPMI sdr, where we will need to set the modifiable bit
> appropriately.
> > > >
> > > > For now, the function to set sensor in redfish code is to set the
> > > > d-bus value
> > > directly (internally writable), if we don't check the EM mutability
> > > in Redfish, follow the Add Mutable property to Sensor Value
> > > interface, we still need to check the sensor mutable property to
> > > know whether or not to write the d-bus value in redfish or we need
> > > other external services to know whether or not to grant write permission to
> their users like IPMI sensor.
> > >
> > > I'm not really following this. My point is that the only thing that
> > > really needs to "check" the mutability requirement is dbus-sensors.
> > > They should only allow setting when sensors are mutable, and reject
> > > when they're not.
> > >
> >
> > IPMI has an external service to check the Mutability and allow setting when it is
> "Write" and reject when it's "not write".
>
> I think we're talking past eachother a little. My point is that dbus should allow
> setting, and enforce the check, not IPMI. That means that we don't have to
> duplicate the is settable logic between IPMI and Redfish.
>
> > Today if we add a mutable property in the d-bus sensor, we also need an
> external-service like IPMI sensor-handler to check the mutable value to grant
> write access or not.
> > You stated that "They should only allow setting when sensors are mutable and
> reject when they're not." are means an external-service as I mention above?
>
> I'm not following what external service would be needed in this case.
> Sensors know their specific type, and the only type of sensor that should be
> settable at this point is external sensor, so we can just encode that logic into the
> sensor system.
>
If we can distinguish an external user then we can use mutable property to encode that logic into the sensor system.
But how to distinguish an external user If no need external service, how to know user is from IPMI, Redfish, or console itself?
> >
> > > >
> > > > >
> > > > > This achieves feature parity with the ipmi::sensor::Mutability
> > > > > parameter of the old hardcoded YAML configuration files
> > > >
> > > > >Not sure what you're referring to. That may have been something
> > > > >done in a
> > > fork.
> > > >
> > > > >
> > > > >
> > > > >
> > > > > Execute steps:
> > > > >
> > > > > 1. Patch command to override sensor.
> > > > >
> > > > > 2. Check the EM of sensor’s Mutable value
> > > > >
> > > > > 3. If Mutable value is true do sensor override action else not do.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Phase 3 is to add a new get command to get the Zone_$id’s
> > > > > "Manual" value
> > > and patch command to change the fan mode from auto to manual mode
> > > ("Manual":true).
> > > > >
> > > > > Because the fan control is use package phosphor-pid-control,
> > > > > when we need
> > > to set fan pwm, it needs to set the fan mode from auto mode to
> > > manual mode, for now, the phosphor-pid-control has already provided
> > > ipmi-oem command to achieve this feature, so we need to implement
> > > this fan mode change via redfish command.
> > > >
> > > > >Doesn't this already work today? I thought we had all that
> > > > >sorted a long
> > > time ago. For some reason I thought we intentionally didn't expose
> > > the manual/automatic param, because that only applied to >the PID
> > > loops, and PWM sensor didn't expose that interface. I need to go
> > > look at the code at some point.
> > > >
> > > > Yes, ipmi-oem is work today. I agree it is not properly to show on
> > > > redfish to let
> > > users can easily change the fan mode, the reason to change fan mode
> > > to the manual is for debugging. Maybe let users use ipmi-oem to
> > > replace show on Redfish URLs.
> > > >
> > > > >
> > > > >
> > > > >
> > > > > Example URLs |Method |Example
> > > Payload
> > > > >
> > > > > --------------------------------------- |-------------- |--
> > > > >
> > > > > /redfish/v1/Managers/bmc |GET |"Oem": {
> > > > >
> > > > > |
> > > | Fan": {
> > > > >
> > > > > |
> > > | "FanZones": {
> > > > >
> > > > > |
> > > | "@odata.id":
> > > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
> > > > >
> > > > > |
> > > | "@odata.type":
> > > | "#OemManager.FanZones",
> > > > >
> > > > > |
> > > | "Zone_0": {
> > > > >
> > > > > |
> > > | "@odata.id":
> > > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
> > > > >
> > > > > |
> > > | "@odata.type":
> > > "#OemManager.FanZone",
> > > > >
> > > > > |
> > > | "Chassis": {
> > > > >
> > > > > |
> > > | "@odata.id":
> > > "/redfish/v1/Chassis/GSZ_EVT"
> > > > >
> > > > > |
> > > | },
> > > > >
> > > > > |
> > > | "FailSafePercent": 100.0,
> > > > >
> > > > > |
> > > | "MinThermalOutput": 0.0,
> > > > >
> > > > > |
> > > | "ZoneIndex": 0.0,
> > > > >
> > > > > |
> > > | "Manual":false
> > > > >
> > > > > |
> > > | },
> > > > >
> > > > > |
> > > | },
> > > > >
> > > > > |
> > > | },
> > > > >
> > > > > |
> > > |}
> > > > >
> > > > > --------------------------------------- |-------------- |----
> > > > >
> > > > > /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan":
> > > { "FanZones": { "Zone_0": { "Manual":true } } }
> > > > >
> > > > >
> > > >
> > > > >It should be noted, this schema needs some serious cleanup to
> > > > >make it proper
> > > resources, paths, and collections, and should version the schema
> > > files appropriately. If you're planning on extending it, I ?>would
> > > expect _some_ effort to be put into cleanup. There's several github
> > > bugs that have more details, and I will leave it up to you to decide
> > > how much you'd like to do as part of this work, but please >plan on some.
> > > >
> > > > >
> > > > > If any thoughts on this topic, feel free to give your comments. Thanks!
> > > > >
> > > > >
> > > > >
> > > > > Sincerely,
> > > > >
> > > > > Bruce
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Fan PWM settings via Redfish
2021-03-18 9:24 ` Bruce Lee (李昀峻)
@ 2021-03-18 16:08 ` Ed Tanous
2021-03-25 7:28 ` Bruce Lee (李昀峻)
0 siblings, 1 reply; 11+ messages in thread
From: Ed Tanous @ 2021-03-18 16:08 UTC (permalink / raw)
To: Bruce Lee (李昀峻); +Cc: openbmc, Nan Zhou, rhanley
On Thu, Mar 18, 2021 at 2:24 AM Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ed Tanous <edtanous@google.com>
> > Sent: Wednesday, March 17, 2021 11:53 PM
> > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > openbmc@lists.ozlabs.org
> > Subject: Re: Fan PWM settings via Redfish
> >
> > On Wed, Mar 17, 2021 at 3:17 AM Bruce Lee (李昀峻)
> > <Bruce_Lee@quantatw.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ed Tanous <edtanous@google.com>
> > > > Sent: Tuesday, March 16, 2021 11:18 PM
> > > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > > openbmc@lists.ozlabs.org
> > > > Subject: Re: Fan PWM settings via Redfish
> > > >
> > > > On Tue, Mar 16, 2021 at 2:35 AM Bruce Lee (李昀峻)
> > > > <Bruce_Lee@quantatw.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > -----Original Message-----
> > > > > From: Ed Tanous <edtanous@google.com>
> > > > > Sent: Saturday, March 13, 2021 1:40 AM
> > > > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > > > openbmc@lists.ozlabs.org
> > > > > Subject: Re: Fan PWM settings via Redfish
> > > > >
> > > > > On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻)
> > > > <Bruce_Lee@quantatw.com> wrote:
> > > > > >
> > > > > > Hi All,
> > > > > >
> > > > > >
> > > > > >
> > > > > > We are designing and implementing the Fan PWM settings via
> > > > > > Redfish. The
> > > > goal is that clients can set sensor value to bmc via Redfish.
> > > > > >
> > > > > >
> > > > > >
> > > > > > We divide the work into three phases.
> > > > > >
> > > > > >
> > > > > >
> > > > > > Phase 1 is to remove the definition
> > > > “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new
> > > > definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
> > > > > >
> > > > > > The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was
> > added
> > > > by
> > > > > > Intel group, please refer to
> > > > > > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > > > 2Fge
> > > > > > rr
> > > > > >
> > > >
> > it.openbmc-project.xyz%2Fc%2Fopenbmc%2Fbmcweb%2F%2B%2F30000&am
> > > > p;data
> > > > > > =0
> > > > > >
> > > >
> > 4%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e5
> > > > 7df35c
> > > > > > %7
> > > > > >
> > > >
> > C179b032707fc4973ac738de7313561b2%7C1%7C0%7C63751167640422711
> > > > 3%7CUnk
> > > > > > no
> > > > > >
> > > >
> > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > > > Ww
> > > > > > iL
> > > > > >
> > > >
> > CJXVCI6Mn0%3D%7C1000&sdata=f604Piz1vDfItDZ3docZOPfryJesavkbOw
> > > > hKy
> > > > > > wJ
> > > > > > oXlU%3D&reserved=0,
> > > > > >
> > > > > > The Intel solution has 4 conditions needs to match one of them
> > > > > > and that can
> > > > be work to override sensor but actually not all project needs those
> > > > conditions, so we want to propose to remove the insecure definition
> > > > and use new definition to include the intel solution and execute
> > > > when compile. It would be no compile time with option for common
> > > > project. And the insecure issue we will discuss in phase 2.
> > > > > >
> > > > > >
> > > > > >
> > > > > > Example below:
> > > > > >
> > > > > > ----------------------------------------------------------------
> > > > > > ----
> > > > > > --
> > > > > > -------------------------------
> > > > > >
> > > > > > [Before modified]
> > > > > >
> > > > > > #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
> > > > > >
> > > > > > // Proceed with sensor override
> > > > > >
> > > > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > > > >
> > > > > > return;
> > > > > >
> > > > > > #endif
> > > > > >
> > > > > > doIntelSpecialModeManager code …
> > > > > >
> > > > > > ----------------------------------------------------------------
> > > > > > ----
> > > > > > --
> > > > > > -------------------------------
> > > > > >
> > > > > > [After modified]
> > > > > >
> > > > > > #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
> > > > > >
> > > > > > doIntelSpecialModeManager code …
> > > > > >
> > > > > > return;
> > > > > >
> > > > > > #endif
> > > > > >
> > > > > > //Proceed with sensor override
> > > > > >
> > > > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > > > >
> > > > > > ----------------------------------------------------------------
> > > > > > ----
> > > > > > --
> > > > > > -------------------------------
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > > >I suspect this check and option needs to be moved into the
> > > > > >individual sensors,
> > > > so that we can differentiate between "should be settable in a test
> > > > context" and "should be settable in a normal context".
> > > > > 1. Does you mean don't change the Intel definition and keep the
> > > > > origin code
> > > > when compile time?
> > > >
> > > > No, this means that the checking code needs to move from redfish
> > > > into dbus-sensors.
> > > >
> > > > > 2. What do you mean this option needs to be moved into the
> > > > > individual sensors
> > > > so that we can differentiate between "should be settable in a test
> > > > context" and "should be settable in a normal context".
> > > > > Please provide more details about your thinking.
> > > >
> > > > Individual sensors need to provide an appropriate dbus interface.
> > > > Part of that is enforcing whether or not they're writable, and
> > > > checking for the debug state of the system to do so.
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Phase 2 is to add a condition to check the sensor name’s Mutable
> > > > > > value of
> > > > EM if the value is true do the sensor override function else not do.
> > > > >
> > > > > >I suspect this patchset needs to be moved forward if you're
> > > > > >hoping to use the
> > > > mutable param:
> > > > > >https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2
> > > > > >Fger
> > > > >
> > > >
> > >rit.openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F
> > > > >%
> > > > 2
> > > > >
> > > >
> > >B%2F36333&data=04%7C01%25>7CBruce_Lee%40quantatw.com%7C64
> > > > a1153cd4
> > > > >
> > > >
> > >5b46eeca4008d8e57df35c%7C179b032707fc4973ac738de7313561b2%7C1
> > > > %7C0%7C6
> > > > >
> > > >
> > >37511676404227113%7CUnknown%>7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> > > > MDAiLCJQIj
> > > > >
> > > >
> > >oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tdExxB%
> > > > 2BY7
> > > > > >O1cKb%2FYMdvPGnw7YThW7J55jytDPh4YWYo%3D&reserved=0
> > > >
> > > > Not quite, but close. I wouldn't expect the configurability to be
> > > > directly configurable. External sensor types should be mutable, all
> > > > other types should not be mutable (except in a debug context). I
> > > > don't think there's any reason to add a separate "IsMutable"
> > > > parameter into the EM json, unless it really needs to be configurable per
> > sensor, which I don't think is the case.
> > > >
> > > > >
> > > > > >
> > > > > > The Mutable value can be set in the sensor configuration of
> > > > > > Entity-Manage,
> > > > when using the patch command to override the sensor, it needs to
> > > > check the EntityManager subtree’s sensor name and its interface
> > > > “xyz.openbmc_project.Configuration.I2CFan.Connector” to check the
> > > > corresponding property name’s mutable value to decide whether
> > > > executing the override function.
> > > > >
> > > > > >See above. I suspect that the redfish code doesn't need to check
> > > > > >the
> > > > mutability of the sensor, the interface should just have the correct behavior.
> > > > The only place I would expect to need to know the >mutability of a
> > > > sensor is in the IPMI sdr, where we will need to set the modifiable bit
> > appropriately.
> > > > >
> > > > > For now, the function to set sensor in redfish code is to set the
> > > > > d-bus value
> > > > directly (internally writable), if we don't check the EM mutability
> > > > in Redfish, follow the Add Mutable property to Sensor Value
> > > > interface, we still need to check the sensor mutable property to
> > > > know whether or not to write the d-bus value in redfish or we need
> > > > other external services to know whether or not to grant write permission to
> > their users like IPMI sensor.
> > > >
> > > > I'm not really following this. My point is that the only thing that
> > > > really needs to "check" the mutability requirement is dbus-sensors.
> > > > They should only allow setting when sensors are mutable, and reject
> > > > when they're not.
> > > >
> > >
> > > IPMI has an external service to check the Mutability and allow setting when it is
> > "Write" and reject when it's "not write".
> >
> > I think we're talking past eachother a little. My point is that dbus should allow
> > setting, and enforce the check, not IPMI. That means that we don't have to
> > duplicate the is settable logic between IPMI and Redfish.
> >
> > > Today if we add a mutable property in the d-bus sensor, we also need an
> > external-service like IPMI sensor-handler to check the mutable value to grant
> > write access or not.
> > > You stated that "They should only allow setting when sensors are mutable and
> > reject when they're not." are means an external-service as I mention above?
> >
> > I'm not following what external service would be needed in this case.
> > Sensors know their specific type, and the only type of sensor that should be
> > settable at this point is external sensor, so we can just encode that logic into the
> > sensor system.
> >
>
> If we can distinguish an external user then we can use mutable property to encode that logic into the sensor system.
> But how to distinguish an external user If no need external service, how to know user is from IPMI, Redfish, or console itself?
>
No need to distinguish external users from internal users in this
case. If a sensor is settable, it doesn't matter if it's being set
from within the bmc or outside the BMC.
> > >
> > > > >
> > > > > >
> > > > > > This achieves feature parity with the ipmi::sensor::Mutability
> > > > > > parameter of the old hardcoded YAML configuration files
> > > > >
> > > > > >Not sure what you're referring to. That may have been something
> > > > > >done in a
> > > > fork.
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Execute steps:
> > > > > >
> > > > > > 1. Patch command to override sensor.
> > > > > >
> > > > > > 2. Check the EM of sensor’s Mutable value
> > > > > >
> > > > > > 3. If Mutable value is true do sensor override action else not do.
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Phase 3 is to add a new get command to get the Zone_$id’s
> > > > > > "Manual" value
> > > > and patch command to change the fan mode from auto to manual mode
> > > > ("Manual":true).
> > > > > >
> > > > > > Because the fan control is use package phosphor-pid-control,
> > > > > > when we need
> > > > to set fan pwm, it needs to set the fan mode from auto mode to
> > > > manual mode, for now, the phosphor-pid-control has already provided
> > > > ipmi-oem command to achieve this feature, so we need to implement
> > > > this fan mode change via redfish command.
> > > > >
> > > > > >Doesn't this already work today? I thought we had all that
> > > > > >sorted a long
> > > > time ago. For some reason I thought we intentionally didn't expose
> > > > the manual/automatic param, because that only applied to >the PID
> > > > loops, and PWM sensor didn't expose that interface. I need to go
> > > > look at the code at some point.
> > > > >
> > > > > Yes, ipmi-oem is work today. I agree it is not properly to show on
> > > > > redfish to let
> > > > users can easily change the fan mode, the reason to change fan mode
> > > > to the manual is for debugging. Maybe let users use ipmi-oem to
> > > > replace show on Redfish URLs.
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Example URLs |Method |Example
> > > > Payload
> > > > > >
> > > > > > --------------------------------------- |-------------- |--
> > > > > >
> > > > > > /redfish/v1/Managers/bmc |GET |"Oem": {
> > > > > >
> > > > > > |
> > > > | Fan": {
> > > > > >
> > > > > > |
> > > > | "FanZones": {
> > > > > >
> > > > > > |
> > > > | "@odata.id":
> > > > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
> > > > > >
> > > > > > |
> > > > | "@odata.type":
> > > > | "#OemManager.FanZones",
> > > > > >
> > > > > > |
> > > > | "Zone_0": {
> > > > > >
> > > > > > |
> > > > | "@odata.id":
> > > > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
> > > > > >
> > > > > > |
> > > > | "@odata.type":
> > > > "#OemManager.FanZone",
> > > > > >
> > > > > > |
> > > > | "Chassis": {
> > > > > >
> > > > > > |
> > > > | "@odata.id":
> > > > "/redfish/v1/Chassis/GSZ_EVT"
> > > > > >
> > > > > > |
> > > > | },
> > > > > >
> > > > > > |
> > > > | "FailSafePercent": 100.0,
> > > > > >
> > > > > > |
> > > > | "MinThermalOutput": 0.0,
> > > > > >
> > > > > > |
> > > > | "ZoneIndex": 0.0,
> > > > > >
> > > > > > |
> > > > | "Manual":false
> > > > > >
> > > > > > |
> > > > | },
> > > > > >
> > > > > > |
> > > > | },
> > > > > >
> > > > > > |
> > > > | },
> > > > > >
> > > > > > |
> > > > |}
> > > > > >
> > > > > > --------------------------------------- |-------------- |----
> > > > > >
> > > > > > /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan":
> > > > { "FanZones": { "Zone_0": { "Manual":true } } }
> > > > > >
> > > > > >
> > > > >
> > > > > >It should be noted, this schema needs some serious cleanup to
> > > > > >make it proper
> > > > resources, paths, and collections, and should version the schema
> > > > files appropriately. If you're planning on extending it, I ?>would
> > > > expect _some_ effort to be put into cleanup. There's several github
> > > > bugs that have more details, and I will leave it up to you to decide
> > > > how much you'd like to do as part of this work, but please >plan on some.
> > > > >
> > > > > >
> > > > > > If any thoughts on this topic, feel free to give your comments. Thanks!
> > > > > >
> > > > > >
> > > > > >
> > > > > > Sincerely,
> > > > > >
> > > > > > Bruce
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: Fan PWM settings via Redfish
2021-03-18 16:08 ` Ed Tanous
@ 2021-03-25 7:28 ` Bruce Lee (李昀峻)
2021-03-31 6:46 ` Bruce Lee (李昀峻)
0 siblings, 1 reply; 11+ messages in thread
From: Bruce Lee (李昀峻) @ 2021-03-25 7:28 UTC (permalink / raw)
To: Ed Tanous; +Cc: openbmc, Nan Zhou, rhanley
> -----Original Message-----
> From: Ed Tanous <edtanous@google.com>
> Sent: Friday, March 19, 2021 12:09 AM
> To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> openbmc@lists.ozlabs.org
> Subject: Re: Fan PWM settings via Redfish
>
> On Thu, Mar 18, 2021 at 2:24 AM Bruce Lee (李昀峻)
> <Bruce_Lee@quantatw.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Ed Tanous <edtanous@google.com>
> > > Sent: Wednesday, March 17, 2021 11:53 PM
> > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > openbmc@lists.ozlabs.org
> > > Subject: Re: Fan PWM settings via Redfish
> > >
> > > On Wed, Mar 17, 2021 at 3:17 AM Bruce Lee (李昀峻)
> > > <Bruce_Lee@quantatw.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ed Tanous <edtanous@google.com>
> > > > > Sent: Tuesday, March 16, 2021 11:18 PM
> > > > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > > > openbmc@lists.ozlabs.org
> > > > > Subject: Re: Fan PWM settings via Redfish
> > > > >
> > > > > On Tue, Mar 16, 2021 at 2:35 AM Bruce Lee (李昀峻)
> > > > > <Bruce_Lee@quantatw.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ed Tanous <edtanous@google.com>
> > > > > > Sent: Saturday, March 13, 2021 1:40 AM
> > > > > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > > > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > > > > openbmc@lists.ozlabs.org
> > > > > > Subject: Re: Fan PWM settings via Redfish
> > > > > >
> > > > > > On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻)
> > > > > <Bruce_Lee@quantatw.com> wrote:
> > > > > > >
> > > > > > > Hi All,
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > We are designing and implementing the Fan PWM settings via
> > > > > > > Redfish. The
> > > > > goal is that clients can set sensor value to bmc via Redfish.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > We divide the work into three phases.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Phase 1 is to remove the definition
> > > > > “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use new
> > > > > definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
> > > > > > >
> > > > > > > The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” was
> > > added
> > > > > by
> > > > > > > Intel group, please refer to
> > > > > > > https://apc01.safelinks.protection.outlook.com/?url=https%3A
> > > > > > > %2F%25
> > > > > > > 2Fge
> > > > > > > rr
> > > > > > >
> > > > >
> > >
> it.openbmc-project.xyz%2Fc%2Fopenbmc%2Fbmcweb%2F%2B%2F30000&am
> > > > > p;data
> > > > > > > =0
> > > > > > >
> > > > >
> > >
> 4%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e5
> > > > > 7df35c
> > > > > > > %7
> > > > > > >
> > > > >
> > >
> C179b032707fc4973ac738de7313561b2%7C1%7C0%7C63751167640422711
> > > > > 3%7CUnk
> > > > > > > no
> > > > > > >
> > > > >
> > >
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > > > > Ww
> > > > > > > iL
> > > > > > >
> > > > >
> > >
> CJXVCI6Mn0%3D%7C1000&sdata=f604Piz1vDfItDZ3docZOPfryJesavkbOw
> > > > > hKy
> > > > > > > wJ
> > > > > > > oXlU%3D&reserved=0,
> > > > > > >
> > > > > > > The Intel solution has 4 conditions needs to match one of
> > > > > > > them and that can
> > > > > be work to override sensor but actually not all project needs
> > > > > those conditions, so we want to propose to remove the insecure
> > > > > definition and use new definition to include the intel solution
> > > > > and execute when compile. It would be no compile time with
> > > > > option for common project. And the insecure issue we will discuss in
> phase 2.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Example below:
> > > > > > >
> > > > > > > ------------------------------------------------------------
> > > > > > > ----
> > > > > > > ----
> > > > > > > --
> > > > > > > -------------------------------
> > > > > > >
> > > > > > > [Before modified]
> > > > > > >
> > > > > > > #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
> > > > > > >
> > > > > > > // Proceed with sensor override
> > > > > > >
> > > > > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > > > > >
> > > > > > > return;
> > > > > > >
> > > > > > > #endif
> > > > > > >
> > > > > > > doIntelSpecialModeManager code …
> > > > > > >
> > > > > > > ------------------------------------------------------------
> > > > > > > ----
> > > > > > > ----
> > > > > > > --
> > > > > > > -------------------------------
> > > > > > >
> > > > > > > [After modified]
> > > > > > >
> > > > > > > #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
> > > > > > >
> > > > > > > doIntelSpecialModeManager code …
> > > > > > >
> > > > > > > return;
> > > > > > >
> > > > > > > #endif
> > > > > > >
> > > > > > > //Proceed with sensor override
> > > > > > >
> > > > > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > > > > >
> > > > > > > ------------------------------------------------------------
> > > > > > > ----
> > > > > > > ----
> > > > > > > --
> > > > > > > -------------------------------
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > >I suspect this check and option needs to be moved into the
> > > > > > >individual sensors,
> > > > > so that we can differentiate between "should be settable in a
> > > > > test context" and "should be settable in a normal context".
> > > > > > 1. Does you mean don't change the Intel definition and keep
> > > > > > the origin code
> > > > > when compile time?
> > > > >
> > > > > No, this means that the checking code needs to move from redfish
> > > > > into dbus-sensors.
> > > > >
> > > > > > 2. What do you mean this option needs to be moved into the
> > > > > > individual sensors
> > > > > so that we can differentiate between "should be settable in a
> > > > > test context" and "should be settable in a normal context".
> > > > > > Please provide more details about your thinking.
> > > > >
> > > > > Individual sensors need to provide an appropriate dbus interface.
> > > > > Part of that is enforcing whether or not they're writable, and
> > > > > checking for the debug state of the system to do so.
> > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Phase 2 is to add a condition to check the sensor name’s
> > > > > > > Mutable value of
> > > > > EM if the value is true do the sensor override function else not do.
> > > > > >
> > > > > > >I suspect this patchset needs to be moved forward if you're
> > > > > > >hoping to use the
> > > > > mutable param:
> > > > > > >https://apc01.safelinks.protection.outlook.com/?url=https%3A%
> > > > > > >2F%252
> > > > > > >Fger
> > > > > >
> > > > >
> > >
> >rit.openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F
> > > > > >%
> > > > > 2
> > > > > >
> > > > >
> > >
> >B%2F36333&data=04%7C01%25>7CBruce_Lee%40quantatw.com%7C64
> > > > > a1153cd4
> > > > > >
> > > > >
> > >
> >5b46eeca4008d8e57df35c%7C179b032707fc4973ac738de7313561b2%7C1
> > > > > %7C0%7C6
> > > > > >
> > > > >
> > >
> >37511676404227113%7CUnknown%>7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> > > > > MDAiLCJQIj
> > > > > >
> > > > >
> > >
> >oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tdExxB%
> > > > > 2BY7
> > > > > >
> >O1cKb%2FYMdvPGnw7YThW7J55jytDPh4YWYo%3D&reserved=0
> > > > >
> > > > > Not quite, but close. I wouldn't expect the configurability to
> > > > > be directly configurable. External sensor types should be
> > > > > mutable, all other types should not be mutable (except in a
> > > > > debug context). I don't think there's any reason to add a separate
> "IsMutable"
> > > > > parameter into the EM json, unless it really needs to be
> > > > > configurable per
> > > sensor, which I don't think is the case.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > The Mutable value can be set in the sensor configuration of
> > > > > > > Entity-Manage,
> > > > > when using the patch command to override the sensor, it needs to
> > > > > check the EntityManager subtree’s sensor name and its interface
> > > > > “xyz.openbmc_project.Configuration.I2CFan.Connector” to check
> > > > > the corresponding property name’s mutable value to decide
> > > > > whether executing the override function.
> > > > > >
> > > > > > >See above. I suspect that the redfish code doesn't need to
> > > > > > >check the
> > > > > mutability of the sensor, the interface should just have the correct
> behavior.
> > > > > The only place I would expect to need to know the >mutability of
> > > > > a sensor is in the IPMI sdr, where we will need to set the
> > > > > modifiable bit
> > > appropriately.
> > > > > >
> > > > > > For now, the function to set sensor in redfish code is to set
> > > > > > the d-bus value
> > > > > directly (internally writable), if we don't check the EM
> > > > > mutability in Redfish, follow the Add Mutable property to Sensor
> > > > > Value interface, we still need to check the sensor mutable
> > > > > property to know whether or not to write the d-bus value in
> > > > > redfish or we need other external services to know whether or
> > > > > not to grant write permission to
> > > their users like IPMI sensor.
> > > > >
> > > > > I'm not really following this. My point is that the only thing
> > > > > that really needs to "check" the mutability requirement is dbus-sensors.
> > > > > They should only allow setting when sensors are mutable, and
> > > > > reject when they're not.
> > > > >
> > > >
> > > > IPMI has an external service to check the Mutability and allow
> > > > setting when it is
> > > "Write" and reject when it's "not write".
> > >
> > > I think we're talking past eachother a little. My point is that
> > > dbus should allow setting, and enforce the check, not IPMI. That
> > > means that we don't have to duplicate the is settable logic between IPMI and
> Redfish.
> > >
> > > > Today if we add a mutable property in the d-bus sensor, we also
> > > > need an
> > > external-service like IPMI sensor-handler to check the mutable value
> > > to grant write access or not.
> > > > You stated that "They should only allow setting when sensors are
> > > > mutable and
> > > reject when they're not." are means an external-service as I mention above?
> > >
> > > I'm not following what external service would be needed in this case.
> > > Sensors know their specific type, and the only type of sensor that
> > > should be settable at this point is external sensor, so we can just
> > > encode that logic into the sensor system.
> > >
> >
> > If we can distinguish an external user then we can use mutable property to
> encode that logic into the sensor system.
> > But how to distinguish an external user If no need external service, how to
> know user is from IPMI, Redfish, or console itself?
> >
>
> No need to distinguish external users from internal users in this case. If a sensor
> is settable, it doesn't matter if it's being set from within the bmc or outside the
> BMC.
>
> > > >
> > > > > >
> > > > > > >
> > > > > > > This achieves feature parity with the
> > > > > > > ipmi::sensor::Mutability parameter of the old hardcoded YAML
> > > > > > > configuration files
> > > > > >
> > > > > > >Not sure what you're referring to. That may have been
> > > > > > >something done in a
> > > > > fork.
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Execute steps:
> > > > > > >
> > > > > > > 1. Patch command to override sensor.
> > > > > > >
> > > > > > > 2. Check the EM of sensor’s Mutable value
> > > > > > >
> > > > > > > 3. If Mutable value is true do sensor override action else not
> do.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Phase 3 is to add a new get command to get the Zone_$id’s
> > > > > > > "Manual" value
> > > > > and patch command to change the fan mode from auto to manual
> > > > > mode ("Manual":true).
> > > > > > >
> > > > > > > Because the fan control is use package phosphor-pid-control,
> > > > > > > when we need
> > > > > to set fan pwm, it needs to set the fan mode from auto mode to
> > > > > manual mode, for now, the phosphor-pid-control has already
> > > > > provided ipmi-oem command to achieve this feature, so we need to
> > > > > implement this fan mode change via redfish command.
> > > > > >
> > > > > > >Doesn't this already work today? I thought we had all that
> > > > > > >sorted a long
> > > > > time ago. For some reason I thought we intentionally didn't
> > > > > expose the manual/automatic param, because that only applied to
> > > > > >the PID loops, and PWM sensor didn't expose that interface. I
> > > > > need to go look at the code at some point.
> > > > > >
> > > > > > Yes, ipmi-oem is work today. I agree it is not properly to
> > > > > > show on redfish to let
> > > > > users can easily change the fan mode, the reason to change fan
> > > > > mode to the manual is for debugging. Maybe let users use
> > > > > ipmi-oem to replace show on Redfish URLs.
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Example URLs |Method
> |Example
> > > > > Payload
> > > > > > >
> > > > > > > --------------------------------------- |-------------- |--
> > > > > > >
> > > > > > > /redfish/v1/Managers/bmc |GET |"Oem": {
> > > > > > >
> > > > > > > |
> > > > > | Fan": {
> > > > > > >
> > > > > > > |
> > > > > | "FanZones": {
> > > > > > >
> > > > > > > |
> > > > > | "@odata.id":
> > > > > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
> > > > > > >
> > > > > > > |
> > > > > | "@odata.type":
> > > > > | "#OemManager.FanZones",
> > > > > > >
> > > > > > > |
> > > > > | "Zone_0": {
> > > > > > >
> > > > > > > |
> > > > > | "@odata.id":
> > > > > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
> > > > > > >
> > > > > > > |
> > > > > | "@odata.type":
> > > > > "#OemManager.FanZone",
> > > > > > >
> > > > > > > |
> > > > > | "Chassis": {
> > > > > > >
> > > > > > > |
> > > > > |
> "@odata.id":
> > > > > "/redfish/v1/Chassis/GSZ_EVT"
> > > > > > >
> > > > > > > |
> > > > > | },
> > > > > > >
> > > > > > > |
> > > > > | "FailSafePercent":
> > > > > | 100.0,
> > > > > > >
> > > > > > > |
> > > > > | "MinThermalOutput":
> > > > > | 0.0,
> > > > > > >
> > > > > > > |
> > > > > | "ZoneIndex": 0.0,
> > > > > > >
> > > > > > > |
> > > > > | "Manual":false
> > > > > > >
> > > > > > > |
> > > > > | },
> > > > > > >
> > > > > > > |
> > > > > | },
> > > > > > >
> > > > > > > |
> > > > > | },
> > > > > > >
> > > > > > > |
> > > > > |}
> > > > > > >
> > > > > > > --------------------------------------- |--------------
> > > > > > > |----
> > > > > > >
> > > > > > > /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan":
> > > > > { "FanZones": { "Zone_0": { "Manual":true } } }
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > >It should be noted, this schema needs some serious cleanup to
> > > > > > >make it proper
> > > > > resources, paths, and collections, and should version the schema
> > > > > files appropriately. If you're planning on extending it, I
> > > > > ?>would expect _some_ effort to be put into cleanup. There's
> > > > > several github bugs that have more details, and I will leave it
> > > > > up to you to decide how much you'd like to do as part of this work, but
> please >plan on some.
> > > > > >
After discussing with Nan and follow up the thread on Chat, conclusion has been reached bellow:
1. Remove the "If in validation mode" check from bmcweb
2. Add the "If in validation mode" check to dbus-sensors, excluding external sensor and fan sensor (the two that should be settable all the time)
3. Propose the mutability interface to phosphor-dbus-interfaces upstream.
4. Add a patch to Willys ipmi-dynamic option that adds mutability support to the sdr, and set sensor command support. Use google-ipmi-dynamic as a reference.
Here is a new proposal with working details:
in bmcweb
1. Remove Intel stuff and only keep setSensorsOverride function.
setSensorsOverride(sensorAsyncResp, allCollections);
2. Change setSensorsOverride function, change to call dbus-sensor for replacing directly to set the dbus value.
in dbus-sensors
1. Add the "If in validation mode" check to dbus-sensors, excluding external sensor and fan sensor (the two that should be settable all the time)
others
1. Propose the mutability interface to phosphor-dbus-interfaces upstream.
> > > > > > >
> > > > > > > If any thoughts on this topic, feel free to give your comments. Thanks!
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Sincerely,
> > > > > > >
> > > > > > > Bruce
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: Fan PWM settings via Redfish
2021-03-25 7:28 ` Bruce Lee (李昀峻)
@ 2021-03-31 6:46 ` Bruce Lee (李昀峻)
0 siblings, 0 replies; 11+ messages in thread
From: Bruce Lee (李昀峻) @ 2021-03-31 6:46 UTC (permalink / raw)
To: Ed Tanous; +Cc: openbmc, Nan Zhou, rhanley
> -----Original Message-----
> From: Bruce Lee (李昀峻)
> Sent: Thursday, March 25, 2021 3:29 PM
> To: Ed Tanous <edtanous@google.com>
> Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> openbmc@lists.ozlabs.org
> Subject: RE: Fan PWM settings via Redfish
>
>
>
> > -----Original Message-----
> > From: Ed Tanous <edtanous@google.com>
> > Sent: Friday, March 19, 2021 12:09 AM
> > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > openbmc@lists.ozlabs.org
> > Subject: Re: Fan PWM settings via Redfish
> >
> > On Thu, Mar 18, 2021 at 2:24 AM Bruce Lee (李昀峻)
> > <Bruce_Lee@quantatw.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ed Tanous <edtanous@google.com>
> > > > Sent: Wednesday, March 17, 2021 11:53 PM
> > > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > > openbmc@lists.ozlabs.org
> > > > Subject: Re: Fan PWM settings via Redfish
> > > >
> > > > On Wed, Mar 17, 2021 at 3:17 AM Bruce Lee (李昀峻)
> > > > <Bruce_Lee@quantatw.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ed Tanous <edtanous@google.com>
> > > > > > Sent: Tuesday, March 16, 2021 11:18 PM
> > > > > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > > > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > > > > openbmc@lists.ozlabs.org
> > > > > > Subject: Re: Fan PWM settings via Redfish
> > > > > >
> > > > > > On Tue, Mar 16, 2021 at 2:35 AM Bruce Lee (李昀峻)
> > > > > > <Bruce_Lee@quantatw.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ed Tanous <edtanous@google.com>
> > > > > > > Sent: Saturday, March 13, 2021 1:40 AM
> > > > > > > To: Bruce Lee (李昀峻) <Bruce_Lee@quantatw.com>
> > > > > > > Cc: Nan Zhou <nanzhou@google.com>; rhanley@google.com;
> > > > > > > openbmc@lists.ozlabs.org
> > > > > > > Subject: Re: Fan PWM settings via Redfish
> > > > > > >
> > > > > > > On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻)
> > > > > > <Bruce_Lee@quantatw.com> wrote:
> > > > > > > >
> > > > > > > > Hi All,
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > We are designing and implementing the Fan PWM settings via
> > > > > > > > Redfish. The
> > > > > > goal is that clients can set sensor value to bmc via Redfish.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > We divide the work into three phases.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Phase 1 is to remove the definition
> > > > > > “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE” and use
> new
> > > > > > definition to “BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE”.
> > > > > > > >
> > > > > > > > The “BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE”
> was
> > > > added
> > > > > > by
> > > > > > > > Intel group, please refer to
> > > > > > > > https://apc01.safelinks.protection.outlook.com/?url=https%
> > > > > > > > 3A
> > > > > > > > %2F%25
> > > > > > > > 2Fge
> > > > > > > > rr
> > > > > > > >
> > > > > >
> > > >
> >
> it.openbmc-project.xyz%2Fc%2Fopenbmc%2Fbmcweb%2F%2B%2F30000&am
> > > > > > p;data
> > > > > > > > =0
> > > > > > > >
> > > > > >
> > > >
> >
> 4%7C01%7CBruce_Lee%40quantatw.com%7C64a1153cd45b46eeca4008d8e5
> > > > > > 7df35c
> > > > > > > > %7
> > > > > > > >
> > > > > >
> > > >
> >
> C179b032707fc4973ac738de7313561b2%7C1%7C0%7C63751167640422711
> > > > > > 3%7CUnk
> > > > > > > > no
> > > > > > > >
> > > > > >
> > > >
> >
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > > > > > Ww
> > > > > > > > iL
> > > > > > > >
> > > > > >
> > > >
> >
> CJXVCI6Mn0%3D%7C1000&sdata=f604Piz1vDfItDZ3docZOPfryJesavkbOw
> > > > > > hKy
> > > > > > > > wJ
> > > > > > > > oXlU%3D&reserved=0,
> > > > > > > >
> > > > > > > > The Intel solution has 4 conditions needs to match one of
> > > > > > > > them and that can
> > > > > > be work to override sensor but actually not all project needs
> > > > > > those conditions, so we want to propose to remove the insecure
> > > > > > definition and use new definition to include the intel
> > > > > > solution and execute when compile. It would be no compile time
> > > > > > with option for common project. And the insecure issue we will
> > > > > > discuss in
> > phase 2.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Example below:
> > > > > > > >
> > > > > > > > ----------------------------------------------------------
> > > > > > > > --
> > > > > > > > ----
> > > > > > > > ----
> > > > > > > > --
> > > > > > > > -------------------------------
> > > > > > > >
> > > > > > > > [Before modified]
> > > > > > > >
> > > > > > > > #ifdef BMCWEB_INSECURE_UNRESTRICTED_SENSOR_OVERRIDE
> > > > > > > >
> > > > > > > > // Proceed with sensor override
> > > > > > > >
> > > > > > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > > > > > >
> > > > > > > > return;
> > > > > > > >
> > > > > > > > #endif
> > > > > > > >
> > > > > > > > doIntelSpecialModeManager code …
> > > > > > > >
> > > > > > > > ----------------------------------------------------------
> > > > > > > > --
> > > > > > > > ----
> > > > > > > > ----
> > > > > > > > --
> > > > > > > > -------------------------------
> > > > > > > >
> > > > > > > > [After modified]
> > > > > > > >
> > > > > > > > #ifdef BMCWEB_SPECIAL_MODE_SENSOR_OVERRIDE
> > > > > > > >
> > > > > > > > doIntelSpecialModeManager code …
> > > > > > > >
> > > > > > > > return;
> > > > > > > >
> > > > > > > > #endif
> > > > > > > >
> > > > > > > > //Proceed with sensor override
> > > > > > > >
> > > > > > > > setSensorsOverride(sensorAsyncResp, allCollections);
> > > > > > > >
> > > > > > > > ----------------------------------------------------------
> > > > > > > > --
> > > > > > > > ----
> > > > > > > > ----
> > > > > > > > --
> > > > > > > > -------------------------------
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > >I suspect this check and option needs to be moved into the
> > > > > > > >individual sensors,
> > > > > > so that we can differentiate between "should be settable in a
> > > > > > test context" and "should be settable in a normal context".
> > > > > > > 1. Does you mean don't change the Intel definition and keep
> > > > > > > the origin code
> > > > > > when compile time?
> > > > > >
> > > > > > No, this means that the checking code needs to move from
> > > > > > redfish into dbus-sensors.
> > > > > >
> > > > > > > 2. What do you mean this option needs to be moved into the
> > > > > > > individual sensors
> > > > > > so that we can differentiate between "should be settable in a
> > > > > > test context" and "should be settable in a normal context".
> > > > > > > Please provide more details about your thinking.
> > > > > >
> > > > > > Individual sensors need to provide an appropriate dbus interface.
> > > > > > Part of that is enforcing whether or not they're writable, and
> > > > > > checking for the debug state of the system to do so.
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Phase 2 is to add a condition to check the sensor name’s
> > > > > > > > Mutable value of
> > > > > > EM if the value is true do the sensor override function else not do.
> > > > > > >
> > > > > > > >I suspect this patchset needs to be moved forward if you're
> > > > > > > >hoping to use the
> > > > > > mutable param:
> > > > > > > >https://apc01.safelinks.protection.outlook.com/?url=https%3
> > > > > > > >A%
> > > > > > > >2F%252
> > > > > > > >Fger
> > > > > > >
> > > > > >
> > > >
> > >rit.openbmc-project.xyz%2Fc%2Fopenbmc%2Fphosphor-dbus-interfaces%2F
> > > > > > >%
> > > > > > 2
> > > > > > >
> > > > > >
> > > >
> >
> >B%2F36333&data=04%7C01%25>7CBruce_Lee%40quantatw.com%7C64
> > > > > > a1153cd4
> > > > > > >
> > > > > >
> > > >
> >
> >5b46eeca4008d8e57df35c%7C179b032707fc4973ac738de7313561b2%7C1
> > > > > > %7C0%7C6
> > > > > > >
> > > > > >
> > > >
> >
> >37511676404227113%7CUnknown%>7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> > > > > > MDAiLCJQIj
> > > > > > >
> > > > > >
> > > >
> >
> >oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tdExxB%
> > > > > > 2BY7
> > > > > > >
> > >O1cKb%2FYMdvPGnw7YThW7J55jytDPh4YWYo%3D&reserved=0
> > > > > >
> > > > > > Not quite, but close. I wouldn't expect the configurability
> > > > > > to be directly configurable. External sensor types should be
> > > > > > mutable, all other types should not be mutable (except in a
> > > > > > debug context). I don't think there's any reason to add a
> > > > > > separate
> > "IsMutable"
> > > > > > parameter into the EM json, unless it really needs to be
> > > > > > configurable per
> > > > sensor, which I don't think is the case.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > The Mutable value can be set in the sensor configuration
> > > > > > > > of Entity-Manage,
> > > > > > when using the patch command to override the sensor, it needs
> > > > > > to check the EntityManager subtree’s sensor name and its
> > > > > > interface “xyz.openbmc_project.Configuration.I2CFan.Connector”
> > > > > > to check the corresponding property name’s mutable value to
> > > > > > decide whether executing the override function.
> > > > > > >
> > > > > > > >See above. I suspect that the redfish code doesn't need to
> > > > > > > >check the
> > > > > > mutability of the sensor, the interface should just have the
> > > > > > correct
> > behavior.
> > > > > > The only place I would expect to need to know the >mutability
> > > > > > of a sensor is in the IPMI sdr, where we will need to set the
> > > > > > modifiable bit
> > > > appropriately.
> > > > > > >
> > > > > > > For now, the function to set sensor in redfish code is to
> > > > > > > set the d-bus value
> > > > > > directly (internally writable), if we don't check the EM
> > > > > > mutability in Redfish, follow the Add Mutable property to
> > > > > > Sensor Value interface, we still need to check the sensor
> > > > > > mutable property to know whether or not to write the d-bus
> > > > > > value in redfish or we need other external services to know
> > > > > > whether or not to grant write permission to
> > > > their users like IPMI sensor.
> > > > > >
> > > > > > I'm not really following this. My point is that the only
> > > > > > thing that really needs to "check" the mutability requirement is
> dbus-sensors.
> > > > > > They should only allow setting when sensors are mutable, and
> > > > > > reject when they're not.
> > > > > >
> > > > >
> > > > > IPMI has an external service to check the Mutability and allow
> > > > > setting when it is
> > > > "Write" and reject when it's "not write".
> > > >
> > > > I think we're talking past eachother a little. My point is that
> > > > dbus should allow setting, and enforce the check, not IPMI. That
> > > > means that we don't have to duplicate the is settable logic
> > > > between IPMI and
> > Redfish.
> > > >
> > > > > Today if we add a mutable property in the d-bus sensor, we also
> > > > > need an
> > > > external-service like IPMI sensor-handler to check the mutable
> > > > value to grant write access or not.
> > > > > You stated that "They should only allow setting when sensors are
> > > > > mutable and
> > > > reject when they're not." are means an external-service as I mention
> above?
> > > >
> > > > I'm not following what external service would be needed in this case.
> > > > Sensors know their specific type, and the only type of sensor that
> > > > should be settable at this point is external sensor, so we can
> > > > just encode that logic into the sensor system.
> > > >
> > >
> > > If we can distinguish an external user then we can use mutable
> > > property to
> > encode that logic into the sensor system.
> > > But how to distinguish an external user If no need external service,
> > > how to
> > know user is from IPMI, Redfish, or console itself?
> > >
> >
> > No need to distinguish external users from internal users in this
> > case. If a sensor is settable, it doesn't matter if it's being set
> > from within the bmc or outside the BMC.
> >
> > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > This achieves feature parity with the
> > > > > > > > ipmi::sensor::Mutability parameter of the old hardcoded
> > > > > > > > YAML configuration files
> > > > > > >
> > > > > > > >Not sure what you're referring to. That may have been
> > > > > > > >something done in a
> > > > > > fork.
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Execute steps:
> > > > > > > >
> > > > > > > > 1. Patch command to override sensor.
> > > > > > > >
> > > > > > > > 2. Check the EM of sensor’s Mutable value
> > > > > > > >
> > > > > > > > 3. If Mutable value is true do sensor override action else not
> > do.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Phase 3 is to add a new get command to get the Zone_$id’s
> > > > > > > > "Manual" value
> > > > > > and patch command to change the fan mode from auto to manual
> > > > > > mode ("Manual":true).
> > > > > > > >
> > > > > > > > Because the fan control is use package
> > > > > > > > phosphor-pid-control, when we need
> > > > > > to set fan pwm, it needs to set the fan mode from auto mode to
> > > > > > manual mode, for now, the phosphor-pid-control has already
> > > > > > provided ipmi-oem command to achieve this feature, so we need
> > > > > > to implement this fan mode change via redfish command.
> > > > > > >
> > > > > > > >Doesn't this already work today? I thought we had all that
> > > > > > > >sorted a long
> > > > > > time ago. For some reason I thought we intentionally didn't
> > > > > > expose the manual/automatic param, because that only applied
> > > > > > to
> > > > > > >the PID loops, and PWM sensor didn't expose that interface.
> > > > > > >I
> > > > > > need to go look at the code at some point.
> > > > > > >
> > > > > > > Yes, ipmi-oem is work today. I agree it is not properly to
> > > > > > > show on redfish to let
> > > > > > users can easily change the fan mode, the reason to change fan
> > > > > > mode to the manual is for debugging. Maybe let users use
> > > > > > ipmi-oem to replace show on Redfish URLs.
in bmcweb, there is no interface about xyz.openbmc_project.State.FanCtrl, the interface is to set fan zone to manual/auto mode, if no this interface the fan PWM setting will be covered the value due to its default mode is auto.
so we propose to add an OEM action to change all fan zone to manual or auto mode.
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Example URLs |Method
> > |Example
> > > > > > Payload
> > > > > > > >
> > > > > > > > --------------------------------------- |--------------
> > > > > > > > |--
> > > > > > > >
> > > > > > > > /redfish/v1/Managers/bmc |GET |"Oem": {
> > > > > > > >
> > > > > > > > |
> > > > > > | Fan": {
> > > > > > > >
> > > > > > > > |
> > > > > > | "FanZones": {
> > > > > > > >
> > > > > > > > |
> > > > > > | "@odata.id":
> > > > > > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones",
> > > > > > > >
> > > > > > > > |
> > > > > > | "@odata.type":
> > > > > > | "#OemManager.FanZones",
> > > > > > > >
> > > > > > > > |
> > > > > > | "Zone_0": {
> > > > > > > >
> > > > > > > > |
> > > > > > | "@odata.id":
> > > > > > "/redfish/v1/Managers/bmc#/Oem/OpenBmc/Fan/FanZones/Zone_0",
> > > > > > > >
> > > > > > > > |
> > > > > > | "@odata.type":
> > > > > > "#OemManager.FanZone",
> > > > > > > >
> > > > > > > > |
> > > > > > | "Chassis": {
> > > > > > > >
> > > > > > > > |
> > > > > > |
> > "@odata.id":
> > > > > > "/redfish/v1/Chassis/GSZ_EVT"
> > > > > > > >
> > > > > > > > |
> > > > > > | },
> > > > > > > >
> > > > > > > > |
> > > > > > | "FailSafePercent":
> > > > > > | 100.0,
> > > > > > > >
> > > > > > > > |
> > > > > > | "MinThermalOutput":
> > > > > > | 0.0,
> > > > > > > >
> > > > > > > > |
> > > > > > | "ZoneIndex": 0.0,
> > > > > > > >
> > > > > > > > |
> > > > > > | "Manual":false
> > > > > > > >
> > > > > > > > |
> > > > > > | },
> > > > > > > >
> > > > > > > > |
> > > > > > | },
> > > > > > > >
> > > > > > > > |
> > > > > > | },
> > > > > > > >
> > > > > > > > |
> > > > > > |}
> > > > > > > >
> > > > > > > > --------------------------------------- |--------------
> > > > > > > > |----
> > > > > > > >
> > > > > > > > /redfish/v1/Managers/bmc | PATCH |"Oem": { "Fan":
> > > > > > { "FanZones": { "Zone_0": { "Manual":true } } }
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > >It should be noted, this schema needs some serious cleanup
> > > > > > > >to make it proper
> > > > > > resources, paths, and collections, and should version the
> > > > > > schema files appropriately. If you're planning on extending
> > > > > > it, I ?>would expect _some_ effort to be put into cleanup.
> > > > > > There's several github bugs that have more details, and I will
> > > > > > leave it up to you to decide how much you'd like to do as part
> > > > > > of this work, but
> > please >plan on some.
> > > > > > >
>
> After discussing with Nan and follow up the thread on Chat, conclusion has been
> reached bellow:
> 1. Remove the "If in validation mode" check from bmcweb 2. Add the "If in
> validation mode" check to dbus-sensors, excluding external sensor and fan
> sensor (the two that should be settable all the time) 3. Propose the mutability
> interface to phosphor-dbus-interfaces upstream.
> 4. Add a patch to Willys ipmi-dynamic option that adds mutability support to the
> sdr, and set sensor command support. Use google-ipmi-dynamic as a reference.
>
> Here is a new proposal with working details:
> in bmcweb
> 1. Remove Intel stuff and only keep setSensorsOverride function.
> setSensorsOverride(sensorAsyncResp, allCollections); 2. Change
> setSensorsOverride function, change to call dbus-sensor for replacing directly to
> set the dbus value.
>
> in dbus-sensors
> 1. Add the "If in validation mode" check to dbus-sensors, excluding external
> sensor and fan sensor (the two that should be settable all the time)
>
> others
> 1. Propose the mutability interface to phosphor-dbus-interfaces upstream.
>
>
> > > > > > > >
> > > > > > > > If any thoughts on this topic, feel free to give your comments.
> Thanks!
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > Sincerely,
> > > > > > > >
> > > > > > > > Bruce
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-03-31 6:47 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12 6:37 Fan PWM settings via Redfish Bruce Lee (李昀峻)
2021-03-12 17:40 ` Ed Tanous
2021-03-16 8:34 ` Bruce Lee (李昀峻)
2021-03-16 9:34 ` Bruce Lee (李昀峻)
2021-03-16 15:18 ` Ed Tanous
2021-03-17 10:17 ` Bruce Lee (李昀峻)
2021-03-17 15:52 ` Ed Tanous
2021-03-18 9:24 ` Bruce Lee (李昀峻)
2021-03-18 16:08 ` Ed Tanous
2021-03-25 7:28 ` Bruce Lee (李昀峻)
2021-03-31 6:46 ` Bruce Lee (李昀峻)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).