[PATCH 1/2] platform/x86: amd-pmc: adjust arguments for `amd_pmc_send_cmd`

[PATCH 1/2] platform/x86: amd-pmc: adjust arguments for `amd_pmc_send_cmd`

[Mario Limonciello]

Currently the "argument" for the "message" is listed as a boolean
value.  This works well for the commands used currently, but an
additional upcoming command will pass more data in the message.

Expand it to be a full 32 bit value.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd-pmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index b7b6ad4629a7..99ac50616bc3 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -126,7 +126,7 @@ struct amd_pmc_dev {
 };
 
 static struct amd_pmc_dev pmc;
-static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set, u32 *data, u8 msg, bool ret);
+static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
 
 static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
 {
@@ -337,7 +337,7 @@ static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
 	dev_dbg(dev->dev, "AMD_PMC_REGISTER_MESSAGE:%x\n", value);
 }
 
-static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set, u32 *data, u8 msg, bool ret)
+static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret)
 {
 	int rc;
 	u32 val;
@@ -356,7 +356,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set, u32 *data, u8 msg
 	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_RESPONSE, 0);
 
 	/* Write argument into response register */
-	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_ARGUMENT, set);
+	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_ARGUMENT, arg);
 
 	/* Write message ID to message ID register */
 	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_MESSAGE, msg);
-- 
2.25.1

[PATCH 2/2] platform/x86: amd-pmc: Add special handling for timer based S0i3 wakeup

[Mario Limonciello]

RTC based wakeup from s0i3 doesn't work properly on some Green Sardine
platforms. Because of this, a newer SMU for Green Sardine has the ability
to pass wakeup time as argument of the upper 16 bits of OS_HINT message.

With older firmware setting the timer value in OS_HINT will cause firmware
to reject the hint, so only run this path on:
1) Green Sardine
2) Minimum SMU FW
3) RTC alarm armed during s0i3 entry

Using this method has some limitations that the s0i3 wakeup will need to
be between 4 seconds and 18 hours, so check those boundary conditions as
well and abort the suspend if RTC is armed for too short or too long of a
duration.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd-pmc.c | 60 +++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 99ac50616bc3..678bf6874c63 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -17,9 +17,11 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
+#include <linux/limits.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/rtc.h>
 #include <linux/suspend.h>
 #include <linux/seq_file.h>
 #include <linux/uaccess.h>
@@ -412,20 +414,76 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
 	return -EINVAL;
 }
 
