[PATCH] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[PATCH] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[RaviChandra Sadineni]

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))
+		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;
+	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);
 
 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

[PATCH V2] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[RaviChandra Sadineni]

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

Re: [PATCH V2] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33044 bytes --]

Re: [PATCH V2] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49917 bytes --]

[PATCH V3] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[RaviChandra Sadineni]

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

V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
V2: Use kstrtobool() instead of kstrtou8() and add documentation.


.../ABI/testing/sysfs-class-chromeos          | 10 ++++
 drivers/platform/chrome/cros_ec_sysfs.c       | 48 +++++++++++++++++++
 2 files changed, 58 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..d5ab22c44977
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-chromeos
@@ -0,0 +1,10 @@
+What:           /sys/class/chromeos/cros_ec/battery-cuttoff
+Date:           February 2019
+Contact:        Ravi Chandra Sadineni <ravisadineni@chromium.org>
+Description:
+                cros_ec battery cuttoff configuration. Only option
+                currently exposed is 'at-shutdown'.
+
+                'at-shutdown' sends a host command to EC requesting
+                battery cutoff on next shutdown. If AC is plugged
+                in before next shutdown, EC ignores the request.
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index f34a50121064..6ef6b860c818 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -322,14 +322,62 @@ static ssize_t kb_wake_angle_store(struct device *dev,
 	return count;
 }
 
