diff mbox series

wifi: iwlwifi: add missing milliseconds to TUs conversion

Message ID 20231107152611.61952-1-dmantipov@yandex.ru
State New
Headers show
Series wifi: iwlwifi: add missing milliseconds to TUs conversion | expand

Commit Message

Dmitry Antipov Nov. 7, 2023, 3:26 p.m. UTC
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(-)

Comments

Johannes Berg Nov. 7, 2023, 3:41 p.m. UTC | #1
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
Dmitry Antipov Nov. 7, 2023, 3:53 p.m. UTC | #2
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
Johannes Berg Nov. 7, 2023, 4:02 p.m. UTC | #3
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 mbox series

Patch

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 |