Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3abb2d127eaa3b76a8f4f34c795ddd6d502ce470.camel@sipsolutions.net>
Subject: Re: [PATCH] wifi: cfg80211: beacon EHT rate support
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Aloka Dixit <quic_alokad@quicinc.com>,
        linux-wireless@vger.kernel.org
Date:   Fri, 31 Mar 2023 11:23:34 +0200
In-Reply-To: <312f1476-b78b-b7f0-26f9-512a7dc2b6b7@quicinc.com>
References: <20230329000902.17992-1-quic_alokad@quicinc.com>
         <d54800ae88be1eb232df7794e741dfc304c3ee9a.camel@sipsolutions.net>
         <312f1476-b78b-b7f0-26f9-512a7dc2b6b7@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-03-30 at 15:40 -0700, Aloka Dixit wrote:
> On 3/30/2023 3:44 AM, Johannes Berg wrote:
> > On Tue, 2023-03-28 at 17:09 -0700, Aloka Dixit wrote:
> >=20
> > > +		switch (wdev->iftype) {
> > > +		case NL80211_IFTYPE_AP:
> > > +			width =3D wdev->u.ap.preset_chandef.width;
> >=20
> > This seems a bit awkward and annoying, it means that we have to keep
> > using the API that sets the preset_chandef first, and it also means it
> > won't work for multi-link.
> >=20
>=20
> Okay, I can do it the same with it is done in he_get_txmcsmap():
> 	chandef =3D wdev_chandef(wdev, link_id);

Right, that should be better.

Though not sure that works already when you're doing START_AP, I'd think
we don't set the chandef before parsing the rate attribute?

Which would mean userspace would have to separately set the beacon rate,
which means even having the whole thing inside start_ap() is pointless?


> > Couldn't you link it with the config in start_ap/join_mesh? And per-lin=
k
> > config in set_tx_bitrate_mask()?
> >=20
>=20
> Please correct me if I'm wrong but ieee80211_set_bitrate_mask() need not=
=20
> be changed for setting the beacon rate in non-MLO case, right?
> Because it does not have anything related to he_mcs currently too.

How is ieee80211_set_bitrate_mask() related, it's in mac80211?

Oh you mean it doesn't need to be changed to support EHT? Looks like to
me that it would have to be changed, and even HE today only works in
non-mac80211 drivers that implement it correctly?

But again set_bitrate_mask() isn't even related to beacon rate?


Regarding beacon rate, it looks like mac80211 _only_ accepts legacy
rates, and then also _only_ in start_ap(), so I'm not sure how the whole
thing even works ...??

> > Also not sure about this at all in parse_tid_conf()?
> >=20
>=20
> Can you please explain this point.

parse_tid_conf() also calls nl80211_parse_tx_bitrate_mask(), and I don't
know how the right channel would be known there, that's all.


johannes