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

[Jeff Johnson <quic_jjohnson@xxxxxxxxxxx>]

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@xxxxxxxxxx>
>
> > Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@xxxxxxxxx>
> > ---
> > changes in v1:
> > - replace the entire double loop with sort()
> >    as suggested by Jiri Slaby <jirislaby@xxxxxxxxxx>
> > ---
> >   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];
> >   }
> >   /**