Hi,
Sorry it took me so long to get here - I'm buried in (other) MLO work.
On Thu, 2022-04-07 at 20:51 +0800, Paul Zhang wrote:
> In order to reconstruct frame for MBSSID feature, per the description of
> the Reduced Neighbor Report(RNR) element about MLD parameters subfield in
> section 9.4.2.170 of Draft P802.11be_D1.4, the RNR IE is modified:
You're a bit inconsistent here - "RNR element" vs. "RNR IE". I'd
generally prefer now to switch to the "element" naming in new code. And
then maybe the subject should be more like
parse MLD params from RNR element
or so.
Also D2.0 is available - please check if anything changed.
> +/*
> + * TBTT Information field, based on Draft P802.11be_D1.4
> + * section 9.4.2.170.2
> + */
> +#define IEEE80211_TBTT_INFO_BSSID_SSID_BSS_PARAM_PSD 13
> +#define IEEE80211_TBTT_INFO_BSSID_SSID_BSS_PARAM_PSD_MLD_PARAM 16
> +#define IEEE80211_TBTT_TYPE_MASK 0xC0
> +#define IEEE80211_TBTT_COUNT_MASK 0x0F
> +/* TBTT infomation header(2) + Operating class(1) + Channel number(1) */
typo: "information"
> +/**
> + * cfg80211_handle_rnr_ie_for_mbssid() - parse and modify RNR IE for MBSSID
> + * feature
> + * @elem: The pointer to RNR IE
> + * @bssid_index: BSSID index from MBSSID index IE
> + * @pos: The buffer pointer to save the transformed RNR IE, caller is expected
> + * to supply a buffer that is at least as big as @elem
I'd prefer IE -> element also here, I think.
> + * Per the description about Neighbor AP Information field about MLD
> + * parameters subfield in section 9.4.2.170.2 of Draft P802.11be_D1.4.
> + * If the reported AP is affiliated with the same MLD of the reporting AP,
> + * the TBTT information is skipped; If the reported AP is affiliated with
> + * the same MLD of the nontransmitted BSSID, the TBTT information is copied
> + * and the MLD ID is changed to 0.
> + *
> + * Return: Length of the element written to @pos
> + */
> +static size_t cfg80211_handle_rnr_ie_for_mbssid(const struct element *elem,
> + u8 bssid_index, u8 *pos)
> +{
> + size_t rnr_len;
> + const u8 *rnr, *data, *rnr_end;
> + u8 *rnr_new, *tbtt_info_field;
> + u8 tbtt_type, tbtt_len, tbtt_count;
> + u8 mld_pos, mld_id;
> + u32 i, copy_len;
> + /* The count of TBTT info field whose MLD ID equals to 0 in a neighbor
> + * AP information field.
> + */
> + u32 tbtt_info_field_count;
> + /* The total bytes of TBTT info fields whose MLD ID equals to 0 in
> + * current RNR IE.
> + */
> + u32 tbtt_info_field_len = 0;
> +
> + rnr_new = pos;
> + rnr = (u8 *)elem;
That's a bit weird, why are you doing manipulations on u8 pointers now?
Shouldn't the elem struct be more useful?
> + rnr_len = elem->datalen;
> + rnr_end = rnr + rnr_len + 2;
> +
> + memcpy(pos, rnr, 2);
> + pos += 2;
That really could be open-coded. And if you have "rnr_new = pos" maybe
use that?
> + data = elem->data;
> + while (data < rnr_end) {
> + tbtt_type = u8_get_bits(data[0], IEEE80211_TBTT_TYPE_MASK);
> + tbtt_count = u8_get_bits(data[0], IEEE80211_TBTT_COUNT_MASK);
> + tbtt_len = data[1];
You're not checking that any data is present, i.e. that you actually
have a suitable length?
[snip]
it's kind of hard to follow this, to be honest ... maybe that's
intrinsic, but maybe we could do something like
#define copy(pos, data, len) do {
memcpy(pos, data, len);
pos += len;
data += len;
} while (0)
or something to simplify? And maybe that should also have a bounds check
... which I feel are missing quite a bit, not just the one I pointed out
above.
> @@ -321,8 +448,13 @@ static size_t cfg80211_gen_new_ie(const u8 *ie, size_t ielen,
> const struct element *old_elem = (void *)tmp_old;
>
> /* ie in old ie but not in subelement */
> - if (cfg80211_is_element_inherited(old_elem,
> - non_inherit_elem)) {
> + if (tmp_old[0] == WLAN_EID_REDUCED_NEIGHBOR_REPORT) {
That comment is now misplaced, it seems? It was probably kind of wrong
from the start though.
johannes