+/* Battery cut-off control */
+static ssize_t battery_cutoff_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);
+	char *p;
+	int len;
+
+	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;
+	msg->outsize = sizeof(*param);
+	msg->insize = 0;
+
+	p = memchr(buf, '\n', count);
+	len = p ? p - buf : count;
+
+	if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
+		param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
+	} else {
+		kfree(msg);
+		return -EINVAL;
+	}
+
+	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);
+/*
+ * Currently EC does not expose a host command to read the status of
+ * battery cut-off configuration. Also there is no requirement to read
+ * the status of these flags from userland. So marking this attribute as
+ *  write-only.
+ */
+static DEVICE_ATTR_WO(battery_cutoff);
 
 static struct attribute *__ec_attrs[] = {
+	&dev_attr_battery_cutoff.attr,
 	&dev_attr_kb_wake_angle.attr,
 	&dev_attr_reboot.attr,
 	&dev_attr_version.attr,
-- 
2.20.1
story | grep send-email

[PATCH V3] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[RaviChandra Sadineni]

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

V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
V2: Use kstrtobool() instead of kstrtou8() and add documentation.


.../ABI/testing/sysfs-class-chromeos          | 10 ++++
 drivers/platform/chrome/cros_ec_sysfs.c       | 48 +++++++++++++++++++
 2 files changed, 58 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..d5ab22c44977
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-chromeos
@@ -0,0 +1,10 @@
+What:           /sys/class/chromeos/cros_ec/battery-cuttoff
+Date:           February 2019
+Contact:        Ravi Chandra Sadineni <ravisadineni@chromium.org>
+Description:
+                cros_ec battery cuttoff configuration. Only option
+                currently exposed is 'at-shutdown'.
+
+                'at-shutdown' sends a host command to EC requesting
+                battery cutoff on next shutdown. If AC is plugged
+                in before next shutdown, EC ignores the request.
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index f34a50121064..6ef6b860c818 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -322,14 +322,62 @@ static ssize_t kb_wake_angle_store(struct device *dev,
 	return count;
 }
 
+/* Battery cut-off control */
+static ssize_t battery_cutoff_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);
+	char *p;
+	int len;
+
+	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;
+	msg->outsize = sizeof(*param);
+	msg->insize = 0;
+
+	p = memchr(buf, '\n', count);
+	len = p ? p - buf : count;
+
+	if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
+		param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
+	} else {
+		kfree(msg);
+		return -EINVAL;
+	}
+
+	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);
+/*
+ * Currently EC does not expose a host command to read the status of
+ * battery cut-off configuration. Also there is no requirement to read
+ * the status of these flags from userland. So marking this attribute as
+ *  write-only.
+ */
+static DEVICE_ATTR_WO(battery_cutoff);
 
 static struct attribute *__ec_attrs[] = {
+	&dev_attr_battery_cutoff.attr,
 	&dev_attr_kb_wake_angle.attr,
 	&dev_attr_reboot.attr,
 	&dev_attr_version.attr,
-- 
2.20.1

Re: [PATCH V3] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[Enric Balletbo i Serra]

Hi RaviChandra,

Some few comments below ...


On 5/3/19 1:53, RaviChandra Sadineni 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

Be coherent using the same spelling along all the patch for cutoff.


> instead of hibernating. On some chromebooks, S5 is optimal enough
> resulting in EC hibernating without battery cut-off. This results in

s/cut-off/cutoff/

> 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

s/cut-off/cutoff/

> S5 next time.
> 
> Signed-off-by: RaviChandra Sadineni <ravisadineni@chromium.org>
> ---
> 
> V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
> V2: Use kstrtobool() instead of kstrtou8() and add documentation.
> 
> 
> .../ABI/testing/sysfs-class-chromeos          | 10 ++++
>  drivers/platform/chrome/cros_ec_sysfs.c       | 48 +++++++++++++++++++
>  2 files changed, 58 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..d5ab22c44977
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-chromeos

Please rebase the patch on top of chrome-platform for-next [1] or 5.1-rc1 when
released, so it applies cleanly.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/log/?h=for-next

> @@ -0,0 +1,10 @@
> +What:           /sys/class/chromeos/cros_ec/battery-cuttoff

The name should match with the attribute name, so battery_cutoff in this case as
you defined the attribute name as battery_cutoff

Is this a common property for all the EC devices?

Note that we have

/sys/class/chromeos/<ec-device-name>/...

where <ec-device-name> can be cros_ec|cros_pd and many others in the future

On those devices that more than one EC is present we will have. E.g

 cros_ec/battery_cutoff and cros_pd/battery_cutoff
 cros_ec/battery_cutoff and cros_ish/battery_cutoff

Which I think is not what you want. Ideally I'd like to see visible the
attribute only on those ECs that support that feature. Is this command
associated with an EC feature?

If that's not possible I can assume writing to battery_cutoff for ECs that don't
implement it would return a protocol/command error.


> +Date:           February 2019
> +Contact:        Ravi Chandra Sadineni <ravisadineni@chromium.org>
> +Description:
> +                cros_ec battery cuttoff configuration. Only option
> +                currently exposed is 'at-shutdown'.
> +
> +                'at-shutdown' sends a host command to EC requesting
> +                battery cutoff on next shutdown. If AC is plugged
> +                in before next shutdown, EC ignores the request.
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index f34a50121064..6ef6b860c818 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -322,14 +322,62 @@ static ssize_t kb_wake_angle_store(struct device *dev,
>  	return count;
>  }
>  
> +/* Battery cut-off control */

s/cut-off/cutoff/

> +static ssize_t battery_cutoff_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);

use to_cros_ec_dev instead of container_of

> +	char *p;
> +	int len;
> +
> +	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;
> +	msg->outsize = sizeof(*param);
> +	msg->insize = 0;
> +
> +	p = memchr(buf, '\n', count);
> +	len = p ? p - buf : count;
> +
> +	if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
> +		param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
> +	} else {
> +		kfree(msg);

Use only one kfree, please. Remove this.

> +		return -EINVAL;

                count = -EINVAL;
                goto exit;

> +	}
> +
> +	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> +	kfree(msg);

Only one point for kfree, please. Remove this.

> +	if (ret < 0)
> +		return ret;
            count = ret;

exit:
        kfree(msg);
> +	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);
> +/*
> + * Currently EC does not expose a host command to read the status of
> + * battery cut-off configuration. Also there is no requirement to read

s/cut-off/cutoff/

> + * the status of these flags from userland. So marking this attribute as
> + *  write-only.
> + */

