Fan PWM settings via Redfish
Ed Tanous
edtanous at google.com
Fri Mar 19 03:08:32 AEDT 2021
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%
> > > > > > 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
More information about the openbmc
mailing list