Re: [PATCH v4 3/3] platform/x86/amd/pmc: Don't log during intermediate wakeups

[Hans de Goede <hansg@kernel.org>]

Hi,

On 6-Jun-26 6:47 AM, Daniel Gibson wrote:
> The ECs in the IdeaPads that need the delay_suspend quirk send lots
> of messages when charging, which not only causes intermediate wakeups
> when suspended, but also prevents the device from reaching the deepest
> suspend state.
> 
> Because of this amd_pmc_intermediate_wakeup_need_delay() returns false
> during intermediate wakeups and amd_pmc_want_suspend_delay() is called.
> So far it always logged its "Delaying suspend by 2.5s ..." messages
> then, which spams dmesg. This commit makes sure that those messages are
> only logged once per suspend.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=221383
> Signed-off-by: Daniel Gibson <daniel@gibson.sh>
> Cc: stable@vger.kernel.org

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
>  drivers/platform/x86/amd/pmc/pmc.c | 39 ++++++++++++++++++++++++------
>  drivers/platform/x86/amd/pmc/pmc.h |  1 +
>  2 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 2d3d180c15d2..7d772ccd17a6 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -619,6 +619,20 @@ static bool amd_pmc_intermediate_wakeup_need_delay(struct amd_pmc_dev *pdev)
>  
>  static bool amd_pmc_want_suspend_delay(struct amd_pmc_dev *pdev)
>  {
> +	/*
> +	 * intermediate_wakeup implies that the machine didn't get to deepest sleep
> +	 * state before - otherwise this function isn't called in amd_pmc_s2idle_check()
> +	 * because amd_pmc_intermediate_wakeup_need_delay() returns true first.
> +	 * On some IdeaPads that happens when charging, because the EC seems
> +	 * to send lots of messages then that wake the machine.
> +	 *
> +	 * But even in that case, the sleep here is necessary (on those IdeaPads),
> +	 * otherwise they wake up completely (resume) after a few seconds.
> +	 * So this variable is only used to avoid spamming dmesg on each
> +	 * intermediate wakeup.
> +	 */
> +	bool intermediate_wakeup = !pdev->is_first_check_after_suspend;
> +
>  	/*
>  	 * Some Lenovo Laptops (like different IdeaPad 3 Slims) need some
>  	 * me-time before sleeping or they get uncooperative after waking
> @@ -637,17 +651,20 @@ static bool amd_pmc_want_suspend_delay(struct amd_pmc_dev *pdev)
>  		 * disabled with disable_workarounds or delay_suspend=0
>  		 */
>  		if (delay_suspend == 1 || (delay_suspend == -1 && !disable_workarounds)) {
> -			dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
> +			if (!intermediate_wakeup)
> +				dev_info(pdev->dev, "Delaying suspend by 2.5s to avoid platform bug\n");
>  			return true;
>  		}
> -		dev_info(pdev->dev, "Not delaying suspend because of module parameter, even though your device is assumed to need it!\n");
> +		if (!intermediate_wakeup)
> +			dev_info(pdev->dev, "Not delaying suspend because of module parameter, even though your device is assumed to need it!\n");
>  	} else if (delay_suspend == 1) {
> -		dev_info(pdev->dev, "Delaying suspend by 2.5s because delay_suspend=1. If this solves problems on your machine, please report this whole line to: platform-driver-x86@vger.kernel.org so it can be automatically detected as affected in the future. System Vendor: \"%s\" Product Name: \"%s\" Product Family: \"%s\" Board Vendor: \"%s\" Board Name: \"%s\"\n",
> -			 dmi_get_system_info(DMI_SYS_VENDOR),
> -			 dmi_get_system_info(DMI_PRODUCT_NAME),
> -			 dmi_get_system_info(DMI_PRODUCT_FAMILY),
> -			 dmi_get_system_info(DMI_BOARD_VENDOR),
> -			 dmi_get_system_info(DMI_BOARD_NAME));
> +		if (!intermediate_wakeup)
> +			dev_info(pdev->dev, "Delaying suspend by 2.5s because delay_suspend=1. If this solves problems on your machine, please report this whole line to: platform-driver-x86@vger.kernel.org so it can be automatically detected as affected in the future. System Vendor: \"%s\" Product Name: \"%s\" Product Family: \"%s\" Board Vendor: \"%s\" Board Name: \"%s\"\n",
> +				 dmi_get_system_info(DMI_SYS_VENDOR),
> +				 dmi_get_system_info(DMI_PRODUCT_NAME),
> +				 dmi_get_system_info(DMI_PRODUCT_FAMILY),
> +				 dmi_get_system_info(DMI_BOARD_VENDOR),
> +				 dmi_get_system_info(DMI_BOARD_NAME));
>  		return true;
>  	}
>  	return false;
> @@ -660,6 +677,9 @@ static void amd_pmc_s2idle_prepare(void)
>  	u8 msg;
>  	u32 arg = 1;
>  
> +	/* Reset this variable because this is a fresh suspend */
> +	pdev->is_first_check_after_suspend = true;
> +
>  	/* Reset and Start SMU logging - to monitor the s0i3 stats */
>  	amd_pmc_setup_smu_logging(pdev);
>  
> @@ -699,6 +719,9 @@ static void amd_pmc_s2idle_check(void)
>  	rc = amd_stb_write(pdev, AMD_PMC_STB_S2IDLE_CHECK);
>  	if (rc)
>  		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
> +
> +	/* remember that first check after suspend is done (until next prepare) */
> +	pdev->is_first_check_after_suspend = false;
>  }
>  
>  static int amd_pmc_dump_data(struct amd_pmc_dev *pdev)
> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
> index f5257e47b8c4..8aa7073ed09f 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.h
> +++ b/drivers/platform/x86/amd/pmc/pmc.h
> @@ -114,6 +114,7 @@ struct amd_pmc_dev {
>  	struct dentry *dbgfs_dir;
>  	struct quirk_entry *quirks;
>  	bool disable_8042_wakeup;
> +	bool is_first_check_after_suspend;
>  	struct amd_mp2_dev *mp2;
>  	struct stb_arg stb_arg;
>  };