Fan PWM settings via Redfish
Bruce Lee (李昀峻)
Bruce_Lee at quantatw.com
Thu Mar 25 18:28:57 AEDT 2021
> -----Original Message-----
> From: Ed Tanous <edtanous at google.com>
> Sent: Friday, March 19, 2021 12:09 AM
> To: Bruce Lee (李昀峻) <Bruce_Lee at quantatw.com>
> Cc: Nan Zhou <nanzhou at google.com>; rhanley at google.com;
> openbmc at lists.ozlabs.org
> Subject: Re: Fan PWM settings via Redfish
>
> On Thu, Mar 18, 2021 at 2:24 AM Bruce Lee (李昀峻)
> <Bruce_Lee at quantatw.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Ed Tanous <edtanous at google.com>
> > > Sent: Wednesday, March 17, 2021 11:53 PM
> > > To: Bruce Lee (李昀峻) <Bruce_Lee at quantatw.com>
> > > Cc: Nan Zhou <nanzhou at google.com>; rhanley at google.com;
> > > openbmc at lists.ozlabs.org
> > > Subject: Re: Fan PWM settings via Redfish
> > >
> > > On Wed, Mar 17, 2021 at 3:17 AM Bruce Lee (李昀峻)
> > > <Bruce_Lee at quantatw.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ed Tanous <edtanous at google.com>
> > > > > Sent: Tuesday, March 16, 2021 11:18 PM
> > > > > To: Bruce Lee (李昀峻) <Bruce_Lee at quantatw.com>
> > > > > Cc: Nan Zhou <nanzhou at google.com>; rhanley at google.com;
> > > > > openbmc at lists.ozlabs.org
> > > > > Subject: Re: Fan PWM settings via Redfish
> > > > >
> > > > > On Tue, Mar 16, 2021 at 2:35 AM Bruce Lee (李昀峻)
> > > > > <Bruce_Lee at quantatw.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ed Tanous <edtanous at google.com>
> > > > > > Sent: Saturday, March 13, 2021 1:40 AM
> > > > > > To: Bruce Lee (李昀峻) <Bruce_Lee at quantatw.com>
> > > > > > Cc: Nan Zhou <nanzhou at google.com>; rhanley at google.com;
> > > > > > openbmc at lists.ozlabs.org
> > > > > > Subject: Re: Fan PWM settings via Redfish
> > > > > >
> > > > > > On Thu, Mar 11, 2021 at 10:37 PM Bruce Lee (李昀峻)
> > > > > <Bruce_Lee at 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
More information about the openbmc
mailing list