From: Johannes Berg <johannes@sipsolutions.net>
To: Lorenzo Bianconi <lorenzo@kernel.org>, nbd@nbd.name
Cc: linux-wireless@vger.kernel.org, lorenzo.bianconi@redhat.com,
ryder.lee@mediatek.com, chui-hao.chiu@mediatek.com
Subject: Re: [PATCH v2 mac80211-next 2/7] mac80211: introduce individual TWT support in AP mode.
Date: Tue, 17 Aug 2021 13:34:00 +0200 [thread overview]
Message-ID: <ece8459373db3b76d38a583ec43f73e65d62a6c0.camel@sipsolutions.net> (raw)
In-Reply-To: <e3b70c37cb366399e944a0aa02f0d7cac25b4bb6.1628529781.git.lorenzo@kernel.org>
>
> +static inline void drv_add_twt_setup(struct ieee80211_local *local,
> + struct ieee80211_sub_if_data *sdata,
> + struct ieee80211_sta *sta,
> + struct ieee80211_twt_params *agrt_req,
> + struct ieee80211_twt_params *agrt_resp)
> +{
> + might_sleep();
> + check_sdata_in_driver(sdata);
> +
> + local->ops->add_twt_setup(&local->hw, sta, agrt_req, agrt_resp);
> +}
> +
> +static inline void drv_twt_teardown_request(struct ieee80211_local *local,
> + struct ieee80211_sub_if_data *sdata,
> + struct ieee80211_sta *sta,
> + u8 flowid)
> +{
> + might_sleep();
> + check_sdata_in_driver(sdata);
> +
> + if (local->ops->twt_teardown_request)
> + local->ops->twt_teardown_request(&local->hw, sta, flowid);
> +}
These should have tracing.
> +++ b/net/mac80211/iface.c
> @@ -1381,6 +1381,19 @@ static void ieee80211_iface_process_skb(struct ieee80211_local *local,
> WARN_ON(1);
> break;
> }
> + } else if (ieee80211_is_action(mgmt->frame_control) &&
> + mgmt->u.action.category == WLAN_CATEGORY_S1G) {
> + switch (mgmt->u.action.u.s1g.action_code) {
> + case WLAN_S1G_TWT_TEARDOWN:
> + case WLAN_S1G_TWT_SETUP:
> + if (skb->pkt_type == IEEE80211_TX_STATUS_MSG)
> + ieee80211_s1g_status_h_twt(sdata, skb);
> + else
> + ieee80211_s1g_rx_h_twt(sdata, skb);
I *really* don't like what you're doing here with the sdata->skb_queue,
which we only ever use for RX today.
We have today ieee80211_mgd_conn_tx_status() that gets called at the
right place, so you should add stuff there, and perhaps it needs to
queue things, or mark a separate TWT data structure before queueing the
work, or something else - but please don't use the RX skb_queue.
>
> +static ieee80211_rx_result debug_noinline
> +ieee80211_rx_h_twt(struct ieee80211_rx_data *rx)
Please rename this - it's not actually an rx_h_ that's called through
the normal RX handler stuff, it's just a sub-function for the action RX
handler.
It also doesn't need the rx_result, it can just be bool/int.
> + case WLAN_CATEGORY_S1G:
> + switch (mgmt->u.action.u.s1g.action_code) {
> + case WLAN_S1G_TWT_SETUP:
> + case WLAN_S1G_TWT_TEARDOWN:
> + if (ieee80211_rx_h_twt(rx) != RX_CONTINUE)
> + goto queue;
(see here)
> + default:
Also missing a "fallthrough" annotation or such.
> +
> +static int
> +ieee80211_s1g_rx_h_twt_teardown(struct ieee80211_sub_if_data *sdata,
> + struct sta_info *sta, struct sk_buff *skb)
> +{
> + struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)skb->data;
> +
> + drv_twt_teardown_request(sdata->local, sdata, &sta->sta,
> + mgmt->u.action.u.s1g.variable[0]);
> +
> + return 0;
Evidently, this cannot fail, so needs no return value.
> +void ieee80211_s1g_rx_h_twt(struct ieee80211_sub_if_data *sdata,
> + struct sk_buff *skb)
again, not a real RX handler
> +{
> + struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)skb->data;
> + struct ieee80211_local *local = sdata->local;
> + struct sta_info *sta;
> +
> + mutex_lock(&local->sta_mtx);
> +
> + sta = sta_info_get_bss(sdata, mgmt->sa);
> + if (!sta)
> + goto out;
> +
> + switch (mgmt->u.action.u.s1g.action_code) {
> + case WLAN_S1G_TWT_SETUP:
> + ieee80211_s1g_rx_h_twt_setup(sdata, sta, skb);
You're completely ignoring the return value. That's probably fine in the
-ENOMEM case, but the other cases you should still send a response. Just
like the driver callback is void because it should just fill in the
response to the other side (even in the failure cases).
johannes
next prev parent reply other threads:[~2021-08-17 11:34 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-09 17:28 [PATCH v2 mac80211-next 0/7] introduce individual TWT support in AP mode Lorenzo Bianconi
2021-08-09 17:28 ` [PATCH v2 mac80211-next 1/7] mac80211: add twt ie in ieee80211_mgmt structure Lorenzo Bianconi
2021-08-09 17:28 ` [PATCH v2 mac80211-next 2/7] mac80211: introduce individual TWT support in AP mode Lorenzo Bianconi
2021-08-17 11:34 ` Johannes Berg [this message]
2021-08-17 12:09 ` Lorenzo Bianconi
2021-08-09 17:28 ` [PATCH v2 mac80211-next 3/7] mt76: mt7915: introduce __mt7915_get_tsf routine Lorenzo Bianconi
2021-08-09 17:28 ` [PATCH v2 mac80211-next 4/7] mt76: mt7915: introduce mt7915_mcu_twt_agrt_update mcu command Lorenzo Bianconi
2021-08-09 17:28 ` [PATCH v2 mac80211-next 5/7] mt76: mt7915: introduce mt7915_mac_add_twt_setup routine Lorenzo Bianconi
2021-08-09 17:28 ` [PATCH v2 mac80211-next 6/7] mt76: mt7915: enable twt responder capability Lorenzo Bianconi
2021-08-09 17:28 ` [PATCH v2 mac80211-next 7/7] mt76: mt7915: add twt_stats knob in debugfs Lorenzo Bianconi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ece8459373db3b76d38a583ec43f73e65d62a6c0.camel@sipsolutions.net \
--to=johannes@sipsolutions.net \
--cc=chui-hao.chiu@mediatek.com \
--cc=linux-wireless@vger.kernel.org \
--cc=lorenzo.bianconi@redhat.com \
--cc=lorenzo@kernel.org \
--cc=nbd@nbd.name \
--cc=ryder.lee@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).