diff mbox series

[v2,06/13] wifi: mac80211: rework RX timestamp flags

Message ID 20231220133549.d0e664832d14.I20c8900106f9bf81316bed778b1e3ce145785274@changeid (mailing list archive)
State Accepted
Delegated to: Johannes Berg
Headers show
Series [v2,01/13] wifi: cfg80211: reg: Support P2P operation on DFS channels | expand

Commit Message

Miri Korenblit Dec. 20, 2023, 11:41 a.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

We only have a single flag free, and before using that for
another mactime flag, instead refactor the mactime flags
to use a 2-bit field.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Reviewed-by: Gregory Greenman <gregory.greenman@intel.com>
Reviewed-by: Benjamin Berg <benjamin.berg@intel.com>
Signed-off-by: Miri Korenblit <miriam.rachel.korenblit@intel.com>
---
v2: Fix wrong email addresses and fix ath code
---
 drivers/net/wireless/ath/ath10k/htt_rx.c |  2 +-
 include/net/mac80211.h                   | 13 +++++++++----
 net/mac80211/ieee80211_i.h               |  5 +----
 net/mac80211/util.c                      | 16 ++++++++++------
 4 files changed, 21 insertions(+), 15 deletions(-)

Comments

Jeff Johnson Dec. 19, 2023, 10:41 p.m. UTC | #1 
On 12/20/2023 3:41 AM, Miri Korenblit wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> We only have a single flag free, and before using that for
> another mactime flag, instead refactor the mactime flags
> to use a 2-bit field.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Reviewed-by: Gregory Greenman <gregory.greenman@intel.com>
> Reviewed-by: Benjamin Berg <benjamin.berg@intel.com>
> Signed-off-by: Miri Korenblit <miriam.rachel.korenblit@intel.com>
> ---
> v2: Fix wrong email addresses and fix ath code
> ---
>  drivers/net/wireless/ath/ath10k/htt_rx.c |  2 +-
>  include/net/mac80211.h                   | 13 +++++++++----
>  net/mac80211/ieee80211_i.h               |  5 +----
>  net/mac80211/util.c                      | 16 ++++++++++------
>  4 files changed, 21 insertions(+), 15 deletions(-)
..snip..
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 29312f6638a1..938f4d255668 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -1775,10 +1775,7 @@ static inline bool txq_has_queue(struct ieee80211_txq *txq)
>  static inline bool
>  ieee80211_have_rx_timestamp(struct ieee80211_rx_status *status)
>  {
> -	WARN_ON_ONCE(status->flag & RX_FLAG_MACTIME_START &&
> -		     status->flag & RX_FLAG_MACTIME_END);
> -	return !!(status->flag & (RX_FLAG_MACTIME_START | RX_FLAG_MACTIME_END |
> -				  RX_FLAG_MACTIME_PLCP_START));
> +	return status->flag & RX_FLAG_MACTIME;

curious why you dropped the !!
now the code can return a value that doesn't map to the true/false bool
enums
Johannes Berg Dec. 19, 2023, 10:43 p.m. UTC | #2 
On Tue, 2023-12-19 at 14:41 -0800, Jeff Johnson wrote:
> 
> >  static inline bool
> >  ieee80211_have_rx_timestamp(struct ieee80211_rx_status *status)
> >  {
> > -	WARN_ON_ONCE(status->flag & RX_FLAG_MACTIME_START &&
> > -		     status->flag & RX_FLAG_MACTIME_END);
> > -	return !!(status->flag & (RX_FLAG_MACTIME_START | RX_FLAG_MACTIME_END |
> > -				  RX_FLAG_MACTIME_PLCP_START));
> > +	return status->flag & RX_FLAG_MACTIME;
> 
> curious why you dropped the !!

Just a general cleanup I guess.

> now the code can return a value that doesn't map to the true/false bool
> enums

No, it cannot, at least not if 'bool' is implemented in a C99-compliant
way :) It's not actually an enum, it will return 0/1 in the machine
register even with this code.

johannes
Jeff Johnson Dec. 19, 2023, 11:16 p.m. UTC | #3 
On 12/19/2023 2:43 PM, Johannes Berg wrote:
> On Tue, 2023-12-19 at 14:41 -0800, Jeff Johnson wrote:
>> now the code can return a value that doesn't map to the true/false bool
>> enums
> 
> No, it cannot, at least not if 'bool' is implemented in a C99-compliant
> way :) It's not actually an enum, it will return 0/1 in the machine
> register even with this code.

