| Message ID | 20230814200234.637583-1-mahmoudmatook.mm@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v1] ath5k: fix WARNING opportunity for swap. | expand |
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]; > } > > /**
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]; >> } >> /** >
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]; } /**
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(-)