linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] platform/chrome: Expose resume result via debugfs
@ 2019-06-17 21:52 Evan Green
  2019-06-25 13:05 ` Lee Jones
  0 siblings, 1 reply; 6+ messages in thread
From: Evan Green @ 2019-06-17 21:52 UTC (permalink / raw)
  To: Enric Balletbo i Serra, Lee Jones
  Cc: Ravi Chandra Sadineni, Rajat Jain, Evan Green, Guenter Roeck,
	linux-kernel, Benson Leung, Tim Wawrzynczak

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 debugfs so
that usermode can detect and report S0ix timeouts.

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

Changes in v2:
 - Moved from sysfs to debugfs (Enric)
 - Added documentation (Enric)


---
 Documentation/ABI/testing/debugfs-cros-ec | 22 ++++++++++++++++++++++
 drivers/mfd/cros_ec.c                     |  6 +++++-
 drivers/platform/chrome/cros_ec_debugfs.c |  7 +++++++
 include/linux/mfd/cros_ec.h               |  1 +
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/debugfs-cros-ec b/Documentation/ABI/testing/debugfs-cros-ec
index 573a82d23c89..008b31422079 100644
--- a/Documentation/ABI/testing/debugfs-cros-ec
+++ b/Documentation/ABI/testing/debugfs-cros-ec
@@ -32,3 +32,25 @@ Description:
 		is used for synchronizing the AP host time with the EC
 		log. An error is returned if the command is not supported
 		by the EC or there is a communication problem.
+
+What:		/sys/kernel/debug/cros_ec/last_resume_result
+Date:		June 2019
+KernelVersion:	5.3
+Description:
+		Some ECs have a feature where they will track transitions to
+		the (Intel) processor's SLP_S0 line, in order to detect cases
+		where a system failed to go into S0ix. When the system resumes,
+		an EC with this feature will return a summary of SLP_S0
+		transitions that occurred. The last_resume_result file returns
+		the most recent response from the AP's resume message to the EC.
+
+		The bottom 31 bits contain a count of the number of SLP_S0
+		transitions that occurred since the suspend message was
+		received. Bit 31 is set if the EC attempted to wake the
+		system due to a timeout when watching for SLP_S0 transitions.
+		Callers can use this to detect a wake from the EC due to
+		S0ix timeouts. The result will be zero if no suspend
+		transitions have been attempted, or the EC does not support
+		this feature.
+
+		Output will be in the format: "0x%08x\n".
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 5d5c41ac3845..2a9ac5213893 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -102,12 +102,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_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
index cd3fb9c22a44..663bebf699bf 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.c
+++ b/drivers/platform/chrome/cros_ec_debugfs.c
@@ -447,6 +447,13 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
 	debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
 			    &cros_ec_uptime_fops);
 
+	if (!strcmp(ec->class_dev.kobj.name, CROS_EC_DEV_NAME)) {
+		debugfs_create_x32("last_resume_result",
+				   0444,
+				   debug_info->dir,
+				   &ec->ec_dev->last_resume_result);
+	}
+
 	ec->debug_info = debug_info;
 
 	dev_set_drvdata(&pd->dev, ec);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 5ddca44be06d..45aba26db964 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -155,6 +155,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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Expose resume result via debugfs
  2019-06-17 21:52 [PATCH v2] platform/chrome: Expose resume result via debugfs Evan Green
@ 2019-06-25 13:05 ` Lee Jones
  2019-06-26 20:55   ` Enric Balletbo i Serra
  0 siblings, 1 reply; 6+ messages in thread
From: Lee Jones @ 2019-06-25 13:05 UTC (permalink / raw)
  To: Evan Green
  Cc: Enric Balletbo i Serra, Ravi Chandra Sadineni, Rajat Jain,
	Guenter Roeck, linux-kernel, Benson Leung, Tim Wawrzynczak

On Mon, 17 Jun 2019, 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 debugfs so
> that usermode can detect and report S0ix timeouts.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>

This still needs a platform/chrome Ack.

> ---
> 
> Changes in v2:
>  - Moved from sysfs to debugfs (Enric)
>  - Added documentation (Enric)
> 
> 
> ---
>  Documentation/ABI/testing/debugfs-cros-ec | 22 ++++++++++++++++++++++
>  drivers/mfd/cros_ec.c                     |  6 +++++-
>  drivers/platform/chrome/cros_ec_debugfs.c |  7 +++++++
>  include/linux/mfd/cros_ec.h               |  1 +
>  4 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/debugfs-cros-ec b/Documentation/ABI/testing/debugfs-cros-ec
> index 573a82d23c89..008b31422079 100644
> --- a/Documentation/ABI/testing/debugfs-cros-ec
> +++ b/Documentation/ABI/testing/debugfs-cros-ec
> @@ -32,3 +32,25 @@ Description:
>  		is used for synchronizing the AP host time with the EC
>  		log. An error is returned if the command is not supported
>  		by the EC or there is a communication problem.
> +
> +What:		/sys/kernel/debug/cros_ec/last_resume_result
> +Date:		June 2019
> +KernelVersion:	5.3
> +Description:
> +		Some ECs have a feature where they will track transitions to
> +		the (Intel) processor's SLP_S0 line, in order to detect cases
> +		where a system failed to go into S0ix. When the system resumes,
> +		an EC with this feature will return a summary of SLP_S0
> +		transitions that occurred. The last_resume_result file returns
> +		the most recent response from the AP's resume message to the EC.
> +
> +		The bottom 31 bits contain a count of the number of SLP_S0
> +		transitions that occurred since the suspend message was
> +		received. Bit 31 is set if the EC attempted to wake the
> +		system due to a timeout when watching for SLP_S0 transitions.
> +		Callers can use this to detect a wake from the EC due to
> +		S0ix timeouts. The result will be zero if no suspend
> +		transitions have been attempted, or the EC does not support
> +		this feature.
> +
> +		Output will be in the format: "0x%08x\n".
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 5d5c41ac3845..2a9ac5213893 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -102,12 +102,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_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
> index cd3fb9c22a44..663bebf699bf 100644
> --- a/drivers/platform/chrome/cros_ec_debugfs.c
> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
> @@ -447,6 +447,13 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
>  	debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
>  			    &cros_ec_uptime_fops);
>  
> +	if (!strcmp(ec->class_dev.kobj.name, CROS_EC_DEV_NAME)) {
> +		debugfs_create_x32("last_resume_result",
> +				   0444,
> +				   debug_info->dir,
> +				   &ec->ec_dev->last_resume_result);
> +	}
> +
>  	ec->debug_info = debug_info;
>  
>  	dev_set_drvdata(&pd->dev, ec);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 5ddca44be06d..45aba26db964 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -155,6 +155,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;
>  };
>  
>  /**

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Expose resume result via debugfs
  2019-06-25 13:05 ` Lee Jones
