[PATCH 0/5] platform/x86/amd/pmf: Updates to AMD PMF driver

[PATCH 0/5] platform/x86/amd/pmf: Updates to AMD PMF driver

[Shyam Sundar S K]

Updates include:

- Add support for newer platforms
- Enhance the TA shared memory size to adapt to the updated TA
- Update the MAINTAINERS record, and
- Simplify certain code handling.

Shyam Sundar S K (5):
  platform/x86/amd/pmf: Add SMU metrics table support for 1Ah family 60h
    model
  platform/x86/amd/pmf: Use dev_err_probe() to simplify error handling
  MAINTAINERS: Change AMD PMF driver status to "Supported"
  platform/x86/amd/pmf: Switch to platform_get_resource() and
    devm_ioremap_resource()
  platform/x86/amd/pmf: Add PMF driver changes to make compatible with
    PMF-TA

 MAINTAINERS                           |  2 +-
 drivers/platform/x86/amd/pmf/Kconfig  |  1 +
 drivers/platform/x86/amd/pmf/acpi.c   | 37 ++++++++-------------------
 drivers/platform/x86/amd/pmf/core.c   |  9 ++++---
 drivers/platform/x86/amd/pmf/pmf.h    |  8 +++---
 drivers/platform/x86/amd/pmf/spc.c    |  1 +
 drivers/platform/x86/amd/pmf/tee-if.c |  8 +++---
 7 files changed, 28 insertions(+), 38 deletions(-)

-- 
2.34.1

[PATCH 1/5] platform/x86/amd/pmf: Add SMU metrics table support for 1Ah family 60h model

[Shyam Sundar S K]

Add SMU metrics table support for 1Ah family 60h model. This information
will be used by the PMF driver to alter the system thermals.

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>
---
 drivers/platform/x86/amd/pmf/core.c | 1 +
 drivers/platform/x86/amd/pmf/spc.c  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index d6af0ca036f1..347bb43a5f2b 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -261,6 +261,7 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer)
 			dev->mtable_size = sizeof(dev->m_table);
 			break;
 		case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
+		case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
 			dev->mtable_size = sizeof(dev->m_table_v2);
 			break;
 		default:
diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
index b5183969f9bf..06226eb0eab3 100644
--- a/drivers/platform/x86/amd/pmf/spc.c
+++ b/drivers/platform/x86/amd/pmf/spc.c
@@ -86,6 +86,7 @@ static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_ta
 					 ARRAY_SIZE(dev->m_table.avg_core_c0residency), in);
 		break;
 	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
+	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
 		memcpy(&dev->m_table_v2, dev->buf, dev->mtable_size);
 		in->ev_info.socket_power = dev->m_table_v2.apu_power + dev->m_table_v2.dgpu_power;
 		in->ev_info.skin_temperature = dev->m_table_v2.skin_temp;
-- 
2.34.1

[PATCH 2/5] platform/x86/amd/pmf: Use dev_err_probe() to simplify error handling

[Shyam Sundar S K]

To simplify error handling in the amd_pmf probe function and reduce code
size, dev_err() is replaced with dev_err_probe().

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>
---
 drivers/platform/x86/amd/pmf/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 347bb43a5f2b..12c3e929fdd1 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -430,18 +430,18 @@ static int amd_pmf_probe(struct platform_device *pdev)
 
 	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);
+		return dev_err_probe(dev->dev, pcibios_err_to_errno(err),
+				     "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
 	}
 
 	base_addr_lo = val & AMD_PMF_BASE_ADDR_HI_MASK;
 
 	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);
+		return dev_err_probe(dev->dev, pcibios_err_to_errno(err),
+				     "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
 	}
 
 	base_addr_hi = val & AMD_PMF_BASE_ADDR_LO_MASK;
-- 
2.34.1

[PATCH 3/5] MAINTAINERS: Change AMD PMF driver status to "Supported"

[Shyam Sundar S K]

The AMD PMF driver is actively being developed, so the MAINTAINERS record
should reflect "Supported" instead of "Maintained." Update the MAINTAINERS
database to reflect this change.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7ad507f49324..c2aecaab779f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1131,7 +1131,7 @@ F:	drivers/platform/x86/amd/pmc/
 AMD PMF DRIVER
 M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
 L:	platform-driver-x86@vger.kernel.org
-S:	Maintained
+S:	Supported
 F:	Documentation/ABI/testing/sysfs-amd-pmf
 F:	drivers/platform/x86/amd/pmf/
 
-- 
2.34.1

[PATCH 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Shyam Sundar S K]

