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