[PATCH] platform/chrome: Expose resume result via sysfs

[PATCH] platform/chrome: Expose resume result via sysfs

[Evan Green]

For ECs that support it, the EC returns the number of slp_s0
transitions and whether or not there was a timeout in the resume
response. Expose the last resume result to usermode via sysfs so
that usermode can detect and report S0ix timeouts.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

Enric, I'm not sure if this conflicts with your giant refactoring.
I can rebase this on your series, but patchwork doesn't have patch
2 of your series, so you'd have to point me to a tree.

---
 drivers/mfd/cros_ec.c                   |  6 +++++-
 drivers/platform/chrome/cros_ec_sysfs.c | 17 +++++++++++++++++
 include/linux/mfd/cros_ec.h             |  1 +
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index bd2bcdd4718b..64a2d3adc729 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -110,12 +110,16 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
 
 	/* For now, report failure to transition to S0ix with a warning. */
 	if (ret >= 0 && ec_dev->host_sleep_v1 &&
-	    (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME))
+	    (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
+		ec_dev->last_resume_result =
+			buf.u.resp1.resume_response.sleep_transitions;
+
 		WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
 			  EC_HOST_RESUME_SLEEP_TIMEOUT,
 			  "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
 			  buf.u.resp1.resume_response.sleep_transitions &
 			  EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
+	}
 
 	return ret;
 }
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index 3edb237bf8ed..2deca98c7a7a 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -308,18 +308,35 @@ static ssize_t kb_wake_angle_store(struct device *dev,
 	return count;
 }
 