@ 2019-06-26 20:55   ` Enric Balletbo i Serra
  2019-06-27 16:07     ` Evan Green
  0 siblings, 1 reply; 6+ messages in thread
From: Enric Balletbo i Serra @ 2019-06-26 20:55 UTC (permalink / raw)
  To: Lee Jones, Evan Green
  Cc: Ravi Chandra Sadineni, Rajat Jain, Guenter Roeck, linux-kernel,
	Benson Leung, Tim Wawrzynczak

Hi Evan,

Two few comments and I think I'm fine with it.

On 25/6/19 15:05, Lee Jones wrote:
> On Mon, 17 Jun 2019, 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 debugfs so
>> that usermode can detect and report S0ix timeouts.
>>
>> Signed-off-by: Evan Green <evgreen@chromium.org>
> 
> This still needs a platform/chrome Ack.
> 
>> ---
>>
>> Changes in v2:
>>  - Moved from sysfs to debugfs (Enric)
>>  - Added documentation (Enric)
>>
>>
>> ---
>>  Documentation/ABI/testing/debugfs-cros-ec | 22 ++++++++++++++++++++++
>>  drivers/mfd/cros_ec.c                     |  6 +++++-
>>  drivers/platform/chrome/cros_ec_debugfs.c |  7 +++++++
>>  include/linux/mfd/cros_ec.h               |  1 +
>>  4 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/debugfs-cros-ec b/Documentation/ABI/testing/debugfs-cros-ec
>> index 573a82d23c89..008b31422079 100644
>> --- a/Documentation/ABI/testing/debugfs-cros-ec
>> +++ b/Documentation/ABI/testing/debugfs-cros-ec
>> @@ -32,3 +32,25 @@ Description:
>>  		is used for synchronizing the AP host time with the EC
>>  		log. An error is returned if the command is not supported
>>  		by the EC or there is a communication problem.
>> +
>> +What:		/sys/kernel/debug/cros_ec/last_resume_result

