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

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/23/2024 09:37, Shyam Sundar S K wrote:
> 
> 
> 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@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   | 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.
> 
>  From a driver design standpoint, the absence of the policy binary
> should be treated as an error, as there's no reason for the BIOS to
> advertise the Smart PC bits without providing the policy binary.
> 
> Therefore, this should trigger a `dev_err()` and be considered a BIOS bug.
> 

OK I agree with this specific error, but I took a closer look at the bug 
associated with
03cea821b82cb ("platform/x86/amd: pmf: Decrease error message to debug")

As _CRS is patched out by BIOS I suspect that system will now start showing:

dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");

So how exactly is a platform designer supposed to not advertise smart PC 
bits?  It seems the only check is the presence of that resource.

> Thanks,
> Shyam
> 
>>
>>>            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..555b8d6314e0 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: %llu 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;
>>>        }
>>>    
>>