| Message ID | 20200313065114.23433-1-yhchuang@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: add debugfs to fix tx rate | expand |
<yhchuang@realtek.com> writes: > From: Yan-Hsuan Chuang <yhchuang@realtek.com> > > It is useful to fix the bit rate of TX packets. For example, if > someone is measuring the TX power, or debugging with the issues > of the TX throughput on the field. > > To set the value of fixed rate, one should input corresponding > desc rate index (ex, 0x0b for DESC_RATE54M to fix at 54 Mbps). > Set a value larger than DESC_RATE_MAX will disable fix rate, so > the rate adaptive mechanism can resume to work. > > Example, > To fix rate at MCS 1: > echo 0x0d > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > To not to fix rate: > echo 0xff > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > To know which rate was fixed at: > cat /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> No way, debugfs is not a method for working around nl80211 and doing whatever idea you come up with. The goal is that we have a generic nl80211 command for all generic actions, like this one. And I think we already have an nl80211 command for fixing the tx rate, right?
Kalle Valo <kvalo@codeaurora.org> writes: > <yhchuang@realtek.com> writes: > > > From: Yan-Hsuan Chuang <yhchuang@realtek.com> > > > > It is useful to fix the bit rate of TX packets. For example, if > > someone is measuring the TX power, or debugging with the issues > > of the TX throughput on the field. > > > > To set the value of fixed rate, one should input corresponding > > desc rate index (ex, 0x0b for DESC_RATE54M to fix at 54 Mbps). > > Set a value larger than DESC_RATE_MAX will disable fix rate, so > > the rate adaptive mechanism can resume to work. > > > > Example, > > To fix rate at MCS 1: > > echo 0x0d > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > > > To not to fix rate: > > echo 0xff > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > > > To know which rate was fixed at: > > cat /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > > No way, debugfs is not a method for working around nl80211 and doing > whatever idea you come up with. The goal is that we have a generic > nl80211 command for all generic actions, like this one. And I think we > already have an nl80211 command for fixing the tx rate, right? > No, as I can see, there's no suitable nl80211 command that can achieve what I want. If you are saying about NL80211_CMD_SET_TX_BITRATE_MASK, it's used to allow some rates. But actually the firmware has its own rate adaptive mechanism, so mask out the other rates does not mean the rate left will be chosen. Moreover, the hardware will choose a lower bit rate when retry, then the TX rate is not fixed at all. So the debugfs can disable the firmware's RA mechanism, also disable the TX rate fall back when retry. Both of them cannot be done by setting TX bitrate mask. I am sorry I need to add another debugfs for it, but to actually fix the TX bitrate, we really need another debugfs or module parameter. Because according to the design of the device there is not a good enough general command I can use to fix the TX rate. If there is a command that can fix the TX bitrate for me, please let me know, I can switch to it. Thanks, Yen-Hsuan
Tony Chuang <yhchuang@realtek.com> writes: > Kalle Valo <kvalo@codeaurora.org> writes: > >> <yhchuang@realtek.com> writes: >> >> > From: Yan-Hsuan Chuang <yhchuang@realtek.com> >> > >> > It is useful to fix the bit rate of TX packets. For example, if >> > someone is measuring the TX power, or debugging with the issues >> > of the TX throughput on the field. >> > >> > To set the value of fixed rate, one should input corresponding >> > desc rate index (ex, 0x0b for DESC_RATE54M to fix at 54 Mbps). >> > Set a value larger than DESC_RATE_MAX will disable fix rate, so >> > the rate adaptive mechanism can resume to work. >> > >> > Example, >> > To fix rate at MCS 1: >> > echo 0x0d > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >> > >> > To not to fix rate: >> > echo 0xff > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >> > >> > To know which rate was fixed at: >> > cat /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >> > >> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> >> >> No way, debugfs is not a method for working around nl80211 and doing >> whatever idea you come up with. The goal is that we have a generic >> nl80211 command for all generic actions, like this one. And I think we >> already have an nl80211 command for fixing the tx rate, right? >> > > No, as I can see, there's no suitable nl80211 command that can achieve > what I want. If you are saying about NL80211_CMD_SET_TX_BITRATE_MASK, > it's used to allow some rates. But actually the firmware has its own rate > adaptive mechanism, so mask out the other rates does not mean the rate > left will be chosen. Moreover, the hardware will choose a lower bit rate > when retry, then the TX rate is not fixed at all. So the debugfs can disable > the firmware's RA mechanism, also disable the TX rate fall back when retry. > Both of them cannot be done by setting TX bitrate mask. I'm confused, here you talk about firmware implementation etc but I'm just talking about replacing the fix_rate debugfs file to an nl80211 command (for providing the fix_rate value). Can you clarify more why you think nl80211 is not suitable? > I am sorry I need to add another debugfs for it, but to actually fix the TX > bitrate, we really need another debugfs or module parameter. Because > according to the design of the device there is not a good enough general > command I can use to fix the TX rate. If there is a command that can fix > the TX bitrate for me, please let me know, I can switch to it. Sorry, but I'm not yet convinced that a debugfs file is justified. Fixing a transmit bitrate sounds like a very generic command, not something which should be in debugfs.
// Add Johannes for commenting on adding another nl80211 commands Kalle Valo <kvalo@codeaurora.org> writes:> > Tony Chuang <yhchuang@realtek.com> writes: > > > Kalle Valo <kvalo@codeaurora.org> writes: > > > >> <yhchuang@realtek.com> writes: > >> > >> > From: Yan-Hsuan Chuang <yhchuang@realtek.com> > >> > > >> > It is useful to fix the bit rate of TX packets. For example, if > >> > someone is measuring the TX power, or debugging with the issues > >> > of the TX throughput on the field. > >> > > >> > To set the value of fixed rate, one should input corresponding > >> > desc rate index (ex, 0x0b for DESC_RATE54M to fix at 54 Mbps). > >> > Set a value larger than DESC_RATE_MAX will disable fix rate, so > >> > the rate adaptive mechanism can resume to work. > >> > > >> > Example, > >> > To fix rate at MCS 1: > >> > echo 0x0d > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > >> > > >> > To not to fix rate: > >> > echo 0xff > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > >> > > >> > To know which rate was fixed at: > >> > cat /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > >> > > >> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > >> > >> No way, debugfs is not a method for working around nl80211 and doing > >> whatever idea you come up with. The goal is that we have a generic > >> nl80211 command for all generic actions, like this one. And I think we > >> already have an nl80211 command for fixing the tx rate, right? > >> > > > > No, as I can see, there's no suitable nl80211 command that can achieve > > what I want. If you are saying about > NL80211_CMD_SET_TX_BITRATE_MASK, > > it's used to allow some rates. But actually the firmware has its own rate > > adaptive mechanism, so mask out the other rates does not mean the rate > > left will be chosen. Moreover, the hardware will choose a lower bit rate > > when retry, then the TX rate is not fixed at all. So the debugfs can disable > > the firmware's RA mechanism, also disable the TX rate fall back when retry. > > Both of them cannot be done by setting TX bitrate mask. > > I'm confused, here you talk about firmware implementation etc but I'm > just talking about replacing the fix_rate debugfs file to an nl80211 > command (for providing the fix_rate value). Can you clarify more why you > think nl80211 is not suitable? Oops, I thought that you wanted me to use the existing nl80211 command. Now I know that you think we can add a new nl80211 command to help drivers to fix the TX bitrate if necessary. If adding another nl80211 command for that is acceptable, I can work on this. But I need Johannes's comment if it's better to add a new nl80211 command or to expand the existing command (ex. NL80211_CMD_SET_TX_BITRATE_MASK). It looks like that adding a new nl80211 command will be better for me as expanding the existing one would have great impact on the already distributed drivers/user-tools. > > > I am sorry I need to add another debugfs for it, but to actually fix the TX > > bitrate, we really need another debugfs or module parameter. Because > > according to the design of the device there is not a good enough general > > command I can use to fix the TX rate. If there is a command that can fix > > the TX bitrate for me, please let me know, I can switch to it. > > Sorry, but I'm not yet convinced that a debugfs file is justified. > Fixing a transmit bitrate sounds like a very generic command, not > something which should be in debugfs. > Thanks. Yen-Hsuan
Tony Chuang <yhchuang@realtek.com> writes: > // Add Johannes for commenting on adding another nl80211 commands > > Kalle Valo <kvalo@codeaurora.org> writes:> > >> Tony Chuang <yhchuang@realtek.com> writes: >> >> > Kalle Valo <kvalo@codeaurora.org> writes: >> > >> >> <yhchuang@realtek.com> writes: >> >> >> >> > From: Yan-Hsuan Chuang <yhchuang@realtek.com> >> >> > >> >> > It is useful to fix the bit rate of TX packets. For example, if >> >> > someone is measuring the TX power, or debugging with the issues >> >> > of the TX throughput on the field. >> >> > >> >> > To set the value of fixed rate, one should input corresponding >> >> > desc rate index (ex, 0x0b for DESC_RATE54M to fix at 54 Mbps). >> >> > Set a value larger than DESC_RATE_MAX will disable fix rate, so >> >> > the rate adaptive mechanism can resume to work. >> >> > >> >> > Example, >> >> > To fix rate at MCS 1: >> >> > echo 0x0d > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >> >> > >> >> > To not to fix rate: >> >> > echo 0xff > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >> >> > >> >> > To know which rate was fixed at: >> >> > cat /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >> >> > >> >> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> >> >> >> >> No way, debugfs is not a method for working around nl80211 and doing >> >> whatever idea you come up with. The goal is that we have a generic >> >> nl80211 command for all generic actions, like this one. And I think we >> >> already have an nl80211 command for fixing the tx rate, right? >> >> >> > >> > No, as I can see, there's no suitable nl80211 command that can achieve >> > what I want. If you are saying about >> NL80211_CMD_SET_TX_BITRATE_MASK, >> > it's used to allow some rates. But actually the firmware has its own rate >> > adaptive mechanism, so mask out the other rates does not mean the rate >> > left will be chosen. Moreover, the hardware will choose a lower bit rate >> > when retry, then the TX rate is not fixed at all. So the debugfs can disable >> > the firmware's RA mechanism, also disable the TX rate fall back when retry. >> > Both of them cannot be done by setting TX bitrate mask. >> >> I'm confused, here you talk about firmware implementation etc but I'm >> just talking about replacing the fix_rate debugfs file to an nl80211 >> command (for providing the fix_rate value). Can you clarify more why you >> think nl80211 is not suitable? > > Oops, I thought that you wanted me to use the existing nl80211 > command. Either use an existing nl80211 command or add a new one if needed. For me most important is that we don't add hacks to debugfs just for avoiding using nl80211. > Now I know that you think we can add a new nl80211 command to help > drivers to fix the TX bitrate if necessary. If adding another nl80211 > command for that is acceptable, I can work on this. But I need > Johannes's comment if it's better to add a new nl80211 command or to > expand the existing command (ex. NL80211_CMD_SET_TX_BITRATE_MASK). _Why_ is NL80211_CMD_SET_TX_BITRATE_MASK not suitable for you? You keep saying that but I have still figured out why exactly you think so. Please clarify this in detail. > It looks like that adding a new nl80211 command will be better for me > as expanding the existing one would have great impact on the already > distributed drivers/user-tools. What kind of great impact are you talking about? Please be specific so that we don't need to guess.
On 3/17/20 8:40 AM, Kalle Valo wrote: > Tony Chuang <yhchuang@realtek.com> writes: > >> // Add Johannes for commenting on adding another nl80211 commands >> >> Kalle Valo <kvalo@codeaurora.org> writes:> >> >>> Tony Chuang <yhchuang@realtek.com> writes: >>> >>>> Kalle Valo <kvalo@codeaurora.org> writes: >>>> >>>>> <yhchuang@realtek.com> writes: >>>>> >>>>>> From: Yan-Hsuan Chuang <yhchuang@realtek.com> >>>>>> >>>>>> It is useful to fix the bit rate of TX packets. For example, if >>>>>> someone is measuring the TX power, or debugging with the issues >>>>>> of the TX throughput on the field. >>>>>> >>>>>> To set the value of fixed rate, one should input corresponding >>>>>> desc rate index (ex, 0x0b for DESC_RATE54M to fix at 54 Mbps). >>>>>> Set a value larger than DESC_RATE_MAX will disable fix rate, so >>>>>> the rate adaptive mechanism can resume to work. >>>>>> >>>>>> Example, >>>>>> To fix rate at MCS 1: >>>>>> echo 0x0d > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >>>>>> >>>>>> To not to fix rate: >>>>>> echo 0xff > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >>>>>> >>>>>> To know which rate was fixed at: >>>>>> cat /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >>>>>> >>>>>> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> >>>>> >>>>> No way, debugfs is not a method for working around nl80211 and doing >>>>> whatever idea you come up with. The goal is that we have a generic >>>>> nl80211 command for all generic actions, like this one. And I think we >>>>> already have an nl80211 command for fixing the tx rate, right? >>>>> >>>> >>>> No, as I can see, there's no suitable nl80211 command that can achieve >>>> what I want. If you are saying about >>> NL80211_CMD_SET_TX_BITRATE_MASK, >>>> it's used to allow some rates. But actually the firmware has its own rate >>>> adaptive mechanism, so mask out the other rates does not mean the rate >>>> left will be chosen. Moreover, the hardware will choose a lower bit rate >>>> when retry, then the TX rate is not fixed at all. So the debugfs can disable >>>> the firmware's RA mechanism, also disable the TX rate fall back when retry. >>>> Both of them cannot be done by setting TX bitrate mask. >>> >>> I'm confused, here you talk about firmware implementation etc but I'm >>> just talking about replacing the fix_rate debugfs file to an nl80211 >>> command (for providing the fix_rate value). Can you clarify more why you >>> think nl80211 is not suitable? >> >> Oops, I thought that you wanted me to use the existing nl80211 >> command. > > Either use an existing nl80211 command or add a new one if needed. For > me most important is that we don't add hacks to debugfs just for > avoiding using nl80211. > >> Now I know that you think we can add a new nl80211 command to help >> drivers to fix the TX bitrate if necessary. If adding another nl80211 >> command for that is acceptable, I can work on this. But I need >> Johannes's comment if it's better to add a new nl80211 command or to >> expand the existing command (ex. NL80211_CMD_SET_TX_BITRATE_MASK). > > _Why_ is NL80211_CMD_SET_TX_BITRATE_MASK not suitable for you? You keep > saying that but I have still figured out why exactly you think so. > Please clarify this in detail. > >> It looks like that adding a new nl80211 command will be better for me >> as expanding the existing one would have great impact on the already >> distributed drivers/user-tools. > > What kind of great impact are you talking about? Please be specific so > that we don't need to guess. At least with ath10k, the issues I found were that nl80211 doesn't like it when you try to disable all legacy rates (and force frames out at 54Mbps encoding, for instance). I'm not even sure upstream ath10k will even let you set a single rate using normal API now. Have you tried it? Another problem is that to keep a connection alive, you probably want mgt and null-func frames to go out normal and only have the firmware use a particular MCS for data frames. Lots of reasons to want a low-level hack for this sort of thing. Thanks, Ben
Ben Greear <greearb@candelatech.com> writes: > On 3/17/20 8:40 AM, Kalle Valo wrote: >> Tony Chuang <yhchuang@realtek.com> writes: >> >>> // Add Johannes for commenting on adding another nl80211 commands >>> >>> Kalle Valo <kvalo@codeaurora.org> writes:> >>> >>>> Tony Chuang <yhchuang@realtek.com> writes: >>>> >>>>> Kalle Valo <kvalo@codeaurora.org> writes: >>>>> >>>>>> <yhchuang@realtek.com> writes: >>>>>> >>>>>>> From: Yan-Hsuan Chuang <yhchuang@realtek.com> >>>>>>> >>>>>>> It is useful to fix the bit rate of TX packets. For example, if >>>>>>> someone is measuring the TX power, or debugging with the issues >>>>>>> of the TX throughput on the field. >>>>>>> >>>>>>> To set the value of fixed rate, one should input corresponding >>>>>>> desc rate index (ex, 0x0b for DESC_RATE54M to fix at 54 Mbps). >>>>>>> Set a value larger than DESC_RATE_MAX will disable fix rate, so >>>>>>> the rate adaptive mechanism can resume to work. >>>>>>> >>>>>>> Example, >>>>>>> To fix rate at MCS 1: >>>>>>> echo 0x0d > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >>>>>>> >>>>>>> To not to fix rate: >>>>>>> echo 0xff > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >>>>>>> >>>>>>> To know which rate was fixed at: >>>>>>> cat /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate >>>>>>> >>>>>>> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> >>>>>> >>>>>> No way, debugfs is not a method for working around nl80211 and >doing >>>>>> whatever idea you come up with. The goal is that we have a generic >>>>>> nl80211 command for all generic actions, like this one. And I think we >>>>>> already have an nl80211 command for fixing the tx rate, right? >>>>>> >>>>> >>>>> No, as I can see, there's no suitable nl80211 command that can achieve >>>>> what I want. If you are saying about >>>> NL80211_CMD_SET_TX_BITRATE_MASK, >>>>> it's used to allow some rates. But actually the firmware has its own rate >>>>> adaptive mechanism, so mask out the other rates does not mean the > rate >>>>> left will be chosen. Moreover, the hardware will choose a lower bit rate >>>>> when retry, then the TX rate is not fixed at all. So the debugfs can > disable >>>>> the firmware's RA mechanism, also disable the TX rate fall back when > retry. >>>>> Both of them cannot be done by setting TX bitrate mask. This is the reason the nl80211 command is not suitable for me. >>>> >>>> I'm confused, here you talk about firmware implementation etc but I'm >>>> just talking about replacing the fix_rate debugfs file to an nl80211 >>>> command (for providing the fix_rate value). Can you clarify more why > you >>>> think nl80211 is not suitable? >>> >>> Oops, I thought that you wanted me to use the existing nl80211 >>> command. >> >> Either use an existing nl80211 command or add a new one if needed. For >> me most important is that we don't add hacks to debugfs just for >> avoiding using nl80211. >> >>> Now I know that you think we can add a new nl80211 command to help >>> drivers to fix the TX bitrate if necessary. If adding another nl80211 >>> command for that is acceptable, I can work on this. But I need >>> Johannes's comment if it's better to add a new nl80211 command or to >>> expand the existing command (ex. > NL80211_CMD_SET_TX_BITRATE_MASK). >> >> _Why_ is NL80211_CMD_SET_TX_BITRATE_MASK not suitable for you? You > keep >> saying that but I have still figured out why exactly you think so. >> Please clarify this in detail. I think I've talked about it in my previous mail, see above. This command just mask out some of rates that are not allowed. But the firmware has its own rate adaptive mechanism to choose the rates. So mask out all of the other rate doesn't make sure the packets will be transmitted by the only rate that was not masked. The hardware/firmware will try to choose a better rate (ex. 1Mbps or 6Mbps) if they think it's necessary. Also the device will fallback the rates to try to find a better rate to transfer data to the peer. >> >>> It looks like that adding a new nl80211 command will be better for me >>> as expanding the existing one would have great impact on the already >>> distributed drivers/user-tools. >> >> What kind of great impact are you talking about? Please be specific so >> that we don't need to guess. We probably have to modify the command parser, from user-space and the nl80211 domain, because as far I don't see a good way to add fix rate option on the NL80211_CMD_SET_TX_BITRATE_MASK without changing the existing mechanism. If the mechanism is changed, then the "old" drivers will fail to interpret the nl80211 attributes. So I think add a new one, which can fix the TX rate, disable the rate adaptive, etc., will be better if necessary. > > At least with ath10k, the issues I found were that nl80211 doesn't like it > when you try to disable all legacy rates (and force frames out at 54Mbps > encoding, for instance). > > I'm not even sure upstream ath10k will even let you set a single rate > using normal API now. Have you tried it? > > Another problem is that to keep a connection alive, you probably want mgt > and null-func frames to go out normal and only have the firmware use a > particular MCS > for data frames. > > Lots of reasons to want a low-level hack for this sort of thing. Thank you for point them out. Control the TX rate is really important when debugging with issues on the field, especially when the air is noisy and the rate adaptive mechanism is not working well. Because usually the device tries to fallback the rate for stability, when lower bitrate can make the peer have a higher opportunity to successfully receive the packet. When the rate fallbacks, the rate is not fixed at all. And when we want a rate that is "fixed", we don't want another rate appear in the air. > > Thanks, > Ben > > -- > Ben Greear <greearb@candelatech.com> > Candela Technologies Inc http://www.candelatech.com > Yen-Hsuan
On Wed, 2020-03-18 at 09:02 +0000, Tony Chuang wrote: > > This command just mask out some of rates that are not allowed. But the > firmware has its own rate adaptive mechanism to choose the rates. So mask > out all of the other rate doesn't make sure the packets will be transmitted by > the only rate that was not masked. The hardware/firmware will try to choose > a better rate (ex. 1Mbps or 6Mbps) if they think it's necessary. Also the device > will fallback the rates to try to find a better rate to transfer data to the peer. [...] > We probably have to modify the command parser, from user-space and the > nl80211 domain, because as far I don't see a good way to add fix rate > option on the NL80211_CMD_SET_TX_BITRATE_MASK without changing > the existing mechanism. If the mechanism is changed, then the "old" drivers > will fail to interpret the nl80211 attributes. So I think add a new one, which > can fix the TX rate, disable the rate adaptive, etc., will be better if necessary. IMHO we should consider the use case here. _Why_ do you need something like this? Brian can probably comment on this - I think ChromeOS (used to) use(s) some kind of fixed rate at the beginning of the connection to force low rates? But I also remember this interacting badly with some APs that just don't want to enable low rates at all... I think we also have a similar debugfs entry in iwlwifi which literally forces a single rate/configuration (including antenna) for the device to use, to test certain things. I'm not convinced that it'd be easy and would make a lot of sense to add support for all these kinds of knobs to nl80211 since they're really just used in limited testing scenarios. So IMHO the "can this be put into nl80211" isn't necessarily the most important thing - we don't *have* to clutter that with various knobs that are only supported by some drivers, and then only used for testing ... johannes
[ To be clear, I haven't asked for this debugfs knob, and as of now, there is no plan for Chrome OS to use $subject feature. Per some of Tony's descriptions, I suppose maybe this would be useful for certain debugging scenarios, but only that -- no intention of wiring this up "in production." IIUC, he CC'd me only because of the "measuring the TX power" portion of the commit message. ] On Fri, Mar 20, 2020 at 6:06 AM Johannes Berg <johannes@sipsolutions.net> wrote: > Brian can probably comment on this - I think ChromeOS (used to) use(s) > some kind of fixed rate at the beginning of the connection to force low > rates? But I also remember this interacting badly with some APs that > just don't want to enable low rates at all... Regarding how Chrome OS used NL80211_CMD_SET_TX_BITRATE_MASK: we used to use this during authentication, association, DHCP, etc., until we determined the connection was "established" -- the goal was to enforce low (and ostensibly "more reliable") bitrates initially, so we get the important stuff done, even in the presence of wacky rate control algorithms. I understand this was actually added years ago mainly because ath9k had poor rate control. Notably, this feature applies (or, is supposed to apply) to both management and data frames. As Johannes noted, masking off these rates caused problems of its own, especially when APs (esp., guided by (mis?)guided I.T. admins who think that low bitrates are evil) removed these low bitrates from their SupportedRates field. Apparently these APs may start to reject clients if they don't obey. Additionally, we found no evidence that forcing low bitrates like this was substantially helpful for anything other than older ath9k systems. So longer story shorter, Chrome OS does not use NL80211_CMD_SET_TX_BITRATE_MASK any more. One is free to improve/extend the NL80211_CMD_SET_TX_BITRATE_MASK command if desired, of course, but I think some of the earlier complaints are valid (and line up with some of the problems I note above): the use case for $subject does not necessarily involve setting rates for management frames -- only data. One could always add extra options to this command to reflect all the different ways this might get used, but I'm not sure if that's worth it, for a feature that has no non-debug use case. One could also argue that, if iwlwifi already has a debugfs knob (looks like rs_sta_dbgfs_scale_table_write()?), rtw88 should be able to have one too ;) Regards, Brian
Brian Norris <briannorris@chromium.org> writes: > [ To be clear, I haven't asked for this debugfs knob, and as of now, > there is no plan for Chrome OS to use $subject feature. Per some of > Tony's descriptions, I suppose maybe this would be useful for certain > debugging scenarios, but only that -- no intention of wiring this up > "in production." IIUC, he CC'd me only because of the "measuring the > TX power" portion of the commit message. ] > > On Fri, Mar 20, 2020 at 6:06 AM Johannes Berg <johannes@sipsolutions.net> > wrote: > > Brian can probably comment on this - I think ChromeOS (used to) use(s) > > some kind of fixed rate at the beginning of the connection to force low > > rates? But I also remember this interacting badly with some APs that > > just don't want to enable low rates at all... > [...] > > As Johannes noted, masking off these rates caused problems of its own, > especially when APs (esp., guided by (mis?)guided I.T. admins who > think that low bitrates are evil) removed these low bitrates from > their SupportedRates field. Apparently these APs may start to reject > clients if they don't obey. > > Additionally, we found no evidence that forcing low bitrates like this > was substantially helpful for anything other than older ath9k systems. > So longer story shorter, Chrome OS does not use > NL80211_CMD_SET_TX_BITRATE_MASK any more. > We want to measure the TX power, and the equipment just cannot detect the signal on some rates, unless we "fix" the rate exactly. So NL80211_CMD_SET_TX_BITRATE_MASK is not so useful for us sometimes. Also we wanted to see not only the TX power, but the signal quality of certain modulations/coding rates, and the equipment still tends to receive fixed rates. [...] > > One could also argue that, if iwlwifi already has a debugfs knob > (looks like rs_sta_dbgfs_scale_table_write()?), rtw88 should be able > to have one too ;) > > Regards, > Brian > Yen-Hsuan
On Tue, Mar 24, 2020 at 7:55 PM Tony Chuang <yhchuang@realtek.com> wrote: > Brian Norris <briannorris@chromium.org> writes: > We want to measure the TX power, and the equipment just cannot > detect the signal on some rates, unless we "fix" the rate exactly. I think we all understand this now. > So NL80211_CMD_SET_TX_BITRATE_MASK is not so useful for us > sometimes. I'm not sure if you have directly explained why this is the case. See your comment earlier: "This command just mask out some of rates that are not allowed." Sure, but if you mask out all but 1 bitrate...voila! A fixed rate! And this: "But actually the firmware has its own rate adaptive mechanism, so mask out the other rates does not mean the rate left will be chosen." That's entirely your fault, not the fault of the API. If your firmware doesn't listen to your driver, then that's either your firmware or your driver's fault. If you set a mask that has exactly 1 bitrate in it... well, that's exactly what you should tell your firmware to do. Not, "use this 1 bitrate, but try something else if you feel like it." Now, there are other problems, like the others that Ben mentioned: the rest of the mac80211 framework doesn't like it too much if you really disable all but 1 rate (arguably a mac80211 bug -- but not a nl80211 bug); or maybe you want to differentiate management frames and data frames (and that's not what the current NL80211_CMD_SET_TX_BITRATE_MASK allows for). I'm still not (personally) expecting that you *must* do this all via the existing CMD_SET_TX_BITRATE_MASK, but to satisfy everyone involved here, you at least need to be clear about why you aren't. Regards, Brian
Brian Norris <briannorris@chromium.org> writes: > > On Tue, Mar 24, 2020 at 7:55 PM Tony Chuang <yhchuang@realtek.com> > wrote: > > Brian Norris <briannorris@chromium.org> writes: > > We want to measure the TX power, and the equipment just cannot > > detect the signal on some rates, unless we "fix" the rate exactly. > > I think we all understand this now. > > > So NL80211_CMD_SET_TX_BITRATE_MASK is not so useful for us > > sometimes. > > I'm not sure if you have directly explained why this is the case. See > your comment earlier: > > "This command just mask out some of rates that are not allowed." > > Sure, but if you mask out all but 1 bitrate...voila! A fixed rate! > > And this: > > "But actually the firmware has its own rate > adaptive mechanism, so mask out the other rates does not mean the rate > left will be chosen." > > That's entirely your fault, not the fault of the API. If your firmware > doesn't listen to your driver, then that's either your firmware or > your driver's fault. If you set a mask that has exactly 1 bitrate in > it... well, that's exactly what you should tell your firmware to do. > Not, "use this 1 bitrate, but try something else if you feel like it." I cannot agree with it. Let's be clear here: If there's a rate mask comes from upper space, does it mean That driver or firmware/hardware can only use those rates masked when *802.11 retry*? And use a lower rate when retry is called rate-fallback as I've mentioned before. So I think the problem here is, do we need another option in the existing nl80211 command, if 802.11 *retry* is still allowed to choose a rate not in the rate mask? In my opinion, if 802.11 retry should be disabled, then all of the algorithm of the existing rate adaptive mechanism for rtw88 should be totally re-designed and we will have to rebuild another one. > > Now, there are other problems, like the others that Ben mentioned: the > rest of the mac80211 framework doesn't like it too much if you really > disable all but 1 rate (arguably a mac80211 bug -- but not a nl80211 > bug); or maybe you want to differentiate management frames and data > frames (and that's not what the current > NL80211_CMD_SET_TX_BITRATE_MASK allows for). > > I'm still not (personally) expecting that you *must* do this all via > the existing CMD_SET_TX_BITRATE_MASK, but to satisfy everyone involved > here, you at least need to be clear about why you aren't. > Yen-Hsuan
On Wed, 2020-03-25 at 05:54 +0000, Tony Chuang wrote: > > That's entirely your fault, not the fault of the API. If your firmware > > doesn't listen to your driver, then that's either your firmware or > > your driver's fault. If you set a mask that has exactly 1 bitrate in > > it... well, that's exactly what you should tell your firmware to do. > > Not, "use this 1 bitrate, but try something else if you feel like it." > > I cannot agree with it. I tend to agree with Brian. If userspace, for some reason, told you that only one rate is acceptable, then you should listen to that - if you support the API at all. > Let's be clear here: > > If there's a rate mask comes from upper space, does it mean > That driver or firmware/hardware can only use those rates > masked when *802.11 retry*? Yes. > And use a lower rate when > retry is called rate-fallback as I've mentioned before. So I > think the problem here is, do we need another option in the > existing nl80211 command, if 802.11 *retry* is still allowed to > choose a rate not in the rate mask? Perhaps you'd like to have such an extension to the API, but that's not what's there today. Today, the expectation is that you use these rates, not some other rates you figured out from something. Now, mac80211 is/was actually slightly buggy here at some point, and not all drivers support this, but that's what the API was intended for. Not some kind of "oh, let's try to start with these rates and then do whatever we like later". > In my opinion, if 802.11 > retry should be disabled, then all of the algorithm of the existing > rate adaptive mechanism for rtw88 should be totally re-designed > and we will have to rebuild another one. Disabling retries has nothing to do with it. Only limiting the rates that can be used (even for retries). johannes
On 03/24/2020 10:16 PM, Brian Norris wrote: > On Tue, Mar 24, 2020 at 7:55 PM Tony Chuang <yhchuang@realtek.com> wrote: >> Brian Norris <briannorris@chromium.org> writes: >> We want to measure the TX power, and the equipment just cannot >> detect the signal on some rates, unless we "fix" the rate exactly. > > I think we all understand this now. > >> So NL80211_CMD_SET_TX_BITRATE_MASK is not so useful for us >> sometimes. > > I'm not sure if you have directly explained why this is the case. See > your comment earlier: > > "This command just mask out some of rates that are not allowed." > > Sure, but if you mask out all but 1 bitrate...voila! A fixed rate! So, see this thread from a while back. Has anyone even *tried* to use this API you are proposing? http://lists.infradead.org/pipermail/ath10k/2017-October/010291.html Here is the fix I have had in my tree for a few years to let ath10k actually set a single rate: [greearb@ben-dt4 linux-5.4.dev.y]$ git show cccf04cc3440ddee0760249da51026bf2532f926 commit cccf04cc3440ddee0760249da51026bf2532f926 Author: Ben Greear <greearb@candelatech.com> Date: Tue Oct 10 13:56:29 2017 -0700 mac80211: Revert some of e8e4f5, fixes setting single rate in ath10k. This lets us successfully set a single rate in ath10k again. Signed-off-by: Ben Greear <greearb@candelatech.com> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 78cf453cda2c..3f248ad70805 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -2886,8 +2886,10 @@ static int ieee80211_set_bitrate_mask(struct wiphy *wiphy, u32 basic_rates = sdata->vif.bss_conf.basic_rates; enum nl80211_band band = sdata->vif.bss_conf.chandef.chan->band; - if (!(mask->control[band].legacy & basic_rates)) - return -EINVAL; + if (!(mask->control[band].legacy & basic_rates)) { + pr_err("%s: WARNING: no legacy rates for band[%d] in set-bitrate-mask.\n", + sdata->dev->name, band); + } } if (ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)) { Thanks, Ben > > And this: > > "But actually the firmware has its own rate > adaptive mechanism, so mask out the other rates does not mean the rate > left will be chosen." > > That's entirely your fault, not the fault of the API. If your firmware > doesn't listen to your driver, then that's either your firmware or > your driver's fault. If you set a mask that has exactly 1 bitrate in > it... well, that's exactly what you should tell your firmware to do. > Not, "use this 1 bitrate, but try something else if you feel like it." > > Now, there are other problems, like the others that Ben mentioned: the > rest of the mac80211 framework doesn't like it too much if you really > disable all but 1 rate (arguably a mac80211 bug -- but not a nl80211 > bug); or maybe you want to differentiate management frames and data > frames (and that's not what the current > NL80211_CMD_SET_TX_BITRATE_MASK allows for). > > I'm still not (personally) expecting that you *must* do this all via > the existing CMD_SET_TX_BITRATE_MASK, but to satisfy everyone involved > here, you at least need to be clear about why you aren't. > > Regards, > Brian >
On Wed, Mar 25, 2020 at 8:52 AM Ben Greear <greearb@candelatech.com> wrote: > On 03/24/2020 10:16 PM, Brian Norris wrote: > > Sure, but if you mask out all but 1 bitrate...voila! A fixed rate! > > So, see this thread from a while back. Has anyone even *tried* to use > this API you are proposing? Yes, in fact, I have! Which is why I noted: > > Now, there are other problems, like the others that Ben mentioned: the > > rest of the mac80211 framework doesn't like it too much if you really > > disable all but 1 rate (arguably a mac80211 bug -- but not a nl80211 > > bug) > http://lists.infradead.org/pipermail/ath10k/2017-October/010291.html I hadn't seen that thread. So it sounds like maybe Johannes isn't quite on the same page as Johannes ;) If we're going to be particular about matching the AP's basic rates, then this API is indeed probably not useful for the "single fixed rate [for debugging/testing]" use case. > mac80211: Revert some of e8e4f5, fixes setting single rate in ath10k. Commit e8e4f5 was an unfortunate consequence of the stuff I mentioned earlier about how Chrome OS used to use SET_TX_BITRATE_MAX -- we weren't nuanced about it at all, so we might configure a set of bitrates that doesn't intersect at all with the AP's BasicRates. That does make it hard for the driver/framework to decide what to do: do we listen to the user, or to the AP? Incidentally, that's also one reason why Chrome OS no longer uses the API; it was too big of a hammer for what we want (initial-connection reliability), and required us to be more delicate about {Supported,Basic}Rates than we really wanted to. Brian
I know there's other discussion on this patch, related to nl80211 vs. debugfs, but in case the discussion ends up permitting debugfs, I'll put my review notes in. Reviewed-by: Brian Norris <briannorris@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> On Thu, Mar 12, 2020 at 11:51 PM <yhchuang@realtek.com> wrote: > +static ssize_t rtw_debugfs_set_fix_rate(struct file *filp, > + const char __user *buffer, > + size_t count, loff_t *loff) > +{ > + struct seq_file *seqpriv = (struct seq_file *)filp->private_data; > + struct rtw_debugfs_priv *debugfs_priv = seqpriv->private; > + struct rtw_dev *rtwdev = debugfs_priv->rtwdev; > + struct rtw_dm_info *dm_info = &rtwdev->dm_info; > + u8 fix_rate; > + char tmp[32 + 1]; > + int ret; > + > + rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 1); > + > + ret = kstrtou8(tmp, 0, &fix_rate); > + if (ret) { > + rtw_warn(rtwdev, "invalid args, [rate]\n"); Seems like you don't actually need this print, as it doesn't provide any additional data that you don't get through the return code. People interacting with this debugfs interface will already see things like "write error: Invalid argument" if they're handling write() errors appropriately. > + return ret; > + } > + > + dm_info->fix_rate = fix_rate; It feels like you should do some real bounds checking here; as-is, you're allowing anything in [DESC_RATE_MAX..0xff] to mean "disabled", whereas it seems much more reasonable to specify a single value that means "disabled". So you could do: if (fix_rate >= DESC_RATE_MAX && fix_rate != U8_MAX) return -EINVAL; Brian > + > + return count; > +} > +
On Wed, 2020-03-25 at 11:14 -0700, Brian Norris wrote: > On Wed, Mar 25, 2020 at 8:52 AM Ben Greear <greearb@candelatech.com> wrote: > > On 03/24/2020 10:16 PM, Brian Norris wrote: > > > Sure, but if you mask out all but 1 bitrate...voila! A fixed rate! > > > > So, see this thread from a while back. Has anyone even *tried* to use > > this API you are proposing? > > Yes, in fact, I have! Which is why I noted: > > > > Now, there are other problems, like the others that Ben mentioned: the > > > rest of the mac80211 framework doesn't like it too much if you really > > > disable all but 1 rate (arguably a mac80211 bug -- but not a nl80211 > > > bug) > > http://lists.infradead.org/pipermail/ath10k/2017-October/010291.html > > I hadn't seen that thread. So it sounds like maybe Johannes isn't > quite on the same page as Johannes ;) Hah. Happens all the time, not sure what that other Johannes is thinking ;-) More seriously though, I'm a bit lost now (and a big part of that is probably because I'm replying to a 2 months old thread, sorry). > If we're going to be particular about matching the AP's basic rates, > then this API is indeed probably not useful for the "single fixed rate > [for debugging/testing]" use case. > > > mac80211: Revert some of e8e4f5, fixes setting single rate in ath10k. > > Commit e8e4f5 was an unfortunate consequence of the stuff I mentioned > earlier about how Chrome OS used to use SET_TX_BITRATE_MAX -- we > weren't nuanced about it at all, so we might configure a set of > bitrates that doesn't intersect at all with the AP's BasicRates. That > does make it hard for the driver/framework to decide what to do: do we > listen to the user, or to the AP? Incidentally, that's also one reason > why Chrome OS no longer uses the API; it was too big of a hammer for > what we want (initial-connection reliability), and required us to be > more delicate about {Supported,Basic}Rates than we really wanted to. Right. But I'm not sure why Ben needs to just do a pr_err()? Shouldn't that just not happen? Hmm. johannes
On 05/25/2020 02:07 AM, Johannes Berg wrote: > On Wed, 2020-03-25 at 11:14 -0700, Brian Norris wrote: >> On Wed, Mar 25, 2020 at 8:52 AM Ben Greear <greearb@candelatech.com> wrote: >>> On 03/24/2020 10:16 PM, Brian Norris wrote: >>>> Sure, but if you mask out all but 1 bitrate...voila! A fixed rate! >>> >>> So, see this thread from a while back. Has anyone even *tried* to use >>> this API you are proposing? >> >> Yes, in fact, I have! Which is why I noted: >> >>>> Now, there are other problems, like the others that Ben mentioned: the >>>> rest of the mac80211 framework doesn't like it too much if you really >>>> disable all but 1 rate (arguably a mac80211 bug -- but not a nl80211 >>>> bug) >>> http://lists.infradead.org/pipermail/ath10k/2017-October/010291.html >> >> I hadn't seen that thread. So it sounds like maybe Johannes isn't >> quite on the same page as Johannes ;) > > Hah. Happens all the time, not sure what that other Johannes is thinking > ;-) > > More seriously though, I'm a bit lost now (and a big part of that is > probably because I'm replying to a 2 months old thread, sorry). > >> If we're going to be particular about matching the AP's basic rates, >> then this API is indeed probably not useful for the "single fixed rate >> [for debugging/testing]" use case. >> >>> mac80211: Revert some of e8e4f5, fixes setting single rate in ath10k. >> >> Commit e8e4f5 was an unfortunate consequence of the stuff I mentioned >> earlier about how Chrome OS used to use SET_TX_BITRATE_MAX -- we >> weren't nuanced about it at all, so we might configure a set of >> bitrates that doesn't intersect at all with the AP's BasicRates. That >> does make it hard for the driver/framework to decide what to do: do we >> listen to the user, or to the AP? Incidentally, that's also one reason >> why Chrome OS no longer uses the API; it was too big of a hammer for >> what we want (initial-connection reliability), and required us to be >> more delicate about {Supported,Basic}Rates than we really wanted to. > > Right. > > But I'm not sure why Ben needs to just do a pr_err()? Shouldn't that > just not happen? Try setting a single rate on ath10k. It will not work unless you add my patch. I put a pr_err in there because you (or whoever pushed the regression upstream) thought it was a problem to set a single rate, so I wanted to warn the user but also let the action work. Thanks, Ben > > Hmm. > > johannes >
<yhchuang@realtek.com> wrote: > From: Yan-Hsuan Chuang <yhchuang@realtek.com> > > It is useful to fix the bit rate of TX packets. For example, if > someone is measuring the TX power, or debugging with the issues > of the TX throughput on the field. > > To set the value of fixed rate, one should input corresponding > desc rate index (ex, 0x0b for DESC_RATE54M to fix at 54 Mbps). > Set a value larger than DESC_RATE_MAX will disable fix rate, so > the rate adaptive mechanism can resume to work. > > Example, > To fix rate at MCS 1: > echo 0x0d > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > To not to fix rate: > echo 0xff > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > To know which rate was fixed at: > cat /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > Reviewed-by: Brian Norris <briannorris@chromium.org> > Tested-by: Brian Norris <briannorris@chromium.org> So the concensus was that doing this from debugfs is ok, but unfortunately this doesn't apply anymore. Please respin. Recorded preimage for 'drivers/net/wireless/realtek/rtw88/tx.c' error: Failed to merge in the changes. hint: Use 'git am --show-current-patch' to see the failed patch Applying: rtw88: add debugfs to fix tx rate Using index info to reconstruct a base tree... M drivers/net/wireless/realtek/rtw88/debug.c M drivers/net/wireless/realtek/rtw88/main.c M drivers/net/wireless/realtek/rtw88/main.h M drivers/net/wireless/realtek/rtw88/tx.c Falling back to patching base and 3-way merge... Auto-merging drivers/net/wireless/realtek/rtw88/tx.c CONFLICT (content): Merge conflict in drivers/net/wireless/realtek/rtw88/tx.c Auto-merging drivers/net/wireless/realtek/rtw88/main.h Auto-merging drivers/net/wireless/realtek/rtw88/main.c Auto-merging drivers/net/wireless/realtek/rtw88/debug.c Patch failed at 0001 rtw88: add debugfs to fix tx rate Patch set to Changes Requested.
> -----Original Message----- > From: kvalo=codeaurora.org@mg.codeaurora.org <kvalo=codeaurora.org@mg.codeaurora.org> On Behalf Of Kalle > Valo > Sent: Saturday, November 27, 2021 12:20 AM > To: yhchuang@realtek.com > Cc: linux-wireless@vger.kernel.org; briannorris@chromium.org > Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate > > <yhchuang@realtek.com> wrote: > > > From: Yan-Hsuan Chuang <yhchuang@realtek.com> > > > > It is useful to fix the bit rate of TX packets. For example, if > > someone is measuring the TX power, or debugging with the issues > > of the TX throughput on the field. > > > > To set the value of fixed rate, one should input corresponding > > desc rate index (ex, 0x0b for DESC_RATE54M to fix at 54 Mbps). > > Set a value larger than DESC_RATE_MAX will disable fix rate, so > > the rate adaptive mechanism can resume to work. > > > > Example, > > To fix rate at MCS 1: > > echo 0x0d > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > > > To not to fix rate: > > echo 0xff > /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > > > To know which rate was fixed at: > > cat /sys/kernel/debug/ieee80211/phy0/rtw88/fix_rate > > > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > > Reviewed-by: Brian Norris <briannorris@chromium.org> > > Tested-by: Brian Norris <briannorris@chromium.org> > > So the concensus was that doing this from debugfs is ok, but > unfortunately this doesn't apply anymore. Please respin. > > Recorded preimage for 'drivers/net/wireless/realtek/rtw88/tx.c' > error: Failed to merge in the changes. > hint: Use 'git am --show-current-patch' to see the failed patch > Applying: rtw88: add debugfs to fix tx rate > Using index info to reconstruct a base tree... > M drivers/net/wireless/realtek/rtw88/debug.c > M drivers/net/wireless/realtek/rtw88/main.c > M drivers/net/wireless/realtek/rtw88/main.h > M drivers/net/wireless/realtek/rtw88/tx.c > Falling back to patching base and 3-way merge... > Auto-merging drivers/net/wireless/realtek/rtw88/tx.c > CONFLICT (content): Merge conflict in drivers/net/wireless/realtek/rtw88/tx.c > Auto-merging drivers/net/wireless/realtek/rtw88/main.h > Auto-merging drivers/net/wireless/realtek/rtw88/main.c > Auto-merging drivers/net/wireless/realtek/rtw88/debug.c > Patch failed at 0001 rtw88: add debugfs to fix tx rate > > Patch set to Changes Requested. > I have done rebase and sent out. Thank you [1] https://lore.kernel.org/linux-wireless/20211129020506.6273-1-pkshih@realtek.com/T/#u -- Ping-Ke
diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c index 5a181e01ebef..72e8877fd5f2 100644 --- a/drivers/net/wireless/realtek/rtw88/debug.c +++ b/drivers/net/wireless/realtek/rtw88/debug.c @@ -140,6 +140,22 @@ static int rtw_debugfs_get_rf_read(struct seq_file *m, void *v) return 0; } +static int rtw_debugfs_get_fix_rate(struct seq_file *m, void *v) +{ + struct rtw_debugfs_priv *debugfs_priv = m->private; + struct rtw_dev *rtwdev = debugfs_priv->rtwdev; + struct rtw_dm_info *dm_info = &rtwdev->dm_info; + u8 fix_rate = dm_info->fix_rate; + + if (fix_rate >= DESC_RATE_MAX) { + seq_printf(m, "Fix rate disabled, fix_rate = %u\n", fix_rate); + return 0; + } + + seq_printf(m, "Data frames fixed at desc rate %u\n", fix_rate); + return 0; +} + static int rtw_debugfs_copy_from_user(char tmp[], int size, const char __user *buffer, size_t count, int num) @@ -397,6 +413,31 @@ static ssize_t rtw_debugfs_set_rf_read(struct file *filp, return count; } +static ssize_t rtw_debugfs_set_fix_rate(struct file *filp, + const char __user *buffer, + size_t count, loff_t *loff) +{ + struct seq_file *seqpriv = (struct seq_file *)filp->private_data; + struct rtw_debugfs_priv *debugfs_priv = seqpriv->private; + struct rtw_dev *rtwdev = debugfs_priv->rtwdev; + struct rtw_dm_info *dm_info = &rtwdev->dm_info; + u8 fix_rate; + char tmp[32 + 1]; + int ret; + + rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 1); + + ret = kstrtou8(tmp, 0, &fix_rate); + if (ret) { + rtw_warn(rtwdev, "invalid args, [rate]\n"); + return ret; + } + + dm_info->fix_rate = fix_rate; + + return count; +} + static int rtw_debug_get_mac_page(struct seq_file *m, void *v) { struct rtw_debugfs_priv *debugfs_priv = m->private; @@ -770,6 +811,11 @@ static struct rtw_debugfs_priv rtw_debug_priv_read_reg = { .cb_read = rtw_debugfs_get_read_reg, }; +static struct rtw_debugfs_priv rtw_debug_priv_fix_rate = { + .cb_write = rtw_debugfs_set_fix_rate, + .cb_read = rtw_debugfs_get_fix_rate, +}; + static struct rtw_debugfs_priv rtw_debug_priv_dump_cam = { .cb_write = rtw_debugfs_set_single_input, .cb_read = rtw_debugfs_get_dump_cam, @@ -811,6 +857,7 @@ void rtw_debugfs_init(struct rtw_dev *rtwdev) rtw_debugfs_add_rw(read_reg); rtw_debugfs_add_w(rf_write); rtw_debugfs_add_rw(rf_read); + rtw_debugfs_add_rw(fix_rate); rtw_debugfs_add_rw(dump_cam); rtw_debugfs_add_rw(rsvd_page); rtw_debugfs_add_r(phy_info); diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c index 2f73820cd9ba..ac49142cc4d7 100644 --- a/drivers/net/wireless/realtek/rtw88/main.c +++ b/drivers/net/wireless/realtek/rtw88/main.c @@ -1394,6 +1394,7 @@ int rtw_core_init(struct rtw_dev *rtwdev) rtwdev->sec.total_cam_num = 32; rtwdev->hal.current_channel = 1; + rtwdev->dm_info.fix_rate = U8_MAX; set_bit(RTW_BC_MC_MACID, rtwdev->mac_id_map); if (!(BIT(rtw_fw_lps_deep_mode) & chip->lps_deep_mode_supported)) rtwdev->lps_conf.deep_mode = LPS_DEEP_MODE_NONE; diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h index c074cef22120..69ec6bf63b8b 100644 --- a/drivers/net/wireless/realtek/rtw88/main.h +++ b/drivers/net/wireless/realtek/rtw88/main.h @@ -1383,6 +1383,7 @@ struct rtw_dm_info { u8 cck_gi_u_bnd; u8 cck_gi_l_bnd; + u8 fix_rate; u8 tx_rate; u8 thermal_avg[RTW_RF_PATH_MAX]; u8 thermal_meter_k; diff --git a/drivers/net/wireless/realtek/rtw88/tx.c b/drivers/net/wireless/realtek/rtw88/tx.c index 24c39c60c99a..5d2c59d2c5a5 100644 --- a/drivers/net/wireless/realtek/rtw88/tx.c +++ b/drivers/net/wireless/realtek/rtw88/tx.c @@ -236,7 +236,9 @@ static void rtw_tx_data_pkt_info_update(struct rtw_dev *rtwdev, struct ieee80211_sta *sta = control->sta; struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data; struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); + struct rtw_dm_info *dm_info = &rtwdev->dm_info; struct rtw_sta_info *si; + u8 fix_rate; u16 seq; u8 ampdu_factor = 0; u8 ampdu_density = 0; @@ -288,6 +290,13 @@ static void rtw_tx_data_pkt_info_update(struct rtw_dev *rtwdev, pkt_info->bw = bw; pkt_info->stbc = stbc; pkt_info->ldpc = ldpc; + + fix_rate = dm_info->fix_rate; + if (fix_rate < DESC_RATE_MAX) { + pkt_info->rate = fix_rate; + pkt_info->dis_rate_fallback = true; + pkt_info->use_rate = true; + } } void rtw_tx_pkt_info_update(struct rtw_dev *rtwdev,