Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190301192156.253010-1-ravisadineni@chromium.org> <CABXOdTdXcCeqkqcH69dG62mLmurbDOcrhhVGm6BgnBzR-1KETg@mail.gmail.com>
In-Reply-To: <CABXOdTdXcCeqkqcH69dG62mLmurbDOcrhhVGm6BgnBzR-1KETg@mail.gmail.com>
From:   Ravi Chandra Sadineni <ravisadineni@chromium.org>
Date:   Sun, 3 Mar 2019 17:02:42 -0800
Message-ID: <CAEZbON7ugRGpFuj6mwHoSqdyYNFyNKEsGkhFjV7XuwsW28JXcQ@mail.gmail.com>
Subject: Re: [PATCH] cros_ec: Expose sysfile to force battery cut-off on shutdown.
To:     Guenter Roeck <groeck@google.com>
Cc:     Benson Leung <bleung@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Guenter Roeck <groeck@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Todd Broch <tbroch@google.com>,
        Daisuke Nojiri <dnojiri@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Guenter,

On Fri, Mar 1, 2019 at 1:00 PM Guenter Roeck <groeck@google.com> wrote:
>
> On Fri, Mar 1, 2019 at 11:22 AM RaviChandra Sadineni <ravisadineni@chromium.org> wrote:
>>
>> On chromebooks, power_manager daemon normally shutsdown(S5) the device
>> when the battery charge falls below 4% threshold. ChromeOS EC then
>> normally spends an hour in S5 before hibernating. If the battery charge
>> falls below critical threshold in the mean time, EC does a battery cutoff
>> instead of hibernating. On some chromebooks, S5 is optimal enough
>> resulting in EC hibernating without battery cut-off. This results in
>> battery deep-discharging. This is a bad user experience as battery
>> has to trickle charge before booting when the A.C is plugged in the next
>> time.
>>
>> This patch exposes a sysfs file for an userland daemon to suggest EC if it
>> has to do a battery cut-off instead of hibernating when the system enters
>> S5 next time.
>>
>> Signed-off-by: RaviChandra Sadineni <ravisadineni@chromium.org>
>> ---
>>  drivers/platform/chrome/cros_ec_sysfs.c | 34 +++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
>> index f34a50121064..151c7c143941 100644
>> --- a/drivers/platform/chrome/cros_ec_sysfs.c
>> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
>> @@ -322,14 +322,48 @@ static ssize_t kb_wake_angle_store(struct device *dev,
>>         return count;
>>  }
>>
>> +/* Battery cut-off control */
>> +static ssize_t cutoff_battery_at_shutdown_store(struct device *dev,
>> +                                  struct device_attribute *attr,
>> +                                  const char *buf, size_t count)
>> +{
>> +       struct ec_params_battery_cutoff *param;
>> +       struct cros_ec_command *msg;
>> +       int ret;
>> +       struct cros_ec_dev *ec = container_of(
>> +                       dev, struct cros_ec_dev, class_dev);
>> +       uint8_t cutoff_battery;
>> +
>> +       if (kstrtou8(buf, 10, &cutoff_battery) || (cutoff_battery != 1))
>
>
> Excessive ( ). Also, why not kstrtobool() ?
Done.
>
>>
>> +               return -EINVAL;
>> +
>>
>> +       msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
>> +       if (!msg)
>> +               return -ENOMEM;
>> +
>> +       param = (struct ec_params_battery_cutoff *)msg->data;
>> +       msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
>> +       msg->version = 1;
>> +       param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
>
>
> For some reason I fail to see where cutoff_battery is used to determine what to send to the EC. Am I missing something ?
Currently there is no way to reset the flag. i.e EC does not expose a
console command to reset the flag once set. So I intend to accept only
true bool flag. Is there a better way to do this ?
>
>>
>> +       msg->outsize = sizeof(*param);
>> +       msg->insize = 0;
>> +       ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
>> +       kfree(msg);
>> +       if (ret < 0)
>> +               return ret;
>> +       return count;
>> +}
>> +
>>  /* Module initialization */
>>
>>  static DEVICE_ATTR_RW(reboot);
>>  static DEVICE_ATTR_RO(version);
>>  static DEVICE_ATTR_RO(flashinfo);
>>  static DEVICE_ATTR_RW(kb_wake_angle);
>> +static DEVICE_ATTR_WO(cutoff_battery_at_shutdown);
>>
>
> I personally dislike excessively long sysfs attribute names. I would prefer to see a shorter name and have it documented in Documentation/ABI/testing/sysfs-class-chromeos.
>
> Is WO really desirable and warranted here ?
Currently EC does not expose a host command to read the status.
Maintaining the state in the kernel might not work as the state can go
out of sync. For example, when AC is plugged in, EC resets the flag
and the kernel will not be aware of it.
>
>>
>>  static struct attribute *__ec_attrs[] = {
>> +       &dev_attr_cutoff_battery_at_shutdown.attr,
>>         &dev_attr_kb_wake_angle.attr,
>>         &dev_attr_reboot.attr,
>>         &dev_attr_version.attr,
>> --
>> 2.20.1
>>

Thanks,
Ravi