Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: rmanohar=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Fri, 16 Oct 2020 12:17:41 -0700
From:   Rajkumar Manoharan <rmanohar@codeaurora.org>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
Subject: Re: [PATCH v2] cfg80211: add support to configure HE MCS for beacon
 rate
In-Reply-To: <7b0080517b301ccd32d03b25b2f16663238959df.camel@sipsolutions.net>
References: <1601762257-14934-1-git-send-email-rmanohar@codeaurora.org>
 <e5b99d2b7215841b184ddcec5ca8498f27795547.camel@sipsolutions.net>
 (sfid-20201008_110659_738848_40D7EFF9)
 <7b0080517b301ccd32d03b25b2f16663238959df.camel@sipsolutions.net>
Message-ID: <21941d6526fa00b9f8e0787537b963b3@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2020-10-08 02:09, Johannes Berg wrote:
> On Thu, 2020-10-08 at 11:06 +0200, Johannes Berg wrote:
>> On Sat, 2020-10-03 at 14:57 -0700, Rajkumar Manoharan wrote:
>> > This allows an option to configure a single HE MCS beacon tx rate.
>> >
>> > Signed-off-by: Rajkumar Manoharan <rmanohar@codeaurora.org>
>> > ---
>> > v2: clear mcs ratemask when beacon tx rate is not set.
>> 
>> I can't believe I didn't realize this before, but ...
> 
> Never mind that comment ... it wasn't there before, I was confused :)
> 
No worries. :)

>> I mean, why not just skip the
>> 
>>         /* Default to all rates enabled */
>>         for (i = 0; i < NUM_NL80211_BANDS; i++) {
>> 		[...]
>> 	}
>> 
>> code? Right now you're first setting it, and then clearing it again 
>> when
>> is_beacon_tx_rate is set ...
>> 
>> 
>> I'd prefer a "default_all_enabled" parameter, and then call the above
>> loop conditionally on it (perhaps moving it to a separate function to
>> not have all the deep indentation to worry about).
> 
> And maybe that should be a separate patch.
> 
Neat.. Will split the patch and send a new series.

-Rajkumar