+/* Last resume result */
+static ssize_t last_resume_result_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
+	int ret;
+
+	ret = scnprintf(buf,
+			PAGE_SIZE,
+			"0x%x\n",
+			ec->ec_dev->last_resume_result);
+
+	return ret;
+}
+
 /* 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_RO(last_resume_result);
 
 static struct attribute *__ec_attrs[] = {
 	&dev_attr_kb_wake_angle.attr,
 	&dev_attr_reboot.attr,
 	&dev_attr_version.attr,
 	&dev_attr_flashinfo.attr,
+	&dev_attr_last_resume_result.attr,
 	NULL,
 };
 
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index cfa78bb4990f..d50ade418a83 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -163,6 +163,7 @@ struct cros_ec_device {
 	struct ec_response_get_next_event_v1 event_data;
 	int event_size;
 	u32 host_event_wake_mask;
+	u32 last_resume_result;
 };
 
 /**
-- 
2.20.1

Re: [PATCH] platform/chrome: Expose resume result via sysfs

[Enric Balletbo i Serra]

Hi Evan,

On 7/6/19 23:05, Evan Green wrote:
> For ECs that support it, the EC returns the number of slp_s0
> transitions and whether or not there was a timeout in the resume
> response. Expose the last resume result to usermode via sysfs so
> that usermode can detect and report S0ix timeouts.

Looks more for a debugfs attribute instead of sysfs?

I'd prefer have it in debugfs unless you have a good reason. As you probably
know I'm not a big fan of having private interfaces and I'd like to maintain the
minimum needed in sysfs.

> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> 
> Enric, I'm not sure if this conflicts with your giant refactoring.
> I can rebase this on your series, but patchwork doesn't have patch
> 2 of your series, so you'd have to point me to a tree.
> 

Probably, but it's fine for now, so don't worry about it. The refactoring still
needs more reviews from other people.

> ---
>  drivers/mfd/cros_ec.c                   |  6 +++++-
>  drivers/platform/chrome/cros_ec_sysfs.c | 17 +++++++++++++++++
>  include/linux/mfd/cros_ec.h             |  1 +
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index bd2bcdd4718b..64a2d3adc729 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -110,12 +110,16 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
>  
>  	/* For now, report failure to transition to S0ix with a warning. */
>  	if (ret >= 0 && ec_dev->host_sleep_v1 &&
> -	    (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME))
> +	    (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> +		ec_dev->last_resume_result =
> +			buf.u.resp1.resume_response.sleep_transitions;
> +
>  		WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
>  			  EC_HOST_RESUME_SLEEP_TIMEOUT,
>  			  "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
>  			  buf.u.resp1.resume_response.sleep_transitions &
>  			  EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
> +	}
>  
>  	return ret;
>  }
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index 3edb237bf8ed..2deca98c7a7a 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -308,18 +308,35 @@ static ssize_t kb_wake_angle_store(struct device *dev,
>  	return count;
>  }
>  
> +/* Last resume result */
> +static ssize_t last_resume_result_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
> +	int ret;
> +
> +	ret = scnprintf(buf,
> +			PAGE_SIZE,
> +			"0x%x\n",
> +			ec->ec_dev->last_resume_result);
> +
> +	return ret;
> +}
> +
>  /* 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_RO(last_resume_result);
>  

The attribute should be documented in

Documentation/ABI/testing/sysfs-class-chromeos

or

Documentation/ABI/testing/debugfs-cros-ec ; yes I know this file does not exist,
but we should add it :-)


>  static struct attribute *__ec_attrs[] = {
>  	&dev_attr_kb_wake_angle.attr,
>  	&dev_attr_reboot.attr,
>  	&dev_attr_version.attr,
>  	&dev_attr_flashinfo.attr,
> +	&dev_attr_last_resume_result.attr,
>  	NULL,
>  };
>  
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index cfa78bb4990f..d50ade418a83 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -163,6 +163,7 @@ struct cros_ec_device {
>  	struct ec_response_get_next_event_v1 event_data;
>  	int event_size;
>  	u32 host_event_wake_mask;
> +	u32 last_resume_result;
>  };
>  
>  /**
> 

Re: [PATCH] platform/chrome: Expose resume result via sysfs

[Evan Green]

On Wed, Jun 12, 2019 at 9:37 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Evan,
>
> On 7/6/19 23:05, Evan Green wrote:
> > For ECs that support it, the EC returns the number of slp_s0
> > transitions and whether or not there was a timeout in the resume
> > response. Expose the last resume result to usermode via sysfs so
> > that usermode can detect and report S0ix timeouts.
>
> Looks more for a debugfs attribute instead of sysfs?
>
> I'd prefer have it in debugfs unless you have a good reason. As you probably
> know I'm not a big fan of having private interfaces and I'd like to maintain the
> minimum needed in sysfs.

That seems fine with me. It's easy enough to move it from debugfs to
sysfs later if needed, but much more difficult to go the other way.
I'll spin it for debugfs.

>
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> > ---
> >
> > Enric, I'm not sure if this conflicts with your giant refactoring.
> > I can rebase this on your series, but patchwork doesn't have patch
> > 2 of your series, so you'd have to point me to a tree.
> >
>
> Probably, but it's fine for now, so don't worry about it. The refactoring still
> needs more reviews from other people.

Ok. I'll base it on linux-next.

>
> > ---
> >  drivers/mfd/cros_ec.c                   |  6 +++++-
> >  drivers/platform/chrome/cros_ec_sysfs.c | 17 +++++++++++++++++
> >  include/linux/mfd/cros_ec.h             |  1 +
> >  3 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> > index bd2bcdd4718b..64a2d3adc729 100644
> > --- a/drivers/mfd/cros_ec.c
> > +++ b/drivers/mfd/cros_ec.c
> > @@ -110,12 +110,16 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> >
> >       /* For now, report failure to transition to S0ix with a warning. */
> >       if (ret >= 0 && ec_dev->host_sleep_v1 &&
> > -         (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME))
> > +         (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> > +             ec_dev->last_resume_result =
> > +                     buf.u.resp1.resume_response.sleep_transitions;
> > +
> >               WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
> >                         EC_HOST_RESUME_SLEEP_TIMEOUT,
> >                         "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
> >                         buf.u.resp1.resume_response.sleep_transitions &
> >                         EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
> > +     }
> >
> >       return ret;
> >  }
> > diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> > index 3edb237bf8ed..2deca98c7a7a 100644
> > --- a/drivers/platform/chrome/cros_ec_sysfs.c
> > +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> > @@ -308,18 +308,35 @@ static ssize_t kb_wake_angle_store(struct device *dev,
> >       return count;
> >  }
> >
> > +/* Last resume result */
> > +static ssize_t last_resume_result_show(struct device *dev,
> > +                                    struct device_attribute *attr, char *buf)
> > +{
> > +     struct cros_ec_dev *ec = to_cros_ec_dev(dev);
> > +     int ret;
> > +
> > +     ret = scnprintf(buf,
> > +                     PAGE_SIZE,
> > +                     "0x%x\n",
> > +                     ec->ec_dev->last_resume_result);
> > +
> > +     return ret;
> > +}
> > +
> >  /* 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_RO(last_resume_result);
> >
>
> The attribute should be documented in
>
> Documentation/ABI/testing/sysfs-class-chromeos
>
> or
>
> Documentation/ABI/testing/debugfs-cros-ec ; yes I know this file does not exist,
> but we should add it :-)

Ok. Thanks for taking a look.
-Evan