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:
signature.asc
Description: This is a digitally signed message part.
