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. defined as: #define POLICY_BUF_MAX_SZ 0x4b000 Now, because of this, it would hit the failure case: 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; } Would you like me to leave a note before using resource_size() on why "-1" is being done? Let me know your thoughts? Thanks, Shyam >> -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); >> + 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"); >> 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..544c5ce08ff0 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: %lld bytes\n", dev->policy_sz); > > resource_size_t is unsigned type. However, it's not unsigned long long > either so this will trigger a warning without cast which is unacceptable. >