Can you also specify that is WO in the sysfs documentation. I understand
correctly that this command only applies when the device runs on battery? And
that if AC is plugged the flag is reset?

> +static DEVICE_ATTR_WO(battery_cutoff);
>  

Note that the name should match with the sysfs documentation.

>  static struct attribute *__ec_attrs[] = {
> +	&dev_attr_battery_cutoff.attr,
>  	&dev_attr_kb_wake_angle.attr,
>  	&dev_attr_reboot.attr,
>  	&dev_attr_version.attr,
> 

Thanks,
 Enric

[PATCH V4] cros_ec: Expose sysfile to force battery cutoff on shutdown.

[RaviChandra Sadineni]

From: RaviChandra Sadineni <ravisadineni@chromium.org>

On chromebooks, power_manager daemon normally shuts down(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 cutoff. This results in
battery deep discharging. This is a bad user experience as battery
has to trickle charge before booting when the AC 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 cutoff instead of hibernating when the system enters
S5 next time.

Signed-off-by: RaviChandra Sadineni <ravisadineni@chromium.org>
---
V4: Addressed comments from Enric.
V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
V2: Use kstrtobool() instead of kstrtou8() and add documentation.

.../ABI/testing/sysfs-class-chromeos          | 16 +++++++
 drivers/platform/chrome/cros_ec_sysfs.c       | 47 +++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
index 5819699d66ec..0927704d1629 100644
--- a/Documentation/ABI/testing/sysfs-class-chromeos
+++ b/Documentation/ABI/testing/sysfs-class-chromeos
@@ -30,3 +30,19 @@ Date:		August 2015
 KernelVersion:	4.2
 Description:
 		Show the information about the EC software and hardware.
+
+What:           /sys/class/chromeos/<ec-device-name>/battery_cuttoff
+Date:           February 2019
+Contact:        Ravi Chandra Sadineni <ravisadineni@chromium.org>
+Description:
+                cros_ec battery cuttoff configuration. Only option
+                currently exposed is 'at-shutdown'.
+
+                'at-shutdown' sends a host command to EC requesting
+                battery cutoff on next shutdown. If AC is plugged
+                in before next shutdown, EC ignores the request and
+                resets the flag.
+
+                Currently EC does not expose a host command to read
+                the status of battery cutoff configuration. Thus this
+                flag is write-only.
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index fe0b7614ae1b..a35e1188f28f 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -308,14 +308,61 @@ static ssize_t kb_wake_angle_store(struct device *dev,
 	return count;
 }
 
+/* Battery cutoff control */
+static ssize_t battery_cutoff_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 = to_cros_ec_dev(dev);
+	char *p;
+	int len;
+
+	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;
+	msg->outsize = sizeof(*param);
+	msg->insize = 0;
+
+	p = memchr(buf, '\n', count);
+	len = p ? p - buf : count;
+
+	if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
+		param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
+	} else {
+		count = -EINVAL;
+		goto exit;
+	}
+
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret < 0)
+		count = ret;
+exit:
+	kfree(msg);
+	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);
+/*
+ * Currently EC does not expose a host command to read the status of
+ * battery cutoff configuration. Also there is no requirement to read
+ * the flag from userland. So marking this attribute as write-only.
+ */
+static DEVICE_ATTR_WO(battery_cutoff);
 
 static struct attribute *__ec_attrs[] = {
+	&dev_attr_battery_cutoff.attr,
 	&dev_attr_kb_wake_angle.attr,
 	&dev_attr_reboot.attr,
 	&dev_attr_version.attr,
-- 
2.20.1

[PATCH V5] platform/chrome: mfd/cros_ec_dev: Add sysfile to force

[RaviChandra Sadineni]

On chromebooks, power_manager daemon normally shuts down(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 cutoff. This results in
battery deep discharging. This is a bad user experience as battery
has to trickle charge before booting when the AC 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 cutoff instead of hibernating when the system enters
S5 next time.

This attribute is present only if EC supports EC_FEATURE_BATTERY.

Signed-off-by: RaviChandra Sadineni <ravisadineni@chromium.org>
---
V5: Expose flag only when EC_FEATURE_BATTERY is supported.
V4: Addressed comments from Enric.
V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
V2: Use kstrtobool() instead of kstrtou8() and add documentation.

.../ABI/testing/sysfs-class-chromeos          | 16 ++++++
 drivers/mfd/cros_ec_dev.c                     |  4 ++
 drivers/platform/chrome/cros_ec_sysfs.c       | 49 +++++++++++++++++++
 include/linux/mfd/cros_ec.h                   |  2 +
 4 files changed, 71 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
index 5819699d66ec..0927704d1629 100644
--- a/Documentation/ABI/testing/sysfs-class-chromeos
+++ b/Documentation/ABI/testing/sysfs-class-chromeos
@@ -30,3 +30,19 @@ Date:		August 2015
 KernelVersion:	4.2
 Description:
 		Show the information about the EC software and hardware.
+
+What:           /sys/class/chromeos/<ec-device-name>/battery_cuttoff
+Date:           February 2019
+Contact:        Ravi Chandra Sadineni <ravisadineni@chromium.org>
+Description:
+                cros_ec battery cuttoff configuration. Only option
+                currently exposed is 'at-shutdown'.
+
+                'at-shutdown' sends a host command to EC requesting
+                battery cutoff on next shutdown. If AC is plugged
+                in before next shutdown, EC ignores the request and
+                resets the flag.
+
+                Currently EC does not expose a host command to read
+                the status of battery cutoff configuration. Thus this
+                flag is write-only.
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index ed809fc97df8..7580be23dfb3 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -502,6 +502,10 @@ static int ec_device_probe(struct platform_device *pdev)
 				 retval);
 	}
 
