[PATCH] firmware: xilinx: Use hash-table for api feature check

[PATCH] firmware: xilinx: Use hash-table for api feature check

[Rajan Vaja]

From: Amit Sunil Dhamne <amit.sunil.dhamne@xilinx.com>

Currently array of fix length PM_API_MAX is used to cache
the pm_api version (valid or invalid). However ATF based
PM APIs values are much higher then PM_API_MAX.
So to include ATF based PM APIs also, use hash-table to
store the pm_api version status.

Signed-off-by: Amit Sunil Dhamne <amit.sunil.dhamne@xilinx.com>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Ravi Patel <ravi.patel@xilinx.com>
Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
---
 drivers/firmware/xilinx/zynqmp.c     | 63 ++++++++++++++++++++++++++++--------
 include/linux/firmware/xlnx-zynqmp.h |  4 ---
 2 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index efb8a66..349ab39 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -20,12 +20,28 @@
 #include <linux/of_platform.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/hashtable.h>
 
 #include <linux/firmware/xlnx-zynqmp.h>
 #include "zynqmp-debug.h"
 
+/* Max HashMap Order for PM API feature check (1<<7 = 128) */
+#define PM_API_FEATURE_CHECK_MAX_ORDER  7
+
 static bool feature_check_enabled;
-static u32 zynqmp_pm_features[PM_API_MAX];
+DEFINE_HASHTABLE(pm_api_features_map, PM_API_FEATURE_CHECK_MAX_ORDER);
+
+/**
+ * struct pm_api_feature_data - PM API Feature data
+ * @pm_api_id:		PM API Id, used as key to index into hashmap
+ * @feature_status:	status of PM API feature: valid, invalid
+ * @hentry:		hlist_node that hooks this entry into hashtable
+ */
+struct pm_api_feature_data {
+	u32 pm_api_id;
+	int feature_status;
+	struct hlist_node hentry;
+};
 
 static const struct mfd_cell firmware_devs[] = {
 	{
@@ -142,29 +158,37 @@ static int zynqmp_pm_feature(u32 api_id)
 	int ret;
 	u32 ret_payload[PAYLOAD_ARG_CNT];
 	u64 smc_arg[2];
+	struct pm_api_feature_data *feature_data;
 
 	if (!feature_check_enabled)
 		return 0;
 
-	/* Return value if feature is already checked */
-	if (api_id > ARRAY_SIZE(zynqmp_pm_features))
-		return PM_FEATURE_INVALID;
+	/* Check for existing entry in hash table for given api */
+	hash_for_each_possible(pm_api_features_map, feature_data, hentry,
+			       api_id) {
+		if (feature_data->pm_api_id == api_id)
+			return feature_data->feature_status;
+	}
 
-	if (zynqmp_pm_features[api_id] != PM_FEATURE_UNCHECKED)
-		return zynqmp_pm_features[api_id];
+	/* Add new entry if not present */
+	feature_data = kmalloc(sizeof(*feature_data), GFP_KERNEL);
+	if (!feature_data)
+		return -ENOMEM;
 
+	feature_data->pm_api_id = api_id;
 	smc_arg[0] = PM_SIP_SVC | PM_FEATURE_CHECK;
 	smc_arg[1] = api_id;
 
 	ret = do_fw_call(smc_arg[0], smc_arg[1], 0, ret_payload);
-	if (ret) {
-		zynqmp_pm_features[api_id] = PM_FEATURE_INVALID;
-		return PM_FEATURE_INVALID;
-	}
+	if (ret)
+		ret = -EOPNOTSUPP;
+	else
+		ret = ret_payload[1];
 
-	zynqmp_pm_features[api_id] = ret_payload[1];
+	feature_data->feature_status = ret;
+	hash_add(pm_api_features_map, &feature_data->hentry, api_id);
 
-	return zynqmp_pm_features[api_id];
+	return ret;
 }
 
 /**
@@ -200,9 +224,12 @@ int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
 	 * Make sure to stay in x0 register
 	 */
 	u64 smc_arg[4];
+	int ret;
 
-	if (zynqmp_pm_feature(pm_api_id) == PM_FEATURE_INVALID)
-		return -ENOTSUPP;
+	/* Check if feature is supported or not */
+	ret = zynqmp_pm_feature(pm_api_id);
+	if (ret < 0)
+		return ret;
 
 	smc_arg[0] = PM_SIP_SVC | pm_api_id;
 	smc_arg[1] = ((u64)arg1 << 32) | arg0;
@@ -1252,9 +1279,17 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
 
 static int zynqmp_firmware_remove(struct platform_device *pdev)
 {
+	struct pm_api_feature_data *feature_data;
+	int i;
+
 	mfd_remove_devices(&pdev->dev);
 	zynqmp_pm_api_debugfs_exit();
 
+	hash_for_each(pm_api_features_map, i, feature_data, hentry) {
+		hash_del(&feature_data->hentry);
+		kfree(feature_data);
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 5968df8..41a1bab 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -50,10 +50,6 @@
 #define	ZYNQMP_PM_CAPABILITY_WAKEUP	0x4U
 #define	ZYNQMP_PM_CAPABILITY_UNUSABLE	0x8U
 
-/* Feature check status */
-#define PM_FEATURE_INVALID		-1
-#define PM_FEATURE_UNCHECKED		0
-
 /*
  * Firmware FPGA Manager flags
  * XILINX_ZYNQMP_PM_FPGA_FULL:	FPGA full reconfiguration
-- 
2.7.4

Re: [PATCH] firmware: xilinx: Use hash-table for api feature check

[Michal Simek]

Hi,

On 24. 11. 20 6:52, Rajan Vaja wrote:
> From: Amit Sunil Dhamne <amit.sunil.dhamne@xilinx.com>
> 
> Currently array of fix length PM_API_MAX is used to cache
> the pm_api version (valid or invalid). However ATF based
> PM APIs values are much higher then PM_API_MAX.
> So to include ATF based PM APIs also, use hash-table to
> store the pm_api version status.
> 
> Signed-off-by: Amit Sunil Dhamne <amit.sunil.dhamne@xilinx.com>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Ravi Patel <ravi.patel@xilinx.com>
> Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
> ---
>  drivers/firmware/xilinx/zynqmp.c     | 63 ++++++++++++++++++++++++++++--------
>  include/linux/firmware/xlnx-zynqmp.h |  4 ---
>  2 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index efb8a66..349ab39 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -20,12 +20,28 @@
>  #include <linux/of_platform.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
> +#include <linux/hashtable.h>
>  
>  #include <linux/firmware/xlnx-zynqmp.h>
>  #include "zynqmp-debug.h"
>  
> +/* Max HashMap Order for PM API feature check (1<<7 = 128) */
> +#define PM_API_FEATURE_CHECK_MAX_ORDER  7
> +
>  static bool feature_check_enabled;
> -static u32 zynqmp_pm_features[PM_API_MAX];
> +DEFINE_HASHTABLE(pm_api_features_map, PM_API_FEATURE_CHECK_MAX_ORDER);
> +
> +/**
> + * struct pm_api_feature_data - PM API Feature data
> + * @pm_api_id:		PM API Id, used as key to index into hashmap
> + * @feature_status:	status of PM API feature: valid, invalid
> + * @hentry:		hlist_node that hooks this entry into hashtable
> + */
> +struct pm_api_feature_data {
> +	u32 pm_api_id;
> +	int feature_status;
> +	struct hlist_node hentry;
> +};
>  
>  static const struct mfd_cell firmware_devs[] = {
>  	{
> @@ -142,29 +158,37 @@ static int zynqmp_pm_feature(u32 api_id)
>  	int ret;
>  	u32 ret_payload[PAYLOAD_ARG_CNT];
>  	u64 smc_arg[2];
> +	struct pm_api_feature_data *feature_data;
>  
>  	if (!feature_check_enabled)
>  		return 0;
>  
> -	/* Return value if feature is already checked */
> -	if (api_id > ARRAY_SIZE(zynqmp_pm_features))
> -		return PM_FEATURE_INVALID;
> +	/* Check for existing entry in hash table for given api */
> +	hash_for_each_possible(pm_api_features_map, feature_data, hentry,
> +			       api_id) {
> +		if (feature_data->pm_api_id == api_id)
> +			return feature_data->feature_status;
> +	}
>  
> -	if (zynqmp_pm_features[api_id] != PM_FEATURE_UNCHECKED)
> -		return zynqmp_pm_features[api_id];
> +	/* Add new entry if not present */
> +	feature_data = kmalloc(sizeof(*feature_data), GFP_KERNEL);
> +	if (!feature_data)
> +		return -ENOMEM;
>  
> +	feature_data->pm_api_id = api_id;
>  	smc_arg[0] = PM_SIP_SVC | PM_FEATURE_CHECK;
>  	smc_arg[1] = api_id;
>  
>  	ret = do_fw_call(smc_arg[0], smc_arg[1], 0, ret_payload);
> -	if (ret) {
> -		zynqmp_pm_features[api_id] = PM_FEATURE_INVALID;
> -		return PM_FEATURE_INVALID;
> -	}
> +	if (ret)
> +		ret = -EOPNOTSUPP;
> +	else
> +		ret = ret_payload[1];
>  
> -	zynqmp_pm_features[api_id] = ret_payload[1];
> +	feature_data->feature_status = ret;
> +	hash_add(pm_api_features_map, &feature_data->hentry, api_id);
>  
> -	return zynqmp_pm_features[api_id];
> +	return ret;
>  }
>  
>  /**
> @@ -200,9 +224,12 @@ int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
>  	 * Make sure to stay in x0 register
>  	 */
>  	u64 smc_arg[4];
> +	int ret;
>  
> -	if (zynqmp_pm_feature(pm_api_id) == PM_FEATURE_INVALID)
> -		return -ENOTSUPP;
> +	/* Check if feature is supported or not */
> +	ret = zynqmp_pm_feature(pm_api_id);
> +	if (ret < 0)
> +		return ret;
>  
>  	smc_arg[0] = PM_SIP_SVC | pm_api_id;
>  	smc_arg[1] = ((u64)arg1 << 32) | arg0;
> @@ -1252,9 +1279,17 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
>  
>  static int zynqmp_firmware_remove(struct platform_device *pdev)
>  {
> +	struct pm_api_feature_data *feature_data;
> +	int i;
> +
>  	mfd_remove_devices(&pdev->dev);
>  	zynqmp_pm_api_debugfs_exit();
>  
> +	hash_for_each(pm_api_features_map, i, feature_data, hentry) {
> +		hash_del(&feature_data->hentry);
> +		kfree(feature_data);
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
> index 5968df8..41a1bab 100644
> --- a/include/linux/firmware/xlnx-zynqmp.h
> +++ b/include/linux/firmware/xlnx-zynqmp.h
> @@ -50,10 +50,6 @@
>  #define	ZYNQMP_PM_CAPABILITY_WAKEUP	0x4U
>  #define	ZYNQMP_PM_CAPABILITY_UNUSABLE	0x8U
>  
> -/* Feature check status */
> -#define PM_FEATURE_INVALID		-1
> -#define PM_FEATURE_UNCHECKED		0
> -
>  /*
>   * Firmware FPGA Manager flags
>   * XILINX_ZYNQMP_PM_FPGA_FULL:	FPGA full reconfiguration
> 

Some lines should be added.

Cc: stable <stable@vger.kernel.org>
Fixes: f3217d6f2f7a ("firmware: xilinx: fix out-of-bounds access")

Tested-by: Michal Simek <michal.simek@xilinx.com>

Arnd: do you see any issue with this?
I found that your origin patch was taken by Greg and pushed to any
5.10-rc version which caused panic on Xilinx Versal. That's why I would
like to get this patch to v5.10 if possible.

Thanks,
Michal

Re: [PATCH] firmware: xilinx: Use hash-table for api feature check

[Arnd Bergmann]

On Tue, Nov 24, 2020 at 1:54 PM Michal Simek <michal.simek@xilinx.com> wrote:
> On 24. 11. 20 6:52, Rajan Vaja wrote:
>
> Some lines should be added.
>
> Cc: stable <stable@vger.kernel.org>
> Fixes: f3217d6f2f7a ("firmware: xilinx: fix out-of-bounds access")
>
> Tested-by: Michal Simek <michal.simek@xilinx.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> Arnd: do you see any issue with this?
> I found that your origin patch was taken by Greg and pushed to any
> 5.10-rc version which caused panic on Xilinx Versal. That's why I would
> like to get this patch to v5.10 if possible.

Looks ok to me. If you'd like me to pick it up through the soc fixes
branch, please forward with the above tags to soc@kernel.org

      Arnd

Re: [PATCH] firmware: xilinx: Use hash-table for api feature check

[Michal Simek]

Hi,

On 24. 11. 20 14:02, Arnd Bergmann wrote:
> On Tue, Nov 24, 2020 at 1:54 PM Michal Simek <michal.simek@xilinx.com> wrote:
>> On 24. 11. 20 6:52, Rajan Vaja wrote:
>>
>> Some lines should be added.
>>
>> Cc: stable <stable@vger.kernel.org>
>> Fixes: f3217d6f2f7a ("firmware: xilinx: fix out-of-bounds access")
>>
>> Tested-by: Michal Simek <michal.simek@xilinx.com>
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
>> Arnd: do you see any issue with this?
>> I found that your origin patch was taken by Greg and pushed to any
>> 5.10-rc version which caused panic on Xilinx Versal. That's why I would
>> like to get this patch to v5.10 if possible.
> 
> Looks ok to me. If you'd like me to pick it up through the soc fixes
> branch, please forward with the above tags to soc@kernel.org

I have one more fix in my queue. Will send pull request with it soon.

Thanks,
Michal

Re: [PATCH] firmware: xilinx: Use hash-table for api feature check

[Michal Simek]

On 24. 11. 20 6:52, Rajan Vaja wrote:
> From: Amit Sunil Dhamne <amit.sunil.dhamne@xilinx.com>
> 
> Currently array of fix length PM_API_MAX is used to cache
> the pm_api version (valid or invalid). However ATF based
> PM APIs values are much higher then PM_API_MAX.
> So to include ATF based PM APIs also, use hash-table to
> store the pm_api version status.
> 
> Signed-off-by: Amit Sunil Dhamne <amit.sunil.dhamne@xilinx.com>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Ravi Patel <ravi.patel@xilinx.com>
> Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
> ---
>  drivers/firmware/xilinx/zynqmp.c     | 63 ++++++++++++++++++++++++++++--------
>  include/linux/firmware/xlnx-zynqmp.h |  4 ---
>  2 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index efb8a66..349ab39 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -20,12 +20,28 @@
>  #include <linux/of_platform.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
> +#include <linux/hashtable.h>
>  
>  #include <linux/firmware/xlnx-zynqmp.h>
>  #include "zynqmp-debug.h"
>  
> +/* Max HashMap Order for PM API feature check (1<<7 = 128) */
> +#define PM_API_FEATURE_CHECK_MAX_ORDER  7
> +
>  static bool feature_check_enabled;
> -static u32 zynqmp_pm_features[PM_API_MAX];
> +DEFINE_HASHTABLE(pm_api_features_map, PM_API_FEATURE_CHECK_MAX_ORDER);
> +
> +/**
> + * struct pm_api_feature_data - PM API Feature data
> + * @pm_api_id:		PM API Id, used as key to index into hashmap
> + * @feature_status:	status of PM API feature: valid, invalid
> + * @hentry:		hlist_node that hooks this entry into hashtable
> + */
> +struct pm_api_feature_data {
> +	u32 pm_api_id;
> +	int feature_status;
> +	struct hlist_node hentry;
> +};
>  
>  static const struct mfd_cell firmware_devs[] = {
>  	{
> @@ -142,29 +158,37 @@ static int zynqmp_pm_feature(u32 api_id)
>  	int ret;
>  	u32 ret_payload[PAYLOAD_ARG_CNT];
>  	u64 smc_arg[2];
> +	struct pm_api_feature_data *feature_data;
>  
>  	if (!feature_check_enabled)
>  		return 0;
>  
> -	/* Return value if feature is already checked */
> -	if (api_id > ARRAY_SIZE(zynqmp_pm_features))
> -		return PM_FEATURE_INVALID;
> +	/* Check for existing entry in hash table for given api */
> +	hash_for_each_possible(pm_api_features_map, feature_data, hentry,
> +			       api_id) {
> +		if (feature_data->pm_api_id == api_id)
> +			return feature_data->feature_status;
> +	}
>  
> -	if (zynqmp_pm_features[api_id] != PM_FEATURE_UNCHECKED)
> -		return zynqmp_pm_features[api_id];
> +	/* Add new entry if not present */
> +	feature_data = kmalloc(sizeof(*feature_data), GFP_KERNEL);
> +	if (!feature_data)
> +		return -ENOMEM;
>  
> +	feature_data->pm_api_id = api_id;
>  	smc_arg[0] = PM_SIP_SVC | PM_FEATURE_CHECK;
>  	smc_arg[1] = api_id;
>  
>  	ret = do_fw_call(smc_arg[0], smc_arg[1], 0, ret_payload);
> -	if (ret) {
> -		zynqmp_pm_features[api_id] = PM_FEATURE_INVALID;
> -		return PM_FEATURE_INVALID;
> -	}
> +	if (ret)
> +		ret = -EOPNOTSUPP;
> +	else
> +		ret = ret_payload[1];
>  
> -	zynqmp_pm_features[api_id] = ret_payload[1];
> +	feature_data->feature_status = ret;
> +	hash_add(pm_api_features_map, &feature_data->hentry, api_id);
>  
> -	return zynqmp_pm_features[api_id];
> +	return ret;
>  }
>  
>  /**
> @@ -200,9 +224,12 @@ int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
>  	 * Make sure to stay in x0 register
>  	 */
>  	u64 smc_arg[4];
> +	int ret;
>  
> -	if (zynqmp_pm_feature(pm_api_id) == PM_FEATURE_INVALID)
> -		return -ENOTSUPP;
> +	/* Check if feature is supported or not */
> +	ret = zynqmp_pm_feature(pm_api_id);
> +	if (ret < 0)
> +		return ret;
>  
>  	smc_arg[0] = PM_SIP_SVC | pm_api_id;
>  	smc_arg[1] = ((u64)arg1 << 32) | arg0;
> @@ -1252,9 +1279,17 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
>  
>  static int zynqmp_firmware_remove(struct platform_device *pdev)
>  {
> +	struct pm_api_feature_data *feature_data;
> +	int i;
> +
>  	mfd_remove_devices(&pdev->dev);
>  	zynqmp_pm_api_debugfs_exit();
>  
> +	hash_for_each(pm_api_features_map, i, feature_data, hentry) {
> +		hash_del(&feature_data->hentry);
> +		kfree(feature_data);
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
> index 5968df8..41a1bab 100644
> --- a/include/linux/firmware/xlnx-zynqmp.h
> +++ b/include/linux/firmware/xlnx-zynqmp.h
> @@ -50,10 +50,6 @@
>  #define	ZYNQMP_PM_CAPABILITY_WAKEUP	0x4U
>  #define	ZYNQMP_PM_CAPABILITY_UNUSABLE	0x8U
>  
> -/* Feature check status */
> -#define PM_FEATURE_INVALID		-1
> -#define PM_FEATURE_UNCHECKED		0
> -
>  /*
>   * Firmware FPGA Manager flags
>   * XILINX_ZYNQMP_PM_FPGA_FULL:	FPGA full reconfiguration
> 

Applied.
M