Hi Jayanath,<div><br></div><div>Find my comments inline.</div><div><br></div><div>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.</div><div><br></div><div>Emily</div><div><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 27, 2018, 6:22 AM Ratan Gupta <<a href="mailto:ratagupt@linux.vnet.ibm.com">ratagupt@linux.vnet.ibm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
Please find my comments inline<br>
<br>
Regards<br>
<br>
Ratan Gupta<br>
<br>
<br>
<br>
On Tuesday 27 February 2018 04:51 PM, Jayanth Othayoth wrote:<br>
> I am working on an issue openbmc/openbmc#2500 (Add support for Turbo<br>
> allowed sensor). As part of this issue implementation , there is a<br>
> requirement  to change this IPMI sensor to read only for host.<br>
> Currently all sensors are read/write by design and doesn’t have way to<br>
> change this behavior. Planing to change this behavior by adding<br>
> “Mutability” field in the config.yaml.<br></blockquote></div><div>ES: The Mutability field already exists in sensor config yaml. <a href="https://github.com/openbmc/phosphor-host-ipmid/blob/2e12a31f39d95c381a466e277e510faa02c351fb/scripts/writesensor.mako.cpp#L68">https://github.com/openbmc/phosphor-host-ipmid/blob/2e12a31f39d95c381a466e277e510faa02c351fb/scripts/writesensor.mako.cpp#L68</a></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> As part of this issue am planing to  Initialize  mutability with<br>
> Mutability::Write|Mutability::Read for all sensors and set the<br>
> required sensor mutability Mutability::Read.<br></blockquote></div><div dir="auto">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).</div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
>       - Minimal impact on testing and not breaking the existing<br>
> functionality.<br>
RG>>How we would be doing it? If we are planning to do introduce a check<br>
for a specific sensor then we should not do like this.<br>
In the current design we have not introduced any such checks and we<br>
should avoid that.<br>
Please correct me if I misunderstood it.<br>
><br>
> Follow-on story is needed to fix this in generic way like<br>
>   -Add “mutable” field in the config.yaml and initialize the the field<br></blockquote></div><div dir="auto">ES: Today it's defaulted to read-only for all uninitialized sensors.</div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> based on System types also update<br>
> sensorhandler.cpp->ipmi_sen_set_sensor() to do set only for write<br>
> enabled sensor.<br>
>     - This required system specific IPMI sensor behavior review and<br>
> updates in corresponding config.yaml.<br>
RG>> I agree with this approach.What do you mean by "System types"<br>
here,We have already system specfic YAML in the system specfic meta layers.<br>
<br>
><br>
> Please add your view on this.<br>
<br>
</blockquote></div></div>