Wakeup for IRQ1 should be disabled only in cases where i8042 had actually
enabled it, otherwise "wake_depth" for this IRQ will try do drop below zero
and there will be an unpleasant WARN() logged:
kernel: atkbd serio0: Disabling IRQ1 wakeup source to avoid platform firmware bug
kernel: ------------[ cut here ]------------
kernel: Unbalanced IRQ 1 wake disable
kernel: WARNING: CPU: 10 PID: 6431 at kernel/irq/manage.c:920 irq_set_irq_wake+0x147/0x1a0

To fix this call the PMC suspend handler only from the same set of
dev_pm_ops handlers as i8042_pm_suspend() is called, which currently means
just the ".suspend" handler.
Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
dev_pm_ops, which also called this handler on ".freeze" and ".poweroff".

To reproduce this issue try hibernating (S4) the machine after a fresh boot
without putting it into s2idle first.

Fixes: 8e60615e8932 ("platform/x86/amd: pmc: Disable IRQ1 wakeup for RN/CZN")
Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
 drivers/platform/x86/amd/pmc/pmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index 26b878ee5191..a254debb9256 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -947,6 +947,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
 {
 	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
 
+	/*
+	 * Must be called only from the same set of dev_pm_ops handlers
+	 * as i8042_pm_suspend() is called: currently just from .suspend.
+	 */
 	if (pdev->disable_8042_wakeup && !disable_workarounds) {
 		int rc = amd_pmc_wa_irq1(pdev);
 
@@ -959,7 +963,9 @@ static int amd_pmc_suspend_handler(struct device *dev)
 	return 0;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler, NULL);
+static const struct dev_pm_ops amd_pmc_pm = {
+	.suspend = amd_pmc_suspend_handler,
+};
 
 static const struct pci_device_id pmc_pci_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_PS) },

On 12/29/2024 10:43, Maciej S. Szmigiero wrote:
> Wakeup for IRQ1 should be disabled only in cases where i8042 had actually
> enabled it, otherwise "wake_depth" for this IRQ will try do drop below zero
> and there will be an unpleasant WARN() logged:
> kernel: atkbd serio0: Disabling IRQ1 wakeup source to avoid platform firmware bug
> kernel: ------------[ cut here ]------------
> kernel: Unbalanced IRQ 1 wake disable
> kernel: WARNING: CPU: 10 PID: 6431 at kernel/irq/manage.c:920 irq_set_irq_wake+0x147/0x1a0
> 
> To fix this call the PMC suspend handler only from the same set of
> dev_pm_ops handlers as i8042_pm_suspend() is called, which currently means
> just the ".suspend" handler.
> Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
> dev_pm_ops, which also called this handler on ".freeze" and ".poweroff".
> 
> To reproduce this issue try hibernating (S4) the machine after a fresh boot
> without putting it into s2idle first.
> 
> Fixes: 8e60615e8932 ("platform/x86/amd: pmc: Disable IRQ1 wakeup for RN/CZN")
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>

The code change makes sense to me.  You can add this when you resubmit 
v2 for the commit message changes requested by Ilpo.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/platform/x86/amd/pmc/pmc.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 26b878ee5191..a254debb9256 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -947,6 +947,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
>   {
>   	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
>   
> +	/*
> +	 * Must be called only from the same set of dev_pm_ops handlers
> +	 * as i8042_pm_suspend() is called: currently just from .suspend.
> +	 */
>   	if (pdev->disable_8042_wakeup && !disable_workarounds) {
>   		int rc = amd_pmc_wa_irq1(pdev);
>   
> @@ -959,7 +963,9 @@ static int amd_pmc_suspend_handler(struct device *dev)
>   	return 0;
>   }
>   
> -static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler, NULL);
> +static const struct dev_pm_ops amd_pmc_pm = {
> +	.suspend = amd_pmc_suspend_handler,
> +};
>   
>   static const struct pci_device_id pmc_pci_ids[] = {
>   	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_PS) },

On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:

> Wakeup for IRQ1 should be disabled only in cases where i8042 had actually
> enabled it, otherwise "wake_depth" for this IRQ will try do drop below zero
> and there will be an unpleasant WARN() logged:
> kernel: atkbd serio0: Disabling IRQ1 wakeup source to avoid platform firmware bug
> kernel: ------------[ cut here ]------------
> kernel: Unbalanced IRQ 1 wake disable
> kernel: WARNING: CPU: 10 PID: 6431 at kernel/irq/manage.c:920 irq_set_irq_wake+0x147/0x1a0
> 
> To fix this call the PMC suspend handler only from the same set of
> dev_pm_ops handlers as i8042_pm_suspend() is called, which currently means
> just the ".suspend" handler.
> Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
> dev_pm_ops, which also called this handler on ".freeze" and ".poweroff".
> 
> To reproduce this issue try hibernating (S4) the machine after a fresh boot
> without putting it into s2idle first.
> 
> Fixes: 8e60615e8932 ("platform/x86/amd: pmc: Disable IRQ1 wakeup for RN/CZN")
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  drivers/platform/x86/amd/pmc/pmc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 26b878ee5191..a254debb9256 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -947,6 +947,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
>  {
>  	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
>  
> +	/*
> +	 * Must be called only from the same set of dev_pm_ops handlers
> +	 * as i8042_pm_suspend() is called: currently just from .suspend.
> +	 */
>  	if (pdev->disable_8042_wakeup && !disable_workarounds) {
>  		int rc = amd_pmc_wa_irq1(pdev);
>  
> @@ -959,7 +963,9 @@ static int amd_pmc_suspend_handler(struct device *dev)
>  	return 0;
>  }
>  
> -static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler, NULL);
> +static const struct dev_pm_ops amd_pmc_pm = {
> +	.suspend = amd_pmc_suspend_handler,
> +};

???

I cannot see what this change is supposed to achieve. 

#define _DEFINE_DEV_PM_OPS(name, \
                           suspend_fn, resume_fn, \
                           runtime_suspend_fn, runtime_resume_fn, idle_fn) \
const struct dev_pm_ops name = { \
        SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
        RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
}

#define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
        _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)

#define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
        .suspend = pm_sleep_ptr(suspend_fn), \
        .resume = pm_sleep_ptr(resume_fn), \
        .freeze = pm_sleep_ptr(suspend_fn), \
        .thaw = pm_sleep_ptr(resume_fn), \
        .poweroff = pm_sleep_ptr(suspend_fn), \
        .restore = pm_sleep_ptr(resume_fn),

#define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))

Under what circumstances does this change result in some difference?

-- 
 i.

On 29.12.2024 17:58, Ilpo Järvinen wrote:
> On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:
> 
>> Wakeup for IRQ1 should be disabled only in cases where i8042 had actually
>> enabled it, otherwise "wake_depth" for this IRQ will try do drop below zero
>> and there will be an unpleasant WARN() logged:
>> kernel: atkbd serio0: Disabling IRQ1 wakeup source to avoid platform firmware bug
>> kernel: ------------[ cut here ]------------
>> kernel: Unbalanced IRQ 1 wake disable
>> kernel: WARNING: CPU: 10 PID: 6431 at kernel/irq/manage.c:920 irq_set_irq_wake+0x147/0x1a0
>>
>> To fix this call the PMC suspend handler only from the same set of
>> dev_pm_ops handlers as i8042_pm_suspend() is called, which currently means
>> just the ".suspend" handler.
>> Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
>> dev_pm_ops, which also called this handler on ".freeze" and ".poweroff".
>>
>> To reproduce this issue try hibernating (S4) the machine after a fresh boot
>> without putting it into s2idle first.
>>
>> Fixes: 8e60615e8932 ("platform/x86/amd: pmc: Disable IRQ1 wakeup for RN/CZN")
>> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>> ---
>>   drivers/platform/x86/amd/pmc/pmc.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index 26b878ee5191..a254debb9256 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -947,6 +947,10 @@ static int amd_pmc_suspend_handler(struct device *dev)
>>   {
>>   	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
>>   
>> +	/*
>> +	 * Must be called only from the same set of dev_pm_ops handlers
>> +	 * as i8042_pm_suspend() is called: currently just from .suspend.
>> +	 */
>>   	if (pdev->disable_8042_wakeup && !disable_workarounds) {
>>   		int rc = amd_pmc_wa_irq1(pdev);
>>   
>> @@ -959,7 +963,9 @@ static int amd_pmc_suspend_handler(struct device *dev)
>>   	return 0;
>>   }
>>   
>> -static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler, NULL);
>> +static const struct dev_pm_ops amd_pmc_pm = {
>> +	.suspend = amd_pmc_suspend_handler,
>> +};
> 
> ???
> 
> I cannot see what this change is supposed to achieve.
> 
> #define _DEFINE_DEV_PM_OPS(name, \
>                             suspend_fn, resume_fn, \
>                             runtime_suspend_fn, runtime_resume_fn, idle_fn) \
> const struct dev_pm_ops name = { \
>          SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>          RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
> }
> 
> #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
>          _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)
> 
> #define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>          .suspend = pm_sleep_ptr(suspend_fn), \
>          .resume = pm_sleep_ptr(resume_fn), \
>          .freeze = pm_sleep_ptr(suspend_fn), \
>          .thaw = pm_sleep_ptr(resume_fn), \
>          .poweroff = pm_sleep_ptr(suspend_fn), \
>          .restore = pm_sleep_ptr(resume_fn),
> 
> #define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
> 
> Under what circumstances does this change result in some difference?
> 

