Re: [PATCH wireless-next v3] wifi: mac80211: Add eMLSR/eMLMR action frame parsing support

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

> I probably missed this the last time already:
> 
> > +	if ((control & IEEE80211_EML_CTRL_EMLSR_MODE) ||
> > +	    (control & IEEE80211_EML_CTRL_EMLMR_MODE)) {
> > +		eml_params.link_bitmap = get_unaligned_le16(ptr);
> > +		if (eml_params.link_bitmap &&
> 
> Surely 0 is an invalid bitmap? To disable it, you send a frame without
> IEEE80211_EML_CTRL_EMLSR_MODE or IEEE80211_EML_CTRL_EMLMR_MODE set, so I
> don't see how an empty bitmap would be valid?

ack, right. When we disable EMLSR IEEE80211_EML_CTRL_EMLSR_MODE so link_bitmap
is not present. I will fix it in v4.

> 
> > +		    !(eml_params.link_bitmap & sdata->vif.active_links))
> 
> That's the wrong check, I think? It only checks that you have any
> overlap, but we need all bits to be valid, i.e. only
> 
> 		(link_bitmap & active_links) == link_bitmap
> 
> is valid, no?

ack, I will fix it in v4.

> 
> > +			return;
> > +	}
> > +
> > +	if (drv_set_eml_op_mode(sdata, &sta->sta, &eml_params))
> > +		return;
> > +
> > +	ieee80211_send_eml_op_mode_notif(sdata, mgmt, opt_len);
> 
> IMHO it would make more sense to pass "ptr, opt_len" rather than "mgmt,
> opt_len", since that's just what gets copied and then the callee doesn't
> need to dig it out.

But I guess we need mgmt pointer to set even other fields (e.g. dst-addr,
dialog_token, ecc.).

Regards,
Lorenzo

> 
> johannes

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]