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

Reply via email to