.freeze and .poweroff are now both NULL, just like in the i8042 driver.

As I wrote in the commit message:
>> Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
>> dev_pm_ops, *which also called this handler on ".freeze" and ".poweroff".*

Thanks,
Maciej

On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:

> On 29.12.2024 17:58, Ilpo Järvinen wrote:
> > On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:
> > 
> > > Wakeup for IRQ1 should be disabled only in cases where i8042 had actually
> > > enabled it, otherwise "wake_depth" for this IRQ will try do drop below
> > > zero
> > > and there will be an unpleasant WARN() logged:
> > > kernel: atkbd serio0: Disabling IRQ1 wakeup source to avoid platform
> > > firmware bug
> > > kernel: ------------[ cut here ]------------
> > > kernel: Unbalanced IRQ 1 wake disable
> > > kernel: WARNING: CPU: 10 PID: 6431 at kernel/irq/manage.c:920
> > > irq_set_irq_wake+0x147/0x1a0
> > > 
> > > To fix this call the PMC suspend handler only from the same set of
> > > dev_pm_ops handlers as i8042_pm_suspend() is called, which currently means
> > > just the ".suspend" handler.
> > > Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
> > > dev_pm_ops, which also called this handler on ".freeze" and ".poweroff".
> > > 
> > > To reproduce this issue try hibernating (S4) the machine after a fresh
> > > boot
> > > without putting it into s2idle first.
> > > 
> > > Fixes: 8e60615e8932 ("platform/x86/amd: pmc: Disable IRQ1 wakeup for
> > > RN/CZN")
> > > Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > > ---
> > >   drivers/platform/x86/amd/pmc/pmc.c | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/platform/x86/amd/pmc/pmc.c
> > > b/drivers/platform/x86/amd/pmc/pmc.c
> > > index 26b878ee5191..a254debb9256 100644
> > > --- a/drivers/platform/x86/amd/pmc/pmc.c
> > > +++ b/drivers/platform/x86/amd/pmc/pmc.c
> > > @@ -947,6 +947,10 @@ static int amd_pmc_suspend_handler(struct device
> > > *dev)
> > >   {
> > >   	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
> > >   +	/*
> > > +	 * Must be called only from the same set of dev_pm_ops handlers
> > > +	 * as i8042_pm_suspend() is called: currently just from .suspend.
> > > +	 */
> > >   	if (pdev->disable_8042_wakeup && !disable_workarounds) {
> > >   		int rc = amd_pmc_wa_irq1(pdev);
> > >   @@ -959,7 +963,9 @@ static int amd_pmc_suspend_handler(struct device
> > > *dev)
> > >   	return 0;
> > >   }
> > >   -static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler,
> > > NULL);
> > > +static const struct dev_pm_ops amd_pmc_pm = {
> > > +	.suspend = amd_pmc_suspend_handler,
> > > +};
> > 
> > ???
> > 
> > I cannot see what this change is supposed to achieve.
> > 
> > #define _DEFINE_DEV_PM_OPS(name, \
> >                             suspend_fn, resume_fn, \
> >                             runtime_suspend_fn, runtime_resume_fn, idle_fn)
> > \
> > const struct dev_pm_ops name = { \
> >          SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> >          RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
> > }
> > 
> > #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> >          _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)
> > 
> > #define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> >          .suspend = pm_sleep_ptr(suspend_fn), \
> >          .resume = pm_sleep_ptr(resume_fn), \
> >          .freeze = pm_sleep_ptr(suspend_fn), \
> >          .thaw = pm_sleep_ptr(resume_fn), \
> >          .poweroff = pm_sleep_ptr(suspend_fn), \
> >          .restore = pm_sleep_ptr(resume_fn),
> > 
> > #define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
> > 
> > Under what circumstances does this change result in some difference?
> > 
> 
> .freeze and .poweroff are now both NULL, just like in the i8042 driver.
> 
> As I wrote in the commit message:
> > > Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
> > > dev_pm_ops, *which also called this handler on ".freeze" and ".poweroff".*

