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@xxxxxxx>
>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
>>>> ---
>>>> 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
