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 <[email protected]>
---
V2: Use kstrtobool() instead of kstrtou8() and add documentation.
.../ABI/testing/sysfs-class-chromeos | 8 ++++
drivers/platform/chrome/cros_ec_sysfs.c | 37 +++++++++++++++++++
2 files changed, 45 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
new file mode 100644
index 000000000000..44d3cee6e7ae
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-chromeos
@@ -0,0 +1,8 @@
+What: /sys/class/chromeos/cros_ec/cutoff_at_shutdown
+Date: February 2019
+Contact: Ravi Chandra Sadineni <[email protected]>
+Description:
+ On writing '[Yy1]' to cutoff_at_shutdown property,
+ kernel sends a host command to EC requesting battery
+ cutoff on next shutdown. If AC is plugged in before
+ next shutdown, EC resets this flag.
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index f34a50121064..f5168ce8bfc7 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -322,14 +322,51 @@ static ssize_t kb_wake_angle_store(struct device *dev,
return count;
}
+/* Battery cut-off control */
+static ssize_t cutoff_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);
+ bool cutoff_battery;
+
+ if (kstrtobool(buf, &cutoff_battery))
+ return -EINVAL;
+
+ if (!cutoff_battery)
+ return count;
+
+ 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;
+ 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_at_shutdown);
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
Hi RaviChandra,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.0]
[cannot apply to next-20190304]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/RaviChandra-Sadineni/cros_ec-Expose-sysfile-to-force-battery-cut-off-on-shutdown/20190304-220513
config: x86_64-randconfig-x017-201909 (attached as .config)
compiler: gcc-8 (Debian 8.2.0-21) 8.2.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> drivers/platform/chrome/cros_ec_sysfs.c:369:3: error: 'dev_attr_cutoff_battery_at_shutdown' undeclared here (not in a function); did you mean 'dev_attr_cutoff_at_shutdown'?
&dev_attr_cutoff_battery_at_shutdown.attr,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
dev_attr_cutoff_at_shutdown
In file included from drivers/platform/chrome/cros_ec_sysfs.c:24:
include/linux/device.h:602:26: warning: 'dev_attr_cutoff_at_shutdown' defined but not used [-Wunused-variable]
struct device_attribute dev_attr_##_name = __ATTR_WO(_name)
^~~~~~~~~
drivers/platform/chrome/cros_ec_sysfs.c:366:8: note: in expansion of macro 'DEVICE_ATTR_WO'
static DEVICE_ATTR_WO(cutoff_at_shutdown);
^~~~~~~~~~~~~~
vim +369 drivers/platform/chrome/cros_ec_sysfs.c
367
368 static struct attribute *__ec_attrs[] = {
> 369 &dev_attr_cutoff_battery_at_shutdown.attr,
370 &dev_attr_kb_wake_angle.attr,
371 &dev_attr_reboot.attr,
372 &dev_attr_version.attr,
373 &dev_attr_flashinfo.attr,
374 NULL,
375 };
376
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi RaviChandra,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.0]
[cannot apply to next-20190301]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/RaviChandra-Sadineni/cros_ec-Expose-sysfile-to-force-battery-cut-off-on-shutdown/20190304-220513
config: nds32-allyesconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 6.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=6.4.0 make.cross ARCH=nds32
All error/warnings (new ones prefixed by >>):
>> drivers/platform/chrome/cros_ec_sysfs.c:369:3: error: 'dev_attr_cutoff_battery_at_shutdown' undeclared here (not in a function)
&dev_attr_cutoff_battery_at_shutdown.attr,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/platform/chrome/cros_ec_sysfs.c:24:0:
include/linux/device.h:602:26: warning: 'dev_attr_cutoff_at_shutdown' defined but not used [-Wunused-variable]
struct device_attribute dev_attr_##_name = __ATTR_WO(_name)
^
>> drivers/platform/chrome/cros_ec_sysfs.c:366:8: note: in expansion of macro 'DEVICE_ATTR_WO'
static DEVICE_ATTR_WO(cutoff_at_shutdown);
^~~~~~~~~~~~~~
vim +/dev_attr_cutoff_battery_at_shutdown +369 drivers/platform/chrome/cros_ec_sysfs.c
361
362 static DEVICE_ATTR_RW(reboot);
363 static DEVICE_ATTR_RO(version);
364 static DEVICE_ATTR_RO(flashinfo);
365 static DEVICE_ATTR_RW(kb_wake_angle);
> 366 static DEVICE_ATTR_WO(cutoff_at_shutdown);
367
368 static struct attribute *__ec_attrs[] = {
> 369 &dev_attr_cutoff_battery_at_shutdown.attr,
370 &dev_attr_kb_wake_angle.attr,
371 &dev_attr_reboot.attr,
372 &dev_attr_version.attr,
373 &dev_attr_flashinfo.attr,
374 NULL,
375 };
376
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Guenter,
On Sun, Mar 3, 2019 at 5:13 PM Guenter Roeck <[email protected]> wrote:
>
> On Sun, Mar 3, 2019 at 4:54 PM RaviChandra Sadineni <[email protected]> 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 <[email protected]>
>> ---
>> V2: Use kstrtobool() instead of kstrtou8() and add documentation.
>>
>> .../ABI/testing/sysfs-class-chromeos | 8 ++++
>> drivers/platform/chrome/cros_ec_sysfs.c | 37 +++++++++++++++++++
>> 2 files changed, 45 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
>> new file mode 100644
>> index 000000000000..44d3cee6e7ae
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-chromeos
>> @@ -0,0 +1,8 @@
>> +What: /sys/class/chromeos/cros_ec/cutoff_at_shutdown
>> +Date: February 2019
>> +Contact: Ravi Chandra Sadineni <[email protected]>
>> +Description:
>> + On writing '[Yy1]' to cutoff_at_shutdown property,
>> + kernel sends a host command to EC requesting battery
>> + cutoff on next shutdown. If AC is plugged in before
>> + next shutdown, EC resets this flag.
>> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
>> index f34a50121064..f5168ce8bfc7 100644
>> --- a/drivers/platform/chrome/cros_ec_sysfs.c
>> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
>> @@ -322,14 +322,51 @@ static ssize_t kb_wake_angle_store(struct device *dev,
>> return count;
>> }
>>
>> +/* Battery cut-off control */
>> +static ssize_t cutoff_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);
>> + bool cutoff_battery;
>> +
>> + if (kstrtobool(buf, &cutoff_battery))
>> + return -EINVAL;
>> +
>> + if (!cutoff_battery)
>> + return count;
>> +
>
>
> It is quite unusual to only be able to set this option, but not to be able to reset it. I think that warrants an explanation and reasoning. Also, if clearing the flag is not supported, I would expect an attempt to do so to return an error. There should also be an explanation in the code explaining why the attribute is write-only.
Added documentation for the reason behind making this flag write only.
Instead of taking a boolean value, made this attribute more generic
and took 'at-shutdown' as an option. This way it will be easy to
extend this sysfs api in the future (if EC exposes more attributes for
this host command).
>
> Guenter
>
>>
>> + 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;
>> + 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_at_shutdown);
>>
>> 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
Hi Daisuke,
On Tue, Mar 5, 2019 at 9:29 AM Daisuke Nojiri <[email protected]> wrote:
>>
>> + if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
>> + param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
>> + } else {
>> + kfree(msg);
>> + return -EINVAL;
>> + }
>
>
> Why don't we just let the EC decide what flag is legal or illegal? If you hard code 'at-shutdown', other calls to the sysfs will be blocked even if the EC supports other flags.
That might not be a wise option. Allowing random values can have
unintended effects. For example consider what would happen if write
random values to EC today. Any flag value other than
EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN will cause the battery to cutoff
immediately before the system shutdowns.
In future, if EC exposes more attributes, it will not be that
difficult to add support here.
>
> On Mon, Mar 4, 2019 at 5:00 PM Ravi Chandra Sadineni <[email protected]> wrote:
>>
>> Hi Guenter,
>>
>> On Sun, Mar 3, 2019 at 5:13 PM Guenter Roeck <[email protected]> wrote:
>> >
>> > On Sun, Mar 3, 2019 at 4:54 PM RaviChandra Sadineni <[email protected]> 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 <[email protected]>
>> >> ---
>> >> V2: Use kstrtobool() instead of kstrtou8() and add documentation.
>> >>
>> >> .../ABI/testing/sysfs-class-chromeos | 8 ++++
>> >> drivers/platform/chrome/cros_ec_sysfs.c | 37 +++++++++++++++++++
>> >> 2 files changed, 45 insertions(+)
>> >> create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
>> >>
>> >> diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
>> >> new file mode 100644
>> >> index 000000000000..44d3cee6e7ae
>> >> --- /dev/null
>> >> +++ b/Documentation/ABI/testing/sysfs-class-chromeos
>> >> @@ -0,0 +1,8 @@
>> >> +What: /sys/class/chromeos/cros_ec/cutoff_at_shutdown
>> >> +Date: February 2019
>> >> +Contact: Ravi Chandra Sadineni <[email protected]>
>> >> +Description:
>> >> + On writing '[Yy1]' to cutoff_at_shutdown property,
>> >> + kernel sends a host command to EC requesting battery
>> >> + cutoff on next shutdown. If AC is plugged in before
>> >> + next shutdown, EC resets this flag.
>> >> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
>> >> index f34a50121064..f5168ce8bfc7 100644
>> >> --- a/drivers/platform/chrome/cros_ec_sysfs.c
>> >> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
>> >> @@ -322,14 +322,51 @@ static ssize_t kb_wake_angle_store(struct device *dev,
>> >> return count;
>> >> }
>> >>
>> >> +/* Battery cut-off control */
>> >> +static ssize_t cutoff_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);
>> >> + bool cutoff_battery;
>> >> +
>> >> + if (kstrtobool(buf, &cutoff_battery))
>> >> + return -EINVAL;
>> >> +
>> >> + if (!cutoff_battery)
>> >> + return count;
>> >> +
>> >
>> >
>> > It is quite unusual to only be able to set this option, but not to be able to reset it. I think that warrants an explanation and reasoning. Also, if clearing the flag is not supported, I would expect an attempt to do so to return an error. There should also be an explanation in the code explaining why the attribute is write-only.
>> Added documentation for the reason behind making this flag write only.
>> Instead of taking a boolean value, made this attribute more generic
>> and took 'at-shutdown' as an option. This way it will be easy to
>> extend this sysfs api in the future (if EC exposes more attributes for
>> this host command).
>> >
>> > Guenter
>> >
>> >>
>> >> + 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;
>> >> + 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_at_shutdown);
>> >>
>> >> 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
Thanks,
Ravi