+static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
+{
+	struct rtc_device *rtc_device;
+	time64_t then, now, duration;
+	struct rtc_wkalrm alarm;
+	struct rtc_time tm;
+	int rc;
+
+	if (pdev->major < 64 || (pdev->major == 64 && pdev->minor < 53))
+		return 0;
+
+	rtc_device = rtc_class_open(CONFIG_RTC_SYSTOHC_DEVICE);
+	if (!rtc_device)
+		return 0;
+	rc = rtc_read_alarm(rtc_device, &alarm);
+	if (rc)
+		return rc;
+	if (!alarm.enabled) {
+		dev_dbg(pdev->dev, "alarm not enabled\n");
+		return 0;
+	}
+	rc = rtc_valid_tm(&alarm.time);
+	if (rc)
+		return rc;
+	rc = rtc_read_time(rtc_device, &tm);
+	if (rc)
+		return rc;
+	then = rtc_tm_to_time64(&alarm.time);
+	now = rtc_tm_to_time64(&tm);
+	duration = then-now;
+
+	/* in the past */
+	if (then < now)
+		return 0;
+
+	/* will be stored in upper 16 bits of s0i3 hint argument,
+	 * so timer wakeup from s0i3 is limited to ~18 hours or less
+	 */
+	if (duration <= 4 || duration > U16_MAX)
+		return -EINVAL;
+
+	*arg |= (duration << 16);
+	rc = rtc_alarm_irq_enable(rtc_device, 0);
+	dev_info(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
+
+	return rc;
+}
+
 static int __maybe_unused amd_pmc_suspend(struct device *dev)
 {
 	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
 	int rc;
 	u8 msg;
+	u32 arg = 1;
 
 	/* Reset and Start SMU logging - to monitor the s0i3 stats */
 	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
 	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
 
+	/* Activate CZN specific RTC functionality */
+	if (pdev->cpu_id == AMD_CPU_ID_CZN) {
+		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
+		if (rc < 0)
+			return rc;
+	}
+
 	/* Dump the IdleMask before we send hint to SMU */
 	amd_pmc_idlemask_read(pdev, dev, NULL);
 	msg = amd_pmc_get_os_hint(pdev);
-	rc = amd_pmc_send_cmd(pdev, 1, NULL, msg, 0);
+	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
 	if (rc)
 		dev_err(pdev->dev, "suspend failed\n");
 
-- 
2.25.1

Re: [PATCH 1/2] platform/x86: amd-pmc: adjust arguments for `amd_pmc_send_cmd`

[Hans de Goede]

Hi,

On 10/20/21 18:29, Mario Limonciello wrote:
> Currently the "argument" for the "message" is listed as a boolean
> value.  This works well for the commands used currently, but an
> additional upcoming command will pass more data in the message.
> 
> Expand it to be a full 32 bit value.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
>  drivers/platform/x86/amd-pmc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index b7b6ad4629a7..99ac50616bc3 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -126,7 +126,7 @@ struct amd_pmc_dev {
>  };
>  
>  static struct amd_pmc_dev pmc;
> -static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set, u32 *data, u8 msg, bool ret);
> +static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
>  
>  static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
>  {
> @@ -337,7 +337,7 @@ static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
>  	dev_dbg(dev->dev, "AMD_PMC_REGISTER_MESSAGE:%x\n", value);
>  }
>  
> -static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set, u32 *data, u8 msg, bool ret)
> +static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret)
>  {
>  	int rc;
>  	u32 val;
> @@ -356,7 +356,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set, u32 *data, u8 msg
>  	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_RESPONSE, 0);
>  
>  	/* Write argument into response register */
> -	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_ARGUMENT, set);
> +	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_ARGUMENT, arg);
>  
>  	/* Write message ID to message ID register */
>  	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_MESSAGE, msg);
> 

Re: [PATCH 2/2] platform/x86: amd-pmc: Add special handling for timer based S0i3 wakeup

[Alexandre Belloni]

Hello Mario,

