On 10/23/2024 20:20, Mario Limonciello wrote:
> 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@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.
>>
>> 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")
ah! but your comment was just inline to:
dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
So, I was thinking you are saying to downgrade this to dev_dbg() and
hence the above clarification.
if the comment is for:
dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
then Yes, I agree we should have dev_dbg() and I will respin a new
version.
Thanks,
Shyam
>
> 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;
>>>> }
>>>>
>>>
>
