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