commit 8f892e225f416fcf2b55a0f9161162e08e2b0cc7 upstream.
This just adds a __le32 that we (currently) don't use.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218963 Signed-off-by: Emmanuel Grumbach emmanuel.grumbach@intel.com Signed-off-by: Miri Korenblit miriam.rachel.korenblit@intel.com Link: https://msgid.link/20240319100755.29ff7a88ddac.I39cf2ff1d1ddf0fa62722538698d... Signed-off-by: Johannes Berg johannes.berg@intel.com --- .../net/wireless/intel/iwlwifi/fw/api/power.h | 30 +++++++++++++++++++ drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 4 ++- .../net/wireless/intel/iwlwifi/mvm/mac80211.c | 4 ++- 3 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/power.h b/drivers/net/wireless/intel/iwlwifi/fw/api/power.h index 0bf38243f88a..ce18ef9d3128 100644 --- a/drivers/net/wireless/intel/iwlwifi/fw/api/power.h +++ b/drivers/net/wireless/intel/iwlwifi/fw/api/power.h @@ -385,6 +385,33 @@ struct iwl_dev_tx_power_cmd_v7 { __le32 timer_period; __le32 flags; } __packed; /* TX_REDUCED_POWER_API_S_VER_7 */ + +/** + * struct iwl_dev_tx_power_cmd_v8 - TX power reduction command version 8 + * @per_chain: per chain restrictions + * @enable_ack_reduction: enable or disable close range ack TX power + * reduction. + * @per_chain_restriction_changed: is per_chain_restriction has changed + * from last command. used if set_mode is + * IWL_TX_POWER_MODE_SET_SAR_TIMER. + * note: if not changed, the command is used for keep alive only. + * @reserved: reserved (padding) + * @timer_period: timer in milliseconds. if expires FW will change to default + * BIOS values. relevant if setMode is IWL_TX_POWER_MODE_SET_SAR_TIMER + * @flags: reduce power flags. + * @tpc_vlp_backoff_level: user backoff of UNII5,7 VLP channels in USA. + * Not in use. + */ +struct iwl_dev_tx_power_cmd_v8 { + __le16 per_chain[IWL_NUM_CHAIN_TABLES_V2][IWL_NUM_CHAIN_LIMITS][IWL_NUM_SUB_BANDS_V2]; + u8 enable_ack_reduction; + u8 per_chain_restriction_changed; + u8 reserved[2]; + __le32 timer_period; + __le32 flags; + __le32 tpc_vlp_backoff_level; +} __packed; /* TX_REDUCED_POWER_API_S_VER_8 */ + /** * struct iwl_dev_tx_power_cmd - TX power reduction command (multiversion) * @common: common part of the command @@ -392,6 +419,8 @@ struct iwl_dev_tx_power_cmd_v7 { * @v4: version 4 part of the command * @v5: version 5 part of the command * @v6: version 6 part of the command + * @v7: version 7 part of the command + * @v8: version 8 part of the command */ struct iwl_dev_tx_power_cmd { struct iwl_dev_tx_power_common common; @@ -401,6 +430,7 @@ struct iwl_dev_tx_power_cmd { struct iwl_dev_tx_power_cmd_v5 v5; struct iwl_dev_tx_power_cmd_v6 v6; struct iwl_dev_tx_power_cmd_v7 v7; + struct iwl_dev_tx_power_cmd_v8 v8; }; };
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c index e1c2b7fc92ab..df3b29b998cf 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c @@ -896,11 +896,13 @@ int iwl_mvm_sar_select_profile(struct iwl_mvm *mvm, int prof_a, int prof_b) u32 n_subbands; u8 cmd_ver = iwl_fw_lookup_cmd_ver(mvm->fw, cmd_id, IWL_FW_CMD_VER_UNKNOWN); - if (cmd_ver == 7) { + if (cmd_ver >= 7) { len = sizeof(cmd.v7); n_subbands = IWL_NUM_SUB_BANDS_V2; per_chain = cmd.v7.per_chain[0][0]; cmd.v7.flags = cpu_to_le32(mvm->fwrt.reduced_power_flags); + if (cmd_ver == 8) + len = sizeof(cmd.v8); } else if (cmd_ver == 6) { len = sizeof(cmd.v6); n_subbands = IWL_NUM_SUB_BANDS_V2; diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c index 7ed7444c9871..2403ac2fcdc3 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c @@ -1399,7 +1399,9 @@ int iwl_mvm_set_tx_power(struct iwl_mvm *mvm, struct ieee80211_vif *vif, if (tx_power == IWL_DEFAULT_MAX_TX_POWER) cmd.common.pwr_restriction = cpu_to_le16(IWL_DEV_MAX_TX_POWER);
- if (cmd_ver == 7) + if (cmd_ver == 8) + len = sizeof(cmd.v8); + else if (cmd_ver == 7) len = sizeof(cmd.v7); else if (cmd_ver == 6) len = sizeof(cmd.v6);
commit 788e4c75f831d06fcfbbec1d455fac429521e607 upstream.
Since IWL_FW_CMD_VER_UNKNOWN = 99, then my change to consider cmd_ver >= 7 instead of cmd_ver = 7 included also firmwares that don't advertise the command version at all. This made us send a command with a bad size and because of that, the firmware hit a BAD_COMMAND immediately after handling the REDUCE_TX_POWER_CMD command.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218963 Fixes: 8f892e225f41 ("wifi: iwlwifi: mvm: support iwl_dev_tx_power_cmd_v8") Signed-off-by: Emmanuel Grumbach emmanuel.grumbach@intel.com Reviewed-by: Johannes Berg johannes.berg@intel.com Signed-off-by: Miri Korenblit miriam.rachel.korenblit@intel.com Link: https://msgid.link/20240512072733.eb20ff5050d3.Ie4fc6f5496cd296fd6ff20d15e98... Signed-off-by: Johannes Berg johannes.berg@intel.com --- drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c index df3b29b998cf..f33e595dcc03 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c @@ -894,8 +894,8 @@ int iwl_mvm_sar_select_profile(struct iwl_mvm *mvm, int prof_a, int prof_b) int ret; u16 len = 0; u32 n_subbands; - u8 cmd_ver = iwl_fw_lookup_cmd_ver(mvm->fw, cmd_id, - IWL_FW_CMD_VER_UNKNOWN); + u8 cmd_ver = iwl_fw_lookup_cmd_ver(mvm->fw, cmd_id, 3); + if (cmd_ver >= 7) { len = sizeof(cmd.v7); n_subbands = IWL_NUM_SUB_BANDS_V2;
On Tue, Jun 18, 2024 at 02:09:23PM +0300, Emmanuel Grumbach wrote:
commit 8f892e225f416fcf2b55a0f9161162e08e2b0cc7 upstream.
This just adds a __le32 that we (currently) don't use.
Why is this needed for a stable tree if this is nothing that is actually used and then we need another fix for it after that?
I can't see how this commit actually does anything on it's own, what am I missing?
What bug is this fixing? A regression? Is this a new feature?
confused,
greg k-h
On Wed, 2024-06-19 at 10:51 +0200, Greg KH wrote:
On Tue, Jun 18, 2024 at 02:09:23PM +0300, Emmanuel Grumbach wrote:
commit 8f892e225f416fcf2b55a0f9161162e08e2b0cc7 upstream.
This just adds a __le32 that we (currently) don't use.
Why is this needed for a stable tree if this is nothing that is actually used and then we need another fix for it after that?
Right, so I totally understand you're confused... I should probably have re-written the commit message to explain why this is needed for stable...
This patch allows to handle a new version of a specific command to the firmware. As explained in the commit message, we don't need the new field, but ... the command got bigger and we must align to the new size of course. If we don't, the firmware will get a command that is shorter than expected and will crash. We originally didn't think we'd need that on the firmware versions supported by kernel 6.9 and this is why we didn't queue this patch for 6.9. Now, it appears that the latest firmware version that 6.9 supports does need the new version of the command. Unfortunately, we learnt that the hard way, through bugzilla :-(
Now, this patch introduced a regression that is fixed by another patch... Would you prefer me to squash them?
I can't see how this commit actually does anything on it's own, what am I missing?
What bug is this fixing? A regression? Is this a new feature?
So, yes, it fixes a bug as explained above. This is a regression because older kernels won't load the new firmware and won't hit the firmware crash.
confused,
I should have re-written the commit message. Sorry. I hope things are now clearer..
greg k-h
On Wed, Jun 19, 2024 at 09:03:34AM +0000, Grumbach, Emmanuel wrote:
On Wed, 2024-06-19 at 10:51 +0200, Greg KH wrote:
On Tue, Jun 18, 2024 at 02:09:23PM +0300, Emmanuel Grumbach wrote:
commit 8f892e225f416fcf2b55a0f9161162e08e2b0cc7 upstream.
This just adds a __le32 that we (currently) don't use.
Why is this needed for a stable tree if this is nothing that is actually used and then we need another fix for it after that?
Right, so I totally understand you're confused... I should probably have re-written the commit message to explain why this is needed for stable...
This patch allows to handle a new version of a specific command to the firmware. As explained in the commit message, we don't need the new field, but ... the command got bigger and we must align to the new size of course. If we don't, the firmware will get a command that is shorter than expected and will crash. We originally didn't think we'd need that on the firmware versions supported by kernel 6.9 and this is why we didn't queue this patch for 6.9. Now, it appears that the latest firmware version that 6.9 supports does need the new version of the command. Unfortunately, we learnt that the hard way, through bugzilla :-(
Now, this patch introduced a regression that is fixed by another patch... Would you prefer me to squash them?
I can't see how this commit actually does anything on it's own, what am I missing?
What bug is this fixing? A regression? Is this a new feature?
So, yes, it fixes a bug as explained above. This is a regression because older kernels won't load the new firmware and won't hit the firmware crash.
confused,
I should have re-written the commit message. Sorry. I hope things are now clearer..
Keeping the commit message the same is fine, and not squashing is also fine, but a huge hint as to _why_ this is relevent for the stable trees would have been appreciated. That's what [0/X] email blurbs are for :)
thanks, I'll go queue these up now.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Emmanuel Grumbach -
Greg KH -
gregkh＠linuxfoundation.org
-
Grumbach, Emmanuel