[PATCH] wifi: ath10k: Drop of_get_property() call

[PATCH] wifi: ath10k: Drop of_get_property() call

[Rob Herring (Arm)]

There's no need to check the property presence and length before calling
of_property_read_u8_array() as it will return an error if the property
is missing or the length is too small. The return errno doesn't matter
to the caller, so no change in behavior there.

Change of_property_read_u8_array() to of_property_read_variable_u8_array()
as the former allows properties to be longer than the requested length.
Now the property has to be the exact length requested as the removed
check required.

This part of a larger effort to remove DT functions like
of_get_property() and of_find_property() which return raw DT data
having no reference counting.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/net/wireless/ath/ath10k/core.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index b3294287bce1..db7444af251d 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1889,7 +1889,7 @@ static int ath10k_download_cal_file(struct ath10k *ar,
 static int ath10k_download_cal_dt(struct ath10k *ar, const char *dt_name)
 {
 	struct device_node *node;
-	int data_len;
+	int data_len = ar->hw_params.cal_data_len;
 	void *data;
 	int ret;
 
@@ -1900,28 +1900,18 @@ static int ath10k_download_cal_dt(struct ath10k *ar, const char *dt_name)
 		 */
 		return -ENOENT;
 
-	if (!of_get_property(node, dt_name, &data_len)) {
-		/* The calibration data node is optional */
-		return -ENOENT;
-	}
-
-	if (data_len != ar->hw_params.cal_data_len) {
-		ath10k_warn(ar, "invalid calibration data length in DT: %d\n",
-			    data_len);
-		ret = -EMSGSIZE;
-		goto out;
-	}
-
 	data = kmalloc(data_len, GFP_KERNEL);
 	if (!data) {
 		ret = -ENOMEM;
 		goto out;
 	}
 
-	ret = of_property_read_u8_array(node, dt_name, data, data_len);
+	ret = of_property_read_variable_u8_array(node, dt_name, data, data_len, data_len);
 	if (ret) {
-		ath10k_warn(ar, "failed to read calibration data from DT: %d\n",
-			    ret);
+		/* Don't warn if optional property not found  */
+		if (ret != -EINVAL)
+			ath10k_warn(ar, "failed to read calibration data from DT: %d\n",
+				    ret);
 		goto out_free;
 	}
 
-- 
2.47.2

Re: [PATCH] wifi: ath10k: Drop of_get_property() call

[Jeff Johnson]

On 3/12/2025 4:42 PM, Rob Herring (Arm) wrote:
> There's no need to check the property presence and length before calling
> of_property_read_u8_array() as it will return an error if the property
> is missing or the length is too small. The return errno doesn't matter
> to the caller, so no change in behavior there.
> 
> Change of_property_read_u8_array() to of_property_read_variable_u8_array()
> as the former allows properties to be longer than the requested length.
> Now the property has to be the exact length requested as the removed
> check required.
> 
> This part of a larger effort to remove DT functions like
> of_get_property() and of_find_property() which return raw DT data
> having no reference counting.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/net/wireless/ath/ath10k/core.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index b3294287bce1..db7444af251d 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -1889,7 +1889,7 @@ static int ath10k_download_cal_file(struct ath10k *ar,
>  static int ath10k_download_cal_dt(struct ath10k *ar, const char *dt_name)
>  {
>  	struct device_node *node;
> -	int data_len;
> +	int data_len = ar->hw_params.cal_data_len;
>  	void *data;
>  	int ret;
>  
> @@ -1900,28 +1900,18 @@ static int ath10k_download_cal_dt(struct ath10k *ar, const char *dt_name)
>  		 */
>  		return -ENOENT;
>  
> -	if (!of_get_property(node, dt_name, &data_len)) {
> -		/* The calibration data node is optional */
> -		return -ENOENT;
> -	}

note that there is one instance where .cal_data_len = 0

so i suggest that there still needs to be an early return here to avoid
kmalloc(0):

	if (!data_len)
		return -ENOENT;

> -
> -	if (data_len != ar->hw_params.cal_data_len) {
> -		ath10k_warn(ar, "invalid calibration data length in DT: %d\n",
> -			    data_len);
> -		ret = -EMSGSIZE;
> -		goto out;
> -	}
> -
>  	data = kmalloc(data_len, GFP_KERNEL);
>  	if (!data) {
>  		ret = -ENOMEM;
>  		goto out;
>  	}
>  
> -	ret = of_property_read_u8_array(node, dt_name, data, data_len);
> +	ret = of_property_read_variable_u8_array(node, dt_name, data, data_len, data_len);
>  	if (ret) {
> -		ath10k_warn(ar, "failed to read calibration data from DT: %d\n",
> -			    ret);
> +		/* Don't warn if optional property not found  */
> +		if (ret != -EINVAL)
> +			ath10k_warn(ar, "failed to read calibration data from DT: %d\n",
> +				    ret);
>  		goto out_free;
>  	}
>  

this could made even cleaner using cleanup.h functionality (all of the gotos
could disappear). perhaps I'll do that as a follow-up patch on top of your patch.

/jeff