Re: [PATCH 3/3] platform/x86/amd/pmf: Move out of BIOS SMN pair for driver probe

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 4/6/2023 11:48, Shyam Sundar S K wrote:
> The current SMN index used for the driver probe seems to be meant
> for the BIOS pair and there are potential concurrency problems that can
> occur with an inopportune SMI.
> 
> It is been advised to use SMN_INDEX_0 instead of SMN_INDEX_2, which is
> what amd_nb.c provides and this function has protections to ensure that
> only one caller can use it at a time.
> 
> Fixes: da5ce22df5fe ("platform/x86/amd/pmf: Add support for PMF core layer")
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

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

> ---
>   drivers/platform/x86/amd/pmf/Kconfig |  1 +
>   drivers/platform/x86/amd/pmf/core.c  | 22 +++++-----------------
>   2 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index 7129de0fb9fb..c7cda8bd478c 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -7,6 +7,7 @@ config AMD_PMF
>   	tristate "AMD Platform Management Framework"
>   	depends on ACPI && PCI
>   	depends on POWER_SUPPLY
> +	depends on AMD_NB
>   	select ACPI_PLATFORM_PROFILE
>   	help
>   	  This driver provides support for the AMD Platform Management Framework.
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index da23639071d7..0acc0b622129 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -8,6 +8,7 @@
>    * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>    */
>   
> +#include <asm/amd_nb.h>
>   #include <linux/debugfs.h>
>   #include <linux/iopoll.h>
>   #include <linux/module.h>
> @@ -22,8 +23,6 @@
>   #define AMD_PMF_REGISTER_ARGUMENT	0xA58
>   
>   /* Base address of SMU for mapping physical address to virtual address */
> -#define AMD_PMF_SMU_INDEX_ADDRESS	0xB8
> -#define AMD_PMF_SMU_INDEX_DATA		0xBC
>   #define AMD_PMF_MAPPING_SIZE		0x01000
>   #define AMD_PMF_BASE_ADDR_OFFSET	0x10000
>   #define AMD_PMF_BASE_ADDR_LO		0x13B102E8
> @@ -348,30 +347,19 @@ static int amd_pmf_probe(struct platform_device *pdev)
>   	}
>   
>   	dev->cpu_id = rdev->device;
> -	err = pci_write_config_dword(rdev, AMD_PMF_SMU_INDEX_ADDRESS, AMD_PMF_BASE_ADDR_LO);
> -	if (err) {
> -		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMF_SMU_INDEX_ADDRESS);
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> -	}
>   
> -	err = pci_read_config_dword(rdev, AMD_PMF_SMU_INDEX_DATA, &val);
> +	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val);
>   	if (err) {
> +		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
>   		pci_dev_put(rdev);
>   		return pcibios_err_to_errno(err);
>   	}
>   
>   	base_addr_lo = val & AMD_PMF_BASE_ADDR_HI_MASK;
>   
> -	err = pci_write_config_dword(rdev, AMD_PMF_SMU_INDEX_ADDRESS, AMD_PMF_BASE_ADDR_HI);
> -	if (err) {
> -		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMF_SMU_INDEX_ADDRESS);
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> -	}
> -
> -	err = pci_read_config_dword(rdev, AMD_PMF_SMU_INDEX_DATA, &val);
> +	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_HI, &val);
>   	if (err) {
> +		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
>   		pci_dev_put(rdev);
>   		return pcibios_err_to_errno(err);
>   	}