Also, please avoid using "previously" like this. I interpreted it like 
some old kernel did that.

-- 
 i.

On 29.12.2024 18:19, Ilpo Järvinen wrote:
> On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:
> 
>> On 29.12.2024 17:58, Ilpo Järvinen wrote:
>>> On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:
>>>
>>>> Wakeup for IRQ1 should be disabled only in cases where i8042 had actually
>>>> enabled it, otherwise "wake_depth" for this IRQ will try do drop below
>>>> zero
>>>> and there will be an unpleasant WARN() logged:
>>>> kernel: atkbd serio0: Disabling IRQ1 wakeup source to avoid platform
>>>> firmware bug
>>>> kernel: ------------[ cut here ]------------
>>>> kernel: Unbalanced IRQ 1 wake disable
>>>> kernel: WARNING: CPU: 10 PID: 6431 at kernel/irq/manage.c:920
>>>> irq_set_irq_wake+0x147/0x1a0
>>>>
>>>> To fix this call the PMC suspend handler only from the same set of
>>>> dev_pm_ops handlers as i8042_pm_suspend() is called, which currently means
>>>> just the ".suspend" handler.
>>>> Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
>>>> dev_pm_ops, which also called this handler on ".freeze" and ".poweroff".
>>>>
>>>> To reproduce this issue try hibernating (S4) the machine after a fresh
>>>> boot
>>>> without putting it into s2idle first.
>>>>
>>>> Fixes: 8e60615e8932 ("platform/x86/amd: pmc: Disable IRQ1 wakeup for
>>>> RN/CZN")
>>>> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>>>> ---
>>>>    drivers/platform/x86/amd/pmc/pmc.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c
>>>> b/drivers/platform/x86/amd/pmc/pmc.c
>>>> index 26b878ee5191..a254debb9256 100644
>>>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>>>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>>>> @@ -947,6 +947,10 @@ static int amd_pmc_suspend_handler(struct device
>>>> *dev)
>>>>    {
>>>>    	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
>>>>    +	/*
>>>> +	 * Must be called only from the same set of dev_pm_ops handlers
>>>> +	 * as i8042_pm_suspend() is called: currently just from .suspend.
>>>> +	 */
>>>>    	if (pdev->disable_8042_wakeup && !disable_workarounds) {
>>>>    		int rc = amd_pmc_wa_irq1(pdev);
>>>>    @@ -959,7 +963,9 @@ static int amd_pmc_suspend_handler(struct device
>>>> *dev)
>>>>    	return 0;
>>>>    }
>>>>    -static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler,
>>>> NULL);
>>>> +static const struct dev_pm_ops amd_pmc_pm = {
>>>> +	.suspend = amd_pmc_suspend_handler,
>>>> +};
>>>
>>> ???
>>>
>>> I cannot see what this change is supposed to achieve.
>>>
>>> #define _DEFINE_DEV_PM_OPS(name, \
>>>                              suspend_fn, resume_fn, \
>>>                              runtime_suspend_fn, runtime_resume_fn, idle_fn)
>>> \
>>> const struct dev_pm_ops name = { \
>>>           SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>>>           RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
>>> }
>>>
>>> #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
>>>           _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)
>>>
>>> #define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>>>           .suspend = pm_sleep_ptr(suspend_fn), \
>>>           .resume = pm_sleep_ptr(resume_fn), \
>>>           .freeze = pm_sleep_ptr(suspend_fn), \
>>>           .thaw = pm_sleep_ptr(resume_fn), \
>>>           .poweroff = pm_sleep_ptr(suspend_fn), \
>>>           .restore = pm_sleep_ptr(resume_fn),
>>>
>>> #define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
>>>
>>> Under what circumstances does this change result in some difference?
>>>
>>
>> .freeze and .poweroff are now both NULL, just like in the i8042 driver.
>>
>> As I wrote in the commit message:
>>>> Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
>>>> dev_pm_ops, *which also called this handler on ".freeze" and ".poweroff".*
> 
> Also, please avoid using "previously" like this. I interpreted it like
> some old kernel did that.
> 

Sure, that sentence in the commit message could be rewritten using
"the code before this patch" or something similar if that's easier to parse.

Thanks,
Maciej

On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:

