| Message ID | 1572957714-16085-1-git-send-email-tamizhr@codeaurora.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | cfg80211/mac80211: Add support for TID specific configuration | expand |
> Add infrastructure to support per TID configurations like noack policy, > retry count, AMPDU control(disable/enable), RTSCTS control(enable/disable) > and TX rate mask configurations. > This will be useful for the driver which can supports data TID > specific configuration rather than phy level configurations. > Here NL80211_CMD_SET_TID_CONFIG added to support this operation by > accepting TID configuration. > This command can accept STA mac addreess to make the configuration > station specific rather than applying to all the connected stations > to the netdev. > And this nested command configuration can accept multiple number of > data TID specific configuration in a single command, > enum ieee80211_tid_conf_mask used to notify the driver that which > configuration got modified for the TID. > > Tamizh chelvam (6): > nl80211: New netlink command for TID specific configuration > nl80211: Add new netlink attribute for TID speicific retry count > nl80211: Add netlink attribute for AMPDU aggregation enable/disable > nl80211: Add netlink attribute to enable/disable RTS_CTS > nl80211: Add netlink attribute to configure TID specific tx rate > mac80211: Add api to support configuring TID specific configuration > > include/net/cfg80211.h | 55 +++++++++ > include/net/mac80211.h | 8 ++ > include/uapi/linux/nl80211.h | 190 +++++++++++++++++++++++++++++ > net/mac80211/cfg.c | 28 +++++ > net/mac80211/driver-ops.h | 15 +++ > net/wireless/nl80211.c | 276 +++++++++++++++++++++++++++++++++++++++--- > net/wireless/rdev-ops.h | 12 ++ > net/wireless/trace.h | 17 +++ > 8 files changed, 584 insertions(+), 17 deletions(-) Hi Tamizh, Johannes, and all, Looks very good to me: Reviewed-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com> BTW, there are two open questions remaining from the previous reviews: - NL80211_TX_RATE_LIMITED and NL80211_TX_RATE_FIXED Interpretation and validation of these two rate options is left up to drivers. - 'apply to all TIDs' usecase Currently, if peer is not specified, then configuration is applied to all the connected STAs. It is tempting to use some spare TID value to inform drivers that provided configuration should be applied to all TIDs of the specified STA or even to all TIDS and STAs. But that can not be left up to drivers since this value needs to be passed from userspace tools, e.g. from iw. IIUC, the first question could be addressed later, after we see some actual users and figure out generic use-cases. But what about the second question ? Maybe it worth to add and document a single define, e.g. using TID value 0xff, that can be used between userspace and drivers for such usecases ? Regards, Sergey
Hi Sergey, Thanks for looking! > BTW, there are two open questions remaining from the previous reviews: > > - NL80211_TX_RATE_LIMITED and NL80211_TX_RATE_FIXED > Interpretation and validation of these two rate options is left > up to drivers. (need to look at this still) > - 'apply to all TIDs' usecase > Currently, if peer is not specified, then configuration is applied to > all the connected STAs. It is tempting to use some spare TID value > to inform drivers that provided configuration should be applied to > all TIDs of the specified STA or even to all TIDS and STAs. But that > can not be left up to drivers since this value needs to be passed > from userspace tools, e.g. from iw. I was *just* replying on exactly the same point over in patch 1 (not sent yet). It's actually not even clear to me that the configuration really would be applied to *all* STAs, it's sort of left open for the driver, afaict? But I agree with you that this is not a good thing. I don't think using a spare TID value is the right signalling, we can add another attribute? E.g. we could easily add NL80211_TID_CONFIG_ATTR_OVERRIDE and make that be @NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribute, valid only if no STA is selected, if set indicates that the new configuration overrides all previous STA configurations, otherwise previous STA-specific configurations should be left untouched You also raise a good point wrt. "all TIDs" - but then we should probably just remove NL80211_TID_CONFIG_ATTR_TID and add a new NL80211_TID_CONFIG_ATTR_TIDS as a bitmap? OTOH, it's not hard to just explicitly spell out all TIDs either, I guess, just makes the message a bit bigger. johannes
> > - 'apply to all TIDs' usecase > > Currently, if peer is not specified, then configuration is applied to > > all the connected STAs. It is tempting to use some spare TID value > > to inform drivers that provided configuration should be applied to > > all TIDs of the specified STA or even to all TIDS and STAs. But that > > can not be left up to drivers since this value needs to be passed > > from userspace tools, e.g. from iw. > > I was *just* replying on exactly the same point over in patch 1 (not > sent yet). It's actually not even clear to me that the configuration > really would be applied to *all* STAs, it's sort of left open for the > driver, afaict? > > But I agree with you that this is not a good thing. > > I don't think using a spare TID value is the right signalling, we can > add another attribute? E.g. we could easily add > > NL80211_TID_CONFIG_ATTR_OVERRIDE > > and make that be > > @NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribute, valid only if no STA > is selected, if set indicates that the new configuration > overrides all previous STA configurations, otherwise previous > STA-specific configurations should be left untouched > > You also raise a good point wrt. "all TIDs" - but then we should > probably just remove NL80211_TID_CONFIG_ATTR_TID and add a new > NL80211_TID_CONFIG_ATTR_TIDS as a bitmap? OTOH, it's not hard to just > explicitly spell out all TIDs either, I guess, just makes the message a > bit bigger. The idea with bitmask for TIDs looks good. It eliminates the need for both 'all TIDs' define and additional attribute NL80211_TID_CONFIG_ATTR_OVERRIDE. In fact, almost no changes are needed for the patch, mostly comments need to be updated. Manual typing in iw will be less convenient, but I guess this interface is not supposed to be used by humans after all... Regards, Sergey
On Fri, 2019-11-08 at 12:05 +0000, Sergey Matyukevich wrote: > > @NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribute, valid only if no STA > > is selected, if set indicates that the new configuration > > overrides all previous STA configurations, otherwise previous > > STA-specific configurations should be left untouched > > > > You also raise a good point wrt. "all TIDs" - but then we should > > probably just remove NL80211_TID_CONFIG_ATTR_TID and add a new > > NL80211_TID_CONFIG_ATTR_TIDS as a bitmap? OTOH, it's not hard to just > > explicitly spell out all TIDs either, I guess, just makes the message a > > bit bigger. > > The idea with bitmask for TIDs looks good. It eliminates the need for both > 'all TIDs' define and additional attribute NL80211_TID_CONFIG_ATTR_OVERRIDE. I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way (maybe only as a flag attribute), since you could have * change all stations (some subset of TIDs) *including* already configured stations * or *excluding* already configured stations > In fact, almost no changes are needed for the patch, mostly comments need > to be updated. Manual typing in iw will be less convenient, but I guess > this interface is not supposed to be used by humans after all... If that's a concern we can parse a list in iw, e.g. "0,1,2,3" instaed of "0xf", right? johannes
> > > @NL80211_TID_CONFIG_ATTR_OVERRIDE: flag attribute, valid only if no STA > > > is selected, if set indicates that the new configuration > > > overrides all previous STA configurations, otherwise previous > > > STA-specific configurations should be left untouched > > > > > > You also raise a good point wrt. "all TIDs" - but then we should > > > probably just remove NL80211_TID_CONFIG_ATTR_TID and add a new > > > NL80211_TID_CONFIG_ATTR_TIDS as a bitmap? OTOH, it's not hard to just > > > explicitly spell out all TIDs either, I guess, just makes the message a > > > bit bigger. > > > > The idea with bitmask for TIDs looks good. It eliminates the need for both > > 'all TIDs' define and additional attribute NL80211_TID_CONFIG_ATTR_OVERRIDE. > > I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way > (maybe only as a flag attribute), since you could have > > * change all stations (some subset of TIDs) *including* already > configured stations > * or *excluding* already configured stations Hmmm... Logic is straightforwad without this flag: - settings are applied to bitmasked TIDs of a single peer if address is specifed - settings are applied to bitmasked TIDs of all the peers if no address is specified It looks like you want to infer too much from a single flag. Why keep this logic in cfg80211/mac80211/driver ? > > In fact, almost no changes are needed for the patch, mostly comments need > > to be updated. Manual typing in iw will be less convenient, but I guess > > this interface is not supposed to be used by humans after all... > > If that's a concern we can parse a list in iw, e.g. "0,1,2,3" instaed of > "0xf", right? Oh, right, it can be that simple. But this is by no means a concern. Regards, Sergey
On Fri, 2019-11-08 at 16:01 +0000, Sergey Matyukevich wrote: > > I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way > > (maybe only as a flag attribute), since you could have > > > > * change all stations (some subset of TIDs) *including* already > > configured stations > > * or *excluding* already configured stations > > Hmmm... Logic is straightforwad without this flag: > - settings are applied to bitmasked TIDs of a single peer if address is specifed > - settings are applied to bitmasked TIDs of all the peers if no address is specified Sure, this is obvious, but what exactly does "all the peers" mean? Say I do set_tid_config(tids=0x1, peer=02:11:22:33:44:55, noack=yes) set_tid_config(tids=0x1, peer=NULL, noack=no) Does that reset peer 02:11:22:33:44:55, or not? This is not documented right now, and one could argue both ways - the override for that particular peer should stick, or should be removed. Which one is it? > It looks like you want to infer too much from a single flag. Why keep this logic in > cfg80211/mac80211/driver ? I just want to disambiguate what "all the peers" means. Not sure what you mean by keeping the logic? johannes
> > > I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way > > > (maybe only as a flag attribute), since you could have > > > > > > * change all stations (some subset of TIDs) *including* already > > > configured stations > > > * or *excluding* already configured stations > > > > Hmmm... Logic is straightforwad without this flag: > > - settings are applied to bitmasked TIDs of a single peer if address is specifed > > - settings are applied to bitmasked TIDs of all the peers if no address is specified > > Sure, this is obvious, but what exactly does "all the peers" mean? > > Say I do > > set_tid_config(tids=0x1, peer=02:11:22:33:44:55, noack=yes) > set_tid_config(tids=0x1, peer=NULL, noack=no) > > Does that reset peer 02:11:22:33:44:55, or not? This is not documented > right now, and one could argue both ways - the override for that > particular peer should stick, or should be removed. Which one is it? Ok, I got the point. My understanding was that any further command would rewrite the previous settings. But now I agree that this is not obvious and should be explicitly documented. Sergey
On 2019-11-08 22:37, Johannes Berg wrote: > On Fri, 2019-11-08 at 16:01 +0000, Sergey Matyukevich wrote: > >> > I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way >> > (maybe only as a flag attribute), since you could have >> > >> > * change all stations (some subset of TIDs) *including* already >> > configured stations >> > * or *excluding* already configured stations >> >> Hmmm... Logic is straightforwad without this flag: >> - settings are applied to bitmasked TIDs of a single peer if address >> is specifed >> - settings are applied to bitmasked TIDs of all the peers if no >> address is specified > > Sure, this is obvious, but what exactly does "all the peers" mean? > > Say I do > > set_tid_config(tids=0x1, peer=02:11:22:33:44:55, noack=yes) > set_tid_config(tids=0x1, peer=NULL, noack=no) > > Does that reset peer 02:11:22:33:44:55, or not? This is not documented > right now, and one could argue both ways - the override for that > particular peer should stick, or should be removed. Which one is it? > Here, the second command won't reset the peer 02:11:22:33:44:55. Here we are giving more preference to the peer specific configuration. We have to reset the peer 02:11:22:33:44:55 using the set_tid_config(tids=0x1, peer=02:11:22:33:44:55, DEFAULT). I will add these in the DOC section and send it in next patchset. >> It looks like you want to infer too much from a single flag. Why keep >> this logic in >> cfg80211/mac80211/driver ? > > I just want to disambiguate what "all the peers" means. Not sure what > you mean by keeping the logic? > Thanks, Tamizh.
On Thu, 2019-11-14 at 13:02 +0530, Tamizh chelvam wrote: > On 2019-11-08 22:37, Johannes Berg wrote: > > On Fri, 2019-11-08 at 16:01 +0000, Sergey Matyukevich wrote: > > > > > > I think we still need NL80211_TID_CONFIG_ATTR_OVERRIDE in some way > > > > (maybe only as a flag attribute), since you could have > > > > > > > > * change all stations (some subset of TIDs) *including* already > > > > configured stations > > > > * or *excluding* already configured stations > > > > > > Hmmm... Logic is straightforwad without this flag: > > > - settings are applied to bitmasked TIDs of a single peer if address > > > is specifed > > > - settings are applied to bitmasked TIDs of all the peers if no > > > address is specified > > > > Sure, this is obvious, but what exactly does "all the peers" mean? > > > > Say I do > > > > set_tid_config(tids=0x1, peer=02:11:22:33:44:55, noack=yes) > > set_tid_config(tids=0x1, peer=NULL, noack=no) > > > > Does that reset peer 02:11:22:33:44:55, or not? This is not documented > > right now, and one could argue both ways - the override for that > > particular peer should stick, or should be removed. Which one is it? > > > Here, the second command won't reset the peer 02:11:22:33:44:55. Here we > are giving more > preference to the peer specific configuration. We have to reset the peer > 02:11:22:33:44:55 using the set_tid_config(tids=0x1, > peer=02:11:22:33:44:55, DEFAULT). I will add these in the DOC section > and send it in next patchset. OK, but maybe in some cases it _is_ desired to actually clear all peer- specific overrides (somehow)? johannes
Add infrastructure to support per TID configurations like noack policy, retry count, AMPDU control(disable/enable), RTSCTS control(enable/disable) and TX rate mask configurations. This will be useful for the driver which can supports data TID specific configuration rather than phy level configurations. Here NL80211_CMD_SET_TID_CONFIG added to support this operation by accepting TID configuration. This command can accept STA mac addreess to make the configuration station specific rather than applying to all the connected stations to the netdev. And this nested command configuration can accept multiple number of data TID specific configuration in a single command, enum ieee80211_tid_conf_mask used to notify the driver that which configuration got modified for the TID. Tamizh chelvam (6): nl80211: New netlink command for TID specific configuration nl80211: Add new netlink attribute for TID speicific retry count nl80211: Add netlink attribute for AMPDU aggregation enable/disable nl80211: Add netlink attribute to enable/disable RTS_CTS nl80211: Add netlink attribute to configure TID specific tx rate mac80211: Add api to support configuring TID specific configuration include/net/cfg80211.h | 55 +++++++++ include/net/mac80211.h | 8 ++ include/uapi/linux/nl80211.h | 190 +++++++++++++++++++++++++++++ net/mac80211/cfg.c | 28 +++++ net/mac80211/driver-ops.h | 15 +++ net/wireless/nl80211.c | 276 +++++++++++++++++++++++++++++++++++++++--- net/wireless/rdev-ops.h | 12 ++ net/wireless/trace.h | 17 +++ 8 files changed, 584 insertions(+), 17 deletions(-) v8: * Fixed enum typecast warning. v7: * Fixed compilation error and removed tid config variables from mac80211 v6: * Addressed Johannes comments. v5: * Fixed possible memleak of 'tid_conf' in nl80211_set_tid_config. v4: * Fixed kbuild warnings. v3: * Modified "nl80211: Add netlink attribute to configure TID specific tx rate" patch to accept multiple TX rate configuration at a time. * Modified noack and ampdu variable data type to int in "mac80211: Add api to support configuring TID specific configuration" patch to store default configuration. * Modified "ath10k: Add new api to support TID specific configuration" patch to handle default values for noack and ampdu. And added sta pointer sanity check in ath10k_mac_tid_bitrate_config function. * Fixed "ath10k: Add extended TID configuration support" wmi command parameters assigned part. v2: * Added support to accept multiple TID configuration * Added support to configure TX rate and RTSCTS control