IPMI Sensor Read-only role updates.

[Emily Shaffer]

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>
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

> >
> > 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.
> >
> > 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/20180227/e744ef08/attachment-0001.html>