1. Patchew
  2. linux
  3. View series

[PATCH v1] ath5k: fix WARNING opportunity for swap.

Mahmoud Maatuq posted 1 patch 2 years, 4 months ago
There is a newer version of this series
drivers/net/wireless/ath/ath5k/phy.c | 29 +++++++++++++---------------
1 file changed, 13 insertions(+), 16 deletions(-)
[PATCH v1] ath5k: fix WARNING opportunity for swap.
Posted by Mahmoud Maatuq 2 years, 4 months ago 
coccinielle reported the following:
./drivers/net/wireless/ath/ath5k/phy.c:1573:25-26: WARNING opportunity for swap()

Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
---
changes in v1:
- replace the entire double loop with sort()
  as suggested by Jiri Slaby <jirislaby@kernel.org>
---
 drivers/net/wireless/ath/ath5k/phy.c | 29 +++++++++++++---------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index 5797ef9c73d7..7ee4e1616f45 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -26,6 +26,7 @@
 
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/sort.h>
 #include <asm/unaligned.h>
 
 #include "ath5k.h"
@@ -1554,6 +1555,11 @@ static void ath5k_hw_update_nfcal_hist(struct ath5k_hw *ah, s16 noise_floor)
 	hist->nfval[hist->index] = noise_floor;
 }
 
