On 10/23/2024 19:35, Mario Limonciello wrote:
> On 10/23/2024 01:32, 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.
>>
>> PS: We cannot use resource_size() here because it adds an extra byte
>> to round
>> off the size. In the case of PMF ResourceTemplate(), this rounding is
>> already handled within the _CRS. Using resource_size() would
>> increase the
>> resource size by 1, causing a mismatch with the length field and
>> leading
>> to issues. Therefore, simply use end-start of the ACPI resource to
>> obtain
>> the actual length.
>>
>> 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 | 46
>> +++++++++++----------------
>> drivers/platform/x86/amd/pmf/pmf.h | 6 ++--
>> drivers/platform/x86/amd/pmf/tee-if.c | 8 ++---
>> 4 files changed, 28 insertions(+), 33 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..62f984fe40c6 100644
>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>> @@ -433,37 +433,29 @@ 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");
here ^^^^^^^
>> + return -EINVAL;
>> }
>> - return AE_OK;
>> -}
>> -
>> -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);
>> + pmf_dev->policy_addr = pmf_dev->res->start;
>> + /*
>> + * We cannot use resource_size() here because it adds an extra
>> byte to round off the size.
>> + * In the case of PMF ResourceTemplate(), this rounding is
>> already handled within the _CRS.
>> + * Using resource_size() would increase the resource size by 1,
>> causing a mismatch with the
>> + * length field and leading to issues. Therefore, simply use
>> end-start of the ACPI resource
>> + * to obtain the actual length.
>> + */
>> + pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
>> +
>> + 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");
>
> This upgrades the previous message from debug to error.
It is dev_err() even before this change.
>
> TL;DR I feel this error should stay as dev_dbg() if no function checks
> are present for Smart PC.
>
> I don't think it's necessarily an error though.
> Smart PC checks are a bit different than the other checks. There
> isn't a check for a bit being set to indicate the function is supported.
>
> So if the BIOS has the declaration for the region but it's not
> populated it might not have a Smart PC policy and this shouldn't be
> reported as a BIOS bug.
This should be included in the CPM package, and the BIOS team is
responsible for packaging a policy binary.