> On 29.12.2024 17:58, Ilpo Järvinen wrote:
> > On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:
> > 
> > > Wakeup for IRQ1 should be disabled only in cases where i8042 had actually
> > > enabled it, otherwise "wake_depth" for this IRQ will try do drop below
> > > zero
> > > and there will be an unpleasant WARN() logged:
> > > kernel: atkbd serio0: Disabling IRQ1 wakeup source to avoid platform
> > > firmware bug
> > > kernel: ------------[ cut here ]------------
> > > kernel: Unbalanced IRQ 1 wake disable
> > > kernel: WARNING: CPU: 10 PID: 6431 at kernel/irq/manage.c:920
> > > irq_set_irq_wake+0x147/0x1a0
> > > 
> > > To fix this call the PMC suspend handler only from the same set of
> > > dev_pm_ops handlers as i8042_pm_suspend() is called, which currently means
> > > just the ".suspend" handler.
> > > Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
> > > dev_pm_ops, which also called this handler on ".freeze" and ".poweroff".
> > > 
> > > To reproduce this issue try hibernating (S4) the machine after a fresh
> > > boot
> > > without putting it into s2idle first.
> > > 
> > > Fixes: 8e60615e8932 ("platform/x86/amd: pmc: Disable IRQ1 wakeup for
> > > RN/CZN")
> > > Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > > ---
> > >   drivers/platform/x86/amd/pmc/pmc.c | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/platform/x86/amd/pmc/pmc.c
> > > b/drivers/platform/x86/amd/pmc/pmc.c
> > > index 26b878ee5191..a254debb9256 100644
> > > --- a/drivers/platform/x86/amd/pmc/pmc.c
> > > +++ b/drivers/platform/x86/amd/pmc/pmc.c
> > > @@ -947,6 +947,10 @@ static int amd_pmc_suspend_handler(struct device
> > > *dev)
> > >   {
> > >   	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
> > >   +	/*
> > > +	 * Must be called only from the same set of dev_pm_ops handlers
> > > +	 * as i8042_pm_suspend() is called: currently just from .suspend.
> > > +	 */
> > >   	if (pdev->disable_8042_wakeup && !disable_workarounds) {
> > >   		int rc = amd_pmc_wa_irq1(pdev);
> > >   @@ -959,7 +963,9 @@ static int amd_pmc_suspend_handler(struct device
> > > *dev)
> > >   	return 0;
> > >   }
> > >   -static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler,
> > > NULL);
> > > +static const struct dev_pm_ops amd_pmc_pm = {
> > > +	.suspend = amd_pmc_suspend_handler,
> > > +};
> > 
> > ???
> > 
> > I cannot see what this change is supposed to achieve.
> > 
> > #define _DEFINE_DEV_PM_OPS(name, \
> >                             suspend_fn, resume_fn, \
> >                             runtime_suspend_fn, runtime_resume_fn, idle_fn)
> > \
> > const struct dev_pm_ops name = { \
> >          SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> >          RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
> > }
> > 
> > #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> >          _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)
> > 
> > #define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> >          .suspend = pm_sleep_ptr(suspend_fn), \
> >          .resume = pm_sleep_ptr(resume_fn), \
> >          .freeze = pm_sleep_ptr(suspend_fn), \
> >          .thaw = pm_sleep_ptr(resume_fn), \
> >          .poweroff = pm_sleep_ptr(suspend_fn), \
> >          .restore = pm_sleep_ptr(resume_fn),
> > 
> > #define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
> > 
> > Under what circumstances does this change result in some difference?
> > 
> 
> .freeze and .poweroff are now both NULL, just like in the i8042 driver.
> 
> As I wrote in the commit message:
> > > Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
> > > dev_pm_ops, *which also called this handler on ".freeze" and ".poweroff".*

Ah, I'm sorry. Too much not aligned macro text to parse.

Will it now trigger a warning if some PM CONFIG is not enabled? Those 
pm_sleep_ptr() are there to avoid those warnings so the handler pointer 
would likely need to be wrapped inside pm_sleep_ptr().

-- 
 i.