On 20/10/2021 11:29:46-0500, Mario Limonciello wrote:
> RTC based wakeup from s0i3 doesn't work properly on some Green Sardine
> platforms. Because of this, a newer SMU for Green Sardine has the ability
> to pass wakeup time as argument of the upper 16 bits of OS_HINT message.
> 
> With older firmware setting the timer value in OS_HINT will cause firmware
> to reject the hint, so only run this path on:
> 1) Green Sardine
> 2) Minimum SMU FW
> 3) RTC alarm armed during s0i3 entry
> 
> Using this method has some limitations that the s0i3 wakeup will need to
> be between 4 seconds and 18 hours, so check those boundary conditions as
> well and abort the suspend if RTC is armed for too short or too long of a
> duration.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/platform/x86/amd-pmc.c | 60 +++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index 99ac50616bc3..678bf6874c63 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -17,9 +17,11 @@
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
> +#include <linux/limits.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
> +#include <linux/rtc.h>
>  #include <linux/suspend.h>
>  #include <linux/seq_file.h>
>  #include <linux/uaccess.h>
> @@ -412,20 +414,76 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
>  	return -EINVAL;
>  }
>  
> +static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
> +{
> +	struct rtc_device *rtc_device;
> +	time64_t then, now, duration;
> +	struct rtc_wkalrm alarm;
> +	struct rtc_time tm;
> +	int rc;
> +
> +	if (pdev->major < 64 || (pdev->major == 64 && pdev->minor < 53))
> +		return 0;
> +
> +	rtc_device = rtc_class_open(CONFIG_RTC_SYSTOHC_DEVICE);

No this will not work as you can see in your and Arnd's patch adding
ifdefery to handle RTC_SYSTOHC_DEVICE not being set. You have no
guarantee that RTC_SYSTOHC_DEVICE is the device you are looking for. You
need to rework that part.

> +	if (!rtc_device)
> +		return 0;
> +	rc = rtc_read_alarm(rtc_device, &alarm);
> +	if (rc)
> +		return rc;
> +	if (!alarm.enabled) {
> +		dev_dbg(pdev->dev, "alarm not enabled\n");
> +		return 0;
> +	}
> +	rc = rtc_valid_tm(&alarm.time);
> +	if (rc)
> +		return rc;

Why do you think the RTC core give you an invalid alarm time?

> +	rc = rtc_read_time(rtc_device, &tm);
> +	if (rc)
> +		return rc;
> +	then = rtc_tm_to_time64(&alarm.time);
> +	now = rtc_tm_to_time64(&tm);
> +	duration = then-now;
> +
> +	/* in the past */
> +	if (then < now)
> +		return 0;
> +
> +	/* will be stored in upper 16 bits of s0i3 hint argument,
> +	 * so timer wakeup from s0i3 is limited to ~18 hours or less
> +	 */
> +	if (duration <= 4 || duration > U16_MAX)
> +		return -EINVAL;
> +
> +	*arg |= (duration << 16);
> +	rc = rtc_alarm_irq_enable(rtc_device, 0);

What if userspace is waiting for the alarm to happen?

> +	dev_info(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
> +

Do you actually need this message, looks like leftover debug to me?

> +	return rc;
> +}
> +
>  static int __maybe_unused amd_pmc_suspend(struct device *dev)
>  {
>  	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
>  	int rc;
>  	u8 msg;
> +	u32 arg = 1;
>  
>  	/* Reset and Start SMU logging - to monitor the s0i3 stats */
>  	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
>  	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
>  
> +	/* Activate CZN specific RTC functionality */
> +	if (pdev->cpu_id == AMD_CPU_ID_CZN) {
> +		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
> +		if (rc < 0)
> +			return rc;
> +	}
> +
>  	/* Dump the IdleMask before we send hint to SMU */
>  	amd_pmc_idlemask_read(pdev, dev, NULL);
>  	msg = amd_pmc_get_os_hint(pdev);
> -	rc = amd_pmc_send_cmd(pdev, 1, NULL, msg, 0);
> +	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
>  	if (rc)
>  		dev_err(pdev->dev, "suspend failed\n");
>  
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/2] platform/x86: amd-pmc: Add special handling for timer based S0i3 wakeup

[Limonciello, Mario]

On 10/26/2021 09:51, Alexandre Belloni wrote:
> Hello Mario,
> 
> On 20/10/2021 11:29:46-0500, Mario Limonciello wrote:
>> RTC based wakeup from s0i3 doesn't work properly on some Green Sardine
>> platforms. Because of this, a newer SMU for Green Sardine has the ability
>> to pass wakeup time as argument of the upper 16 bits of OS_HINT message.
>>
>> With older firmware setting the timer value in OS_HINT will cause firmware
>> to reject the hint, so only run this path on:
>> 1) Green Sardine
>> 2) Minimum SMU FW
>> 3) RTC alarm armed during s0i3 entry
>>
>> Using this method has some limitations that the s0i3 wakeup will need to
>> be between 4 seconds and 18 hours, so check those boundary conditions as
>> well and abort the suspend if RTC is armed for too short or too long of a
>> duration.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/platform/x86/amd-pmc.c | 60 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
>> index 99ac50616bc3..678bf6874c63 100644
>> --- a/drivers/platform/x86/amd-pmc.c
>> +++ b/drivers/platform/x86/amd-pmc.c
>> @@ -17,9 +17,11 @@
>>   #include <linux/delay.h>
>>   #include <linux/io.h>
>>   #include <linux/iopoll.h>
>> +#include <linux/limits.h>
>>   #include <linux/module.h>
>>   #include <linux/pci.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/rtc.h>
>>   #include <linux/suspend.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/uaccess.h>
>> @@ -412,20 +414,76 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
>>   	return -EINVAL;
>>   }
>>   
>> +static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>> +{
>> +	struct rtc_device *rtc_device;
>> +	time64_t then, now, duration;
>> +	struct rtc_wkalrm alarm;
>> +	struct rtc_time tm;
>> +	int rc;
>> +
>> +	if (pdev->major < 64 || (pdev->major == 64 && pdev->minor < 53))
>> +		return 0;
>> +
>> +	rtc_device = rtc_class_open(CONFIG_RTC_SYSTOHC_DEVICE);
> 
> No this will not work as you can see in your and Arnd's patch adding
> ifdefery to handle RTC_SYSTOHC_DEVICE not being set. You have no
> guarantee that RTC_SYSTOHC_DEVICE is the device you are looking for. You
> need to rework that part.

I'm intending to send a new one back up to the ML to just hardcode it to 
"rtc0" per Hans' suggestion.

> 
>> +	if (!rtc_device)
>> +		return 0;
>> +	rc = rtc_read_alarm(rtc_device, &alarm);
>> +	if (rc)
>> +		return rc;
>> +	if (!alarm.enabled) {
>> +		dev_dbg(pdev->dev, "alarm not enabled\n");
>> +		return 0;
>> +	}
>> +	rc = rtc_valid_tm(&alarm.time);
>> +	if (rc)
>> +		return rc;
> 
> Why do you think the RTC core give you an invalid alarm time?

It doesn't matter "why" to me.  It's defensive programming.
The function has a return code to check for problems.  If an invalid 
alarm time was somehow programmed, we shouldn't be using it.

> 
>> +	rc = rtc_read_time(rtc_device, &tm);
>> +	if (rc)
>> +		return rc;
>> +	then = rtc_tm_to_time64(&alarm.time);
>> +	now = rtc_tm_to_time64(&tm);
>> +	duration = then-now;
>> +
>> +	/* in the past */
>> +	if (then < now)
>> +		return 0;
>> +
>> +	/* will be stored in upper 16 bits of s0i3 hint argument,
>> +	 * so timer wakeup from s0i3 is limited to ~18 hours or less
>> +	 */
>> +	if (duration <= 4 || duration > U16_MAX)
>> +		return -EINVAL;
>> +
>> +	*arg |= (duration << 16);
>> +	rc = rtc_alarm_irq_enable(rtc_device, 0);
> 
> What if userspace is waiting for the alarm to happen?

They won't receive it.  Note; this is no different than before the patch 
on these affected systems.

> 
>> +	dev_info(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
>> +
> 
> Do you actually need this message, looks like leftover debug to me?

Yes it was definitely used in debugging to produce this patch in the 
first place.

I left it for two reasons:
1) It makes it clear if someone is using this functionality when they 
report an error.

2) It shows if they were operating really close to the boundary 
conditions (and thus it may be required to adjust).

