On Wed, 2014-01-08 at 16:26 +0100, Michal Kazior wrote: > This extends NL80211_CMD_CHANNEL_SWITCH by adding > a new attribute NL80211_ATTR_SWITCH_NEXT. The > attribute holds nested channel switch attributes > (including itself) allowing > multi-interface/chained CSA. That's ... creative? Interesting? I'm not sure I like that API much. OTOH, it's a neat way of reusing all the attributes that we have, etc. However, I think it'd be easier to implement on both sides of the fence if we structured this differently and made the "more vifs" to be a nested attribute with data inside each one of them. Actually, I'd suggest that in the multi-vif case all of the attributes should be inside the list, like this: wiphy, chswitch=[1=[ifidx,...], 2=[ifidx,...], 3=[ifidx,...]] alternatively, maybe something that would be nice would be wiphy, chswitch=[ifidx1=[...], ifidx2=[...], ifidx3=[...]] but the common way would be to ignore the array index completely. Today you get wiphy, ifidx, ..., next=[ifidx, ..., next=[ifidx, ..., next=[...]]] and I don't really like the deep nesting. I would also argue that doing wiphy, ifidx1, ..., more=[1=[ifidx2, ...], ...] or wiphy, ifidx1, ..., next=[...] is more error prone as it would allow older kernels to parse the whole thing while ignoring the next/more/whatever, so you'd get some weird subset of the intended behaviour. Forcing *all* interfaces into the sub-attribute when more than one is desired (or in fact for a single one, if you're ok with requiring a kernel with support) would IMHO be less error prone. Haven't looked over the code in much detail, but a few small comments: * I think you should preserve tracing, it has facilities for arrays too * hard-coding a limit of 8 vifs seems pointless, you can easily allocate dynamic memory, no? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
