| Message ID | 20231107152611.61952-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: iwlwifi: add missing milliseconds to TUs conversion | expand |
On Tue, 2023-11-07 at 18:26 +0300, Dmitry Antipov wrote: > Since 'max_delay' and 'duration' of 'struct iwl_time_event_cmd' > are in TUs rather than milliseconds, add missing 'MSEC_TO_TU()' > in 'iwl_mvm_protect_session()' and 'iwl_mvm_schedule_csa_period()'. > Compile tested only. Well, the function is actually documenting both TUs and ms depending where you look ;-) Not sure I'd want to actually change the behaviour here, but maybe clean up the docs a bit? It's also only 2.4% off, so ... johannes
On 11/7/23 18:41, Johannes Berg wrote: > Well, the function is actually documenting both TUs and ms depending > where you look ;-) I've relied on the comments around 'struct iwl_time_event_cmd'. > It's also only 2.4% off, so ... The corner case when MSEC_TO_TU(1) yields to 0 may be more interesting. Dmitry
On Tue, 2023-11-07 at 18:53 +0300, Dmitry Antipov wrote: > On 11/7/23 18:41, Johannes Berg wrote: > > > Well, the function is actually documenting both TUs and ms depending > > where you look ;-) > > I've relied on the comments around 'struct iwl_time_event_cmd'. Yes but the function docs also say: * @min_duration: will start a new session if the current session will end * in less than min_duration. * @max_delay: maximum delay before starting the time event (in TU) so it expects input in TU already. Then it goes on to say: * This function can be used to start a session protection which means that the * fw will stay on the channel for %duration_ms milliseconds. This function so it's not consistent, but I'm not surprised, my son's teachers always praise him for tracking units and I think it's obvious you have to ;-) Why should the code be different :P (Then again, "duration_ms" isn't even an argument) The value from mac80211 that's passed in comes from * struct ieee80211_prep_tx_info - prepare TX information * @duration: if non-zero, hint about the required duration, * only used with the mgd_prepare_tx() method. which doesn't even say the unit ... but in the one place setting it uses jiffies_to_msecs() to fill it, so it's also not necessarily very accurate (depending on CONFIG_HZ.) Maybe we should rename IWL_MVM_TE_SESSION_PROTECTION_MAX_TIME_MS and IWL_MVM_TE_SESSION_PROTECTION_MIN_TIME_MS to _TU to make it more aligned? Dunno. > > It's also only 2.4% off, so ... > > The corner case when MSEC_TO_TU(1) yields to 0 may be more interesting. The input should be in the order of (a) hundred(s) TU/ms, so that won't really ever happen. The reason why I don't really want to convert it is that the beacon intervals are typically 100 TU, and so if we use 400 ms which converts to 390 TU we _just_ don't cover 4 beacon intervals which is a bit stupid since the precise timing doesn't matter. Covering 4 gives us a better chance here, and anyway the firmware will also have some delays etc. johannes
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c b/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c index 218fdf1ed530..084fdc5fa186 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c @@ -656,10 +656,10 @@ void iwl_mvm_protect_session(struct iwl_mvm *mvm, time_cmd.apply_time = cpu_to_le32(0); time_cmd.max_frags = TE_V2_FRAG_NONE; - time_cmd.max_delay = cpu_to_le32(max_delay); + time_cmd.max_delay = cpu_to_le32(MSEC_TO_TU(max_delay)); /* TODO: why do we need to interval = bi if it is not periodic? */ time_cmd.interval = cpu_to_le32(1); - time_cmd.duration = cpu_to_le32(duration); + time_cmd.duration = cpu_to_le32(MSEC_TO_TU(duration)); time_cmd.repeat = 1; time_cmd.policy = cpu_to_le16(TE_V2_NOTIF_HOST_EVENT_START | TE_V2_NOTIF_HOST_EVENT_END | @@ -1249,7 +1249,7 @@ int iwl_mvm_schedule_csa_period(struct iwl_mvm *mvm, time_cmd.id = cpu_to_le32(TE_CHANNEL_SWITCH_PERIOD); time_cmd.apply_time = cpu_to_le32(apply_time); time_cmd.max_frags = TE_V2_FRAG_NONE; - time_cmd.duration = cpu_to_le32(duration); + time_cmd.duration = cpu_to_le32(MSEC_TO_TU(duration)); time_cmd.repeat = 1; time_cmd.interval = cpu_to_le32(1); time_cmd.policy = cpu_to_le16(TE_V2_NOTIF_HOST_EVENT_START |
Since 'max_delay' and 'duration' of 'struct iwl_time_event_cmd' are in TUs rather than milliseconds, add missing 'MSEC_TO_TU()' in 'iwl_mvm_protect_session()' and 'iwl_mvm_schedule_csa_period()'. Compile tested only. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- drivers/net/wireless/intel/iwlwifi/mvm/time-event.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)