Re: [PATCH 5/7] mac80211: add wide bandwidth channel switch announcement to CSA action frames and mesh beacons

[Simon Wunderlich <sw@simonwunderlich.de>]

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

Hi Johannes,

On Friday, May 19, 2017 1:33:37 PM CEST Johannes Berg wrote:
> I've applied patches 1-4 now.
> 
> The subject is a bit long - I was going to change it to
> 
>     mac80211: mesh: support sending wide bandwidth CSA
>     
>     To support HT and VHT channel switch announcements, both beacons
>     and action frames must include the corresponding IEs.
> 
> but:
> > +		   2 + 2 + sizeof(struct
> > ieee80211_wide_bw_chansw_ie) +
> > +		   2 + sizeof(struct ieee80211_sec_chan_offs_ie) +
> 
> The "2 + 2" should have a comment - no that I'm even really sure that
> you need the wrapper?
> 

right, I'll add a comment. The spec says I need it, and if I understood the 
parsing function correctly it will only search for the wide bw IE when it finds 
the wrapper.

> > -		pos = skb_put(skb, 13);
> > -		memset(pos, 0, 13);
> 
> Removing that is nice - but why do you do this:
> > +		bool have_secondary_chan_offset = false;
> > +		bool have_wide_bandwidth_cs = false;
> > +		int ie_len = 2 + sizeof(struct
> > ieee80211_channel_sw_ie) +
> > +			     2 + sizeof(struct
> > ieee80211_mesh_chansw_params_ie);
> > +
> > +		switch (csa->settings.chandef.width) {
> > +		case NL80211_CHAN_WIDTH_80:
> > +		case NL80211_CHAN_WIDTH_80P80:
> > +		case NL80211_CHAN_WIDTH_160:
> > +			have_wide_bandwidth_cs = true;
> > +			ie_len += 2 + 2 +
> > +				  sizeof(struct
> > ieee80211_wide_bw_chansw_ie);
> > +			break;
> > +		case NL80211_CHAN_WIDTH_40:
> > +			have_secondary_chan_offset = true;
> > +			ie_len += 2 + sizeof(struct
> > ieee80211_sec_chan_offs_ie);
> > +		default:
> > +			break;
> > +		}
> > +		pos = skb_put(skb, ie_len);
> > +		memset(pos, 0, ie_len);
> 
> I think having multiple calls to skb_put() would be better.
> 

OK.

>[...]
> 
> and likewise here.
> 
> That'd also be safer in a way.

Understood, I can change it this way.

Thanks a lot for the feedback!
      Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]