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

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

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.
>