Thinking about it, as other the other interfaces, I'd do

s/cros_ec/<cros-ec-device>/

I know that for now only cros_ec supports that, but we don't know what will
happen in the future, specially now that the number of cros devices is incrementing.

>> +Date:		June 2019
>> +KernelVersion:	5.3
>> +Description:
>> +		Some ECs have a feature where they will track transitions to
>> +		the (Intel) processor's SLP_S0 line, in order to detect cases
>> +		where a system failed to go into S0ix. When the system resumes,
>> +		an EC with this feature will return a summary of SLP_S0
>> +		transitions that occurred. The last_resume_result file returns
>> +		the most recent response from the AP's resume message to the EC.
>> +
>> +		The bottom 31 bits contain a count of the number of SLP_S0
>> +		transitions that occurred since the suspend message was
>> +		received. Bit 31 is set if the EC attempted to wake the
>> +		system due to a timeout when watching for SLP_S0 transitions.
>> +		Callers can use this to detect a wake from the EC due to
>> +		S0ix timeouts. The result will be zero if no suspend
>> +		transitions have been attempted, or the EC does not support
>> +		this feature.
>> +
>> +		Output will be in the format: "0x%08x\n".
>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> index 5d5c41ac3845..2a9ac5213893 100644
>> --- a/drivers/mfd/cros_ec.c
>> +++ b/drivers/mfd/cros_ec.c
>> @@ -102,12 +102,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_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
>> index cd3fb9c22a44..663bebf699bf 100644
>> --- a/drivers/platform/chrome/cros_ec_debugfs.c
>> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
>> @@ -447,6 +447,13 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
>>  	debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
>>  			    &cros_ec_uptime_fops);
>>  
>> +	if (!strcmp(ec->class_dev.kobj.name, CROS_EC_DEV_NAME)) {

For debugfs I don't care having the file exposed even is not supported, anyway
there are some CROS_EC_DEV_NAME that won't support it, so just make this simple
and create the file always.

>> +		debugfs_create_x32("last_resume_result",
>> +				   0444,
>> +				   debug_info->dir,
>> +				   &ec->ec_dev->last_resume_result);
>> +	}
>> +
>>  	ec->debug_info = debug_info;
>>  
>>  	dev_set_drvdata(&pd->dev, ec);
>> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
>> index 5ddca44be06d..45aba26db964 100644
>> --- a/include/linux/mfd/cros_ec.h
>> +++ b/include/linux/mfd/cros_ec.h
>> @@ -155,6 +155,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;
>>  };
>>  
>>  /**
> 

Thanks,
~ Enric

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Expose resume result via debugfs
  2019-06-26 20:55   ` Enric Balletbo i Serra
@ 2019-06-27 16:07     ` Evan Green
  2019-06-27 16:42       ` Enric Balletbo i Serra
  0 siblings, 1 reply; 6+ messages in thread
From: Evan Green @ 2019-06-27 16:07 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: Lee Jones, Ravi Chandra Sadineni, Rajat Jain, Guenter Roeck,
	LKML, Benson Leung, Tim Wawrzynczak

On Wed, Jun 26, 2019 at 1:55 PM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Evan,
>
> Two few comments and I think I'm fine with it.
>
> On 25/6/19 15:05, Lee Jones wrote:
> > On Mon, 17 Jun 2019, 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 debugfs so
> >> that usermode can detect and report S0ix timeouts.
> >>
> >> Signed-off-by: Evan Green <evgreen@chromium.org>
> >
> > This still needs a platform/chrome Ack.
> >
> >> ---
> >>
> >> Changes in v2:
> >>  - Moved from sysfs to debugfs (Enric)
> >>  - Added documentation (Enric)
> >>
> >>
> >> ---
> >>  Documentation/ABI/testing/debugfs-cros-ec | 22 ++++++++++++++++++++++
> >>  drivers/mfd/cros_ec.c                     |  6 +++++-
> >>  drivers/platform/chrome/cros_ec_debugfs.c |  7 +++++++
> >>  include/linux/mfd/cros_ec.h               |  1 +
> >>  4 files changed, 35 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/ABI/testing/debugfs-cros-ec b/Documentation/ABI/testing/debugfs-cros-ec
> >> index 573a82d23c89..008b31422079 100644
> >> --- a/Documentation/ABI/testing/debugfs-cros-ec
> >> +++ b/Documentation/ABI/testing/debugfs-cros-ec
> >> @@ -32,3 +32,25 @@ Description:
> >>              is used for synchronizing the AP host time with the EC
> >>              log. An error is returned if the command is not supported
> >>              by the EC or there is a communication problem.
> >> +
> >> +What:               /sys/kernel/debug/cros_ec/last_resume_result
>
> Thinking about it, as other the other interfaces, I'd do
>
> s/cros_ec/<cros-ec-device>/
>
> I know that for now only cros_ec supports that, but we don't know what will
> happen in the future, specially now that the number of cros devices is incrementing.

See my comment below, I suppose the fate of these two comments are
tied together.

>
> >> +Date:               June 2019
> >> +KernelVersion:      5.3
> >> +Description:
> >> +            Some ECs have a feature where they will track transitions to
> >> +            the (Intel) processor's SLP_S0 line, in order to detect cases
> >> +            where a system failed to go into S0ix. When the system resumes,
> >> +            an EC with this feature will return a summary of SLP_S0
> >> +            transitions that occurred. The last_resume_result file returns
> >> +            the most recent response from the AP's resume message to the EC.
> >> +
> >> +            The bottom 31 bits contain a count of the number of SLP_S0
> >> +            transitions that occurred since the suspend message was
> >> +            received. Bit 31 is set if the EC attempted to wake the
> >> +            system due to a timeout when watching for SLP_S0 transitions.
> >> +            Callers can use this to detect a wake from the EC due to
> >> +            S0ix timeouts. The result will be zero if no suspend
> >> +            transitions have been attempted, or the EC does not support
> >> +            this feature.
> >> +
> >> +            Output will be in the format: "0x%08x\n".
> >> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> >> index 5d5c41ac3845..2a9ac5213893 100644
> >> --- a/drivers/mfd/cros_ec.c
> >> +++ b/drivers/mfd/cros_ec.c
> >> @@ -102,12 +102,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_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
> >> index cd3fb9c22a44..663bebf699bf 100644
> >> --- a/drivers/platform/chrome/cros_ec_debugfs.c
> >> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
> >> @@ -447,6 +447,13 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
> >>      debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
> >>                          &cros_ec_uptime_fops);
> >>
> >> +    if (!strcmp(ec->class_dev.kobj.name, CROS_EC_DEV_NAME)) {
>
> For debugfs I don't care having the file exposed even is not supported, anyway
> there are some CROS_EC_DEV_NAME that won't support it, so just make this simple
> and create the file always.

Aw, really? This file is very specific to system suspend/resume. My
original patch had it everywhere, but I was finding it very ugly that
this was showing up on things like the fingerprint device. I can
change it if you think it's better to have it everywhere, but it also
seems like an easy change to make in the future if this file is for
some reason needed on other EC types.

-Evan

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Expose resume result via debugfs
  2019-06-27 16:07     ` Evan Green
@ 2019-06-27 16:42       ` Enric Balletbo i Serra
  2019-06-27 17:53         ` Evan Green
  0 siblings, 1 reply; 6+ messages in thread
From: Enric Balletbo i Serra @ 2019-06-27 16:42 UTC (permalink / raw)
  To: Evan Green
  Cc: Lee Jones, Ravi Chandra Sadineni, Rajat Jain, Guenter Roeck,
	LKML, Benson Leung, Tim Wawrzynczak

Hi Evan,

On 27/6/19 18:07, Evan Green wrote:
> On Wed, Jun 26, 2019 at 1:55 PM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>>
>> Hi Evan,
>>
>> Two few comments and I think I'm fine with it.
>>
>> On 25/6/19 15:05, Lee Jones wrote:
>>> On Mon, 17 Jun 2019, 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 debugfs so
>>>> that usermode can detect and report S0ix timeouts.
>>>>
>>>> Signed-off-by: Evan Green <evgreen@chromium.org>
>>>
>>> This still needs a platform/chrome Ack.
>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>>  - Moved from sysfs to debugfs (Enric)
>>>>  - Added documentation (Enric)
>>>>
>>>>
>>>> ---
>>>>  Documentation/ABI/testing/debugfs-cros-ec | 22 ++++++++++++++++++++++
>>>>  drivers/mfd/cros_ec.c                     |  6 +++++-
>>>>  drivers/platform/chrome/cros_ec_debugfs.c |  7 +++++++
>>>>  include/linux/mfd/cros_ec.h               |  1 +
>>>>  4 files changed, 35 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/debugfs-cros-ec b/Documentation/ABI/testing/debugfs-cros-ec
>>>> index 573a82d23c89..008b31422079 100644
>>>> --- a/Documentation/ABI/testing/debugfs-cros-ec
>>>> +++ b/Documentation/ABI/testing/debugfs-cros-ec
>>>> @@ -32,3 +32,25 @@ Description:
>>>>              is used for synchronizing the AP host time with the EC
>>>>              log. An error is returned if the command is not supported
>>>>              by the EC or there is a communication problem.
>>>> +
>>>> +What:               /sys/kernel/debug/cros_ec/last_resume_result
>>
>> Thinking about it, as other the other interfaces, I'd do
>>
>> s/cros_ec/<cros-ec-device>/
>>
>> I know that for now only cros_ec supports that, but we don't know what will
>> happen in the future, specially now that the number of cros devices is incrementing.
> 
> See my comment below, I suppose the fate of these two comments are
> tied together.
> 
>>
>>>> +Date:               June 2019
>>>> +KernelVersion:      5.3
>>>> +Description:
>>>> +            Some ECs have a feature where they will track transitions to
>>>> +            the (Intel) processor's SLP_S0 line, in order to detect cases
>>>> +            where a system failed to go into S0ix. When the system resumes,
>>>> +            an EC with this feature will return a summary of SLP_S0
>>>> +            transitions that occurred. The last_resume_result file returns
>>>> +            the most recent response from the AP's resume message to the EC.
>>>> +
>>>> +            The bottom 31 bits contain a count of the number of SLP_S0
>>>> +            transitions that occurred since the suspend message was
>>>> +            received. Bit 31 is set if the EC attempted to wake the
>>>> +            system due to a timeout when watching for SLP_S0 transitions.
>>>> +            Callers can use this to detect a wake from the EC due to
>>>> +            S0ix timeouts. The result will be zero if no suspend
>>>> +            transitions have been attempted, or the EC does not support
>>>> +            this feature.
>>>> +
>>>> +            Output will be in the format: "0x%08x\n".
>>>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>>>> index 5d5c41ac3845..2a9ac5213893 100644
>>>> --- a/drivers/mfd/cros_ec.c
>>>> +++ b/drivers/mfd/cros_ec.c
>>>> @@ -102,12 +102,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_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
>>>> index cd3fb9c22a44..663bebf699bf 100644
>>>> --- a/drivers/platform/chrome/cros_ec_debugfs.c
>>>> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
>>>> @@ -447,6 +447,13 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
>>>>      debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
>>>>                          &cros_ec_uptime_fops);
>>>>
>>>> +    if (!strcmp(ec->class_dev.kobj.name, CROS_EC_DEV_NAME)) {
>>
>> For debugfs I don't care having the file exposed even is not supported, anyway
>> there are some CROS_EC_DEV_NAME that won't support it, so just make this simple
>> and create the file always.
> 
> Aw, really? This file is very specific to system suspend/resume. My
> original patch had it everywhere, but I was finding it very ugly that
> this was showing up on things like the fingerprint device. I can
> change it if you think it's better to have it everywhere, but it also
> seems like an easy change to make in the future if this file is for
> some reason needed on other EC types.
> 

I'd think different if it was a sysfs property but it's a debugfs.

Right now we have:

#define CROS_EC_DEV_NAME        "cros_ec"
#define CROS_EC_DEV_FP_NAME     "cros_fp"
#define CROS_EC_DEV_ISH_NAME    "cros_ish"
#define CROS_EC_DEV_PD_NAME     "cros_pd"
#define CROS_EC_DEV_SCP_NAME    "cros_scp"
#define CROS_EC_DEV_TP_NAME     "cros_tp"

Is really the named cros_ec the only that has this feature? What about cros_scp
(is not supposed to run the same base code as cros_ec)? And cros_ish ?

And all the named cros_ec devices have this feature? Maybe is supported by
Nocturne but not Veyron? Wouldn't be exposed also on those cros_ec that doesn't
support it?

And don't have the answer to all these questions, hence my concerns.

As per your documentation

 +                ...           The result will be zero if no suspend
 +            transitions have been attempted, or the EC does not support
 +            this feature.

I can accept if _all_ the CROS_EC_DEV_NAMEs and _only_ the CROS_EC_DEV_NAME
supports it, is that the case?

But if you are anyway exposing this on CROS_EC_DEV_NAMEs that doesn't support it
why not just expose to all and skip a future bunch of code to filter, it is
clear what a 0 means and at the end it's just a debugfs file.

~ Enric

> -Evan
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Expose resume result via debugfs
  2019-06-27 16:42       ` Enric Balletbo i Serra
@ 2019-06-27 17:53         ` Evan Green
  0 siblings, 0 replies; 6+ messages in thread
From: Evan Green @ 2019-06-27 17:53 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: Lee Jones, Ravi Chandra Sadineni, Rajat Jain, Guenter Roeck,
	LKML, Benson Leung, Tim Wawrzynczak

On Thu, Jun 27, 2019 at 9:42 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Evan,
>
> On 27/6/19 18:07, Evan Green wrote:
> > On Wed, Jun 26, 2019 at 1:55 PM Enric Balletbo i Serra
> > <enric.balletbo@collabora.com> wrote:
> >>
> >> Hi Evan,
> >>
> >> Two few comments and I think I'm fine with it.
> >>
> >> On 25/6/19 15:05, Lee Jones wrote:
> >>> On Mon, 17 Jun 2019, 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 debugfs so
> >>>> that usermode can detect and report S0ix timeouts.
> >>>>
> >>>> Signed-off-by: Evan Green <evgreen@chromium.org>
> >>>
> >>> This still needs a platform/chrome Ack.
> >>>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>>  - Moved from sysfs to debugfs (Enric)
> >>>>  - Added documentation (Enric)
> >>>>
> >>>>
> >>>> ---
> >>>>  Documentation/ABI/testing/debugfs-cros-ec | 22 ++++++++++++++++++++++
> >>>>  drivers/mfd/cros_ec.c                     |  6 +++++-
> >>>>  drivers/platform/chrome/cros_ec_debugfs.c |  7 +++++++
> >>>>  include/linux/mfd/cros_ec.h               |  1 +
> >>>>  4 files changed, 35 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/Documentation/ABI/testing/debugfs-cros-ec b/Documentation/ABI/testing/debugfs-cros-ec
> >>>> index 573a82d23c89..008b31422079 100644
> >>>> --- a/Documentation/ABI/testing/debugfs-cros-ec
> >>>> +++ b/Documentation/ABI/testing/debugfs-cros-ec
> >>>> @@ -32,3 +32,25 @@ Description:
> >>>>              is used for synchronizing the AP host time with the EC
> >>>>              log. An error is returned if the command is not supported
> >>>>              by the EC or there is a communication problem.
> >>>> +
> >>>> +What:               /sys/kernel/debug/cros_ec/last_resume_result
> >>
> >> Thinking about it, as other the other interfaces, I'd do
> >>
> >> s/cros_ec/<cros-ec-device>/
> >>
> >> I know that for now only cros_ec supports that, but we don't know what will
> >> happen in the future, specially now that the number of cros devices is incrementing.
> >
> > See my comment below, I suppose the fate of these two comments are
> > tied together.
> >
> >>
> >>>> +Date:               June 2019
> >>>> +KernelVersion:      5.3
> >>>> +Description:
> >>>> +            Some ECs have a feature where they will track transitions to
> >>>> +            the (Intel) processor's SLP_S0 line, in order to detect cases
> >>>> +            where a system failed to go into S0ix. When the system resumes,
> >>>> +            an EC with this feature will return a summary of SLP_S0
> >>>> +            transitions that occurred. The last_resume_result file returns
> >>>> +            the most recent response from the AP's resume message to the EC.
> >>>> +
> >>>> +            The bottom 31 bits contain a count of the number of SLP_S0
> >>>> +            transitions that occurred since the suspend message was
> >>>> +            received. Bit 31 is set if the EC attempted to wake the
> >>>> +            system due to a timeout when watching for SLP_S0 transitions.
> >>>> +            Callers can use this to detect a wake from the EC due to
> >>>> +            S0ix timeouts. The result will be zero if no suspend
> >>>> +            transitions have been attempted, or the EC does not support
> >>>> +            this feature.
> >>>> +
> >>>> +            Output will be in the format: "0x%08x\n".
> >>>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> >>>> index 5d5c41ac3845..2a9ac5213893 100644
> >>>> --- a/drivers/mfd/cros_ec.c
> >>>> +++ b/drivers/mfd/cros_ec.c
> >>>> @@ -102,12 +102,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_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
> >>>> index cd3fb9c22a44..663bebf699bf 100644
> >>>> --- a/drivers/platform/chrome/cros_ec_debugfs.c
> >>>> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
> >>>> @@ -447,6 +447,13 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
> >>>>      debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
> >>>>                          &cros_ec_uptime_fops);
> >>>>
> >>>> +    if (!strcmp(ec->class_dev.kobj.name, CROS_EC_DEV_NAME)) {
> >>
> >> For debugfs I don't care having the file exposed even is not supported, anyway
> >> there are some CROS_EC_DEV_NAME that won't support it, so just make this simple
> >> and create the file always.
> >
> > Aw, really? This file is very specific to system suspend/resume. My
> > original patch had it everywhere, but I was finding it very ugly that
> > this was showing up on things like the fingerprint device. I can
> > change it if you think it's better to have it everywhere, but it also
> > seems like an easy change to make in the future if this file is for
> > some reason needed on other EC types.
> >
>
> I'd think different if it was a sysfs property but it's a debugfs.
>
> Right now we have:
>
> #define CROS_EC_DEV_NAME        "cros_ec"
> #define CROS_EC_DEV_FP_NAME     "cros_fp"
> #define CROS_EC_DEV_ISH_NAME    "cros_ish"
> #define CROS_EC_DEV_PD_NAME     "cros_pd"
> #define CROS_EC_DEV_SCP_NAME    "cros_scp"
> #define CROS_EC_DEV_TP_NAME     "cros_tp"
>
> Is really the named cros_ec the only that has this feature? What about cros_scp
> (is not supposed to run the same base code as cros_ec)? And cros_ish ?
>
> And all the named cros_ec devices have this feature? Maybe is supported by
> Nocturne but not Veyron? Wouldn't be exposed also on those cros_ec that doesn't
> support it?
>
> And don't have the answer to all these questions, hence my concerns.
>
> As per your documentation
>
>  +                ...           The result will be zero if no suspend
>  +            transitions have been attempted, or the EC does not support
>  +            this feature.
>
> I can accept if _all_ the CROS_EC_DEV_NAMEs and _only_ the CROS_EC_DEV_NAME
> supports it, is that the case?
>
> But if you are anyway exposing this on CROS_EC_DEV_NAMEs that doesn't support it
> why not just expose to all and skip a future bunch of code to filter, it is
> clear what a 0 means and at the end it's just a debugfs file.

Yes, my patch was attempting to expose it for all "cros_ec" devices,
even if the EC didn't support that feature. I was hoping to avoid
polluting other devices like cros_fp and cros_pd, but I think you may
be correct about cros_scp. That's something that will act as the
system EC but is named differently because it can send IPIs as well?
Weird, but ok.

Ok, I'll add this debugfs file to all EC devices.
-Evan

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-27 17:54 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 21:52 [PATCH v2] platform/chrome: Expose resume result via debugfs Evan Green
2019-06-25 13:05 ` Lee Jones
2019-06-26 20:55   ` Enric Balletbo i Serra
2019-06-27 16:07     ` Evan Green
2019-06-27 16:42       ` Enric Balletbo i Serra
2019-06-27 17:53         ` Evan Green

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).