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

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jerome Pouiller <Jerome.Pouiller@silabs.com>
Cc: devel@driverdev.osuosl.org, linux-wireless@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"David S . Miller" <davem@davemloft.net>,
	Kalle Valo <kvalo@codeaurora.org>
Subject: Re: [PATCH 04/32] staging: wfx: remove "burst" mechanism
Date: Thu, 2 Apr 2020 16:05:26 +0300	[thread overview]
Message-ID: <20200402130526.GR2001@kadam> (raw)
In-Reply-To: <20200401110405.80282-5-Jerome.Pouiller@silabs.com>

On Wed, Apr 01, 2020 at 01:03:37PM +0200, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.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@silabs.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