Use platform_get_resource() to fetch the memory resource instead of
acpi_walk_resources() and devm_ioremap_resource() for mapping the
resources.

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>
---
 drivers/platform/x86/amd/pmf/Kconfig  |  1 +
 drivers/platform/x86/amd/pmf/acpi.c   | 37 ++++++++-------------------
 drivers/platform/x86/amd/pmf/pmf.h    |  6 +++--
 drivers/platform/x86/amd/pmf/tee-if.c |  8 +++---
 4 files changed, 20 insertions(+), 32 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
index f4fa8bd8bda8..99d67cdbd91e 100644
--- a/drivers/platform/x86/amd/pmf/Kconfig
+++ b/drivers/platform/x86/amd/pmf/Kconfig
@@ -11,6 +11,7 @@ config AMD_PMF
 	select ACPI_PLATFORM_PROFILE
 	depends on TEE && AMDTEE
 	depends on AMD_SFH_HID
+	depends on HAS_IOMEM
 	help
 	  This driver provides support for the AMD Platform Management Framework.
 	  The goal is to enhance end user experience by making AMD PCs smarter,
diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index d5b496433d69..40f1c0e9ec6d 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -433,37 +433,22 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
 	return 0;
 }
 
-static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
+int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
 {
-	struct amd_pmf_dev *dev = data;
+	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
 
-	switch (res->type) {
-	case ACPI_RESOURCE_TYPE_ADDRESS64:
-		dev->policy_addr = res->data.address64.address.minimum;
-		dev->policy_sz = res->data.address64.address.address_length;
-		break;
-	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
-		dev->policy_addr = res->data.fixed_memory32.address;
-		dev->policy_sz = res->data.fixed_memory32.address_length;
-		break;
-	}
-
-	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
-		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
-		return AE_ERROR;
+	pmf_dev->res =  platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!pmf_dev->res) {
+		dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
+		return -EINVAL;
 	}
 
-	return AE_OK;
-}
+	pmf_dev->policy_addr = pmf_dev->res->start;
+	pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;
 