+	/* check whether EC_FEATURE_BATTERY is supported. */
+	if (cros_ec_check_features(ec, EC_FEATURE_BATTERY))
+		ec->has_battery = true;
+
 	return 0;
 
 failed:
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index fe0b7614ae1b..3d9ab55dddc0 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -308,14 +308,61 @@ static ssize_t kb_wake_angle_store(struct device *dev,
 	return count;
 }
 
+/* Battery cutoff control */
+static ssize_t battery_cutoff_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 = to_cros_ec_dev(dev);
+	char *p;
+	int len;
+
+	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;
+	msg->outsize = sizeof(*param);
+	msg->insize = 0;
+
+	p = memchr(buf, '\n', count);
+	len = p ? p - buf : count;
+
+	if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
+		param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
+	} else {
+		count = -EINVAL;
+		goto exit;
+	}
+
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret < 0)
+		count = ret;
+exit:
+	kfree(msg);
+	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);
+/*
+ * Currently EC does not expose a host command to read the status of
+ * battery cutoff configuration. Also there is no requirement to read
+ * the flag from userland. So marking this attribute as write-only.
+ */
+static DEVICE_ATTR_WO(battery_cutoff);
 
 static struct attribute *__ec_attrs[] = {
+	&dev_attr_battery_cutoff.attr,
 	&dev_attr_kb_wake_angle.attr,
 	&dev_attr_reboot.attr,
 	&dev_attr_version.attr,
@@ -331,6 +378,8 @@ static umode_t cros_ec_ctrl_visible(struct kobject *kobj,
 
 	if (a == &dev_attr_kb_wake_angle.attr && !ec->has_kb_wake_angle)
 		return 0;
+	if (a == &dev_attr_battery_cutoff.attr && !ec->has_battery)
+		return 0;
 
 	return a->mode;
 }
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 8f2a8918bfa3..de5280c96bd9 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -192,6 +192,7 @@ struct cros_ec_debugfs;
  * @has_kb_wake_angle: True if at least 2 accelerometer are connected to the EC.
  * @cmd_offset: Offset to apply for each command.
  * @features: Features supported by the EC.
+ * @has_battery: True if EC supports EC_FEATURE_BATTERY.
  */
 struct cros_ec_dev {
 	struct device class_dev;
@@ -202,6 +203,7 @@ struct cros_ec_dev {
 	bool has_kb_wake_angle;
 	u16 cmd_offset;
 	u32 features[2];
+	bool has_battery;
 };
 
 #define to_cros_ec_dev(dev)  container_of(dev, struct cros_ec_dev, class_dev)
-- 
2.20.1

Re: [PATCH V5] platform/chrome: mfd/cros_ec_dev: Add sysfile to force

[Enric Balletbo i Serra]

Hi RaviChandra,

Many thanks for pushing this upstream. Some more comments below.

On 9/3/19 2:42, RaviChandra Sadineni wrote:
> On chromebooks, power_manager daemon normally shuts down(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 cutoff. This results in
> battery deep discharging. This is a bad user experience as battery
> has to trickle charge before booting when the AC 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 cutoff instead of hibernating when the system enters
> S5 next time.
> 
> This attribute is present only if EC supports EC_FEATURE_BATTERY.
> 
> Signed-off-by: RaviChandra Sadineni <ravisadineni@chromium.org>
> ---
> V5: Expose flag only when EC_FEATURE_BATTERY is supported.
> V4: Addressed comments from Enric.
> V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
> V2: Use kstrtobool() instead of kstrtou8() and add documentation.
> 
> .../ABI/testing/sysfs-class-chromeos          | 16 ++++++
>  drivers/mfd/cros_ec_dev.c                     |  4 ++
>  drivers/platform/chrome/cros_ec_sysfs.c       | 49 +++++++++++++++++++
>  include/linux/mfd/cros_ec.h                   |  2 +
>  4 files changed, 71 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
> index 5819699d66ec..0927704d1629 100644
> --- a/Documentation/ABI/testing/sysfs-class-chromeos
> +++ b/Documentation/ABI/testing/sysfs-class-chromeos
> @@ -30,3 +30,19 @@ Date:		August 2015
>  KernelVersion:	4.2
>  Description:
>  		Show the information about the EC software and hardware.
> +
> +What:           /sys/class/chromeos/<ec-device-name>/battery_cuttoff

Typo s/battery_cuttoff/battery_cutoff/

> +Date:           February 2019
> +Contact:        Ravi Chandra Sadineni <ravisadineni@chromium.org>
> +Description:
> +                cros_ec battery cuttoff configuration. Only option

Typo s/battery_cuttoff/battery_cutoff/

> +                currently exposed is 'at-shutdown'.
> +
> +                'at-shutdown' sends a host command to EC requesting
> +                battery cutoff on next shutdown. If AC is plugged
> +                in before next shutdown, EC ignores the request and
> +                resets the flag.
> +
> +                Currently EC does not expose a host command to read
> +                the status of battery cutoff configuration. Thus this
> +                flag is write-only.
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index ed809fc97df8..7580be23dfb3 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -502,6 +502,10 @@ static int ec_device_probe(struct platform_device *pdev)
>  				 retval);
>  	}
>  
> +	/* check whether EC_FEATURE_BATTERY is supported. */
> +	if (cros_ec_check_features(ec, EC_FEATURE_BATTERY))
> +		ec->has_battery = true;
> +

You can check in cros-ec-sysfs driver if the feature is supported, no need to do
this here and add a new variable. See below.

>  	return 0;
>  
>  failed:
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index fe0b7614ae1b..3d9ab55dddc0 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -308,14 +308,61 @@ static ssize_t kb_wake_angle_store(struct device *dev,
>  	return count;
>  }
>  
> +/* Battery cutoff control */
> +static ssize_t battery_cutoff_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 = to_cros_ec_dev(dev);
> +	char *p;
> +	int len;

nit: Reversed X-mas tree order if you don't mind.

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

I looked at ectool cmd_battery_cut_off and if I understand correctly this
command is also possible for protocol version 0. I am wondering if we should
also handle this case. There is any reason to don't do that?

> +	msg->outsize = sizeof(*param);
> +	msg->insize = 0;
> +
> +	p = memchr(buf, '\n', count);
> +	len = p ? p - buf : count;
> +
> +	if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
> +		param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;

Can you prepare this for future flags defining and array of strings and using
sysfs_match_string helper?

> +	} else {
> +		count = -EINVAL;
> +		goto exit;
> +	}
> +
> +	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> +	if (ret < 0)
> +		count = ret;
> +exit:
> +	kfree(msg);
> +	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);
> +/*
> + * Currently EC does not expose a host command to read the status of
> + * battery cutoff configuration. Also there is no requirement to read
> + * the flag from userland. So marking this attribute as write-only.
> + */
> +static DEVICE_ATTR_WO(battery_cutoff);
>  
>  static struct attribute *__ec_attrs[] = {
> +	&dev_attr_battery_cutoff.attr,
>  	&dev_attr_kb_wake_angle.attr,
>  	&dev_attr_reboot.attr,
>  	&dev_attr_version.attr,
> @@ -331,6 +378,8 @@ static umode_t cros_ec_ctrl_visible(struct kobject *kobj,
>  
>  	if (a == &dev_attr_kb_wake_angle.attr && !ec->has_kb_wake_angle)
>  		return 0;
> +	if (a == &dev_attr_battery_cutoff.attr && !ec->has_battery)
> +		return 0;
>  

You can check directly if the feature is available. i.e

static int cros_ec_has_battery(struct cros_ec_dev *ec)
{
       return ec->features[EC_FEATURE_BATTERY / 32] &
              EC_FEATURE_MASK_0(EC_FEATURE_BATTERY);
}

>  	return a->mode;
>  }
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 8f2a8918bfa3..de5280c96bd9 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -192,6 +192,7 @@ struct cros_ec_debugfs;
>   * @has_kb_wake_angle: True if at least 2 accelerometer are connected to the EC.
>   * @cmd_offset: Offset to apply for each command.
>   * @features: Features supported by the EC.
> + * @has_battery: True if EC supports EC_FEATURE_BATTERY.
>   */
>  struct cros_ec_dev {
>  	struct device class_dev;
> @@ -202,6 +203,7 @@ struct cros_ec_dev {
>  	bool has_kb_wake_angle;
>  	u16 cmd_offset;
>  	u32 features[2];
> +	bool has_battery;

Not needed.

Note that the reason why we have a has_kb_wake_angle is because there isn't an
EC_FEATURE for the wake_angle propriety, but this is not the case here.
features[2] has this info.

>  };
>  
>  #define to_cros_ec_dev(dev)  container_of(dev, struct cros_ec_dev, class_dev)
> 

Thanks,
 Enric

Re: [PATCH V3] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[Ravi Chandra Sadineni]

Hi Enric,

Thanks for the review.

On Thu, Mar 7, 2019 at 3:57 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi RaviChandra,
>
> Some few comments below ...
>
>
> On 5/3/19 1:53, RaviChandra Sadineni 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
>
> Be coherent using the same spelling along all the patch for cutoff.
Done.
>
>
> > instead of hibernating. On some chromebooks, S5 is optimal enough
> > resulting in EC hibernating without battery cut-off. This results in
>
> s/cut-off/cutoff/
Done.
>
> > 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
>
> s/cut-off/cutoff/
Done.
>
> > S5 next time.
> >
> > Signed-off-by: RaviChandra Sadineni <ravisadineni@chromium.org>
> > ---
> >
> > V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
> > V2: Use kstrtobool() instead of kstrtou8() and add documentation.
> >
> >
> > .../ABI/testing/sysfs-class-chromeos          | 10 ++++
> >  drivers/platform/chrome/cros_ec_sysfs.c       | 48 +++++++++++++++++++
> >  2 files changed, 58 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..d5ab22c44977
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-chromeos
>
> Please rebase the patch on top of chrome-platform for-next [1] or 5.1-rc1 when
> released, so it applies cleanly.
Done.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/log/?h=for-next
>
> > @@ -0,0 +1,10 @@
> > +What:           /sys/class/chromeos/cros_ec/battery-cuttoff
>
> The name should match with the attribute name, so battery_cutoff in this case as
> you defined the attribute name as battery_cutoff
>
> Is this a common property for all the EC devices?
>
> Note that we have
>
> /sys/class/chromeos/<ec-device-name>/...
>
> where <ec-device-name> can be cros_ec|cros_pd and many others in the future
>
> On those devices that more than one EC is present we will have. E.g
>
>  cros_ec/battery_cutoff and cros_pd/battery_cutoff
>  cros_ec/battery_cutoff and cros_ish/battery_cutoff
>
> Which I think is not what you want. Ideally I'd like to see visible the
> attribute only on those ECs that support that feature. Is this command
> associated with an EC feature?
Yes with EC_FEATURE_BATTERY.  Now checking if EC supports EC_FEATURE_BATTERY.
>
> If that's not possible I can assume writing to battery_cutoff for ECs that don't
> implement it would return a protocol/command error.

>
>
> > +Date:           February 2019
> > +Contact:        Ravi Chandra Sadineni <ravisadineni@chromium.org>
> > +Description:
> > +                cros_ec battery cuttoff configuration. Only option
> > +                currently exposed is 'at-shutdown'.
> > +
> > +                'at-shutdown' sends a host command to EC requesting
> > +                battery cutoff on next shutdown. If AC is plugged
> > +                in before next shutdown, EC ignores the request.
> > diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> > index f34a50121064..6ef6b860c818 100644
> > --- a/drivers/platform/chrome/cros_ec_sysfs.c
> > +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> > @@ -322,14 +322,62 @@ static ssize_t kb_wake_angle_store(struct device *dev,
> >       return count;
> >  }
> >
> > +/* Battery cut-off control */
>
> s/cut-off/cutoff/
Done.
>
> > +static ssize_t battery_cutoff_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);
>
> use to_cros_ec_dev instead of container_of
>
> > +     char *p;
> > +     int len;
> > +
> > +     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;
> > +     msg->outsize = sizeof(*param);
> > +     msg->insize = 0;
> > +
> > +     p = memchr(buf, '\n', count);
> > +     len = p ? p - buf : count;
> > +
> > +     if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
> > +             param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
> > +     } else {
> > +             kfree(msg);
>
> Use only one kfree, please. Remove this.
>
> > +             return -EINVAL;
>
>                 count = -EINVAL;
>                 goto exit;
>
> > +     }
> > +
> > +     ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> > +     kfree(msg);
>
> Only one point for kfree, please. Remove this.
>
> > +     if (ret < 0)
> > +             return ret;
>             count = ret;
>
> exit:
>         kfree(msg);
> > +     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);
> > +/*
> > + * Currently EC does not expose a host command to read the status of
> > + * battery cut-off configuration. Also there is no requirement to read
>
> s/cut-off/cutoff/
>
> > + * the status of these flags from userland. So marking this attribute as
> > + *  write-only.
> > + */
>
> Can you also specify that is WO in the sysfs documentation. I understand
> correctly that this command only applies when the device runs on battery? And
> that if AC is plugged the flag is reset?
Done.


Thanks,
Ravi


>
> > +static DEVICE_ATTR_WO(battery_cutoff);
> >
>
> Note that the name should match with the sysfs documentation.
>
> >  static struct attribute *__ec_attrs[] = {
> > +     &dev_attr_battery_cutoff.attr,
> >       &dev_attr_kb_wake_angle.attr,
> >       &dev_attr_reboot.attr,
> >       &dev_attr_version.attr,
> >
>
> Thanks,
>  Enric

Re: [PATCH V2] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[Ravi Chandra Sadineni]

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

Re: [PATCH V2] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[Ravi Chandra Sadineni]

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

Re: [PATCH] cros_ec: Expose sysfile to force battery cut-off on shutdown.

[Ravi Chandra Sadineni]

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