Received-SPF: pass (google.com: domain of linux-wireless+bounces-8166-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <edb8cf11187349003b2d61c8cc71e4bed28c31f2.camel@sipsolutions.net>
Subject: Re: [PATCH v7 12/12] wifi: rtlwifi: Enable the new rtl8192du driver
From: Johannes Berg <johannes@sipsolutions.net>
To: Ping-Ke Shih <pkshih@realtek.com>, Bitterblue Smith
	 <rtl8821cerfe2@gmail.com>, Stefan Lippers-Hollmann <s.l-h@gmx.de>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>, 
 Larry Finger <Larry.Finger@lwfinger.net>, Christian Hewitt
 <chewitt@libreelec.tv>
Date: Tue, 28 May 2024 08:59:05 +0200
In-Reply-To: <ee442840754e4afba8388951f56c5e82@realtek.com>
References: <8805826b-60b9-4026-9509-7d92c3a43577@gmail.com>
	 <5f2da7ee-876a-42fc-8fec-ec5386fa8c26@gmail.com>
	 <0002749a3b584bc39fa18b3137153fdf@realtek.com>
	 <20240527112534.4dbcdf75@mir>
	 <29f850c5-4f61-466f-9a7a-437b05bc8251@gmail.com>
	 <ee442840754e4afba8388951f56c5e82@realtek.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Tue, 2024-05-28 at 02:39 +0000, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> > Johannes suggested that we should set hw->wiphy->retry_{long,short}
> > before ieee80211_register_hw(). So that would go in
> > _rtl_init_mac80211(). That has the added benefit of making the
> > true retry limits visible to userspace ("iw phy").
> >=20
> > The problem is that setting hw->wiphy->retry_{long,short} is
> > not enough. rtlwifi still gets the default retry limits of 4
> > and 7, because ieee80211_register_hw() doesn't set
> > hw->conf.long_frame_max_tx_count (and hw->conf.short_frame_max_tx_count=
).
> > Johannes suggested moving this code from ieee80211_alloc_hw_nm()
> > to ieee80211_register_hw():
> >=20
> >         local->hw.conf.long_frame_max_tx_count =3D wiphy->retry_long;
> >         local->hw.conf.short_frame_max_tx_count =3D wiphy->retry_short;

Right, so I suggested that because then driver changes between alloc and
register can take effect=C2=A0rather than being overwritten, and these are
the retry limits that are actually visible to userspace.

> > I didn't do this yet partly because I don't want to compile
> > the entire kernel, and partly because I'm not sure how to handle
> > the different retry limits for AP/IBSS mode and station mode.
> >=20
> > Can we change hw->wiphy->retry_{long,short} any time, not just
> > before ieee80211_register_hw()? If yes, what is even the point
> > of hw->conf.{short,long}_frame_max_tx_count ? It would be simpler
> > if we can ignore them and use hw->wiphy->retry_{long,short}
> > directly.
> >=20
> > What do y'all think?
>=20
> Logically I think you can change hw->wiphy->retry_{long,short} any time,
> because cfg80211/mac80211 seemingly just bypass the values to driver.=20

Not quite, I think there's some additional layer of thing in mac80211?

> But we can't know if user space has set the value, right?

You can't check from the value, but that's why I said to change the
defaults before registering etc.

> One thing is that should we honor the values set by user space?
>=20
> If user space has not set, driver wants to control this value by itself
> according to AP/IBSS/station modes.
> If user space has set, driver fully follows the value from user space.
> Is above the behavior you want?

Honestly, is it? I think most drivers these days probably ignore the
values from userspace for lesser reasons (e.g. iwlwifi always has a
firmware-internal limit of 16 IIRC.)

This API also comes from the dawn of wireless in Linux, and was just
ported from wireless extensions to nl80211. So I wouldn't feel too bad
about simply ignoring it either.

johannes