Today I learned something new. Guess I'm still carrying baggage from
pre-C99.

/jeff
Johannes Berg Dec. 21, 2023, 7:13 p.m. UTC | #4 
On Tue, 2023-12-19 at 15:16 -0800, Jeff Johnson wrote:
> On 12/19/2023 2:43 PM, Johannes Berg wrote:
> > On Tue, 2023-12-19 at 14:41 -0800, Jeff Johnson wrote:
> > > now the code can return a value that doesn't map to the true/false bool
> > > enums
> > 
> > No, it cannot, at least not if 'bool' is implemented in a C99-compliant
> > way :) It's not actually an enum, it will return 0/1 in the machine
> > register even with this code.
> 
> Today I learned something new. Guess I'm still carrying baggage from
> pre-C99.

Which is not necessarily bad - there are to this day compilers that
implement bool as if it was an enum, and then it can hold values other
than 0/1 ... but at least for kernel work we can assume bool is
implemented as specified :)

johannes
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index fa0f598ed6bf..7d28ae5453cf 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1295,7 +1295,7 @@  static void ath10k_htt_rx_h_ppdu(struct ath10k *ar,
 		status->encoding = RX_ENC_LEGACY;
 		status->bw = RATE_INFO_BW_20;
 
-		status->flag &= ~RX_FLAG_MACTIME_END;
+		status->flag &= ~RX_FLAG_MACTIME;
 		status->flag |= RX_FLAG_NO_SIGNAL_VAL;
 
 		status->flag &= ~(RX_FLAG_AMPDU_IS_LAST);
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 77a71b1396b1..00274d1cdeeb 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1362,6 +1362,9 @@  ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
  *	the frame.
  * @RX_FLAG_FAILED_PLCP_CRC: Set this flag if the PCLP check failed on
  *	the frame.
+ * @RX_FLAG_MACTIME: The timestamp passed in the RX status (@mactime
+ *	field) is valid if this field is non-zero, and the position
+ *	where the timestamp was sampled depends on the value.
  * @RX_FLAG_MACTIME_START: The timestamp passed in the RX status (@mactime
  *	field) is valid and contains the time the first symbol of the MPDU
  *	was received. This is useful in monitor mode and for proper IBSS
@@ -1441,12 +1444,12 @@  ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
 enum mac80211_rx_flags {
 	RX_FLAG_MMIC_ERROR		= BIT(0),
 	RX_FLAG_DECRYPTED		= BIT(1),
-	RX_FLAG_MACTIME_PLCP_START	= BIT(2),
+	RX_FLAG_ONLY_MONITOR		= BIT(2),
 	RX_FLAG_MMIC_STRIPPED		= BIT(3),
 	RX_FLAG_IV_STRIPPED		= BIT(4),
 	RX_FLAG_FAILED_FCS_CRC		= BIT(5),
 	RX_FLAG_FAILED_PLCP_CRC 	= BIT(6),
-	RX_FLAG_MACTIME_START		= BIT(7),
+	/* one free bit at 7 */
 	RX_FLAG_NO_SIGNAL_VAL		= BIT(8),
 	RX_FLAG_AMPDU_DETAILS		= BIT(9),
 	RX_FLAG_PN_VALIDATED		= BIT(10),
@@ -1455,8 +1458,10 @@  enum mac80211_rx_flags {
 	RX_FLAG_AMPDU_IS_LAST		= BIT(13),
 	RX_FLAG_AMPDU_DELIM_CRC_ERROR	= BIT(14),
 	RX_FLAG_AMPDU_DELIM_CRC_KNOWN	= BIT(15),
-	RX_FLAG_MACTIME_END		= BIT(16),
-	RX_FLAG_ONLY_MONITOR		= BIT(17),
+	RX_FLAG_MACTIME			= BIT(16) | BIT(17),
+	RX_FLAG_MACTIME_PLCP_START	= 1 << 16,
+	RX_FLAG_MACTIME_START		= 2 << 16,
+	RX_FLAG_MACTIME_END		= 3 << 16,
 	RX_FLAG_SKIP_MONITOR		= BIT(18),
 	RX_FLAG_AMSDU_MORE		= BIT(19),
 	RX_FLAG_RADIOTAP_TLV_AT_END	= BIT(20),
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 29312f6638a1..938f4d255668 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1775,10 +1775,7 @@  static inline bool txq_has_queue(struct ieee80211_txq *txq)
 static inline bool
 ieee80211_have_rx_timestamp(struct ieee80211_rx_status *status)
 {
-	WARN_ON_ONCE(status->flag & RX_FLAG_MACTIME_START &&
-		     status->flag & RX_FLAG_MACTIME_END);
-	return !!(status->flag & (RX_FLAG_MACTIME_START | RX_FLAG_MACTIME_END |
-				  RX_FLAG_MACTIME_PLCP_START));
+	return status->flag & RX_FLAG_MACTIME;
 }
 
 void ieee80211_vif_inc_num_mcast(struct ieee80211_sub_if_data *sdata);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index ed680120d5a7..643c54855be6 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -4176,6 +4176,7 @@  u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
 				     unsigned int mpdu_offset)
 {
 	u64 ts = status->mactime;
+	bool mactime_plcp_start;
 	struct rate_info ri;
 	u16 rate;
 	u8 n_ltf;
@@ -4183,6 +4184,9 @@  u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
 	if (WARN_ON(!ieee80211_have_rx_timestamp(status)))
 		return 0;
 
+	mactime_plcp_start = (status->flag & RX_FLAG_MACTIME) ==
+				RX_FLAG_MACTIME_PLCP_START;
+
 	memset(&ri, 0, sizeof(ri));
 
 	ri.bw = status->bw;
@@ -4197,7 +4201,7 @@  u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
 		if (status->enc_flags & RX_ENC_FLAG_SHORT_GI)
 			ri.flags |= RATE_INFO_FLAGS_SHORT_GI;
 		/* TODO/FIXME: is this right? handle other PPDUs */
-		if (status->flag & RX_FLAG_MACTIME_PLCP_START) {
+		if (mactime_plcp_start) {
 			mpdu_offset += 2;
 			ts += 36;
 		}
@@ -4214,7 +4218,7 @@  u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
 		 * See P802.11ax_D6.0, section 27.3.4 for
 		 * VHT PPDU format.
 		 */
-		if (status->flag & RX_FLAG_MACTIME_PLCP_START) {
+		if (mactime_plcp_start) {
 			mpdu_offset += 2;
 			ts += 36;
 
@@ -4238,7 +4242,7 @@  u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
 		 * See P802.11REVmd_D3.0, section 19.3.2 for
 		 * HT PPDU format.
 		 */
-		if (status->flag & RX_FLAG_MACTIME_PLCP_START) {
+		if (mactime_plcp_start) {
 			mpdu_offset += 2;
 			if (status->enc_flags & RX_ENC_FLAG_HT_GF)
 				ts += 24;
@@ -4266,7 +4270,7 @@  u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
 		 * See P802.11REVmd_D3.0, section 21.3.2 for
 		 * VHT PPDU format.
 		 */
-		if (status->flag & RX_FLAG_MACTIME_PLCP_START) {
+		if (mactime_plcp_start) {
 			mpdu_offset += 2;
 			ts += 36;
 
@@ -4288,7 +4292,7 @@  u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
 		sband = local->hw.wiphy->bands[status->band];
 		ri.legacy = sband->bitrates[status->rate_idx].bitrate;
 
-		if (status->flag & RX_FLAG_MACTIME_PLCP_START) {
+		if (mactime_plcp_start) {
 			if (status->band == NL80211_BAND_5GHZ) {
 				ts += 20;
 				mpdu_offset += 2;
@@ -4310,7 +4314,7 @@  u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
 		return 0;
 
 	/* rewind from end of MPDU */
-	if (status->flag & RX_FLAG_MACTIME_END)
+	if ((status->flag & RX_FLAG_MACTIME) == RX_FLAG_MACTIME_END)
 		ts -= mpdu_len * 8 * 10 / rate;
 
 	ts += mpdu_offset * 8 * 10 / rate;