On 29.12.2024 18:13, Ilpo Järvinen wrote:
> On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:
> 
>> On 29.12.2024 17:58, Ilpo Järvinen wrote:
>>> On Sun, 29 Dec 2024, Maciej S. Szmigiero wrote:
>>>
>>>> Wakeup for IRQ1 should be disabled only in cases where i8042 had actually
>>>> enabled it, otherwise "wake_depth" for this IRQ will try do drop below
>>>> zero
>>>> and there will be an unpleasant WARN() logged:
>>>> kernel: atkbd serio0: Disabling IRQ1 wakeup source to avoid platform
>>>> firmware bug
>>>> kernel: ------------[ cut here ]------------
>>>> kernel: Unbalanced IRQ 1 wake disable
>>>> kernel: WARNING: CPU: 10 PID: 6431 at kernel/irq/manage.c:920
>>>> irq_set_irq_wake+0x147/0x1a0
>>>>
>>>> To fix this call the PMC suspend handler only from the same set of
>>>> dev_pm_ops handlers as i8042_pm_suspend() is called, which currently means
>>>> just the ".suspend" handler.
>>>> Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
>>>> dev_pm_ops, which also called this handler on ".freeze" and ".poweroff".
>>>>
>>>> To reproduce this issue try hibernating (S4) the machine after a fresh
>>>> boot
>>>> without putting it into s2idle first.
>>>>
>>>> Fixes: 8e60615e8932 ("platform/x86/amd: pmc: Disable IRQ1 wakeup for
>>>> RN/CZN")
>>>> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>>>> ---
>>>>    drivers/platform/x86/amd/pmc/pmc.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c
>>>> b/drivers/platform/x86/amd/pmc/pmc.c
>>>> index 26b878ee5191..a254debb9256 100644
>>>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>>>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>>>> @@ -947,6 +947,10 @@ static int amd_pmc_suspend_handler(struct device
>>>> *dev)
>>>>    {
>>>>    	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
>>>>    +	/*
>>>> +	 * Must be called only from the same set of dev_pm_ops handlers
>>>> +	 * as i8042_pm_suspend() is called: currently just from .suspend.
>>>> +	 */
>>>>    	if (pdev->disable_8042_wakeup && !disable_workarounds) {
>>>>    		int rc = amd_pmc_wa_irq1(pdev);
>>>>    @@ -959,7 +963,9 @@ static int amd_pmc_suspend_handler(struct device
>>>> *dev)
>>>>    	return 0;
>>>>    }
>>>>    -static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler,
>>>> NULL);
>>>> +static const struct dev_pm_ops amd_pmc_pm = {
>>>> +	.suspend = amd_pmc_suspend_handler,
>>>> +};
>>>
>>> ???
>>>
>>> I cannot see what this change is supposed to achieve.
>>>
>>> #define _DEFINE_DEV_PM_OPS(name, \
>>>                              suspend_fn, resume_fn, \
>>>                              runtime_suspend_fn, runtime_resume_fn, idle_fn)
>>> \
>>> const struct dev_pm_ops name = { \
>>>           SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>>>           RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
>>> }
>>>
>>> #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
>>>           _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)
>>>
>>> #define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>>>           .suspend = pm_sleep_ptr(suspend_fn), \
>>>           .resume = pm_sleep_ptr(resume_fn), \
>>>           .freeze = pm_sleep_ptr(suspend_fn), \
>>>           .thaw = pm_sleep_ptr(resume_fn), \
>>>           .poweroff = pm_sleep_ptr(suspend_fn), \
>>>           .restore = pm_sleep_ptr(resume_fn),
>>>
>>> #define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
>>>
>>> Under what circumstances does this change result in some difference?
>>>
>>
>> .freeze and .poweroff are now both NULL, just like in the i8042 driver.
>>
>> As I wrote in the commit message:
>>>> Previously, the code would use DEFINE_SIMPLE_DEV_PM_OPS() to define its
>>>> dev_pm_ops, *which also called this handler on ".freeze" and ".poweroff".*
> 
> Ah, I'm sorry. Too much not aligned macro text to parse.
> 
> Will it now trigger a warning if some PM CONFIG is not enabled? Those
> pm_sleep_ptr() are there to avoid those warnings so the handler pointer
> would likely need to be wrapped inside pm_sleep_ptr().
> 

pm_sleep_ptr() only changes its meaning based on CONFIG_PM_SLEEP:
#define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))

CONFIG_PM_SLEEP gets enabled if CONFIG_SUSPEND is on:
> config PM_SLEEP
>	def_bool y
>	depends on SUSPEND || HIBERNATE_CALLBACKS

And CONFIG_SUSPEND has to be on since the whole driver depends on it:
> config AMD_PMC
>        tristate "AMD SoC PMC driver"
>        depends on ACPI && PCI && RTC_CLASS && AMD_NB
>        depends on SUSPEND

Thanks,
Maciej