netdev - Re: [PATCH 04/32] staging: wfx: remove "burst" mechanism

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Apr 2020 16:05:26 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Jerome Pouiller <Jerome.Pouiller@...abs.com>
Cc:     devel@...verdev.osuosl.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S . Miller" <davem@...emloft.net>,
        Kalle Valo <kvalo@...eaurora.org>
Subject: Re: [PATCH 04/32] staging: wfx: remove "burst" mechanism

On Wed, Apr 01, 2020 at 01:03:37PM +0200, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@...abs.com>
> 
> In the old days, the driver tried to reorder frames in order to send
> frames from the same queue grouped to the firmware. However, the
> firmware is able to do the job internally for a long time. There is no
> reasons to keep this mechanism.
> 
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@...abs.com>
> ---
>  drivers/staging/wfx/queue.c | 23 -----------------------
>  drivers/staging/wfx/sta.c   |  2 --
>  drivers/staging/wfx/wfx.h   |  1 -
>  3 files changed, 26 deletions(-)
> 
> diff --git a/drivers/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
> index e3aa1e346c70..712ac783514b 100644
> --- a/drivers/staging/wfx/queue.c
> +++ b/drivers/staging/wfx/queue.c
> @@ -363,8 +363,6 @@ static bool hif_handle_tx_data(struct wfx_vif *wvif, struct sk_buff *skb,
>  static int wfx_get_prio_queue(struct wfx_vif *wvif,
>  				 u32 tx_allowed_mask, int *total)
>  {
> -	static const int urgent = BIT(WFX_LINK_ID_AFTER_DTIM) |
> -		BIT(WFX_LINK_ID_UAPSD);
>  	const struct ieee80211_tx_queue_params *edca;
>  	unsigned int score, best = -1;
                            ^^^^^^^^^
Not related to this this patch but this confused me initially.  UINT_MAX
would be more readable.

The other unrelated question I had about this function was:

   402          /* search for a winner using edca params */
   403          for (i = 0; i < IEEE80211_NUM_ACS; ++i) {
                                ^^^^^^^^^^^^^^^^^
IEEE80211_NUM_ACS is 4.

   404                  int queued;
   405  
   406                  edca = &wvif->edca_params[i];
   407                  queued = wfx_tx_queue_get_num_queued(&wvif->wdev->tx_queue[i],
   408                                  tx_allowed_mask);
   409                  if (!queued)
   410                          continue;
   411                  *total += queued;
   412                  score = ((edca->aifs + edca->cw_min) << 16) +
   413                          ((edca->cw_max - edca->cw_min) *
   414                           (get_random_int() & 0xFFFF));
   415                  if (score < best && (winner < 0 || i != 3)) {
                                                           ^^^^^^

Why do we not want winner to be 3?  It's unrelated to the patch but
there should be a comment next to that code probably.

   416                          best = score;
   417                          winner = i;
   418                  }
   419          }

regards,
dan carpenter