Messages in this thread Patch in this message | | | From | Ravi Chandra Sadineni <> | Date | Tue, 5 Mar 2019 09:59:29 -0800 | Subject | Re: [PATCH V2] cros_ec: Expose sysfile to force battery cut-off on shutdown. |
| |
Hi Daisuke,
On Tue, Mar 5, 2019 at 9:29 AM Daisuke Nojiri <dnojiri@google.com> 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 <ravisadineni@chromium.org> wrote: >> >> Hi Guenter, >> >> On Sun, Mar 3, 2019 at 5:13 PM Guenter Roeck <groeck@google.com> wrote: >> > >> > On Sun, Mar 3, 2019 at 4:54 PM 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> >> >> --- >> >> 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 <ravisadineni@chromium.org> >> >> +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
| |