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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7264 bytes --]

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)?

-- 
 i.

> 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.
> > 
>