| Message ID | 1445361858-24976-1-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
On 10/20/2015 10:24 AM, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > If a user restricts the rateset for some reason, then the > probe requests should not advertise rates that are not > selected by the user. > > To implement this, we save the requested bitrates at > the mac80211 level and take it into account when building > the IEs. > > This allows one to create a more realistic /b mode > station on modern hardware, for instance. Good for > testing, and likely good for other things as well. Any comments on these patches? Thanks, Ben
On Thu, 2015-11-05 at 10:47 -0800, Ben Greear wrote: > > Any comments on these patches? > As I said to Janusz earlier, I haven't been looking much due to kernel summit and now the merge window. I'll resume next week or so once the trees have stabilized a bit. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-10-20 at 10:24 -0700, greearb@candelatech.com wrote: > --- a/include/uapi/linux/nl80211.h > +++ b/include/uapi/linux/nl80211.h > @@ -2130,6 +2130,8 @@ enum nl80211_attrs { > > NL80211_ATTR_REG_INDOOR, > > + NL80211_ATTR_TX_ADVERT_RATEMASK, First of all, this is missing documentation. It's a FLAG as far as I can tell, but if it's set should it affect only the advertised or both? I also think you added this because I had commented that we might not want to do it unconditionally, but is there really a good reason not to do it unconditionally? I was probably more thinking out loud what would happen with P2P, but there we already say "no cck" for scanning so it shouldn't really have any effect. But given that we have NL80211_ATTR_SCAN_SUPP_RATES, where does your patch even have an effect, other than this not handling HT/VHT? I'm a bit wary that we're introducing two ways of achieving a very similar thing. Also, adding these overrides all over the code seems very complex. Perhaps we can achieve the goal of being able to pretend to have limited hardware capabilities by actually restricting hardware capabilities instead? That would also avoid having to play whack-a-mole with code paths using the device capabilities. I'd imagine an API along the lines of doing some kind of higher-layer re-register of the wiphy in mac80211 with limited capabilites. At that point you'd allocate a copy of the original capabilities (as far as the new restricted ones overwrite the data), and remove the device from the system as far as higher layers like cfg80211 are concerned. Would something like that work for you? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/26/2016 03:16 AM, Johannes Berg wrote: > On Tue, 2015-10-20 at 10:24 -0700, greearb@candelatech.com wrote: > >> --- a/include/uapi/linux/nl80211.h >> +++ b/include/uapi/linux/nl80211.h >> @@ -2130,6 +2130,8 @@ enum nl80211_attrs { >> >> NL80211_ATTR_REG_INDOOR, >> >> + NL80211_ATTR_TX_ADVERT_RATEMASK, > > First of all, this is missing documentation. It's a FLAG as far as I > can tell, but if it's set should it affect only the advertised or both? > > I also think you added this because I had commented that we might not > want to do it unconditionally, but is there really a good reason not to > do it unconditionally? I was probably more thinking out loud what would > happen with P2P, but there we already say "no cck" for scanning so it > shouldn't really have any effect. Turns out, I did want this flag. The reason is that I might want to advertise as a normal-ish /g station (ie, full /b/g rateset), but actually fix the TX rate as 24Mbps (or whatever). So, I do need a way to set an advertise ratemask vs a tx-rate-mask. With my current patch, and my patches to supplicant, it seems to at least mostly do what I want, but there is a user-space issue where if you set the ADVERT_RATEMASK on kernels that do not support that flag, it will just set the tx rates instead. Not the end of the world, but possibly there needs to be a feature flag that user-space could query for this feature. > But given that we have NL80211_ATTR_SCAN_SUPP_RATES, where does your > patch even have an effect, other than this not handling HT/VHT? > > I'm a bit wary that we're introducing two ways of achieving a very > similar thing. > > Also, adding these overrides all over the code seems very complex. > Perhaps we can achieve the goal of being able to pretend to have > limited hardware capabilities by actually restricting hardware > capabilities instead? That would also avoid having to play whack-a-mole > with code paths using the device capabilities. > > I'd imagine an API along the lines of doing some kind of higher-layer > re-register of the wiphy in mac80211 with limited capabilites. At that > point you'd allocate a copy of the original capabilities (as far as the > new restricted ones overwrite the data), and remove the device from the > system as far as higher layers like cfg80211 are concerned. > > Would something like that work for you? As far as I can tell, that will not work, because I want to have multiple station devices per radio, and have each of them be able to use a different configuration. So, one station may be /g, and another /n and another /AC. Same with APs. In addition, some stations may want to use all available rates for their mode, and others may want to use a fixed rate or subset of available rates. Thanks, Ben > > johannes >
On Tue, 2016-01-26 at 07:19 -0800, Ben Greear wrote: > As far as I can tell, that will not work, because I want to have > multiple station devices per radio, and have each of them be able to > use a different configuration. So, one station may be /g, and > another /n and another /AC. Same with APs. In addition, some > stations may want to use all available rates for their mode, and > others may want to use a fixed rate or subset of available rates. So let's agree that we're splitting the *used* rates (which we have today) and the *advertised* rates/modes/... Even if we can't modify the wiphy capabilities, perhaps we could still handle it more consistently at mac80211 level, e.g. by introducing and using "sdata->capa.bands" (and similar for all the other wiphy capabilities you want to manipulate), and then allowing that to point to something modified? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/04/2016 01:02 AM, Johannes Berg wrote: > On Tue, 2016-01-26 at 07:19 -0800, Ben Greear wrote: > >> As far as I can tell, that will not work, because I want to have >> multiple station devices per radio, and have each of them be able to >> use a different configuration. So, one station may be /g, and >> another /n and another /AC. Same with APs. In addition, some >> stations may want to use all available rates for their mode, and >> others may want to use a fixed rate or subset of available rates. > > So let's agree that we're splitting the *used* rates (which we have > today) and the *advertised* rates/modes/... Yes, I think that will work well, and unless I mis-understand, that is basically what I implemented so far. > Even if we can't modify the wiphy capabilities, perhaps we could still > handle it more consistently at mac80211 level, e.g. by introducing and > using "sdata->capa.bands" (and similar for all the other wiphy > capabilities you want to manipulate), and then allowing that to point > to something modified? Copied state might be tricky. I think if we hold any copies of capabilities data in the sdata, then it should be logically compared with a mask and then treated as an AND with whatever the wiphy has. I'm reluctant to propose any serious mac80211 change at this point, though perhaps as more of this type of features are added, then it will become more obvious how to nicely consolidate things in mac80211. To be honest, I thought my -v2 patches were fairly non-invasive compared to my normal hackings :) Thanks, Ben
On Thu, 2016-02-04 at 09:52 -0800, Ben Greear wrote: > On 02/04/2016 01:02 AM, Johannes Berg wrote: > > On Tue, 2016-01-26 at 07:19 -0800, Ben Greear wrote: > > > > > As far as I can tell, that will not work, because I want to have > > > multiple station devices per radio, and have each of them be able > > > to > > > use a different configuration. So, one station may be /g, and > > > another /n and another /AC. Same with APs. In addition, some > > > stations may want to use all available rates for their mode, and > > > others may want to use a fixed rate or subset of available rates. > > > > So let's agree that we're splitting the *used* rates (which we have > > today) and the *advertised* rates/modes/... > > Yes, I think that will work well, and unless I mis-understand, that > is basically what I implemented so far. Yes. > Copied state might be tricky. I think if we hold any copies of > capabilities data in the sdata, then it should be logically compared > with a mask and then treated as an AND with whatever the wiphy has. Not sure what you're saying here. I was thinking we'd simply do one of these two: 1) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht & user-config (semantically, not implementation of course) 2) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht and change mac80211 to use sdata->sband instead of wiphy->sband wherever the latter is used today. That way, we can avoid touching all these things. I think, for example, that you missed TDLS in your changes. Changing everything throughout would mean that grepping for "wiphy->sband" would immediately show such bugs, making it far easier to maintain. > I'm reluctant to propose any serious mac80211 change at this point, > though perhaps as more of this type of features are added, then it > will become more obvious how to nicely consolidate things in > mac80211. I don't really think this would be a "serious" change? It's basically pointering changes, you could (and perhaps should, to catch it all) use an spatch to make the initial change. > To be honest, I thought my -v2 patches were fairly non-invasive > compared to my normal hackings :) > :) johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/18/2016 12:32 PM, Johannes Berg wrote: > On Thu, 2016-02-04 at 09:52 -0800, Ben Greear wrote: >> On 02/04/2016 01:02 AM, Johannes Berg wrote: >>> On Tue, 2016-01-26 at 07:19 -0800, Ben Greear wrote: >>> >>>> As far as I can tell, that will not work, because I want to have >>>> multiple station devices per radio, and have each of them be able >>>> to >>>> use a different configuration. So, one station may be /g, and >>>> another /n and another /AC. Same with APs. In addition, some >>>> stations may want to use all available rates for their mode, and >>>> others may want to use a fixed rate or subset of available rates. >>> >>> So let's agree that we're splitting the *used* rates (which we have >>> today) and the *advertised* rates/modes/... >> >> Yes, I think that will work well, and unless I mis-understand, that >> is basically what I implemented so far. > > Yes. > >> Copied state might be tricky. I think if we hold any copies of >> capabilities data in the sdata, then it should be logically compared >> with a mask and then treated as an AND with whatever the wiphy has. > > Not sure what you're saying here. I was thinking we'd simply do one of > these two: > > 1) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht & user-config > (semantically, not implementation of course) > 2) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht > > and change mac80211 to use sdata->sband instead of wiphy->sband > wherever the latter is used today. That way, we can avoid touching all > these things. > > I think, for example, that you missed TDLS in your changes. Changing > everything throughout would mean that grepping for "wiphy->sband" would > immediately show such bugs, making it far easier to maintain. I think option 1 might be better. For option 2, I'd be worried about someone changing the wiphy somehow and vdevs getting out of sync. Any opinions on the lifetime for the sdata->sband_user_config data? For instance, could be cleared when station dis-associates, or we could make it persist until changed or until vdev goes away? And, while this would take care of some of the issues (3x3 vs 2x2?), it still wouldn't let someone have full control over the advertised rates vs configured-rates and such, would it? So, your proposal is in addition to allowing advertised-rates to be configured? Thanks, Ben >> I'm reluctant to propose any serious mac80211 change at this point, >> though perhaps as more of this type of features are added, then it >> will become more obvious how to nicely consolidate things in >> mac80211. > > I don't really think this would be a "serious" change? It's basically > pointering changes, you could (and perhaps should, to catch it all) use > an spatch to make the initial change. > >> To be honest, I thought my -v2 patches were fairly non-invasive >> compared to my normal hackings :) >> > > :) > > johannes >
On Thu, 2016-02-18 at 12:40 -0800, Ben Greear wrote: > > 1) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht & user-config > > (semantically, not implementation of course) > > 2) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht > I think option 1 might be better. For option 2, I'd be worried about > someone changing the wiphy somehow and vdevs getting out of sync. Huh? I'm confused. Those two options above weren't meant as options, they were meant as two sides of an "if (have-user-config)" branch :) > Any opinions on the lifetime for the sdata->sband_user_config data? > > For instance, could be cleared when station dis-associates, > or we could make it persist until changed or until vdev goes away? Should probably persist. > And, while this would take care of some of the issues (3x3 vs 2x2?), > it still wouldn't let someone have full control over the advertised > rates vs configured-rates and such, would it? Why not? The sband also contains the rates list. I was just using VHT as an example. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/18/2016 12:45 PM, Johannes Berg wrote: > On Thu, 2016-02-18 at 12:40 -0800, Ben Greear wrote: > >>> 1) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht & user-config >>> (semantically, not implementation of course) >>> 2) sdata->sband[5GHZ].vht = wiphy->sband[5GHZ].vht > >> I think option 1 might be better. For option 2, I'd be worried about >> someone changing the wiphy somehow and vdevs getting out of sync. > > Huh? I'm confused. Those two options above weren't meant as options, > they were meant as two sides of an "if (have-user-config)" branch :) Oh. In that case, what if this happens: wiphy created and init vdev created, vht copied from wiphy .... wiphy vht info changes? Can this ever happen? I'm thinking maybe when a user uses 'iw' to set the rx/rx chainmask on a phy? Possibly nasty debugfs hacks that have worked so far? I wouldn't want to try to re-propagate that to already-created vdevs. So, I think it should be more like: if (vdata->have-user-config) { return sdata->user_config_vht & sdata->wiphy->vht; } else { return sdata->wiphy->vht; } Point of the above 'code' is that we do not want to ever have duplicate state (ie, a copy of wiphy->vht in sdata->vht, because then they can get out of sync. But, we can have a set of over-ride data in sdata, and since we logically AND that with wiphy data, then we should never exceed the capabilities of either sdata or wiphy. That make sense? > >> Any opinions on the lifetime for the sdata->sband_user_config data? >> >> For instance, could be cleared when station dis-associates, >> or we could make it persist until changed or until vdev goes away? > > Should probably persist. That is fine with me, would make supplicant patches easier than my method, probably. > >> And, while this would take care of some of the issues (3x3 vs 2x2?), >> it still wouldn't let someone have full control over the advertised >> rates vs configured-rates and such, would it? > > Why not? The sband also contains the rates list. I was just using VHT > as an example. Ok, the details of mac80211 have leaked out of my skull since the start of this thread...I'll see if I can make a stab at implementing this proposal. But, likely it will be a bit, as I'm swamped with other things at the moment. If someone else is interested in trying this, then of course be my guest! Thanks, Ben
On Thu, 2016-02-18 at 12:59 -0800, Ben Greear wrote: > Oh. In that case, what if this happens: > > wiphy created and init > vdev created, vht copied from wiphy > .... > wiphy vht info changes? Can this ever happen? I'm thinking maybe > when > a user uses 'iw' to set the rx/rx chainmask on a phy? Possibly > nasty debugfs hacks that have worked so far? Ah, but I don't think this is something that's supposed to happen. Then again, there were some people talking about this last week, with a use case that seemed valid (shared antennas and reconfiguration or so), but we can require that to cause some action to propagate it. My gut reaction was to just re-register the entire wiphy, but that may be too big of a hammer. In any case, I think this isn't something we really need to consider too much. If there are bugs in this area then we'll fix them one way or another (or perhaps we'll just never find them since I don't expect many people to use this functionality). > So, I think it should be more like: > > if (vdata->have-user-config) { > return sdata->user_config_vht & sdata->wiphy->vht; > } > else { > return sdata->wiphy->vht; > } > > Point of the above 'code' is that we do not want to ever have > duplicate state (ie, a copy of wiphy->vht in sdata->vht, because then > they can get out of sync. But, we can have a set of over-ride data > in sdata, and since we logically AND that with wiphy data, then we > should never exceed the capabilities of either sdata or wiphy. > > That make sense? Yeah, it does, at least if you assume that the capabilities can actually change. We assume they don't though, hostapd/wpa_s for example will never re-query them after startup. I also don't like the whole "call a function to get the capabilities" not much more than what you have now in the code, since then we'll have to deal with allocations (or stack memory) for the data we want and it just generally makes the code using the capabilities much more complex (for a pretty fringe use case.) I'd much rather have the data structure that we can use as today without additional hoops to jump through, just possibly "redirecting" it for the use cases you have in mind. > Ok, the details of mac80211 have leaked out of my skull since the > start of this thread...I'll see if I can make a stab at implementing > this proposal. But, likely it will be a bit, as I'm swamped with > other things at the moment. If someone else is interested in trying > this, then of course be my guest! > Looked like Cedric Debarge had a similar idea, I've pointed him to this thread (as you probably saw.) johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/18/2016 01:14 PM, Johannes Berg wrote: > On Thu, 2016-02-18 at 12:59 -0800, Ben Greear wrote: > >> Oh. In that case, what if this happens: >> >> wiphy created and init >> vdev created, vht copied from wiphy >> .... >> wiphy vht info changes? Can this ever happen? I'm thinking maybe >> when >> a user uses 'iw' to set the rx/rx chainmask on a phy? Possibly >> nasty debugfs hacks that have worked so far? > > Ah, but I don't think this is something that's supposed to happen. > > Then again, there were some people talking about this last week, with a > use case that seemed valid (shared antennas and reconfiguration or so), > but we can require that to cause some action to propagate it. My gut > reaction was to just re-register the entire wiphy, but that may be too > big of a hammer. > > In any case, I think this isn't something we really need to consider > too much. If there are bugs in this area then we'll fix them one way or > another (or perhaps we'll just never find them since I don't expect > many people to use this functionality). > >> So, I think it should be more like: >> >> if (vdata->have-user-config) { >> return sdata->user_config_vht & sdata->wiphy->vht; >> } >> else { >> return sdata->wiphy->vht; >> } >> >> Point of the above 'code' is that we do not want to ever have >> duplicate state (ie, a copy of wiphy->vht in sdata->vht, because then >> they can get out of sync. But, we can have a set of over-ride data >> in sdata, and since we logically AND that with wiphy data, then we >> should never exceed the capabilities of either sdata or wiphy. >> >> That make sense? > > Yeah, it does, at least if you assume that the capabilities can > actually change. We assume they don't though, hostapd/wpa_s for example > will never re-query them after startup. Restarting supplicant, even if it came to that, might be less of an issue than having to re-create the vdev to have it grab proper new settings from the wiphy... > I also don't like the whole "call a function to get the capabilities" > not much more than what you have now in the code, since then we'll have > to deal with allocations (or stack memory) for the data we want and it > just generally makes the code using the capabilities much more complex > (for a pretty fringe use case.) > > I'd much rather have the data structure that we can use as today > without additional hoops to jump through, just possibly "redirecting" > it for the use cases you have in mind. Well, it would be easy enough to make a method that updated an vdev/sdata->vht from wiphy, so could just call that as needed (ie, when certain things set or are changed by wiphy). >> Ok, the details of mac80211 have leaked out of my skull since the >> start of this thread...I'll see if I can make a stab at implementing >> this proposal. But, likely it will be a bit, as I'm swamped with >> other things at the moment. If someone else is interested in trying >> this, then of course be my guest! >> > > Looked like Cedric Debarge had a similar idea, I've pointed him to this > thread (as you probably saw.) Yeah, I tried to read the patch when first posted, but I am not sure I understand what he is trying to do. Thanks, Ben
On Thu, 2016-02-18 at 13:54 -0800, Ben Greear wrote: > > Yeah, it does, at least if you assume that the capabilities can > > actually change. We assume they don't though, hostapd/wpa_s for > > example will never re-query them after startup. > > Restarting supplicant, even if it came to that, might be less > of an issue than having to re-create the vdev to have it grab > proper new settings from the wiphy... > Well, it would be easy enough to make a method that updated an > vdev/sdata->vht from wiphy, so could just call that as needed (ie, > when certain things set or are changed by wiphy). Yeah, I think we'll just have to deal with this when it comes along. This multi-antenna-sharing case (unfortunately I forgot who was talking about it) is the only thing that comes to mind where we really may have to deal with this. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/23/2016 03:06 AM, Johannes Berg wrote: > On Thu, 2016-02-18 at 13:54 -0800, Ben Greear wrote: > >>> Yeah, it does, at least if you assume that the capabilities can >>> actually change. We assume they don't though, hostapd/wpa_s for >>> example will never re-query them after startup. >> >> Restarting supplicant, even if it came to that, might be less >> of an issue than having to re-create the vdev to have it grab >> proper new settings from the wiphy... > >> Well, it would be easy enough to make a method that updated an >> vdev/sdata->vht from wiphy, so could just call that as needed (ie, >> when certain things set or are changed by wiphy). > > Yeah, I think we'll just have to deal with this when it comes along. > This multi-antenna-sharing case (unfortunately I forgot who was talking > about it) is the only thing that comes to mind where we really may have > to deal with this. I started looking at making these changes...and I have a few questions. First, are you proposing that I make a copy of the entire local->hw.wiphy->bands array and store it locally in sdata? And then, I would provide some API to modify the bands[i]->bitrates and other variables to properly select the advertised features? I am a bit concerned about making copies (and then changing) the driver's bitrates array. Likely it will work with ath10k, but it seems fragile at best. Or, did I mis-understand your suggestion with regards to what data structures should be copied to sdata in mac80211 and then made modifiable? With regard to my original patch, are there any other things that I could do to make it more acceptable without making copies of the driver data? Thanks, Ben
On Thu, 2016-03-10 at 09:56 -0800, Ben Greear wrote: > First, are you proposing that I make a copy of the entire local- > >hw.wiphy->bands array and store it locally in sdata? No, I think it should be pointers there, say sdata->bands[X] pointing to the same thing as local->hw.wiphy->bands[X] is pointing to. So the first patch would introduce that, and replace each and every use of local->hw.wiphy->bands[] with sdata->bands[], if necessary annotating the remaining ones with why they're OK and should stay. > And then, I would provide some API to modify the bands[i]->bitrates > and other variables to properly select the advertised features? Kinda. You'd provide some kind of helper function or API to take the local->hw.wiphy->bands[X] and duplicate it into a newly allocated buffer, modifying it along the way, before assigning it to the per- sdata stuff in sdata->bands[X]. > I am a bit concerned about making copies (and then changing) the > driver's bitrates array. Likely it will work with ath10k, but it > seems fragile at best. That's an interesting point, we currently use the rate *index* a lot, so we have to find some way of preserving that. Perhaps we need to introduce a flag indicating a given rate is disabled? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2016-03-10 at 09:56 -0800, Ben Greear wrote: > > With regard to my original patch, are there any other things that I > could do to make it more acceptable without making copies of the > driver data? > Oops, saw this too late. I can't really think of anything that would really make me think it gets much better - by way of how it works we'd have to put code into every single place using this data, and I can't think of a way to abstract that out easily. Perhaps if you can find a way to have some kind of "accessor functions" that would do the overrides, but I don't really see how that'd be possible. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/15/2016 07:15 AM, Johannes Berg wrote: > On Thu, 2016-03-10 at 09:56 -0800, Ben Greear wrote: > >> First, are you proposing that I make a copy of the entire local- >>> hw.wiphy->bands array and store it locally in sdata? > > No, I think it should be pointers there, say > > sdata->bands[X] pointing to the same thing as local->hw.wiphy->bands[X] > is pointing to. > > So the first patch would introduce that, and replace each and every use > of local->hw.wiphy->bands[] with sdata->bands[], if necessary > annotating the remaining ones with why they're OK and should stay. > >> And then, I would provide some API to modify the bands[i]->bitrates >> and other variables to properly select the advertised features? > > Kinda. You'd provide some kind of helper function or API to take the > local->hw.wiphy->bands[X] and duplicate it into a newly allocated > buffer, modifying it along the way, before assigning it to the per- > sdata stuff in sdata->bands[X]. > >> I am a bit concerned about making copies (and then changing) the >> driver's bitrates array. Likely it will work with ath10k, but it >> seems fragile at best. > > That's an interesting point, we currently use the rate *index* a lot, > so we have to find some way of preserving that. Perhaps we need to > introduce a flag indicating a given rate is disabled? The logic I wrote is basically exactly this. It uses the configured rates to specify which of the hardware's rates are allowed and disabled. I will look more at providing some accessor functions or otherwise abstracting it without making copies of the driver's ratesets. Thanks, Ben
On Tue, 2016-03-15 at 09:10 -0700, Ben Greear wrote: > > The logic I wrote is basically exactly this. It uses the configured > rates to specify which of the hardware's rates are allowed and > disabled. > I understand that. I just take issue with the fact that we have to sprinkle "magic pixie dust" (in form of the override function calls) everywhere throughout the code. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c index a511ef3..5b3d79e 100644 --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c @@ -3354,7 +3354,8 @@ static int ath6kl_cfg80211_sscan_stop(struct wiphy *wiphy, static int ath6kl_cfg80211_set_bitrate(struct wiphy *wiphy, struct net_device *dev, const u8 *addr, - const struct cfg80211_bitrate_mask *mask) + const struct cfg80211_bitrate_mask *mask, + bool is_advert_mask) { struct ath6kl *ar = ath6kl_priv(dev); struct ath6kl_vif *vif = netdev_priv(dev); diff --git a/drivers/net/wireless/mwifiex/cfg80211.c b/drivers/net/wireless/mwifiex/cfg80211.c index b15e4c7..7888916 100644 --- a/drivers/net/wireless/mwifiex/cfg80211.c +++ b/drivers/net/wireless/mwifiex/cfg80211.c @@ -1542,7 +1542,8 @@ mwifiex_mgmt_stypes[NUM_NL80211_IFTYPES] = { static int mwifiex_cfg80211_set_bitrate_mask(struct wiphy *wiphy, struct net_device *dev, const u8 *peer, - const struct cfg80211_bitrate_mask *mask) + const struct cfg80211_bitrate_mask *mask, + bool is_advert_mask) { struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev); u16 bitmap_rates[MAX_BITMAP_RATES_SIZE]; diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 038de33..0804268 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -2594,7 +2594,8 @@ struct cfg80211_ops { int (*set_bitrate_mask)(struct wiphy *wiphy, struct net_device *dev, const u8 *peer, - const struct cfg80211_bitrate_mask *mask); + const struct cfg80211_bitrate_mask *mask, + bool is_advert_bitmask); int (*dump_survey)(struct wiphy *wiphy, struct net_device *netdev, int idx, struct survey_info *info); diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 6b1077c..970372d 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -2073,10 +2073,14 @@ static inline void _ieee80211_hw_set(struct ieee80211_hw *hw, * struct ieee80211_scan_request - hw scan request * * @ies: pointers different parts of IEs (in req.ie) + * @disable_ht: Ensure nothing related to HT is in the probe request + * @disable_vht: Ensure nothing related to VHT is in the probe request * @req: cfg80211 request. */ struct ieee80211_scan_request { struct ieee80211_scan_ies ies; + bool disable_ht[IEEE80211_NUM_BANDS]; + bool disable_vht[IEEE80211_NUM_BANDS]; /* Keep last */ struct cfg80211_scan_request req; diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index c0ab6b0..4573f87 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -2130,6 +2130,8 @@ enum nl80211_attrs { NL80211_ATTR_REG_INDOOR, + NL80211_ATTR_TX_ADVERT_RATEMASK, + /* add attributes here, update the policy in nl80211.c */ __NL80211_ATTR_AFTER_LAST, diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index bf7023f..0c0c5cf 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -2457,12 +2457,19 @@ static int ieee80211_set_cqm_rssi_config(struct wiphy *wiphy, static int ieee80211_set_bitrate_mask(struct wiphy *wiphy, struct net_device *dev, const u8 *addr, - const struct cfg80211_bitrate_mask *mask) + const struct cfg80211_bitrate_mask *mask, + bool is_advert_bitmask) { struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev); struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr); int i, ret; + if (is_advert_bitmask) { + memcpy(&sdata->cfg_advert_bitrate_mask, mask, sizeof(*mask)); + sdata->cfg_advert_bitrate_mask_set = true; + return 0; + } + if (!ieee80211_sdata_running(sdata)) return -ENETDOWN; diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index b15559c..5e43e99 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -904,6 +904,13 @@ struct ieee80211_sub_if_data { bool rc_has_mcs_mask[IEEE80211_NUM_BANDS]; u8 rc_rateidx_mcs_mask[IEEE80211_NUM_BANDS][IEEE80211_HT_MCS_MASK_LEN]; + /* Store bitrate mask configured from user-space. This is for + * rates that should be advertised in probe requests, etc. This + * is NOT directly related to the tx-rate-ctrl logic configuration. + */ + struct cfg80211_bitrate_mask cfg_advert_bitrate_mask; + bool cfg_advert_bitrate_mask_set; /* Has user set the mask? */ + union { struct ieee80211_if_ap ap; struct ieee80211_if_wds wds; @@ -1549,6 +1556,14 @@ int ieee80211_mesh_csa_beacon(struct ieee80211_sub_if_data *sdata, int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata); /* scan/BSS handling */ +void ieee80211_check_disabled_rates(struct ieee80211_sub_if_data *sdata, + bool *disable_ht, + bool *disable_vht); +void ieee80211_check_all_rates_disabled(u8 bands_used, + bool *disable_ht_cfg, + bool *disable_vht_cfg, + bool *disable_ht, + bool *disable_vht); void ieee80211_scan_work(struct work_struct *work); int ieee80211_request_ibss_scan(struct ieee80211_sub_if_data *sdata, const u8 *ssid, u8 ssid_len, @@ -1937,7 +1952,8 @@ int ieee80211_build_preq_ies(struct ieee80211_local *local, u8 *buffer, struct ieee80211_scan_ies *ie_desc, const u8 *ie, size_t ie_len, u8 bands_used, u32 *rate_masks, - struct cfg80211_chan_def *chandef); + struct cfg80211_chan_def *chandef, + bool disable_ht, bool disable_vht); struct sk_buff *ieee80211_build_probe_req(struct ieee80211_sub_if_data *sdata, const u8 *src, const u8 *dst, u32 ratemask, diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c index 6e857d1..6ff1138 100644 --- a/net/mac80211/scan.c +++ b/net/mac80211/scan.c @@ -262,6 +262,8 @@ static bool ieee80211_prep_hw_scan(struct ieee80211_local *local) struct cfg80211_chan_def chandef; u8 bands_used = 0; int i, ielen, n_chans; + bool disable_ht; + bool disable_vht; req = rcu_dereference_protected(local->scan_req, lockdep_is_held(&local->mtx)); @@ -297,6 +299,11 @@ static bool ieee80211_prep_hw_scan(struct ieee80211_local *local) } while (!n_chans); } + ieee80211_check_all_rates_disabled(bands_used, + local->hw_scan_req->disable_ht, + local->hw_scan_req->disable_vht, + &disable_ht, &disable_vht); + local->hw_scan_req->req.n_channels = n_chans; ieee80211_prepare_scan_chandef(&chandef, req->scan_width); @@ -305,7 +312,8 @@ static bool ieee80211_prep_hw_scan(struct ieee80211_local *local) local->hw_scan_ies_bufsize, &local->hw_scan_req->ies, req->ie, req->ie_len, - bands_used, req->rates, &chandef); + bands_used, req->rates, &chandef, + disable_ht, disable_vht); local->hw_scan_req->req.ie_len = ielen; local->hw_scan_req->req.no_cck = req->no_cck; ether_addr_copy(local->hw_scan_req->req.mac_addr, req->mac_addr); @@ -563,6 +571,10 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata, local->hw_scan_band = 0; + ieee80211_check_disabled_rates(sdata, + local->hw_scan_req->disable_ht, + local->hw_scan_req->disable_vht); + /* * After allocating local->hw_scan_req, we must * go through until ieee80211_prep_hw_scan(), so @@ -1079,6 +1091,10 @@ int __ieee80211_request_sched_scan_start(struct ieee80211_sub_if_data *sdata, struct cfg80211_chan_def chandef; int ret, i, iebufsz, num_bands = 0; u32 rate_masks[IEEE80211_NUM_BANDS] = {}; + bool disable_ht_cfg[IEEE80211_NUM_BANDS]; + bool disable_vht_cfg[IEEE80211_NUM_BANDS]; + bool disable_ht; + bool disable_vht; u8 bands_used = 0; u8 *ie; size_t len; @@ -1106,10 +1122,16 @@ int __ieee80211_request_sched_scan_start(struct ieee80211_sub_if_data *sdata, ieee80211_prepare_scan_chandef(&chandef, req->scan_width); + ieee80211_check_disabled_rates(sdata, disable_ht_cfg, disable_vht_cfg); + ieee80211_check_all_rates_disabled(bands_used, disable_ht_cfg, + disable_vht_cfg, + &disable_ht, &disable_vht); + len = ieee80211_build_preq_ies(local, ie, num_bands * iebufsz, &sched_scan_ies, req->ie, req->ie_len, bands_used, - rate_masks, &chandef); + rate_masks, &chandef, disable_ht, + disable_vht); ret = drv_sched_scan_start(local, sdata, req, &sched_scan_ies); if (ret == 0) { diff --git a/net/mac80211/util.c b/net/mac80211/util.c index b75925a..2ac75d6 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -47,6 +47,72 @@ struct ieee80211_hw *wiphy_to_ieee80211_hw(struct wiphy *wiphy) } EXPORT_SYMBOL(wiphy_to_ieee80211_hw); +/** + * ieee80211_check_disabled_rates: Calculate which rate-sets are disabled + * in all bands. + * @disable_ht_cfg Holds array of enable/disable values for each band. + * @disable_vht_cfg Holds array of enable/disable values for each band. + * @disable_ht Return value: is HT disabled for all bands. + * @disable_vht Return value: is VHT disabled for all bands. + */ +void ieee80211_check_all_rates_disabled(u8 bands_used, + bool *disable_ht_cfg, + bool *disable_vht_cfg, + bool *disable_ht, + bool *disable_vht) +{ + int i; + + *disable_ht = true; + *disable_vht = true; + for (i = 0; i < IEEE80211_NUM_BANDS; i++) { + if (bands_used & (1 << i)) { + if (!disable_ht_cfg[i]) + *disable_ht = false; + if (!disable_vht_cfg[i]) + *disable_vht = false; + } + } +} + +/** + * ieee80211_check_disabled_rates: Calculate which bands have zero rates + * configured for HT and VHT. + * @disable_ht Holds return value, array of length IEEE80211_NUM_BANDS. + * @disable_vht Holds return value, array of length IEEE80211_NUM_BANDS. + */ +void ieee80211_check_disabled_rates(struct ieee80211_sub_if_data *sdata, + bool *disable_ht, + bool *disable_vht) +{ + int i; + int j; + + /* Disable HT, VHT where we have no rates set. */ + for (i = 0; i < IEEE80211_NUM_BANDS; i++) { + disable_ht[i] = false; + disable_vht[i] = false; + if (!sdata->cfg_advert_bitrate_mask_set) + break; + + disable_ht[i] = true; + disable_vht[i] = true; + for (j = 0; j < IEEE80211_HT_MCS_MASK_LEN; j++) { + if (sdata->cfg_advert_bitrate_mask.control[i].ht_mcs[j]) { + disable_ht[i] = false; + break; + } + } + + for (j = 0; j < NL80211_VHT_NSS_MAX; j++) { + if (sdata->cfg_advert_bitrate_mask.control[i].vht_mcs[j]) { + disable_vht[i] = false; + break; + } + } + } +} + u8 *ieee80211_get_bssid(struct ieee80211_hdr *hdr, size_t len, enum nl80211_iftype type) { @@ -1414,7 +1480,8 @@ static int ieee80211_build_preq_ies_band(struct ieee80211_local *local, enum ieee80211_band band, u32 rate_mask, struct cfg80211_chan_def *chandef, - size_t *offset) + size_t *offset, bool disable_ht, + bool disable_vht) { struct ieee80211_supported_band *sband; u8 *pos = buffer, *end = buffer + buffer_len; @@ -1514,7 +1581,7 @@ static int ieee80211_build_preq_ies_band(struct ieee80211_local *local, *offset = noffset; } - if (sband->ht_cap.ht_supported) { + if (sband->ht_cap.ht_supported && !disable_ht) { if (end - pos < 2 + sizeof(struct ieee80211_ht_cap)) goto out_err; pos = ieee80211_ie_build_ht_cap(pos, &sband->ht_cap, @@ -1564,7 +1631,7 @@ static int ieee80211_build_preq_ies_band(struct ieee80211_local *local, break; } - if (sband->vht_cap.vht_supported && have_80mhz) { + if (sband->vht_cap.vht_supported && have_80mhz && !disable_vht) { if (end - pos < 2 + sizeof(struct ieee80211_vht_cap)) goto out_err; pos = ieee80211_ie_build_vht_cap(pos, &sband->vht_cap, @@ -1582,7 +1649,8 @@ int ieee80211_build_preq_ies(struct ieee80211_local *local, u8 *buffer, struct ieee80211_scan_ies *ie_desc, const u8 *ie, size_t ie_len, u8 bands_used, u32 *rate_masks, - struct cfg80211_chan_def *chandef) + struct cfg80211_chan_def *chandef, + bool disable_ht, bool disable_vht) { size_t pos = 0, old_pos = 0, custom_ie_offset = 0; int i; @@ -1597,7 +1665,9 @@ int ieee80211_build_preq_ies(struct ieee80211_local *local, u8 *buffer, ie, ie_len, i, rate_masks[i], chandef, - &custom_ie_offset); + &custom_ie_offset, + disable_ht, + disable_vht); ie_desc->ies[i] = buffer + old_pos; ie_desc->len[i] = pos - old_pos; old_pos = pos; @@ -1633,6 +1703,11 @@ struct sk_buff *ieee80211_build_probe_req(struct ieee80211_sub_if_data *sdata, struct ieee80211_mgmt *mgmt; int ies_len; u32 rate_masks[IEEE80211_NUM_BANDS] = {}; + bool disable_ht_cfg[IEEE80211_NUM_BANDS]; + bool disable_vht_cfg[IEEE80211_NUM_BANDS]; + bool disable_ht; + bool disable_vht; + u8 bands_used; struct ieee80211_scan_ies dummy_ie_desc; /* @@ -1652,10 +1727,18 @@ struct sk_buff *ieee80211_build_probe_req(struct ieee80211_sub_if_data *sdata, return NULL; rate_masks[chan->band] = ratemask; + bands_used = BIT(chan->band); + + ieee80211_check_disabled_rates(sdata, disable_ht_cfg, disable_vht_cfg); + ieee80211_check_all_rates_disabled(bands_used, disable_ht_cfg, + disable_vht_cfg, + &disable_ht, &disable_vht); + ies_len = ieee80211_build_preq_ies(local, skb_tail_pointer(skb), skb_tailroom(skb), &dummy_ie_desc, - ie, ie_len, BIT(chan->band), - rate_masks, &chandef); + ie, ie_len, bands_used, + rate_masks, &chandef, disable_ht, + disable_vht); skb_put(skb, ies_len); if (dst) { diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 24fac43..7b0a82b 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -400,6 +400,7 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { [NL80211_ATTR_NETNS_FD] = { .type = NLA_U32 }, [NL80211_ATTR_SCHED_SCAN_DELAY] = { .type = NLA_U32 }, [NL80211_ATTR_REG_INDOOR] = { .type = NLA_FLAG }, + [NL80211_ATTR_TX_ADVERT_RATEMASK] = { .type = NLA_FLAG }, }; /* policy for the key attributes */ @@ -8284,6 +8285,7 @@ static int nl80211_set_tx_bitrate_mask(struct sk_buff *skb, struct nlattr *tx_rates; struct ieee80211_supported_band *sband; u16 vht_tx_mcs_map; + bool is_advert_mask; if (!rdev->ops->set_bitrate_mask) return -EOPNOTSUPP; @@ -8312,6 +8314,7 @@ static int nl80211_set_tx_bitrate_mask(struct sk_buff *skb, if (!info->attrs[NL80211_ATTR_TX_RATES]) goto out; + is_advert_mask = nla_get_flag(info->attrs[NL80211_ATTR_TX_ADVERT_RATEMASK]); /* * The nested attribute uses enum nl80211_band as the index. This maps * directly to the enum ieee80211_band values used in cfg80211. @@ -8404,7 +8407,7 @@ static int nl80211_set_tx_bitrate_mask(struct sk_buff *skb, } out: - return rdev_set_bitrate_mask(rdev, dev, NULL, &mask); + return rdev_set_bitrate_mask(rdev, dev, NULL, &mask, is_advert_mask); } static int nl80211_register_mgmt(struct sk_buff *skb, struct genl_info *info) diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h index c6e83a7..2531881 100644 --- a/net/wireless/rdev-ops.h +++ b/net/wireless/rdev-ops.h @@ -590,11 +590,13 @@ static inline int rdev_testmode_dump(struct cfg80211_registered_device *rdev, static inline int rdev_set_bitrate_mask(struct cfg80211_registered_device *rdev, struct net_device *dev, const u8 *peer, - const struct cfg80211_bitrate_mask *mask) + const struct cfg80211_bitrate_mask *mask, + bool is_advert_mask) { int ret; trace_rdev_set_bitrate_mask(&rdev->wiphy, dev, peer, mask); - ret = rdev->ops->set_bitrate_mask(&rdev->wiphy, dev, peer, mask); + ret = rdev->ops->set_bitrate_mask(&rdev->wiphy, dev, peer, mask, + is_advert_mask); trace_rdev_return_int(&rdev->wiphy, ret); return ret; } diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c index fd68283..7874f4f 100644 --- a/net/wireless/wext-compat.c +++ b/net/wireless/wext-compat.c @@ -1266,7 +1266,7 @@ static int cfg80211_wext_siwrate(struct net_device *dev, if (!match) return -EINVAL; - return rdev_set_bitrate_mask(rdev, dev, NULL, &mask); + return rdev_set_bitrate_mask(rdev, dev, NULL, &mask, false); } static int cfg80211_wext_giwrate(struct net_device *dev,