+static int cmps16(const void *a, const void *b)
+{
+	return *(s16 *)a - *(s16 *)b;
+}
+
 /**
  * ath5k_hw_get_median_noise_floor() - Get median NF from history buffer
  * @ah: The &struct ath5k_hw
@@ -1561,25 +1567,16 @@ static void ath5k_hw_update_nfcal_hist(struct ath5k_hw *ah, s16 noise_floor)
 static s16
 ath5k_hw_get_median_noise_floor(struct ath5k_hw *ah)
 {
-	s16 sort[ATH5K_NF_CAL_HIST_MAX];
-	s16 tmp;
-	int i, j;
-
-	memcpy(sort, ah->ah_nfcal_hist.nfval, sizeof(sort));
-	for (i = 0; i < ATH5K_NF_CAL_HIST_MAX - 1; i++) {
-		for (j = 1; j < ATH5K_NF_CAL_HIST_MAX - i; j++) {
-			if (sort[j] > sort[j - 1]) {
-				tmp = sort[j];
-				sort[j] = sort[j - 1];
-				sort[j - 1] = tmp;
-			}
-		}
-	}
+	s16 sorted_nfval[ATH5K_NF_CAL_HIST_MAX];
+	int i;
+
+	memcpy(sorted_nfval, ah->ah_nfcal_hist.nfval, sizeof(sorted_nfval));
+	sort(sorted_nfval, ATH5K_NF_CAL_HIST_MAX, sizeof(s16), cmps16, NULL);
 	for (i = 0; i < ATH5K_NF_CAL_HIST_MAX; i++) {
 		ATH5K_DBG(ah, ATH5K_DEBUG_CALIBRATE,
-			"cal %d:%d\n", i, sort[i]);
+			"cal %d:%d\n", i, sorted_nfval[i]);
 	}
-	return sort[(ATH5K_NF_CAL_HIST_MAX - 1) / 2];
+	return sorted_nfval[(ATH5K_NF_CAL_HIST_MAX - 1) / 2];
 }
 
 /**
-- 
2.34.1
Re: [PATCH v1] ath5k: fix WARNING opportunity for swap.
Posted by Jeff Johnson 2 years, 4 months ago 
On 8/14/2023 1:02 PM, Mahmoud Maatuq wrote:
> coccinielle reported the following:
> ./drivers/net/wireless/ath/ath5k/phy.c:1573:25-26: WARNING opportunity for swap()

Suggest you add something like:

This revealed that ath5k_hw_get_median_noise_floor() had open-coded 
sort() functionality. Since ath5k_hw_get_median_noise_floor() only 
executes once every 10 seconds, any extra overhead due to sort() calling 
it's "compare" and "swap" functions can be ignored, so replace the 
existing logic with a call to sort().

and before your SOB add:
Suggested-by: Jiri Slaby <jirislaby@kernel.org>

> Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> ---
> changes in v1:
> - replace the entire double loop with sort()
>    as suggested by Jiri Slaby <jirislaby@kernel.org>
> ---
>   drivers/net/wireless/ath/ath5k/phy.c | 29 +++++++++++++---------------
>   1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
> index 5797ef9c73d7..7ee4e1616f45 100644
> --- a/drivers/net/wireless/ath/ath5k/phy.c
> +++ b/drivers/net/wireless/ath/ath5k/phy.c
> @@ -26,6 +26,7 @@
>   
>   #include <linux/delay.h>
>   #include <linux/slab.h>
> +#include <linux/sort.h>
>   #include <asm/unaligned.h>
>   
>   #include "ath5k.h"
> @@ -1554,6 +1555,11 @@ static void ath5k_hw_update_nfcal_hist(struct ath5k_hw *ah, s16 noise_floor)
>   	hist->nfval[hist->index] = noise_floor;
>   }
>   
> +static int cmps16(const void *a, const void *b)
> +{
> +	return *(s16 *)a - *(s16 *)b;
> +}
> +
>   /**
>    * ath5k_hw_get_median_noise_floor() - Get median NF from history buffer
>    * @ah: The &struct ath5k_hw
> @@ -1561,25 +1567,16 @@ static void ath5k_hw_update_nfcal_hist(struct ath5k_hw *ah, s16 noise_floor)
>   static s16
>   ath5k_hw_get_median_noise_floor(struct ath5k_hw *ah)
>   {
> -	s16 sort[ATH5K_NF_CAL_HIST_MAX];
> -	s16 tmp;
> -	int i, j;
> -
> -	memcpy(sort, ah->ah_nfcal_hist.nfval, sizeof(sort));
> -	for (i = 0; i < ATH5K_NF_CAL_HIST_MAX - 1; i++) {
> -		for (j = 1; j < ATH5K_NF_CAL_HIST_MAX - i; j++) {
> -			if (sort[j] > sort[j - 1]) {
> -				tmp = sort[j];
> -				sort[j] = sort[j - 1];
> -				sort[j - 1] = tmp;
> -			}
> -		}
> -	}
> +	s16 sorted_nfval[ATH5K_NF_CAL_HIST_MAX];
> +	int i;
> +
> +	memcpy(sorted_nfval, ah->ah_nfcal_hist.nfval, sizeof(sorted_nfval));
> +	sort(sorted_nfval, ATH5K_NF_CAL_HIST_MAX, sizeof(s16), cmps16, NULL);
>   	for (i = 0; i < ATH5K_NF_CAL_HIST_MAX; i++) {
>   		ATH5K_DBG(ah, ATH5K_DEBUG_CALIBRATE,
> -			"cal %d:%d\n", i, sort[i]);
> +			"cal %d:%d\n", i, sorted_nfval[i]);
>   	}
> -	return sort[(ATH5K_NF_CAL_HIST_MAX - 1) / 2];
> +	return sorted_nfval[(ATH5K_NF_CAL_HIST_MAX - 1) / 2];
>   }
>   
>   /**
Re: [PATCH v1] ath5k: fix WARNING opportunity for swap.
Posted by Jeff Johnson 2 years, 4 months ago 
On 8/14/2023 3:28 PM, Jeff Johnson wrote:
> On 8/14/2023 1:02 PM, Mahmoud Maatuq wrote:
>> coccinielle reported the following:
>> ./drivers/net/wireless/ath/ath5k/phy.c:1573:25-26: WARNING opportunity 
>> for swap()
> 
> Suggest you add something like:
> 
> This revealed that ath5k_hw_get_median_noise_floor() had open-coded 
> sort() functionality. Since ath5k_hw_get_median_noise_floor() only 
> executes once every 10 seconds, any extra overhead due to sort() calling 
> it's "compare" and "swap" functions can be ignored, so replace the 

<blush> and of course that should be its and not it's </blush>

> existing logic with a call to sort().
> 
> and before your SOB add:
> Suggested-by: Jiri Slaby <jirislaby@kernel.org>
> 
>> Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
>> ---
>> changes in v1:
>> - replace the entire double loop with sort()
>>    as suggested by Jiri Slaby <jirislaby@kernel.org>
>> ---
>>   drivers/net/wireless/ath/ath5k/phy.c | 29 +++++++++++++---------------
>>   1 file changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath5k/phy.c 
>> b/drivers/net/wireless/ath/ath5k/phy.c
>> index 5797ef9c73d7..7ee4e1616f45 100644
>> --- a/drivers/net/wireless/ath/ath5k/phy.c
>> +++ b/drivers/net/wireless/ath/ath5k/phy.c
>> @@ -26,6 +26,7 @@
>>   #include <linux/delay.h>
>>   #include <linux/slab.h>
>> +#include <linux/sort.h>
>>   #include <asm/unaligned.h>
>>   #include "ath5k.h"
>> @@ -1554,6 +1555,11 @@ static void ath5k_hw_update_nfcal_hist(struct 
>> ath5k_hw *ah, s16 noise_floor)
>>       hist->nfval[hist->index] = noise_floor;
>>   }
>> +static int cmps16(const void *a, const void *b)
>> +{
>> +    return *(s16 *)a - *(s16 *)b;
>> +}
>> +
>>   /**
>>    * ath5k_hw_get_median_noise_floor() - Get median NF from history 
>> buffer
>>    * @ah: The &struct ath5k_hw
>> @@ -1561,25 +1567,16 @@ static void ath5k_hw_update_nfcal_hist(struct 
>> ath5k_hw *ah, s16 noise_floor)
>>   static s16
>>   ath5k_hw_get_median_noise_floor(struct ath5k_hw *ah)
>>   {
>> -    s16 sort[ATH5K_NF_CAL_HIST_MAX];
>> -    s16 tmp;
>> -    int i, j;
>> -
>> -    memcpy(sort, ah->ah_nfcal_hist.nfval, sizeof(sort));
>> -    for (i = 0; i < ATH5K_NF_CAL_HIST_MAX - 1; i++) {
>> -        for (j = 1; j < ATH5K_NF_CAL_HIST_MAX - i; j++) {
>> -            if (sort[j] > sort[j - 1]) {
>> -                tmp = sort[j];
>> -                sort[j] = sort[j - 1];
>> -                sort[j - 1] = tmp;
>> -            }
>> -        }
>> -    }
>> +    s16 sorted_nfval[ATH5K_NF_CAL_HIST_MAX];
>> +    int i;
>> +
>> +    memcpy(sorted_nfval, ah->ah_nfcal_hist.nfval, sizeof(sorted_nfval));
>> +    sort(sorted_nfval, ATH5K_NF_CAL_HIST_MAX, sizeof(s16), cmps16, 
>> NULL);
>>       for (i = 0; i < ATH5K_NF_CAL_HIST_MAX; i++) {
>>           ATH5K_DBG(ah, ATH5K_DEBUG_CALIBRATE,
>> -            "cal %d:%d\n", i, sort[i]);
>> +            "cal %d:%d\n", i, sorted_nfval[i]);
>>       }
>> -    return sort[(ATH5K_NF_CAL_HIST_MAX - 1) / 2];
>> +    return sorted_nfval[(ATH5K_NF_CAL_HIST_MAX - 1) / 2];
>>   }
>>   /**
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.