-int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
-{
-	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
-	acpi_status status;
-
-	status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
-	if (ACPI_FAILURE(status)) {
-		dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n", status);
+	if (!pmf_dev->policy_addr || pmf_dev->policy_sz > POLICY_BUF_MAX_SZ ||
+	    pmf_dev->policy_sz == 0) {
+		dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
 		return -EINVAL;
 	}
 
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 8ce8816da9c1..a79808fda1d8 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -13,6 +13,7 @@
 
 #include <linux/acpi.h>
 #include <linux/input.h>
+#include <linux/platform_device.h>
 #include <linux/platform_profile.h>
 
 #define POLICY_BUF_MAX_SZ		0x4b000
@@ -355,19 +356,20 @@ struct amd_pmf_dev {
 	/* Smart PC solution builder */
 	struct dentry *esbin;
 	unsigned char *policy_buf;
-	u32 policy_sz;
+	resource_size_t policy_sz;
 	struct tee_context *tee_ctx;
 	struct tee_shm *fw_shm_pool;
 	u32 session_id;
 	void *shbuf;
 	struct delayed_work pb_work;
 	struct pmf_action_table *prev_data;
-	u64 policy_addr;
+	resource_size_t policy_addr;
 	void __iomem *policy_base;
 	bool smart_pc_enabled;
 	u16 pmf_if_version;
 	struct input_dev *pmf_idev;
 	size_t mtable_size;
+	struct resource *res;
 };
 
 struct apmf_sps_prop_granular_v2 {
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 19c27b6e4666..544c5ce08ff0 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
 		return -ENODEV;
 	}
 
-	dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
+	dev_dbg(dev->dev, "Policy Binary size: %lld bytes\n", dev->policy_sz);
 	memset(dev->shbuf, 0, dev->policy_sz);
 	ta_sm = dev->shbuf;
 	in = &ta_sm->pmf_input.init_table;
@@ -512,9 +512,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	if (ret)
 		goto error;
 
-	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
-	if (!dev->policy_base) {
-		ret = -ENOMEM;
+	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
+	if (IS_ERR(dev->policy_base)) {
+		ret = PTR_ERR(dev->policy_base);
 		goto error;
 	}
 
-- 
2.34.1

[PATCH 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Shyam Sundar S K]

The PMF driver will allocate shared buffer memory using the
tee_shm_alloc_kernel_buf(). This allocated memory is located in the
secure world and is used for communication with the PMF-TA.

The latest PMF-TA version introduces new structures with OEM debug
information and additional policy input conditions for evaluating the
policy binary. Consequently, the shared memory size must be increased to
ensure compatibility between the PMF driver and the updated PMF-TA.

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>
---
 drivers/platform/x86/amd/pmf/pmf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index a79808fda1d8..18f12aad46a9 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -106,7 +106,7 @@ struct cookie_header {
 #define PMF_TA_IF_VERSION_MAJOR				1
 #define TA_PMF_ACTION_MAX					32
 #define TA_PMF_UNDO_MAX						8
-#define TA_OUTPUT_RESERVED_MEM				906
+#define TA_OUTPUT_RESERVED_MEM				922
 #define MAX_OPERATION_PARAMS					4
 
 #define PMF_IF_V1		1
-- 
2.34.1

Re: [PATCH 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Ilpo Järvinen]

On Mon, 14 Oct 2024, Shyam Sundar S K wrote:

> Use platform_get_resource() to fetch the memory resource instead of
> acpi_walk_resources() and devm_ioremap_resource() for mapping the
> resources.
> 
> 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>
> ---
>  drivers/platform/x86/amd/pmf/Kconfig  |  1 +
>  drivers/platform/x86/amd/pmf/acpi.c   | 37 ++++++++-------------------
>  drivers/platform/x86/amd/pmf/pmf.h    |  6 +++--
>  drivers/platform/x86/amd/pmf/tee-if.c |  8 +++---
>  4 files changed, 20 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index f4fa8bd8bda8..99d67cdbd91e 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -11,6 +11,7 @@ config AMD_PMF
>  	select ACPI_PLATFORM_PROFILE
>  	depends on TEE && AMDTEE
>  	depends on AMD_SFH_HID
> +	depends on HAS_IOMEM
>  	help
>  	  This driver provides support for the AMD Platform Management Framework.
>  	  The goal is to enhance end user experience by making AMD PCs smarter,
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index d5b496433d69..40f1c0e9ec6d 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -433,37 +433,22 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
>  	return 0;
>  }
>  
> -static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>  {
> -	struct amd_pmf_dev *dev = data;
> +	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>  
> -	switch (res->type) {
> -	case ACPI_RESOURCE_TYPE_ADDRESS64:
> -		dev->policy_addr = res->data.address64.address.minimum;
> -		dev->policy_sz = res->data.address64.address.address_length;
> -		break;
> -	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> -		dev->policy_addr = res->data.fixed_memory32.address;
> -		dev->policy_sz = res->data.fixed_memory32.address_length;
> -		break;
> -	}
> -
> -	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
> -		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
> -		return AE_ERROR;
> +	pmf_dev->res =  platform_get_resource(pdev, IORESOURCE_MEM, 0);

Extra space.

> +	if (!pmf_dev->res) {
> +		dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
> +		return -EINVAL;
>  	}
>  
> -	return AE_OK;
> -}
> +	pmf_dev->policy_addr = pmf_dev->res->start;
> +	pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;

If you have a resource, why you convert it into something custom like 
this?

> -int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
> -{
> -	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> -	acpi_status status;
> -
> -	status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
> -	if (ACPI_FAILURE(status)) {
> -		dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n", status);
> +	if (!pmf_dev->policy_addr || pmf_dev->policy_sz > POLICY_BUF_MAX_SZ ||
> +	    pmf_dev->policy_sz == 0) {
> +		dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 8ce8816da9c1..a79808fda1d8 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -13,6 +13,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/input.h>
> +#include <linux/platform_device.h>
>  #include <linux/platform_profile.h>
>  
>  #define POLICY_BUF_MAX_SZ		0x4b000
> @@ -355,19 +356,20 @@ struct amd_pmf_dev {
>  	/* Smart PC solution builder */
>  	struct dentry *esbin;
>  	unsigned char *policy_buf;
> -	u32 policy_sz;
> +	resource_size_t policy_sz;
>  	struct tee_context *tee_ctx;
>  	struct tee_shm *fw_shm_pool;
>  	u32 session_id;
>  	void *shbuf;
>  	struct delayed_work pb_work;
>  	struct pmf_action_table *prev_data;
> -	u64 policy_addr;
> +	resource_size_t policy_addr;
>  	void __iomem *policy_base;
>  	bool smart_pc_enabled;
>  	u16 pmf_if_version;
>  	struct input_dev *pmf_idev;
>  	size_t mtable_size;
> +	struct resource *res;
>  };
>  
>  struct apmf_sps_prop_granular_v2 {
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 19c27b6e4666..544c5ce08ff0 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
>  		return -ENODEV;
>  	}
>  
> -	dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
> +	dev_dbg(dev->dev, "Policy Binary size: %lld bytes\n", dev->policy_sz);

resource_size_t is unsigned type. However, it's not unsigned long long 
either so this will trigger a warning without cast which is unacceptable.

-- 
 i.

>  	memset(dev->shbuf, 0, dev->policy_sz);
>  	ta_sm = dev->shbuf;
>  	in = &ta_sm->pmf_input.init_table;
> @@ -512,9 +512,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  	if (ret)
>  		goto error;
>  
> -	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
> -	if (!dev->policy_base) {
> -		ret = -ENOMEM;
> +	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
> +	if (IS_ERR(dev->policy_base)) {
> +		ret = PTR_ERR(dev->policy_base);
>  		goto error;
>  	}
>  
> 

Re: [PATCH 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Shyam Sundar S K]

Hi Ilpo,

On 10/14/2024 13:26, Ilpo Järvinen wrote:
> On Mon, 14 Oct 2024, Shyam Sundar S K wrote:
> 
>> Use platform_get_resource() to fetch the memory resource instead of
>> acpi_walk_resources() and devm_ioremap_resource() for mapping the
>> resources.
>>
>> 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>
>> ---
>>  drivers/platform/x86/amd/pmf/Kconfig  |  1 +
>>  drivers/platform/x86/amd/pmf/acpi.c   | 37 ++++++++-------------------
>>  drivers/platform/x86/amd/pmf/pmf.h    |  6 +++--
>>  drivers/platform/x86/amd/pmf/tee-if.c |  8 +++---
>>  4 files changed, 20 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
>> index f4fa8bd8bda8..99d67cdbd91e 100644
>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>> @@ -11,6 +11,7 @@ config AMD_PMF
>>  	select ACPI_PLATFORM_PROFILE
>>  	depends on TEE && AMDTEE
>>  	depends on AMD_SFH_HID
>> +	depends on HAS_IOMEM
>>  	help
>>  	  This driver provides support for the AMD Platform Management Framework.
>>  	  The goal is to enhance end user experience by making AMD PCs smarter,
>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>> index d5b496433d69..40f1c0e9ec6d 100644
>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>> @@ -433,37 +433,22 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
>>  	return 0;
>>  }
>>  
>> -static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
>> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>  {
>> -	struct amd_pmf_dev *dev = data;
>> +	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>>  
>> -	switch (res->type) {
>> -	case ACPI_RESOURCE_TYPE_ADDRESS64:
>> -		dev->policy_addr = res->data.address64.address.minimum;
>> -		dev->policy_sz = res->data.address64.address.address_length;
>> -		break;
>> -	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>> -		dev->policy_addr = res->data.fixed_memory32.address;
>> -		dev->policy_sz = res->data.fixed_memory32.address_length;
>> -		break;
>> -	}
>> -
>> -	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
>> -		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
>> -		return AE_ERROR;
>> +	pmf_dev->res =  platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 
> Extra space.
> 
>> +	if (!pmf_dev->res) {
>> +		dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
>> +		return -EINVAL;
>>  	}
>>  
>> -	return AE_OK;
>> -}
>> +	pmf_dev->policy_addr = pmf_dev->res->start;
>> +	pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;
> 
> If you have a resource, why you convert it into something custom like 
> this?
> 

I will address your comments in v2. But for this specific comment:

the DSDT looks like this:

Device (PMF)
{
	...

	Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
	{
		Name (RBUF, ResourceTemplate ()
		{
			QWordMemory (ResourceConsumer, PosDecode, MinNotFixed, MaxNotFixed,
NonCacheable, ReadOnly,
				0x0000000000000000, // Granularity
				0x000000FD00BC1000, // Range Minimum
				0x000000FD00C0C000, // Range Maximum
				0x0000000000000000, // Translation Offset
				0x000000000004B000, // Length	
				,, , AddressRangeMemory, TypeStatic)
		}

		...
	}
}

But, resource_size() will do 'res->end - res->start + 1;'

So, that will become 0x4B000 + 1 = 0x4B001 which will exceed
POLICY_BUF_MAX_SZ.

defined as:
#define POLICY_BUF_MAX_SZ		0x4b000

Now, because of this, it would hit the failure case:

if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ ||
dev->policy_sz == 0) {
		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
		return AE_ERROR;
	}


Would you like me to leave a note before using resource_size() on why
"-1" is being done?

Let me know your thoughts?

Thanks,
Shyam

>> -int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>> -{
>> -	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>> -	acpi_status status;
>> -
>> -	status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
>> -	if (ACPI_FAILURE(status)) {
>> -		dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n", status);
>> +	if (!pmf_dev->policy_addr || pmf_dev->policy_sz > POLICY_BUF_MAX_SZ ||
>> +	    pmf_dev->policy_sz == 0) {
>> +		dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
>>  		return -EINVAL;
>>  	}
>>  
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index 8ce8816da9c1..a79808fda1d8 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -13,6 +13,7 @@
>>  
>>  #include <linux/acpi.h>
>>  #include <linux/input.h>
>> +#include <linux/platform_device.h>
>>  #include <linux/platform_profile.h>
>>  
>>  #define POLICY_BUF_MAX_SZ		0x4b000
>> @@ -355,19 +356,20 @@ struct amd_pmf_dev {
>>  	/* Smart PC solution builder */
>>  	struct dentry *esbin;
>>  	unsigned char *policy_buf;
>> -	u32 policy_sz;
>> +	resource_size_t policy_sz;
>>  	struct tee_context *tee_ctx;
>>  	struct tee_shm *fw_shm_pool;
>>  	u32 session_id;
>>  	void *shbuf;
>>  	struct delayed_work pb_work;
>>  	struct pmf_action_table *prev_data;
>> -	u64 policy_addr;
>> +	resource_size_t policy_addr;
>>  	void __iomem *policy_base;
>>  	bool smart_pc_enabled;
>>  	u16 pmf_if_version;
>>  	struct input_dev *pmf_idev;
>>  	size_t mtable_size;
>> +	struct resource *res;
>>  };
>>  
>>  struct apmf_sps_prop_granular_v2 {
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>> index 19c27b6e4666..544c5ce08ff0 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
>>  		return -ENODEV;
>>  	}
>>  
>> -	dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
>> +	dev_dbg(dev->dev, "Policy Binary size: %lld bytes\n", dev->policy_sz);
> 
> resource_size_t is unsigned type. However, it's not unsigned long long 
> either so this will trigger a warning without cast which is unacceptable.
> 

Re: [PATCH 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7264 bytes --]

On Mon, 14 Oct 2024, Shyam Sundar S K wrote:

> Hi Ilpo,
> 
> On 10/14/2024 13:26, Ilpo Järvinen wrote:
> > On Mon, 14 Oct 2024, Shyam Sundar S K wrote:
> > 
> >> Use platform_get_resource() to fetch the memory resource instead of
> >> acpi_walk_resources() and devm_ioremap_resource() for mapping the
> >> resources.
> >>
> >> 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>
> >> ---
> >>  drivers/platform/x86/amd/pmf/Kconfig  |  1 +
> >>  drivers/platform/x86/amd/pmf/acpi.c   | 37 ++++++++-------------------
> >>  drivers/platform/x86/amd/pmf/pmf.h    |  6 +++--
> >>  drivers/platform/x86/amd/pmf/tee-if.c |  8 +++---
> >>  4 files changed, 20 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> >> index f4fa8bd8bda8..99d67cdbd91e 100644
> >> --- a/drivers/platform/x86/amd/pmf/Kconfig
> >> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> >> @@ -11,6 +11,7 @@ config AMD_PMF
> >>  	select ACPI_PLATFORM_PROFILE
> >>  	depends on TEE && AMDTEE
> >>  	depends on AMD_SFH_HID
> >> +	depends on HAS_IOMEM
> >>  	help
> >>  	  This driver provides support for the AMD Platform Management Framework.
> >>  	  The goal is to enhance end user experience by making AMD PCs smarter,
> >> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> >> index d5b496433d69..40f1c0e9ec6d 100644
> >> --- a/drivers/platform/x86/amd/pmf/acpi.c
> >> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> >> @@ -433,37 +433,22 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
> >>  	return 0;
> >>  }
> >>  
> >> -static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
> >> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
> >>  {
> >> -	struct amd_pmf_dev *dev = data;
> >> +	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
> >>  
> >> -	switch (res->type) {
> >> -	case ACPI_RESOURCE_TYPE_ADDRESS64:
> >> -		dev->policy_addr = res->data.address64.address.minimum;
> >> -		dev->policy_sz = res->data.address64.address.address_length;
> >> -		break;
> >> -	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> >> -		dev->policy_addr = res->data.fixed_memory32.address;
> >> -		dev->policy_sz = res->data.fixed_memory32.address_length;
> >> -		break;
> >> -	}
> >> -
> >> -	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
> >> -		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
> >> -		return AE_ERROR;
> >> +	pmf_dev->res =  platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > 
> > Extra space.
> > 
> >> +	if (!pmf_dev->res) {
> >> +		dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
> >> +		return -EINVAL;
> >>  	}
> >>  
> >> -	return AE_OK;
> >> -}
> >> +	pmf_dev->policy_addr = pmf_dev->res->start;
> >> +	pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;
> > 
> > If you have a resource, why you convert it into something custom like 
> > this?
> > 
> 
> I will address your comments in v2. But for this specific comment:
> 
> the DSDT looks like this:
> 
> Device (PMF)
> {
> 	...
> 
> 	Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> 	{
> 		Name (RBUF, ResourceTemplate ()
> 		{
> 			QWordMemory (ResourceConsumer, PosDecode, MinNotFixed, MaxNotFixed,
> NonCacheable, ReadOnly,
> 				0x0000000000000000, // Granularity
> 				0x000000FD00BC1000, // Range Minimum
> 				0x000000FD00C0C000, // Range Maximum
> 				0x0000000000000000, // Translation Offset
> 				0x000000000004B000, // Length	
> 				,, , AddressRangeMemory, TypeStatic)
> 		}
> 
> 		...
> 	}
> }
> 
> But, resource_size() will do 'res->end - res->start + 1;'
> 
> So, that will become 0x4B000 + 1 = 0x4B001 which will exceed
> POLICY_BUF_MAX_SZ.

That +1 is there to counter the -1 done on the set side. res->end is 
supposed to point last valid address of the resource, not the address 
after it. With round sizes, the end address is expected to end with lots 
of Fs (hex) but in your example it ends into zeros (if I interpret your 
numbers right)?

-- 
 i.

> defined as:
> #define POLICY_BUF_MAX_SZ		0x4b000
> 
> Now, because of this, it would hit the failure case:
> 
> if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ ||
> dev->policy_sz == 0) {
> 		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
> 		return AE_ERROR;
> 	}
> 
> 
> Would you like me to leave a note before using resource_size() on why
> "-1" is being done?
> 
> Let me know your thoughts?
> 
> Thanks,
> Shyam
> 
> >> -int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
> >> -{
> >> -	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> >> -	acpi_status status;
> >> -
> >> -	status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
> >> -	if (ACPI_FAILURE(status)) {
> >> -		dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n", status);
> >> +	if (!pmf_dev->policy_addr || pmf_dev->policy_sz > POLICY_BUF_MAX_SZ ||
> >> +	    pmf_dev->policy_sz == 0) {
> >> +		dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> >> index 8ce8816da9c1..a79808fda1d8 100644
> >> --- a/drivers/platform/x86/amd/pmf/pmf.h
> >> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> >> @@ -13,6 +13,7 @@
> >>  
> >>  #include <linux/acpi.h>
> >>  #include <linux/input.h>
> >> +#include <linux/platform_device.h>
> >>  #include <linux/platform_profile.h>
> >>  
> >>  #define POLICY_BUF_MAX_SZ		0x4b000
> >> @@ -355,19 +356,20 @@ struct amd_pmf_dev {
> >>  	/* Smart PC solution builder */
> >>  	struct dentry *esbin;
> >>  	unsigned char *policy_buf;
> >> -	u32 policy_sz;
> >> +	resource_size_t policy_sz;
> >>  	struct tee_context *tee_ctx;
> >>  	struct tee_shm *fw_shm_pool;
> >>  	u32 session_id;
> >>  	void *shbuf;
> >>  	struct delayed_work pb_work;
> >>  	struct pmf_action_table *prev_data;
> >> -	u64 policy_addr;
> >> +	resource_size_t policy_addr;
> >>  	void __iomem *policy_base;
> >>  	bool smart_pc_enabled;
> >>  	u16 pmf_if_version;
> >>  	struct input_dev *pmf_idev;
> >>  	size_t mtable_size;
> >> +	struct resource *res;
> >>  };
> >>  
> >>  struct apmf_sps_prop_granular_v2 {
> >> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> >> index 19c27b6e4666..544c5ce08ff0 100644
> >> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> >> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> >> @@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
> >>  		return -ENODEV;
> >>  	}
> >>  
> >> -	dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
> >> +	dev_dbg(dev->dev, "Policy Binary size: %lld bytes\n", dev->policy_sz);
> > 
> > resource_size_t is unsigned type. However, it's not unsigned long long 
> > either so this will trigger a warning without cast which is unacceptable.
> > 
> 

Re: [PATCH 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Shyam Sundar S K]

Hi Ilpo,

On 10/14/2024 23:24, Ilpo Järvinen wrote:
> On Mon, 14 Oct 2024, Shyam Sundar S K wrote:
> 
>> Hi Ilpo,
>>
>> On 10/14/2024 13:26, Ilpo Järvinen wrote:
>>> On Mon, 14 Oct 2024, Shyam Sundar S K wrote:
>>>
>>>> Use platform_get_resource() to fetch the memory resource instead of
>>>> acpi_walk_resources() and devm_ioremap_resource() for mapping the
>>>> resources.
>>>>
>>>> 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>
>>>> ---
>>>>  drivers/platform/x86/amd/pmf/Kconfig  |  1 +
>>>>  drivers/platform/x86/amd/pmf/acpi.c   | 37 ++++++++-------------------
>>>>  drivers/platform/x86/amd/pmf/pmf.h    |  6 +++--
>>>>  drivers/platform/x86/amd/pmf/tee-if.c |  8 +++---
>>>>  4 files changed, 20 insertions(+), 32 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
>>>> index f4fa8bd8bda8..99d67cdbd91e 100644
>>>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>>>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>>>> @@ -11,6 +11,7 @@ config AMD_PMF
>>>>  	select ACPI_PLATFORM_PROFILE
>>>>  	depends on TEE && AMDTEE
>>>>  	depends on AMD_SFH_HID
>>>> +	depends on HAS_IOMEM
>>>>  	help
>>>>  	  This driver provides support for the AMD Platform Management Framework.
>>>>  	  The goal is to enhance end user experience by making AMD PCs smarter,
>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>>>> index d5b496433d69..40f1c0e9ec6d 100644
>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>> @@ -433,37 +433,22 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
>>>> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>>>  {
>>>> -	struct amd_pmf_dev *dev = data;
>>>> +	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>>>>  
>>>> -	switch (res->type) {
>>>> -	case ACPI_RESOURCE_TYPE_ADDRESS64:
>>>> -		dev->policy_addr = res->data.address64.address.minimum;
>>>> -		dev->policy_sz = res->data.address64.address.address_length;
>>>> -		break;
>>>> -	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>>>> -		dev->policy_addr = res->data.fixed_memory32.address;
>>>> -		dev->policy_sz = res->data.fixed_memory32.address_length;
>>>> -		break;
>>>> -	}
>>>> -
>>>> -	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
>>>> -		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
>>>> -		return AE_ERROR;
>>>> +	pmf_dev->res =  platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>
>>> Extra space.
>>>
>>>> +	if (!pmf_dev->res) {
>>>> +		dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
>>>> +		return -EINVAL;
>>>>  	}
>>>>  
>>>> -	return AE_OK;
>>>> -}
>>>> +	pmf_dev->policy_addr = pmf_dev->res->start;
>>>> +	pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;
>>>
>>> If you have a resource, why you convert it into something custom like 
>>> this?
>>>
>>
>> I will address your comments in v2. But for this specific comment:
>>
>> the DSDT looks like this:
>>
>> Device (PMF)
>> {
>> 	...
>>
>> 	Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>> 	{
>> 		Name (RBUF, ResourceTemplate ()
>> 		{
>> 			QWordMemory (ResourceConsumer, PosDecode, MinNotFixed, MaxNotFixed,
>> NonCacheable, ReadOnly,
>> 				0x0000000000000000, // Granularity
>> 				0x000000FD00BC1000, // Range Minimum
>> 				0x000000FD00C0C000, // Range Maximum
>> 				0x0000000000000000, // Translation Offset
>> 				0x000000000004B000, // Length	
>> 				,, , AddressRangeMemory, TypeStatic)
>> 		}
>>
>> 		...
>> 	}
>> }
>>
>> But, resource_size() will do 'res->end - res->start + 1;'
>>
>> So, that will become 0x4B000 + 1 = 0x4B001 which will exceed
>> POLICY_BUF_MAX_SZ.
> 
> That +1 is there to counter the -1 done on the set side. res->end is 
> supposed to point last valid address of the resource, not the address 
> after it. With round sizes, the end address is expected to end with lots 
> of Fs (hex) but in your example it ends into zeros (if I interpret your 
> numbers right)?
> 

Yes, that's right.


Couple of more examples, where resource_size() will fit correctly.


Example #1:
WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
                    0x0000,             // Granularity
                    0x0D00,             // Range Minimum
                    0x0FFF,             // Range Maximum
                    0x0000,             // Translation Offset
                    0x0300,             // Length
                    ,, , TypeStatic, DenseTranslation)

resource_size() will do 'res->end - res->start + 1;'
So,
0xFFF-0xD00 = 0x2FF
0x2FF + 1 = 0x300 (which matches the length field)

Example #2:
DWordMemory (ResourceProducer, SubDecode, MinFixed, MaxFixed,
NonCacheable, ReadWrite,
                    0x00000000,         // Granularity
                    0xFED45000,         // Range Minimum
                    0xFED811FF,         // Range Maximum
                    0x00000000,         // Translation Offset
                    0x0003C200,         // Length
                    ,, , AddressRangeMemory, TypeStatic)

Here,
0xFED811FF - 0xFED45000 = 0x3C1FF
0x3C1FF + 1 = 0x3C200 (which again matches the length field)

But the same resource_size() will not help in case of PMF _CRS
ResourceTemplate(). Hence I had to make a custom change like doing "-1".

So, in the current change proposed here, we can have two options:

1) just use:
pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;

2) use resource_size() with -1 and leave a note on why "-1" is required
pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;

Let me know your thoughts.

Thanks,
Shyam

Re: [PATCH 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4472 bytes --]

On Tue, 15 Oct 2024, Shyam Sundar S K wrote:
> On 10/14/2024 23:24, Ilpo Järvinen wrote:
> > On Mon, 14 Oct 2024, Shyam Sundar S K wrote:
> >> On 10/14/2024 13:26, Ilpo Järvinen wrote:
> >>> On Mon, 14 Oct 2024, Shyam Sundar S K wrote:
> >>>
> >>>> Use platform_get_resource() to fetch the memory resource instead of
> >>>> acpi_walk_resources() and devm_ioremap_resource() for mapping the
> >>>> resources.
> >>>>
> >>>> 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>
> >>>> ---
> >>>>  drivers/platform/x86/amd/pmf/Kconfig  |  1 +
> >>>>  drivers/platform/x86/amd/pmf/acpi.c   | 37 ++++++++-------------------
> >>>>  drivers/platform/x86/amd/pmf/pmf.h    |  6 +++--
> >>>>  drivers/platform/x86/amd/pmf/tee-if.c |  8 +++---
> >>>>  4 files changed, 20 insertions(+), 32 deletions(-)
> >>>>

> >>>> +	if (!pmf_dev->res) {
> >>>> +		dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
> >>>> +		return -EINVAL;
> >>>>  	}
> >>>>  
> >>>> -	return AE_OK;
> >>>> -}
> >>>> +	pmf_dev->policy_addr = pmf_dev->res->start;
> >>>> +	pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;
> >>>
> >>> If you have a resource, why you convert it into something custom like 
> >>> this?
> >>>
> >>
> >> I will address your comments in v2. But for this specific comment:
> >>
> >> the DSDT looks like this:
> >>
> >> Device (PMF)
> >> {
> >> 	...
> >>
> >> 	Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> >> 	{
> >> 		Name (RBUF, ResourceTemplate ()
> >> 		{
> >> 			QWordMemory (ResourceConsumer, PosDecode, MinNotFixed, MaxNotFixed,
> >> NonCacheable, ReadOnly,
> >> 				0x0000000000000000, // Granularity
> >> 				0x000000FD00BC1000, // Range Minimum
> >> 				0x000000FD00C0C000, // Range Maximum
> >> 				0x0000000000000000, // Translation Offset
> >> 				0x000000000004B000, // Length	
> >> 				,, , AddressRangeMemory, TypeStatic)
> >> 		}
> >>
> >> 		...
> >> 	}
> >> }
> >>
> >> But, resource_size() will do 'res->end - res->start + 1;'
> >>
> >> So, that will become 0x4B000 + 1 = 0x4B001 which will exceed
> >> POLICY_BUF_MAX_SZ.
> > 
> > That +1 is there to counter the -1 done on the set side. res->end is 
> > supposed to point last valid address of the resource, not the address 
> > after it. With round sizes, the end address is expected to end with lots 
> > of Fs (hex) but in your example it ends into zeros (if I interpret your 
> > numbers right)?
> > 
> 
> Yes, that's right.
> 
> 
> Couple of more examples, where resource_size() will fit correctly.
> 
> 
> Example #1:
> WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
>                     0x0000,             // Granularity
>                     0x0D00,             // Range Minimum
>                     0x0FFF,             // Range Maximum
>                     0x0000,             // Translation Offset
>                     0x0300,             // Length
>                     ,, , TypeStatic, DenseTranslation)
> 
> resource_size() will do 'res->end - res->start + 1;'
> So,
> 0xFFF-0xD00 = 0x2FF
> 0x2FF + 1 = 0x300 (which matches the length field)
> 
> Example #2:
> DWordMemory (ResourceProducer, SubDecode, MinFixed, MaxFixed,
> NonCacheable, ReadWrite,
>                     0x00000000,         // Granularity
>                     0xFED45000,         // Range Minimum
>                     0xFED811FF,         // Range Maximum
>                     0x00000000,         // Translation Offset
>                     0x0003C200,         // Length
>                     ,, , AddressRangeMemory, TypeStatic)
> 
> Here,
> 0xFED811FF - 0xFED45000 = 0x3C1FF
> 0x3C1FF + 1 = 0x3C200 (which again matches the length field)
> 
> But the same resource_size() will not help in case of PMF _CRS
> ResourceTemplate(). Hence I had to make a custom change like doing "-1".
> 
> So, in the current change proposed here, we can have two options:
> 
> 1) just use:
> pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;

Since it doesn't seem to cover the entire resource, I think this option 1) 
is better for now.

> 2) use resource_size() with -1 and leave a note on why "-1" is required
> pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;
> 
> Let me know your thoughts.

-- 
 i.