I am amenable to downgrading it to dev_dbg and on reported issues around 
this asking people to +p dynamic debugging for amd_pmc.

> 
>> +	return rc;
>> +}
>> +
>>   static int __maybe_unused amd_pmc_suspend(struct device *dev)
>>   {
>>   	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
>>   	int rc;
>>   	u8 msg;
>> +	u32 arg = 1;
>>   
>>   	/* Reset and Start SMU logging - to monitor the s0i3 stats */
>>   	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
>>   	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
>>   
>> +	/* Activate CZN specific RTC functionality */
>> +	if (pdev->cpu_id == AMD_CPU_ID_CZN) {
>> +		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
>> +		if (rc < 0)
>> +			return rc;
>> +	}
>> +
>>   	/* Dump the IdleMask before we send hint to SMU */
>>   	amd_pmc_idlemask_read(pdev, dev, NULL);
>>   	msg = amd_pmc_get_os_hint(pdev);
>> -	rc = amd_pmc_send_cmd(pdev, 1, NULL, msg, 0);
>> +	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
>>   	if (rc)
>>   		dev_err(pdev->dev, "suspend failed\n");
>>   
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH 2/2] platform/x86: amd-pmc: Add special handling for timer based S0i3 wakeup

[Alexandre Belloni]

On 26/10/2021 10:15:06-0500, Limonciello, Mario wrote:
> > 
> > > +	if (!rtc_device)
> > > +		return 0;
> > > +	rc = rtc_read_alarm(rtc_device, &alarm);
> > > +	if (rc)
> > > +		return rc;
> > > +	if (!alarm.enabled) {
> > > +		dev_dbg(pdev->dev, "alarm not enabled\n");
> > > +		return 0;
> > > +	}
> > > +	rc = rtc_valid_tm(&alarm.time);
> > > +	if (rc)
> > > +		return rc;
> > 
> > Why do you think the RTC core give you an invalid alarm time?
> 
> It doesn't matter "why" to me.  It's defensive programming.
> The function has a return code to check for problems.  If an invalid alarm
> time was somehow programmed, we shouldn't be using it.
> 

