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: <1aa6178f-2041-797e-fcb8-78fe2c08de24@quicinc.com>
Date:   Fri, 31 Mar 2023 10:55:14 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH] wifi: cfg80211: beacon EHT rate support
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>,
        <linux-wireless@vger.kernel.org>
References: <20230329000902.17992-1-quic_alokad@quicinc.com>
 <d54800ae88be1eb232df7794e741dfc304c3ee9a.camel@sipsolutions.net>
 <312f1476-b78b-b7f0-26f9-512a7dc2b6b7@quicinc.com>
 <3abb2d127eaa3b76a8f4f34c795ddd6d502ce470.camel@sipsolutions.net>
From:   Aloka Dixit <quic_alokad@quicinc.com>
In-Reply-To: <3abb2d127eaa3b76a8f4f34c795ddd6d502ce470.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/31/2023 2:23 AM, Johannes Berg wrote:
> 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:
>>>
>>>> +		switch (wdev->iftype) {
>>>> +		case NL80211_IFTYPE_AP:
>>>> +			width = wdev->u.ap.preset_chandef.width;
>>>
>>> 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.
>>>
>>
>> Okay, I can do it the same with it is done in he_get_txmcsmap():
>> 	chandef = 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-link
>>> config in set_tx_bitrate_mask()?
>>>
>>
>> Please correct me if I'm wrong but ieee80211_set_bitrate_mask() need not
>> 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 ...??
> 

I agree with all of your points and as said above, I mainly tested by 
checking the final mask generated in nl80211.c.

Do you remember a similar discussion when HE code was added (I wasn't 
the submitter). May be that will have some pointers regarding how it was 
tested.

Do you think we can continue with this EHT support for non-mac80211 
drivers similar to HE for now?
Or should we just skip this patch?

Thanks.