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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/6/23 18: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>

Since this is a fix and since this applies cleanly without
the other 2 debug patches I have merged this into my
review-hans branch now, so that it can be merged by Linus for
6.4-rc1 :

Thank you for your patch, I've applied this patch 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/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);
>  	}