diff mbox series

[1/2] wifi: iwlwifi: Report link-id for transmitted frames.

Message ID 20241007173851.2207849-1-greearb@candelatech.com (mailing list archive)
State Changes Requested
Delegated to: Miri Korenblit
Headers show
Series [1/2] wifi: iwlwifi: Report link-id for transmitted frames. | expand

Commit Message

Ben Greear Oct. 7, 2024, 5:38 p.m. UTC 
From: Ben Greear <greearb@candelatech.com>

This will let upper stack properly record stats per link.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Johannes Berg Oct. 9, 2024, 9:04 a.m. UTC | #1 
Hi,

So ... I suppose I kind of get it, your product doesn't really need
things upstream and so you don't care all _that_ much, but I'd still
appreciate if you could take a bit more care.

There's always a large amount of friction with patches you send, which
at this point makes me not even want to really look at them, and then I
wonder if I should just fix it up or send it back ... Yes I can and do
fix trivial things, but it really isn't something I feel I should need
to be doing all the time.

And yes, I also understand you want to just throw ideas over the wall,
but really in a v2 patchset I think we're beyond that. I'd also
appreciate not seeing obviously wrong patches (e.g. with a ton of marker
comments or prints left in them) since that just detracts from the
purpose, but that's not relevant to this patchset here.

Anyway ...

Here, the subject should've had "v2", and in the commit message, below a
--- marker, a description of what you changed.

>  		memset(&info->status, 0, sizeof(info->status));
>  		info->flags &= ~(IEEE80211_TX_STAT_ACK | IEEE80211_TX_STAT_TX_FILTERED);
> +		if (link_sta_id != -1)
> +			info->control.flags = u32_replace_bits(info->control.flags, link_sta_id, IEEE80211_TX_CTRL_MLO_LINK);

That line is clearly too long for no reason at all, and same below.

johannes
Ben Greear Oct. 9, 2024, 5:01 p.m. UTC | #2 
On 10/9/24 02:04, Johannes Berg wrote:
> Hi,
> 
> So ... I suppose I kind of get it, your product doesn't really need
> things upstream and so you don't care all _that_ much, but I'd still
> appreciate if you could take a bit more care.
> 
> There's always a large amount of friction with patches you send, which
> at this point makes me not even want to really look at them, and then I
> wonder if I should just fix it up or send it back ... Yes I can and do
> fix trivial things, but it really isn't something I feel I should need
> to be doing all the time.
> 
> And yes, I also understand you want to just throw ideas over the wall,
> but really in a v2 patchset I think we're beyond that. I'd also
> appreciate not seeing obviously wrong patches (e.g. with a ton of marker
> comments or prints left in them) since that just detracts from the
> purpose, but that's not relevant to this patchset here.
> 
> Anyway ...

I appreciate getting patches upstream, and sorry for sending a broken patch
and missing the fact that I had posted a v1 instead of just the RFC.

I believe the v3 cleans up the problems.

Thanks,
Ben

> 
> Here, the subject should've had "v2", and in the commit message, below a
> --- marker, a description of what you changed.
> 
>>   		memset(&info->status, 0, sizeof(info->status));
>>   		info->flags &= ~(IEEE80211_TX_STAT_ACK | IEEE80211_TX_STAT_TX_FILTERED);
>> +		if (link_sta_id != -1)
>> +			info->control.flags = u32_replace_bits(info->control.flags, link_sta_id, IEEE80211_TX_CTRL_MLO_LINK);
> 
> That line is clearly too long for no reason at all, and same below.
> 
> johannes
>
diff mbox series

Patch

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index ca026b5256ce..f22f8a269988 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -1709,6 +1709,14 @@  static void iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm,
 	u8 lq_color;
 	u16 next_reclaimed, seq_ctl;
 	bool is_ndp = false;
+	struct ieee80211_link_sta *link_sta;
+	int link_sta_id = -1;
+
+	rcu_read_lock();
+	link_sta = rcu_dereference(mvm->fw_id_to_link_sta[sta_id]);
+	if (link_sta)
+		link_sta_id = link_sta->link_id;
+	rcu_read_unlock();
 
 	__skb_queue_head_init(&skbs);
 
@@ -1732,6 +1740,8 @@  static void iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm,
 
 		memset(&info->status, 0, sizeof(info->status));
 		info->flags &= ~(IEEE80211_TX_STAT_ACK | IEEE80211_TX_STAT_TX_FILTERED);
+		if (link_sta_id != -1)
+			info->control.flags = u32_replace_bits(info->control.flags, link_sta_id, IEEE80211_TX_CTRL_MLO_LINK);
 
 		/* inform mac80211 about what happened with the frame */
 		switch (status & TX_STATUS_MSK) {
@@ -2048,6 +2058,7 @@  static void iwl_mvm_tx_reclaim(struct iwl_mvm *mvm, int sta_id, int tid,
 	struct iwl_mvm_sta *mvmsta = NULL;
 	struct sk_buff *skb;
 	int freed;
+	struct ieee80211_link_sta *link_sta;
 
 	if (WARN_ONCE(sta_id >= mvm->fw->ucode_capa.num_stations ||
 		      tid > IWL_MAX_TID_COUNT,
@@ -2064,6 +2075,8 @@  static void iwl_mvm_tx_reclaim(struct iwl_mvm *mvm, int sta_id, int tid,
 		return;
 	}
 
+	link_sta = rcu_dereference(mvm->fw_id_to_link_sta[sta_id]);
+
 	__skb_queue_head_init(&reclaimed_skbs);
 
 	/*
@@ -2087,6 +2100,9 @@  static void iwl_mvm_tx_reclaim(struct iwl_mvm *mvm, int sta_id, int tid,
 			info->flags |= IEEE80211_TX_STAT_ACK;
 		else
 			info->flags &= ~IEEE80211_TX_STAT_ACK;
+
+		if (link_sta)
+			info->control.flags |= u32_replace_bits(info->control.flags, link_sta->link_id, IEEE80211_TX_CTRL_MLO_LINK);
 	}
 
 	/*