The question was not why it would happen, but why you think it could.
The RTC core will not return an invalid alarm, else, you'd already
catch that checking the return value of rtc_read_alarm. This is not
defensive programming, this is useless.

> > 
> > > +	rc = rtc_read_time(rtc_device, &tm);
> > > +	if (rc)
> > > +		return rc;
> > > +	then = rtc_tm_to_time64(&alarm.time);
> > > +	now = rtc_tm_to_time64(&tm);
> > > +	duration = then-now;
> > > +
> > > +	/* in the past */
> > > +	if (then < now)
> > > +		return 0;
> > > +
> > > +	/* will be stored in upper 16 bits of s0i3 hint argument,
> > > +	 * so timer wakeup from s0i3 is limited to ~18 hours or less
> > > +	 */
> > > +	if (duration <= 4 || duration > U16_MAX)
> > > +		return -EINVAL;
> > > +
> > > +	*arg |= (duration << 16);
> > > +	rc = rtc_alarm_irq_enable(rtc_device, 0);
> > 
> > What if userspace is waiting for the alarm to happen?
> 
> They won't receive it.  Note; this is no different than before the patch on
> these affected systems.
> 

But then shouldn't that be fixed? Why does the alarm need to be
disabled?

> > 
> > > +	dev_info(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
> > > +
> > 
> > Do you actually need this message, looks like leftover debug to me?
> 
> Yes it was definitely used in debugging to produce this patch in the first
> place.
> 
> I left it for two reasons:
> 1) It makes it clear if someone is using this functionality when they report
> an error.
> 
> 2) It shows if they were operating really close to the boundary conditions
> (and thus it may be required to adjust).
> 
> I am amenable to downgrading it to dev_dbg and on reported issues around
> this asking people to +p dynamic debugging for amd_pmc.
> 
> > 
> > > +	return rc;
> > > +}
> > > +
> > >   static int __maybe_unused amd_pmc_suspend(struct device *dev)
> > >   {
> > >   	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
> > >   	int rc;
> > >   	u8 msg;
> > > +	u32 arg = 1;
> > >   	/* Reset and Start SMU logging - to monitor the s0i3 stats */
> > >   	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
> > >   	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
> > > +	/* Activate CZN specific RTC functionality */
> > > +	if (pdev->cpu_id == AMD_CPU_ID_CZN) {
> > > +		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
> > > +		if (rc < 0)
> > > +			return rc;
> > > +	}
> > > +
> > >   	/* Dump the IdleMask before we send hint to SMU */
> > >   	amd_pmc_idlemask_read(pdev, dev, NULL);
> > >   	msg = amd_pmc_get_os_hint(pdev);
> > > -	rc = amd_pmc_send_cmd(pdev, 1, NULL, msg, 0);
> > > +	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
> > >   	if (rc)
> > >   		dev_err(pdev->dev, "suspend failed\n");
> > > -- 
> > > 2.25.1
> > > 
> > 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/2] platform/x86: amd-pmc: Add special handling for timer based S0i3 wakeup

[Limonciello, Mario]

