| Message ID | 20241014045759.1517226-5-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86/amd/pmf: Updates to AMD PMF driver | expand |
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@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 | 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? > -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.
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@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 | 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. >
On Mon, 14 Oct 2024, Shyam Sundar S K wrote: > 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@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 | 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. That +1 is there to counter the -1 done on the set side. res->end is supposed to point last valid address of the resource, not the address after it. With round sizes, the end address is expected to end with lots of Fs (hex) but in your example it ends into zeros (if I interpret your numbers right)?
Hi Ilpo, On 10/14/2024 23:24, Ilpo Järvinen wrote: > On Mon, 14 Oct 2024, Shyam Sundar S K wrote: > >> 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@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 | 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. > > That +1 is there to counter the -1 done on the set side. res->end is > supposed to point last valid address of the resource, not the address > after it. With round sizes, the end address is expected to end with lots > of Fs (hex) but in your example it ends into zeros (if I interpret your > numbers right)? > Yes, that's right. Couple of more examples, where resource_size() will fit correctly. Example #1: WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, 0x0000, // Granularity 0x0D00, // Range Minimum 0x0FFF, // Range Maximum 0x0000, // Translation Offset 0x0300, // Length ,, , TypeStatic, DenseTranslation) resource_size() will do 'res->end - res->start + 1;' So, 0xFFF-0xD00 = 0x2FF 0x2FF + 1 = 0x300 (which matches the length field) Example #2: DWordMemory (ResourceProducer, SubDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite, 0x00000000, // Granularity 0xFED45000, // Range Minimum 0xFED811FF, // Range Maximum 0x00000000, // Translation Offset 0x0003C200, // Length ,, , AddressRangeMemory, TypeStatic) Here, 0xFED811FF - 0xFED45000 = 0x3C1FF 0x3C1FF + 1 = 0x3C200 (which again matches the length field) But the same resource_size() will not help in case of PMF _CRS ResourceTemplate(). Hence I had to make a custom change like doing "-1". So, in the current change proposed here, we can have two options: 1) just use: pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start; 2) use resource_size() with -1 and leave a note on why "-1" is required pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1; Let me know your thoughts. Thanks, Shyam
On Tue, 15 Oct 2024, Shyam Sundar S K wrote: > On 10/14/2024 23:24, Ilpo Järvinen wrote: > > On Mon, 14 Oct 2024, Shyam Sundar S K wrote: > >> 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@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 | 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(-) > >>>> > >>>> + 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. > > > > That +1 is there to counter the -1 done on the set side. res->end is > > supposed to point last valid address of the resource, not the address > > after it. With round sizes, the end address is expected to end with lots > > of Fs (hex) but in your example it ends into zeros (if I interpret your > > numbers right)? > > > > Yes, that's right. > > > Couple of more examples, where resource_size() will fit correctly. > > > Example #1: > WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, > 0x0000, // Granularity > 0x0D00, // Range Minimum > 0x0FFF, // Range Maximum > 0x0000, // Translation Offset > 0x0300, // Length > ,, , TypeStatic, DenseTranslation) > > resource_size() will do 'res->end - res->start + 1;' > So, > 0xFFF-0xD00 = 0x2FF > 0x2FF + 1 = 0x300 (which matches the length field) > > Example #2: > DWordMemory (ResourceProducer, SubDecode, MinFixed, MaxFixed, > NonCacheable, ReadWrite, > 0x00000000, // Granularity > 0xFED45000, // Range Minimum > 0xFED811FF, // Range Maximum > 0x00000000, // Translation Offset > 0x0003C200, // Length > ,, , AddressRangeMemory, TypeStatic) > > Here, > 0xFED811FF - 0xFED45000 = 0x3C1FF > 0x3C1FF + 1 = 0x3C200 (which again matches the length field) > > But the same resource_size() will not help in case of PMF _CRS > ResourceTemplate(). Hence I had to make a custom change like doing "-1". > > So, in the current change proposed here, we can have two options: > > 1) just use: > pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start; Since it doesn't seem to cover the entire resource, I think this option 1) is better for now. > 2) use resource_size() with -1 and leave a note on why "-1" is required > pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1; > > Let me know your thoughts.
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); + 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; -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); 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; }