On Thu, Mar 18, 2021 at 11:41:33AM +0800, zxystd wrote:
> The indexes into the ac array in the iwx_mac_ctxt_cmd_common are from the
> iwx_ac enum and not the txfs. The current code therefore puts the edca params
> in the wrong indexes of the array, causing wrong priority for data-streams of
> different ACs. This bug is not exists in iwm as its txf number is equal to
> the corresponding ac in the iwx_ac enum.
Thank you!
This doesn't really cause any problem as far as I can see, since we
don't actually make use of QoS. But I agree it should be fixed.
> Index: src/sys/dev/pci/if_iwx.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_iwx.c,v
> retrieving revision 1.50
> diff -u -p -u -r1.50 if_iwx.c
> --- src/sys/dev/pci/if_iwx.c 17 Mar 2021 15:59:27 -0000 1.50
> +++ src/sys/dev/pci/if_iwx.c 18 Mar 2021 03:28:04 -0000
> @@ -5135,6 +5135,17 @@ iwx_ack_rates(struct iwx_softc *sc, stru
> *ofdm_rates = ofdm;
> }
>
> +static uint8_t iwx_mac80211_ac_to_ucode_ac(enum ieee80211_edca_ac ac)
> +{
> + static const uint8_t iwx_ac_to_ucode_ac[] = {
> + IWX_AC_VO,
> + IWX_AC_VI,
> + IWX_AC_BE,
> + IWX_AC_BK
> + };
I would prefer to use a global array without wrapping it inside a function.
Other code can simply index the array directly, see below.
> + return iwx_ac_to_ucode_ac[ac];
> +}
> +
> void
> iwx_mac_ctxt_cmd_common(struct iwx_softc *sc, struct iwx_node *in,
> struct iwx_mac_ctx_cmd *cmd, uint32_t action)
> @@ -5181,12 +5192,13 @@ iwx_mac_ctxt_cmd_common(struct iwx_softc
> for (i = 0; i < EDCA_NUM_AC; i++) {
> struct ieee80211_edca_ac_params *ac = &ic->ic_edca_ac[i];
> int txf = iwx_ac_to_tx_fifo[i];
> + uint8_t ucode_ac = iwx_mac80211_ac_to_ucode_ac(i);
Instead of calling a function, we could do an array lookup:
uint8_t ucode_ac = iwx_ac_to_ucode_ac[ac];
>
> - cmd->ac[txf].cw_min = htole16(IWX_EXP2(ac->ac_ecwmin));
> - cmd->ac[txf].cw_max = htole16(IWX_EXP2(ac->ac_ecwmax));
> - cmd->ac[txf].aifsn = ac->ac_aifsn;
> - cmd->ac[txf].fifos_mask = (1 << txf);
> - cmd->ac[txf].edca_txop = htole16(ac->ac_txoplimit * 32);
> + cmd->ac[ucode_ac].cw_min = htole16(IWX_EXP2(ac->ac_ecwmin));
> + cmd->ac[ucode_ac].cw_max = htole16(IWX_EXP2(ac->ac_ecwmax));
> + cmd->ac[ucode_ac].aifsn = ac->ac_aifsn;
> + cmd->ac[ucode_ac].fifos_mask = (1 << txf);
> + cmd->ac[ucode_ac].edca_txop = htole16(ac->ac_txoplimit * 32);
> }
> if (ni->ni_flags & IEEE80211_NODE_QOS)
> cmd->qos_flags |= htole32(IWX_MAC_QOS_FLG_UPDATE_EDCA);
