diff mbox series

[v2,1/2] rtl8xxxu: unset the hw capability HAS_RATE_CONTROL

Message ID 20210603120609.58932-2-chris.chiu@canonical.com
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series Make ampdu tx work correctly | expand

Commit Message

Chris Chiu June 3, 2021, 12:06 p.m. UTC
From: Chris Chiu <chris.chiu@canonical.com>

Since AMPDU_AGGREGATION is set so packets will be handed to the
driver with a flag indicating A-MPDU aggregation and device should
be responsible for setting up and starting the TX aggregation with
the AMPDU_TX_START action. The TX aggregation is usually started by
the rate control algorithm so the HAS_RATE_CONTROL has to be unset
for the mac80211 to start BA session by ieee80211_start_tx_ba_session.

The realtek chips tx rate will still be handled by the rate adaptive
mechanism in the underlying firmware which is controlled by the
rate mask H2C command in the driver. Unset HAS_RATE_CONTROL cause
no change for the tx rate control and the TX BA session can be started
by the mac80211 default rate control mechanism.

Signed-off-by: Chris Chiu <chris.chiu@canonical.com>
---

Changelog:
  v2:
   - Revise the commit message to desribe the purpose of the change
     in detail.

 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Johannes Berg June 10, 2021, 8:18 p.m. UTC | #1
Hi Chris,

> Since AMPDU_AGGREGATION is set so packets will be handed to the
> driver with a flag indicating A-MPDU aggregation and device should
> be responsible for setting up and starting the TX aggregation with
> the AMPDU_TX_START action. The TX aggregation is usually started by
> the rate control algorithm so the HAS_RATE_CONTROL has to be unset
> for the mac80211 to start BA session by ieee80211_start_tx_ba_session.
> 
> The realtek chips tx rate will still be handled by the rate adaptive
> mechanism in the underlying firmware which is controlled by the
> rate mask H2C command in the driver. Unset HAS_RATE_CONTROL cause
> no change for the tx rate control and the TX BA session can be started
> by the mac80211 default rate control mechanism.

This seems ... strange, to say the least? You want to run the full
minstrel algorithm just to have it start aggregation sessions at the
beginning?

I really don't think this makes sense, and it's super confusing. It may
also result in things like reporting a TX rate to userspace/other
components that *minstrel* thinks is the best rate, rather than your
driver's implementation, etc.

I suggest you instead just call ieee80211_start_tx_ba_session() at some
appropriate time, maybe copying parts of the logic of
minstrel_aggr_check().

johannes
Chris Chiu June 11, 2021, 2:47 p.m. UTC | #2
On Fri, Jun 11, 2021 at 4:18 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi Chris,
>
> > Since AMPDU_AGGREGATION is set so packets will be handed to the
> > driver with a flag indicating A-MPDU aggregation and device should
> > be responsible for setting up and starting the TX aggregation with
> > the AMPDU_TX_START action. The TX aggregation is usually started by
> > the rate control algorithm so the HAS_RATE_CONTROL has to be unset
> > for the mac80211 to start BA session by ieee80211_start_tx_ba_session.
> >
> > The realtek chips tx rate will still be handled by the rate adaptive
> > mechanism in the underlying firmware which is controlled by the
> > rate mask H2C command in the driver. Unset HAS_RATE_CONTROL cause
> > no change for the tx rate control and the TX BA session can be started
> > by the mac80211 default rate control mechanism.
>
> This seems ... strange, to say the least? You want to run the full
> minstrel algorithm just to have it start aggregation sessions at the
> beginning?
>
> I really don't think this makes sense, and it's super confusing. It may
> also result in things like reporting a TX rate to userspace/other
> components that *minstrel* thinks is the best rate, rather than your
> driver's implementation, etc.
>
> I suggest you instead just call ieee80211_start_tx_ba_session() at some
> appropriate time, maybe copying parts of the logic of
> minstrel_aggr_check().
>
> johannes
>
>
Based on the description in
https://github.com/torvalds/linux/blob/master/net/mac80211/agg-tx.c#L32
to L36, if we set HAS_RATE_CONTROL, which means we don't want the
software rate control (default minstrel), then we will have to deal
with both the rate control and the TX aggregation in the driver, and
the .ampdu_action is not really required. Since the rtl8xxxu driver
doesn't handle the TX aggregation, and the minstrel is the default
rate control (can't even be disabled), that's the reason why I want to
unset the HAS_RATE_CONTROL to make use of the existing mac80211
aggregation handling.

And the minstrel doesn't really take effect for rate selection in HT
mode because most drivers don't provide HT/VHT rates in .bitrates of
the ieee80211_supported_band data structure which is required for
hw->wiphy->bands. The mac80211 API ieee80211_get_tx_rate() will
return 0 when the IEEE80211_TX_RC_MCS is set in rate flags. The tx
rate which is filled in the tx descriptor makes no difference because
the underlying rate selection will be actually controlled by the
controller which we can set rate mask via H2C command. Unless we force
the fixed rate in the TX descriptor, we don't really have to fill the
tx rate. Reporting TX rate of each packet will not depend on the rate
from the minstrel, drivers have to handle it by itself. I'll also try
to address that in my next PATCH series.

Chris
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 9ff09cf7eb62..4cf13d2f86b1 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -6678,7 +6678,6 @@  static int rtl8xxxu_probe(struct usb_interface *interface,
 	/*
 	 * The firmware handles rate control
 	 */
-	ieee80211_hw_set(hw, HAS_RATE_CONTROL);
 	ieee80211_hw_set(hw, AMPDU_AGGREGATION);
 
 	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);