Re: [PATCH v2 mac80211-next 2/7] mac80211: introduce individual TWT support in AP mode.

quoted

+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.

ack, I will add tracing in v3

quoted

+++ 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.

ack, I will review/work on ieee80211_mgd_conn_tx_status() in v3

quoted

+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.

ack, naming is hard :)

quoted

+	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)

quoted

+		default:

Also missing a "fallthrough" annotation or such.

ack, I will fix it

quoted

+
+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.

ack, I will fix it

quoted

+void ieee80211_s1g_rx_h_twt(struct ieee80211_sub_if_data *sdata,
+			    struct sk_buff *skb)

again, not a real RX handler

ack, I will fix it

quoted

+{
+	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).

ack, I will fix it in v3

Regards,
Lorenzo

johannes