<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Hi Emily,</p>
<p>Minor correction inline.</p>
<p>Regards</p>
<p>Ratan <br>
</p>
<div class="moz-cite-prefix">On Tuesday 27 February 2018 09:06 PM,
Emily Shaffer wrote:<br>
</div>
<blockquote
cite="mid:CAJoAoZn91rnQaB0p_hsJm6Vg4KPSxYxBQH0JAgKV_-Fo-yvqXw@mail.gmail.com"
type="cite">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
moz-do-not-send="true"
href="mailto:ratagupt@linux.vnet.ibm.com"><a class="moz-txt-link-abbreviated" href="mailto:ratagupt@linux.vnet.ibm.com">ratagupt@linux.vnet.ibm.com</a></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 moz-do-not-send="true"
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>
</blockquote>
RG>> Here in mako we always set the mutability property to
READ as config YAML doesn't have the Mutability property.<br>
<a class="moz-txt-link-freetext" href="https://github.com/openbmc/openbmc/blob/master/meta-phosphor/common/recipes-phosphor/ipmi/phosphor-ipmi-sensor-inventory-mrw-config/config.yaml">https://github.com/openbmc/openbmc/blob/master/meta-phosphor/common/recipes-phosphor/ipmi/phosphor-ipmi-sensor-inventory-mrw-config/config.yaml</a><br>
<br>
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. <br>
<br>
<blockquote
cite="mid:CAJoAoZn91rnQaB0p_hsJm6Vg4KPSxYxBQH0JAgKV_-Fo-yvqXw@mail.gmail.com"
type="cite">
<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>
</blockquote>
</div>
</div>
</blockquote>
RG>>if we add the Mutability property in the config.yaml
with configured values as READ and WRITE for all other sensor<br>
Except this one(turboAllowedSensor) which is required to
be READ<br>
In this sense we are not breaking the current code as
the interpretation in the code for all the sensor is READ and WRITE<br>
<blockquote
cite="mid:CAJoAoZn91rnQaB0p_hsJm6Vg4KPSxYxBQH0JAgKV_-Fo-yvqXw@mail.gmail.com"
type="cite">
<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">
><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>
</blockquote>
<br>
</body>
</html>