On Fri, May 15, 2020 at 9:29 AM Stefan Sperling <[email protected]> wrote:
> iwx(4) firmware understands two different variants of the "PHY_CONTEXT"
> command. Both variants are documented with the same command API version
> number, but they use different sizes for an embedded struct that contains
> information about channels.
>
> Which variant to use depends on whether the firmware advertises support for
> the "ULTRA_HB_CHANNELS" feature (we don't use this feature; it's about 6
> GHz
> channels; but if the command we send has the wrong size the firmware will
> reject the command).
>
> The code I wrote to handle both of these cases has a bug in case this
> feature is *not* supported. Our current iwx(4) firmware supports the
> feature, so there is no actual problem for now.
>
> I have considered removing the command variant which is currently not
> needed.
> However, additional devices may be added to this driver at some point.
> And some of those are kind of hybrids between the 9k and the ax200 device
> generations. I do not know which variant those devices will need. So this
> diff fixes the bug in the non-UHB code path, in case it will be needed.
> We can always remove this later.
>
> ok?
>
> M sys/dev/pci/if_iwx.c
> M sys/dev/pci/if_iwxreg.h
>
> diff f896dbcaba91a37b23dc6cbeb42839fba3e329be
> 24bd56723f1f029278b7c9cc71d56349db3c5950
> blob - 64c3641a2d0d07a9d899c0b7ccdbe46d46e17b96
> blob + d01db87880cc583922d1610de8319e491e7f148c
> --- sys/dev/pci/if_iwx.c
> +++ sys/dev/pci/if_iwx.c
> @@ -343,10 +343,8 @@ void iwx_rx_tx_cmd(struct iwx_softc *, struct
> iwx_rx_p
> void iwx_rx_bmiss(struct iwx_softc *, struct iwx_rx_packet *,
> struct iwx_rx_data *);
> int iwx_binding_cmd(struct iwx_softc *, struct iwx_node *, uint32_t);
> -void iwx_phy_ctxt_cmd_hdr(struct iwx_softc *, struct iwx_phy_ctxt *,
> - struct iwx_phy_context_cmd *, uint32_t, uint32_t);
> -void iwx_phy_ctxt_cmd_data(struct iwx_softc *, struct
> iwx_phy_context_cmd *,
> - struct ieee80211_channel *, uint8_t, uint8_t);
> +int iwx_phy_ctxt_cmd_uhb(struct iwx_softc *, struct iwx_phy_ctxt *,
> uint8_t,
> + uint8_t, uint32_t, uint32_t);
> int iwx_phy_ctxt_cmd(struct iwx_softc *, struct iwx_phy_ctxt *,
> uint8_t,
> uint8_t, uint32_t, uint32_t);
> int iwx_send_cmd(struct iwx_softc *, struct iwx_host_cmd *);
> @@ -3773,52 +3771,38 @@ iwx_binding_cmd(struct iwx_softc *sc, struct
> iwx_node
> return err;
> }
>
> -void
> -iwx_phy_ctxt_cmd_hdr(struct iwx_softc *sc, struct iwx_phy_ctxt *ctxt,
> - struct iwx_phy_context_cmd *cmd, uint32_t action, uint32_t apply_time)
> +int
> +iwx_phy_ctxt_cmd_uhb(struct iwx_softc *sc, struct iwx_phy_ctxt *ctxt,
> + uint8_t chains_static, uint8_t chains_dynamic, uint32_t action,
> + uint32_t apply_time)
> {
> - memset(cmd, 0, sizeof(struct iwx_phy_context_cmd));
> + struct ieee80211com *ic = &sc->sc_ic;
> + struct iwx_phy_context_cmd_uhb cmd;
> + uint8_t active_cnt, idle_cnt;
> + struct ieee80211_channel *chan = ctxt->channel;
>
> - cmd->id_and_color = htole32(IWX_FW_CMD_ID_AND_COLOR(ctxt->id,
> + memset(&cmd, 0, sizeof(cmd));
> + cmd.id_and_color = htole32(IWX_FW_CMD_ID_AND_COLOR(ctxt->id,
> ctxt->color));
> - cmd->action = htole32(action);
> - cmd->apply_time = htole32(apply_time);
> -}
> + cmd.action = htole32(action);
> + cmd.apply_time = htole32(apply_time);
>
> -void
> -iwx_phy_ctxt_cmd_data(struct iwx_softc *sc, struct iwx_phy_context_cmd
> *cmd,
> - struct ieee80211_channel *chan, uint8_t chains_static,
> - uint8_t chains_dynamic)
> -{
> - struct ieee80211com *ic = &sc->sc_ic;
> - uint8_t active_cnt, idle_cnt;
> + cmd.ci.band = IEEE80211_IS_CHAN_2GHZ(chan) ?
> + IWX_PHY_BAND_24 : IWX_PHY_BAND_5;
> + cmd.ci.channel = htole32(ieee80211_chan2ieee(ic, chan));
> + cmd.ci.width = IWX_PHY_VHT_CHANNEL_MODE20;
> + cmd.ci.ctrl_pos = IWX_PHY_VHT_CTRL_POS_1_BELOW;
>
> - if (isset(sc->sc_enabled_capa,
> IWX_UCODE_TLV_CAPA_ULTRA_HB_CHANNELS)) {
> - cmd->ci.band = IEEE80211_IS_CHAN_2GHZ(chan) ?
> - IWX_PHY_BAND_24 : IWX_PHY_BAND_5;
> - cmd->ci.channel = htole32(ieee80211_chan2ieee(ic, chan));
> - cmd->ci.width = IWX_PHY_VHT_CHANNEL_MODE20;
> - cmd->ci.ctrl_pos = IWX_PHY_VHT_CTRL_POS_1_BELOW;
> - } else {
> - struct iwx_fw_channel_info_v1 *ci_v1;
> - ci_v1 = (struct iwx_fw_channel_info_v1 *)&cmd->ci;
> - ci_v1->band = IEEE80211_IS_CHAN_2GHZ(chan) ?
> - IWX_PHY_BAND_24 : IWX_PHY_BAND_5;
> - ci_v1->channel = ieee80211_chan2ieee(ic, chan);
> - ci_v1->width = IWX_PHY_VHT_CHANNEL_MODE20;
> - ci_v1->ctrl_pos = IWX_PHY_VHT_CTRL_POS_1_BELOW;
> - }
> - /* Set rx the chains */
> idle_cnt = chains_static;
> active_cnt = chains_dynamic;
> -
> - cmd->rxchain_info = htole32(iwx_fw_valid_rx_ant(sc) <<
> + cmd.rxchain_info = htole32(iwx_fw_valid_rx_ant(sc) <<
> IWX_PHY_RX_CHAIN_VALID_POS);
> - cmd->rxchain_info |= htole32(idle_cnt << IWX_PHY_RX_CHAIN_CNT_POS);
> - cmd->rxchain_info |= htole32(active_cnt <<
> + cmd.rxchain_info |= htole32(idle_cnt << IWX_PHY_RX_CHAIN_CNT_POS);
> + cmd.rxchain_info |= htole32(active_cnt <<
> IWX_PHY_RX_CHAIN_MIMO_CNT_POS);
> + cmd.txchain_info = htole32(iwx_fw_valid_tx_ant(sc));
>
> - cmd->txchain_info = htole32(iwx_fw_valid_tx_ant(sc));
> + return iwx_send_cmd_pdu(sc, IWX_PHY_CONTEXT_CMD, 0, sizeof(cmd),
> &cmd);
> }
>
> int
> @@ -3826,23 +3810,44 @@ iwx_phy_ctxt_cmd(struct iwx_softc *sc, struct
> iwx_phy_
> uint8_t chains_static, uint8_t chains_dynamic, uint32_t action,
> uint32_t apply_time)
> {
> + struct ieee80211com *ic = &sc->sc_ic;
> struct iwx_phy_context_cmd cmd;
> - size_t len;
> + uint8_t active_cnt, idle_cnt;
> + struct ieee80211_channel *chan = ctxt->channel;
>
> - iwx_phy_ctxt_cmd_hdr(sc, ctxt, &cmd, action, apply_time);
> -
> /*
> - * Intel resized fw_channel_info struct and neglected to resize the
> - * phy_context_cmd struct which contains it; so magic happens with
> - * command length adjustments at run-time... :(
> + * Intel increased the size of the fw_channel_info struct and
> neglected
> + * to bump the phy_context_cmd struct, which contains an
> fw_channel_info
> + * member in the middle.
> + * To keep things simple we use a separate function to handle the
> larger
> + * variant of the phy context command.
> */
> - iwx_phy_ctxt_cmd_data(sc, &cmd, ctxt->channel,
> - chains_static, chains_dynamic);
> - len = sizeof(struct iwx_phy_context_cmd);
> - if (!isset(sc->sc_enabled_capa,
> IWX_UCODE_TLV_CAPA_ULTRA_HB_CHANNELS))
> - len -= (sizeof(struct iwx_fw_channel_info) -
> - sizeof(struct iwx_fw_channel_info_v1));
> - return iwx_send_cmd_pdu(sc, IWX_PHY_CONTEXT_CMD, 0, len, &cmd);
> + if (isset(sc->sc_enabled_capa,
> IWX_UCODE_TLV_CAPA_ULTRA_HB_CHANNELS))
> + return iwx_phy_ctxt_cmd_uhb(sc, ctxt, chains_static,
> + chains_dynamic, action, apply_time);
> +
> + memset(&cmd, 0, sizeof(cmd));
> + cmd.id_and_color = htole32(IWX_FW_CMD_ID_AND_COLOR(ctxt->id,
> + ctxt->color));
> + cmd.action = htole32(action);
> + cmd.apply_time = htole32(apply_time);
> +
> + cmd.ci.band = IEEE80211_IS_CHAN_2GHZ(chan) ?
> + IWX_PHY_BAND_24 : IWX_PHY_BAND_5;
> + cmd.ci.channel = ieee80211_chan2ieee(ic, chan);
> + cmd.ci.width = IWX_PHY_VHT_CHANNEL_MODE20;
> + cmd.ci.ctrl_pos = IWX_PHY_VHT_CTRL_POS_1_BELOW;
> +
> + idle_cnt = chains_static;
> + active_cnt = chains_dynamic;
> + cmd.rxchain_info = htole32(iwx_fw_valid_rx_ant(sc) <<
> + IWX_PHY_RX_CHAIN_VALID_POS);
> + cmd.rxchain_info |= htole32(idle_cnt << IWX_PHY_RX_CHAIN_CNT_POS);
> + cmd.rxchain_info |= htole32(active_cnt <<
> + IWX_PHY_RX_CHAIN_MIMO_CNT_POS);
> + cmd.txchain_info = htole32(iwx_fw_valid_tx_ant(sc));
> +
> + return iwx_send_cmd_pdu(sc, IWX_PHY_CONTEXT_CMD, 0, sizeof(cmd),
> &cmd);
> }
>
> int
> blob - bf3209c2d3bb508f36dff179ab3e7cf5f45725dc
> blob + 210e2e89a61c004c04afe87cc829ecc2d37cfcd3
> --- sys/dev/pci/if_iwxreg.h
> +++ sys/dev/pci/if_iwxreg.h
> @@ -2794,7 +2794,15 @@ struct iwx_fw_channel_info {
> * @acquisition_data: ???
> * @dsp_cfg_flags: set to 0
> */
> -struct iwx_phy_context_cmd {
> +/*
> + * XXX Intel forgot to bump the PHY_CONTEXT command API when they
> increased
> + * the size of fw_channel_info from v1 to v2.
> + * To keep things simple we define two versions of this struct, and both
> + * are labled as CMD_API_VER_1. (The Linux iwlwifi driver performs dark
> + * magic with pointers to struct members instead.)
> + */
> +/* This version must be used if IWX_UCODE_TLV_CAPA_ULTRA_HB_CHANNELS is
> set: */
> +struct iwx_phy_context_cmd_uhb {
> /* COMMON_INDEX_HDR_API_S_VER_1 */
> uint32_t id_and_color;
> uint32_t action;
> @@ -2807,6 +2815,21 @@ struct iwx_phy_context_cmd {
> uint32_t acquisition_data;
> uint32_t dsp_cfg_flags;
> } __packed; /* IWX_PHY_CONTEXT_CMD_API_VER_1 */
> +/* This version must be used otherwise: */
> +struct iwx_phy_context_cmd {
> + /* COMMON_INDEX_HDR_API_S_VER_1 */
> + uint32_t id_and_color;
> + uint32_t action;
> + /* IWX_PHY_CONTEXT_DATA_API_S_VER_1 */
> + uint32_t apply_time;
> + uint32_t tx_param_color;
> + struct iwx_fw_channel_info_v1 ci;
> + uint32_t txchain_info;
> + uint32_t rxchain_info;
> + uint32_t acquisition_data;
> + uint32_t dsp_cfg_flags;
> +} __packed; /* IWX_PHY_CONTEXT_CMD_API_VER_1 */
> +
>
> #define IWX_RX_INFO_PHY_CNT 8
> #define IWX_RX_INFO_ENERGY_ANT_ABC_IDX 1
>
>
# patch -p0 -l < ./patch.diff
--------------------------
|-- sys/dev/pci/if_iwx.c
|+++ sys/dev/pci/if_iwx.c
--------------------------
Patching file sys/dev/pci/if_iwx.c using Plan A...
Hunk #1 succeeded at 343.
Hunk #2 succeeded at 3771.
Hunk #3 succeeded at 3810.
Hmm... The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|blob - bf3209c2d3bb508f36dff179ab3e7cf5f45725dc
|blob + 210e2e89a61c004c04afe87cc829ecc2d37cfcd3
|--- sys/dev/pci/if_iwxreg.h
|+++ sys/dev/pci/if_iwxreg.h
--------------------------
Patching file sys/dev/pci/if_iwxreg.h using Plan A...
Hunk #1 succeeded at 2794.
Hunk #2 succeeded at 2815.
done
It worked here
iwx0: hw rev 0x340, fw ver 46.393952838.0, address f8:e4:e3:23:3c:46
without the Debug IWX_DEBUG, will reboot a few time more to be sure
--
--
---------------------------------------------------------------------------------------------------------------------
Knowing is not enough; we must apply. Willing is not enough; we must do
