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: <76540463-9e4e-3dc7-bdcd-35c6b709169a@quicinc.com>
Date:   Mon, 1 May 2023 13:03:33 -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
From:   Aloka Dixit <quic_alokad@quicinc.com>
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>
 <1aa6178f-2041-797e-fcb8-78fe2c08de24@quicinc.com>
Content-Language: en-US
In-Reply-To: <1aa6178f-2041-797e-fcb8-78fe2c08de24@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/31/2023 10:55 AM, Aloka Dixit wrote:
> 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.

Hi Joannes,
Thoughts on this one?
Thanks.