On 10/26/2021 10:29, Alexandre Belloni wrote:
> On 26/10/2021 10:15:06-0500, Limonciello, Mario wrote:
>>>
>>>> +	if (!rtc_device)
>>>> +		return 0;
>>>> +	rc = rtc_read_alarm(rtc_device, &alarm);
>>>> +	if (rc)
>>>> +		return rc;
>>>> +	if (!alarm.enabled) {
>>>> +		dev_dbg(pdev->dev, "alarm not enabled\n");
>>>> +		return 0;
>>>> +	}
>>>> +	rc = rtc_valid_tm(&alarm.time);
>>>> +	if (rc)
>>>> +		return rc;
>>>
>>> Why do you think the RTC core give you an invalid alarm time?
>>
>> It doesn't matter "why" to me.  It's defensive programming.
>> The function has a return code to check for problems.  If an invalid alarm
>> time was somehow programmed, we shouldn't be using it.
>>
> 
> The question was not why it would happen, but why you think it could.
> The RTC core will not return an invalid alarm, else, you'd already
> catch that checking the return value of rtc_read_alarm. This is not
> defensive programming, this is useless.

Very well, I'll modify and drop this part in a follow up series.

> 
>>>
>>>> +	rc = rtc_read_time(rtc_device, &tm);
>>>> +	if (rc)
>>>> +		return rc;
>>>> +	then = rtc_tm_to_time64(&alarm.time);
>>>> +	now = rtc_tm_to_time64(&tm);
>>>> +	duration = then-now;
>>>> +
>>>> +	/* in the past */
>>>> +	if (then < now)
>>>> +		return 0;
>>>> +
>>>> +	/* will be stored in upper 16 bits of s0i3 hint argument,
>>>> +	 * so timer wakeup from s0i3 is limited to ~18 hours or less
>>>> +	 */
>>>> +	if (duration <= 4 || duration > U16_MAX)
>>>> +		return -EINVAL;
>>>> +
>>>> +	*arg |= (duration << 16);
>>>> +	rc = rtc_alarm_irq_enable(rtc_device, 0);
>>>
>>> What if userspace is waiting for the alarm to happen?
>>
>> They won't receive it.  Note; this is no different than before the patch on
>> these affected systems.
>>
> 
> But then shouldn't that be fixed? Why does the alarm need to be
> disabled?

The reason for this patch was that the RTC is not a valid wake source 
during s0i3 on the affected designs but the Linux ecosystem heavily uses 
it for stress testing.  Leaving the RTC armed will cause undefined behavior.

I do want to note that this constraint does not exist in future designs 
(such as Yellow Carp).

> 
>>>
>>>> +	dev_info(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
>>>> +
>>>
>>> Do you actually need this message, looks like leftover debug to me?
>>
>> Yes it was definitely used in debugging to produce this patch in the first
>> place.
>>
>> I left it for two reasons:
>> 1) It makes it clear if someone is using this functionality when they report
>> an error.
>>
>> 2) It shows if they were operating really close to the boundary conditions
>> (and thus it may be required to adjust).
>>
>> I am amenable to downgrading it to dev_dbg and on reported issues around
>> this asking people to +p dynamic debugging for amd_pmc.
>>
>>>
>>>> +	return rc;
>>>> +}
>>>> +
>>>>    static int __maybe_unused amd_pmc_suspend(struct device *dev)
>>>>    {
>>>>    	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
>>>>    	int rc;
>>>>    	u8 msg;
>>>> +	u32 arg = 1;
>>>>    	/* Reset and Start SMU logging - to monitor the s0i3 stats */
>>>>    	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
>>>>    	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
>>>> +	/* Activate CZN specific RTC functionality */
>>>> +	if (pdev->cpu_id == AMD_CPU_ID_CZN) {
>>>> +		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
>>>> +		if (rc < 0)
>>>> +			return rc;
>>>> +	}
>>>> +
>>>>    	/* Dump the IdleMask before we send hint to SMU */
>>>>    	amd_pmc_idlemask_read(pdev, dev, NULL);
>>>>    	msg = amd_pmc_get_os_hint(pdev);
>>>> -	rc = amd_pmc_send_cmd(pdev, 1, NULL, msg, 0);
>>>> +	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
>>>>    	if (rc)
>>>>    		dev_err(pdev->dev, "suspend failed\n");
>>>> -- 
>>>> 2.25.1
>>>>
>>>
>>
>