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")
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;
}
