On Tue, 2025-07-22 at 17:16 +1000, Lachlan Hodges wrote:
> When parsing a TIM element from an S1G AP, ensure we correctly
> parse based on the S1G PPDU TIM PVB format, allowing an S1G STA
> to correctly enter/exit power save states to receive buffered
> unicast traffic.
I think this isn't quite correct.
> Also make sure we don't allocate over the
> mac80211 supported S1G PPDU AID count.
But this specifically caught my eye - how can the client ensure it
doesn't allocate over, since it doesn't even allocate? First I thought
this really belongs into the AP side code (i.e. the other patch), but
you actually did something here:
> @@ -6374,10 +6375,12 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
> reassoc = ieee80211_is_reassoc_resp(mgmt->frame_control);
> capab_info = le16_to_cpu(mgmt->u.assoc_resp.capab_info);
> status_code = le16_to_cpu(mgmt->u.assoc_resp.status_code);
> - if (assoc_data->s1g)
> + if (assoc_data->s1g) {
> elem_start = mgmt->u.s1g_assoc_resp.variable;
> - else
> + max_aid = IEEE80211_MAX_AID_S1G_NO_PS;
> + } else {
> elem_start = mgmt->u.assoc_resp.variable;
> + }
>
> /*
> * Note: this may not be perfect, AP might misbehave - if
> @@ -6401,8 +6404,6 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
>
> if (elems->aid_resp)
> aid = le16_to_cpu(elems->aid_resp->aid);
> - else if (assoc_data->s1g)
> - aid = 0; /* TODO */
> else
> aid = le16_to_cpu(mgmt->u.assoc_resp.aid);
>
> @@ -6447,7 +6448,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
> event.u.mlme.reason = status_code;
> drv_event_callback(sdata->local, sdata, &event);
> } else {
> - if (aid == 0 || aid > IEEE80211_MAX_AID) {
> + if (aid == 0 || aid > max_aid) {
> sdata_info(sdata,
> "invalid AID value %d (out of range), turn off PS\n",
> aid);
Which ... seems questionable? At least in terms of message it's not
actually _invalid_ if it's out of this range, in theory S1G allows up to
8191.
And you also forgot to change the masking - S1G says 3 bits are
reserved, where we actually check the top two bits are 0b11 I think?
Also the parsing for S1G only parses a subset of the valid formats,
notably the format that mac80211 can generate. That seems questionable,
unless you expect there never to be another implementation (or the
mac80211 one to never be extended) and you can basically make up your
own standard?
And even if you _do_ get an AID that's <1600 there's still no guarantee
that the encoding will be in the bitmap format, if only two stations
have traffic the AP might well decide to use the "Single AID" mode?
So I'm overall a bit confused how all this is meant to work - even if
only partially - because AID<1600 is no guarantee that you can parse the
bitmap, so turning off powersave only for AID>=1600 will not really be
sufficient?
Which also means that
> +#define IEEE80211_MAX_AID_S1G_NO_PS 1600
really doesn't belong into ieee80211.h, if it should exist at all then
as part of mac80211 (but above I'm arguing it shouldn't.)
johannes
