LinuxLists.cc - [PATCH] rtw88: add debugfs to fix tx rate

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

<[email protected]> writes:

> From: Yan-Hsuan Chuang <[email protected]>
>
> 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 <[email protected]>

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?

--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2020-03-16 02:30:25

[permalink] [raw]

Subject: RE: [PATCH] rtw88: add debugfs to fix tx rate

Kalle Valo <[email protected]> writes:

> <[email protected]> writes:
>
> > From: Yan-Hsuan Chuang <[email protected]>
> >
> > 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 <[email protected]>
>
> 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

2020-03-17 07:13:13

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

Tony Chuang <[email protected]> writes:

> Kalle Valo <[email protected]> writes:
>
>> <[email protected]> writes:
>>
>> > From: Yan-Hsuan Chuang <[email protected]>
>> >
>> > 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 <[email protected]>
>>
>> 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.

--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2020-03-17 10:35:18

[permalink] [raw]

Subject: RE: [PATCH] rtw88: add debugfs to fix tx rate

// Add Johannes for commenting on adding another nl80211 commands

Kalle Valo <[email protected]> writes:>

> Tony Chuang <[email protected]> writes:
>
> > Kalle Valo <[email protected]> writes:
> >
> >> <[email protected]> writes:
> >>
> >> > From: Yan-Hsuan Chuang <[email protected]>
> >> >
> >> > 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 <[email protected]>
> >>
> >> 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

2020-03-17 15:44:33

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

Tony Chuang <[email protected]> writes:

> // Add Johannes for commenting on adding another nl80211 commands
>
> Kalle Valo <[email protected]> writes:>
>
>> Tony Chuang <[email protected]> writes:
>>
>> > Kalle Valo <[email protected]> writes:
>> >
>> >> <[email protected]> writes:
>> >>
>> >> > From: Yan-Hsuan Chuang <[email protected]>
>> >> >
>> >> > 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 <[email protected]>
>> >>
>> >> 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.

--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2020-03-17 15:50:50

by Ben Greear

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

On 3/17/20 8:40 AM, Kalle Valo wrote:
> Tony Chuang <[email protected]> writes:
>
>> // Add Johannes for commenting on adding another nl80211 commands
>>
>> Kalle Valo <[email protected]> writes:>
>>
>>> Tony Chuang <[email protected]> writes:
>>>
>>>> Kalle Valo <[email protected]> writes:
>>>>
>>>>> <[email protected]> writes:
>>>>>
>>>>>> From: Yan-Hsuan Chuang <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com

2020-03-18 09:05:23

[permalink] [raw]

Subject: RE: [PATCH] rtw88: add debugfs to fix tx rate

Ben Greear <[email protected]> writes:

> On 3/17/20 8:40 AM, Kalle Valo wrote:
>> Tony Chuang <[email protected]> writes:
>>
>>> // Add Johannes for commenting on adding another nl80211 commands
>>>
>>> Kalle Valo <[email protected]> writes:>
>>>
>>>> Tony Chuang <[email protected]> writes:
>>>>
>>>>> Kalle Valo <[email protected]> writes:
>>>>>
>>>>>> <[email protected]> writes:
>>>>>>
>>>>>>> From: Yan-Hsuan Chuang <[email protected]>
>>>>>>>
>>>>>>> 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 <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
> Candela Technologies Inc http://www.candelatech.com
>

Yen-Hsuan

2020-03-20 13:07:14

by Johannes Berg

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

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

2020-03-25 00:06:10

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

[ 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 <[email protected]> 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

2020-03-25 02:56:17

[permalink] [raw]

Subject: RE: [PATCH] rtw88: add debugfs to fix tx rate

Brian Norris <[email protected]> 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 <[email protected]>
> 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

2020-03-25 05:17:28

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

On Tue, Mar 24, 2020 at 7:55 PM Tony Chuang <[email protected]> wrote:
> Brian Norris <[email protected]> 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

2020-03-25 05:55:16

[permalink] [raw]

Subject: RE: [PATCH] rtw88: add debugfs to fix tx rate

Brian Norris <[email protected]> writes:
>
> On Tue, Mar 24, 2020 at 7:55 PM Tony Chuang <[email protected]>
> wrote:
> > Brian Norris <[email protected]> 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

2020-03-25 09:12:30

by Johannes Berg

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

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

2020-03-25 15:52:50

by Ben Greear

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

On 03/24/2020 10:16 PM, Brian Norris wrote:
> On Tue, Mar 24, 2020 at 7:55 PM Tony Chuang <[email protected]> wrote:
>> Brian Norris <[email protected]> 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 <[email protected]>
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 <[email protected]>

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
>

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

2020-03-25 18:15:47

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

On Wed, Mar 25, 2020 at 8:52 AM Ben Greear <[email protected]> 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

2020-03-26 18:28:56

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

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 <[email protected]>
Tested-by: Brian Norris <[email protected]>

On Thu, Mar 12, 2020 at 11:51 PM <[email protected]> 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;
> +}
> +

2020-05-25 09:07:46

by Johannes Berg

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

On Wed, 2020-03-25 at 11:14 -0700, Brian Norris wrote:
> On Wed, Mar 25, 2020 at 8:52 AM Ben Greear <[email protected]> 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

2020-05-25 21:03:34

by Ben Greear

[permalink] [raw]

Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate

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 <[email protected]> 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
>

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

2021-11-26 16:21:38