| Message ID | 20200204151135.25302-3-john@phrozen.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [RESEND,V2,1/4] ath11k: drop tx_info from ath11k_sta | expand |
John Crispin <john@phrozen.org> writes: > This allows us to pass HE rates down into the stack. > > Signed-off-by: John Crispin <john@phrozen.org> > --- > drivers/net/wireless/ath/ath11k/dp_tx.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c b/drivers/net/wireless/ath/ath11k/dp_tx.c > index 7b532bf9acd8..66a6cfd54ad9 100644 > --- a/drivers/net/wireless/ath/ath11k/dp_tx.c > +++ b/drivers/net/wireless/ath/ath11k/dp_tx.c > @@ -357,9 +357,12 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar, > struct sk_buff *msdu, > struct hal_tx_status *ts) > { > + struct ieee80211_tx_status status = { 0 }; This adds a sparse warning: drivers/net/wireless/ath/ath11k/dp_tx.c:350:47: warning: Using plain integer as NULL pointer Seems like a false warning, no? But not sure how to shut up the warning, using '{ NULL }' would do that but just feels wrong. Any opinions? There was also a trivial checkpatch warning: drivers/net/wireless/ath/ath11k/dp_tx.c:419: Please don't use multiple blank lines
On Tuesday, 11 February 2020 14:10:04 CET Kalle Valo wrote: [...] > > diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c b/drivers/net/wireless/ath/ath11k/dp_tx.c > > index 7b532bf9acd8..66a6cfd54ad9 100644 > > --- a/drivers/net/wireless/ath/ath11k/dp_tx.c > > +++ b/drivers/net/wireless/ath/ath11k/dp_tx.c > > @@ -357,9 +357,12 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar, > > struct sk_buff *msdu, > > struct hal_tx_status *ts) > > { > > + struct ieee80211_tx_status status = { 0 }; > > This adds a sparse warning: > > drivers/net/wireless/ath/ath11k/dp_tx.c:350:47: warning: Using plain integer as NULL pointer > > Seems like a false warning, no? But not sure how to shut up the warning, > using '{ NULL }' would do that but just feels wrong. Any opinions? Why is this a false warning? The structure is following: struct ieee80211_tx_status { struct ieee80211_sta *sta; struct ieee80211_tx_info *info; struct sk_buff *skb; struct rate_info *rate; }; And this is a pre-C99 initializer. The equal C99-Initializer would be struct ieee80211_tx_status status = { .sta = NULL, }; So it is initializing status.sta with 0. But status.sta is a pointer and we should use NULL for pointers instead of plain 0. If you want to initialize the object on stack to zero but not initialize each member then just use {}. Kind regards, Sven
On 11/02/2020 15:38, Sven Eckelmann wrote: > On Tuesday, 11 February 2020 14:10:04 CET Kalle Valo wrote: > [...] >>> diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c b/drivers/net/wireless/ath/ath11k/dp_tx.c >>> index 7b532bf9acd8..66a6cfd54ad9 100644 >>> --- a/drivers/net/wireless/ath/ath11k/dp_tx.c >>> +++ b/drivers/net/wireless/ath/ath11k/dp_tx.c >>> @@ -357,9 +357,12 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar, >>> struct sk_buff *msdu, >>> struct hal_tx_status *ts) >>> { >>> + struct ieee80211_tx_status status = { 0 }; >> >> This adds a sparse warning: >> >> drivers/net/wireless/ath/ath11k/dp_tx.c:350:47: warning: Using plain integer as NULL pointer >> >> Seems like a false warning, no? But not sure how to shut up the warning, >> using '{ NULL }' would do that but just feels wrong. Any opinions? > > Why is this a false warning? The structure is following: > > struct ieee80211_tx_status { > struct ieee80211_sta *sta; > struct ieee80211_tx_info *info; > struct sk_buff *skb; > struct rate_info *rate; > }; > > And this is a pre-C99 initializer. The equal C99-Initializer would be > > struct ieee80211_tx_status status = { > .sta = NULL, > }; > > So it is initializing status.sta with 0. But status.sta is a pointer > and we should use NULL for pointers instead of plain 0. If you want > to initialize the object on stack to zero but not initialize each > member then just use {}. > > Kind regards, > Sven > yup, i just need to drop the 0 and use {}. I'll respin/send John
Sven Eckelmann <sven@narfation.org> writes: > On Tuesday, 11 February 2020 14:10:04 CET Kalle Valo wrote: > [...] >> > diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c b/drivers/net/wireless/ath/ath11k/dp_tx.c >> > index 7b532bf9acd8..66a6cfd54ad9 100644 >> > --- a/drivers/net/wireless/ath/ath11k/dp_tx.c >> > +++ b/drivers/net/wireless/ath/ath11k/dp_tx.c >> > @@ -357,9 +357,12 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar, >> > struct sk_buff *msdu, >> > struct hal_tx_status *ts) >> > { >> > + struct ieee80211_tx_status status = { 0 }; >> >> This adds a sparse warning: >> >> drivers/net/wireless/ath/ath11k/dp_tx.c:350:47: warning: Using plain >> integer as NULL pointer >> >> Seems like a false warning, no? But not sure how to shut up the warning, >> using '{ NULL }' would do that but just feels wrong. Any opinions? > > Why is this a false warning? The structure is following: > > struct ieee80211_tx_status { > struct ieee80211_sta *sta; > struct ieee80211_tx_info *info; > struct sk_buff *skb; > struct rate_info *rate; > }; > > And this is a pre-C99 initializer. The equal C99-Initializer would be > > struct ieee80211_tx_status status = { > .sta = NULL, > }; > > So it is initializing status.sta with 0. But status.sta is a pointer > and we should use NULL for pointers instead of plain 0. If you want > to initialize the object on stack to zero but not initialize each > member then just use {}. Ah, of course. Thanks for pointing this out.
diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c b/drivers/net/wireless/ath/ath11k/dp_tx.c index 7b532bf9acd8..66a6cfd54ad9 100644 --- a/drivers/net/wireless/ath/ath11k/dp_tx.c +++ b/drivers/net/wireless/ath/ath11k/dp_tx.c @@ -357,9 +357,12 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar, struct sk_buff *msdu, struct hal_tx_status *ts) { + struct ieee80211_tx_status status = { 0 }; struct ath11k_base *ab = ar->ab; struct ieee80211_tx_info *info; struct ath11k_skb_cb *skb_cb; + struct ath11k_peer *peer; + struct ath11k_sta *arsta; if (WARN_ON_ONCE(ts->buf_rel_source != HAL_WBM_REL_SRC_MODULE_TQM)) { /* Must not happen */ @@ -423,12 +426,23 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar, ath11k_dp_tx_cache_peer_stats(ar, msdu, ts); } - /* NOTE: Tx rate status reporting. Tx completion status does not have - * necessary information (for example nss) to build the tx rate. - * Might end up reporting it out-of-band from HTT stats. - */ - ieee80211_tx_status(ar->hw, msdu); + spin_lock_bh(&ab->base_lock); + peer = ath11k_peer_find_by_id(ab, ts->peer_id); + if (peer) { + arsta = (struct ath11k_sta *)peer->sta->drv_priv; + status.sta = peer->sta; + status.skb = msdu; + status.info = info; + status.rate = &arsta->last_txrate; + } + rcu_read_unlock(); + if (peer) + ieee80211_tx_status_ext(ar->hw, &status); + else + dev_kfree_skb_any(msdu); + spin_unlock_bh(&ab->base_lock); + return; exit: rcu_read_unlock();
This allows us to pass HE rates down into the stack. Signed-off-by: John Crispin <john@phrozen.org> --- drivers/net/wireless/ath/ath11k/dp_tx.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)