RE: [PATCH] platform/x86/amd: pmc: remove CONFIG_SUSPEND checks

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Arnd Bergmann <arnd@xxxxxxxxxx>
> Sent: Tuesday, February 14, 2023 09:25
> To: S-k, Shyam-sundar <Shyam-sundar.S-k@xxxxxxx>; Hans de Goede
> <hdegoede@xxxxxxxxxx>; Mark Gross <markgross@xxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>; Nathan Chancellor <nathan@xxxxxxxxxx>;
> Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>; platform-driver-
> x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [PATCH] platform/x86/amd: pmc: remove CONFIG_SUSPEND checks
> 
> From: Arnd Bergmann <arnd@xxxxxxxx>
> 
> The amd_pmc_write_stb() function was previously hidden in an
> ifdef to avoid a warning when CONFIG_SUSPEND is disabled, but
> now there is an additional caller:
> 
> drivers/platform/x86/amd/pmc.c: In function
> 'amd_pmc_stb_debugfs_open_v2':
> drivers/platform/x86/amd/pmc.c:256:8: error: implicit declaration of function
> 'amd_pmc_write_stb'; did you mean 'amd_pmc_read_stb'? [-
> Werror=implicit-function-declaration]
>   256 |  ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
>       |        ^~~~~~~~~~~~~~~~~
>       |        amd_pmc_read_stb
> 
> There is now an easier way to handle this by using
> DEFINE_SIMPLE_DEV_PM_OPS()
> to replace all the #ifdefs, letting gcc drop any of the unused functions
> silently.
> 
> Fixes: b0d4bb973539 ("platform/x86/amd: pmc: Write dummy postcode into
> the STB DRAM")

I suspect this is not an appropriate fixes tag.  SIMPLE_DEV_PM_OPS only came in
8e60615e89321 ("platform/x86/amd: pmc: Disable IRQ1 wakeup for RN/CZN")

> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
>  drivers/platform/x86/amd/pmc.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc.c
> b/drivers/platform/x86/amd/pmc.c
> index ab05b9ee6655..641085906baf 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -171,9 +171,7 @@ MODULE_PARM_DESC(disable_workarounds,
> "Disable workarounds for platform bugs");
>  static struct amd_pmc_dev pmc;
>  static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32
> *data, u8 msg, bool ret);
>  static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
> -#ifdef CONFIG_SUSPEND
>  static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
> -#endif
> 
>  static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int
> reg_offset)
>  {
> @@ -386,7 +384,6 @@ static int get_metrics_table(struct amd_pmc_dev
> *pdev, struct smu_metrics *table
>  	return 0;
>  }
> 
> -#ifdef CONFIG_SUSPEND
>  static void amd_pmc_validate_deepest(struct amd_pmc_dev *pdev)
>  {
>  	struct smu_metrics table;
> @@ -400,7 +397,6 @@ static void amd_pmc_validate_deepest(struct
> amd_pmc_dev *pdev)
>  		dev_dbg(pdev->dev, "Last suspend in deepest state for
> %lluus\n",
>  			 table.timein_s0i3_lastcapture);
>  }
> -#endif
> 
>  static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
>  {
> @@ -673,7 +669,6 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev
> *dev, u32 arg, u32 *data, u8 msg,
>  	return rc;
>  }
> 
> -#ifdef CONFIG_SUSPEND
>  static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
>  {
>  	switch (dev->cpu_id) {
> @@ -861,9 +856,7 @@ static int __maybe_unused
> amd_pmc_suspend_handler(struct device *dev)
>  	return 0;
>  }
> 
> -static SIMPLE_DEV_PM_OPS(amd_pmc_pm, amd_pmc_suspend_handler,
> NULL);
> -
> -#endif
> +static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmc_pm,
> amd_pmc_suspend_handler, NULL);
> 
>  static const struct pci_device_id pmc_pci_ids[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_PS) },
> @@ -905,7 +898,6 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev
> *dev)
>  	return 0;
>  }
> 
> -#ifdef CONFIG_SUSPEND
>  static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
>  {
>  	int err;
> @@ -926,7 +918,6 @@ static int amd_pmc_write_stb(struct amd_pmc_dev
> *dev, u32 data)
> 
>  	return 0;
>  }
> -#endif
> 
>  static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
>  {
> @@ -1017,11 +1008,10 @@ static int amd_pmc_probe(struct
> platform_device *pdev)
>  	}
> 
>  	platform_set_drvdata(pdev, dev);
> -#ifdef CONFIG_SUSPEND
> -	err = acpi_register_lps0_dev(&amd_pmc_s2idle_dev_ops);
> +	if (IS_ENABLED(CONFIG_SUSPEND))
> +		err = acpi_register_lps0_dev(&amd_pmc_s2idle_dev_ops);
>  	if (err)
>  		dev_warn(dev->dev, "failed to register LPS0 sleep handler,
> expect increased power consumption\n");
> -#endif
> 
>  	amd_pmc_dbgfs_register(dev);
>  	return 0;
> @@ -1035,9 +1025,8 @@ static int amd_pmc_remove(struct platform_device
> *pdev)
>  {
>  	struct amd_pmc_dev *dev = platform_get_drvdata(pdev);
> 
> -#ifdef CONFIG_SUSPEND
> -	acpi_unregister_lps0_dev(&amd_pmc_s2idle_dev_ops);
> -#endif
> +	if (IS_ENABLED(CONFIG_SUSPEND))
> +		acpi_unregister_lps0_dev(&amd_pmc_s2idle_dev_ops);
>  	amd_pmc_dbgfs_unregister(dev);
>  	pci_dev_put(dev->rdev);
>  	mutex_destroy(&dev->lock);
> @@ -1061,9 +1050,7 @@ static struct platform_driver amd_pmc_driver = {
>  		.name = "amd_pmc",
>  		.acpi_match_table = amd_pmc_acpi_ids,
>  		.dev_groups = pmc_groups,
> -#ifdef CONFIG_SUSPEND
> -		.pm = &amd_pmc_pm,
> -#endif
> +		.pm = pm_sleep_ptr(&amd_pmc_pm),
>  	},
>  	.probe = amd_pmc_probe,
>  	.remove = amd_pmc_remove,
> --
> 2.39.1