IPMI Sensor Read-only role updates.

[Ratan Gupta]

Hi Emily,

Minor correction inline.

Regards

Ratan

On Tuesday 27 February 2018 09:06 PM, Emily Shaffer wrote:
> Hi Jayanath,
>
> Find my comments inline.
>
> It seems you may be thinking of only changes for a system specific 
> yaml, in which case I don't think you need to ask for permission :) 
> But the architecture you're discussing is already there today.
>
> Emily
>
> On Tue, Feb 27, 2018, 6:22 AM Ratan Gupta <ratagupt at linux.vnet.ibm.com 
> <mailto:ratagupt at linux.vnet.ibm.com>> wrote:
>
>     Hi,
>
>     Please find my comments inline
>
>     Regards
>
>     Ratan Gupta
>
>
>
>     On Tuesday 27 February 2018 04:51 PM, Jayanth Othayoth wrote:
>     > I am working on an issue openbmc/openbmc#2500 (Add support for Turbo
>     > allowed sensor). As part of this issue implementation , there is a
>     > requirement  to change this IPMI sensor to read only for host.
>     > Currently all sensors are read/write by design and doesn’t have
>     way to
>     > change this behavior. Planing to change this behavior by adding
>     > “Mutability” field in the config.yaml.
>
> ES: The Mutability field already exists in sensor config yaml. 
> https://github.com/openbmc/phosphor-host-ipmid/blob/2e12a31f39d95c381a466e277e510faa02c351fb/scripts/writesensor.mako.cpp#L68
     RG>> Here in mako we always set the mutability property to READ as 
config YAML doesn't have the Mutability property.
https://github.com/openbmc/openbmc/blob/master/meta-phosphor/common/recipes-phosphor/ipmi/phosphor-ipmi-sensor-inventory-mrw-config/config.yaml

     RG>>We have the Mutability check in the getSensorReading but we 
don't have that check in the setSensorReading so we can introduce that 
check there with the required changes in the config.YAML.

>     >
>     > As part of this issue am planing to  Initialize mutability with
>     > Mutability::Write|Mutability::Read for all sensors and set the
>     > required sensor mutability Mutability::Read.
>
> ES: You're saying you'd like to ensure all sensors are readable? This 
> doesn't make sense in some cases, notably fan PWM in some cases where 
> the only read response would be DBus parroting back what we wrote (not 
> an actual value reflecting reality).
>
>     >       - Minimal impact on testing and not breaking the existing
>     > functionality.
>     RG>>How we would be doing it? If we are planning to do introduce a
>     check
>     for a specific sensor then we should not do like this.
>     In the current design we have not introduced any such checks and we
>     should avoid that.
>     Please correct me if I misunderstood it.
>
   RG>>if we add the Mutability property in the config.yaml with 
configured values as READ and WRITE for all other sensor
             Except this one(turboAllowedSensor) which is required to be 
READ
             In this sense we are not breaking the current code as the 
interpretation in the code for all the sensor is READ and WRITE
>
>     >
>     > Follow-on story is needed to fix this in generic way like
>     >   -Add “mutable” field in the config.yaml and initialize the the
>     field
>
> ES: Today it's defaulted to read-only for all uninitialized sensors.
>
>     > based on System types also update
>     > sensorhandler.cpp->ipmi_sen_set_sensor() to do set only for write
>     > enabled sensor.
>     >     - This required system specific IPMI sensor behavior review and
>     > updates in corresponding config.yaml.
>     RG>> I agree with this approach.What do you mean by "System types"
>     here,We have already system specfic YAML in the system specfic
>     meta layers.
>
>     >
>     > Please add your view on this.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20180228/7b00e7f4/attachment-0001.html>