> >
> > +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
>
> > +++ 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
>
> >
> > +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 :)
>
> > + 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.
ack, I will fix it
>
> > +
> > +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
>
> > +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
>
> > +{
> > + 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
>
Attachment:
signature.asc
Description: PGP signature
