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;
Date:   Fri, 11 Jun 2021 15:57:19 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     tony0620emma@gmail.com, kvalo@codeaurora.org,
        linux-wireless@vger.kernel.org, steventing@realtek.com
Subject: Re: [PATCH v2 2/2] rtw88: add debugfs to force lowest basic rate
Message-ID: <YMPqT8VH5alHQXXA@google.com>
References: <20210422030413.9738-1-pkshih@realtek.com>
 <20210422030413.9738-2-pkshih@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210422030413.9738-2-pkshih@realtek.com>
Precedence: bulk

On Thu, Apr 22, 2021 at 11:04:13AM +0800, Ping-Ke Shih wrote:
> From: Yu-Yen Ting <steventing@realtek.com>
> 
> The management frame with high rate e.g. 24M may not be transmitted
> smoothly in long range environment.
> Add a debugfs to force to use the lowest basic rate
> in order to debug the reachability of transmitting management frame.
> 
> obtain current setting
> cat /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates
> 
> force lowest rate:
> echo 1 > /sys/kernel/debug/ieee80211/phyX/rtw88/basic_rates
> 
> Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

I believe some initial objection to this was because it was unclear if
this is for "production" use (e.g., recommending distros to play with
this) or for debugging. I'll admit, I requested the feature for patch 1,
because I've seen that for those networks where people *do* configure
odd Basic Rates, they intend for stations to follow those, and not use
the lowest (and most airtime-hogging) rates.

And I can say, I don't see why distributions should be turning that back
off. If the Basic Rates setting is wrong, then the that's up to the
network admin to fix.

All that is to say: I agree that this patch is purely for debugging, as
stated, and that it belongs in debugfs. I also maintain a distribution,
and I don't plan on using this beyond debugging.

Therefore:

Reviewed-by: Brian Norris <briannorris@chromium.org>

BTW, if we have clear guidelines on debugfs, module parameters, etc.,
maybe those should be going on the wiki? I know this came up before:

https://lore.kernel.org/linux-wireless/87d09u7tyr.fsf@codeaurora.org/

At this point, I'm willing to write such guidelines, if I get an ack
from the relevant folks (I guess that's just Kalle?). It probably
belongs somewhere in this tree:

https://wireless.wiki.kernel.org/en/developers/documentation

similar to this:
https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
except it's not really an nl80211 thing. Suggestions welcome.

Side note: it could really use some cleanup -- like this page:
https://wireless.wiki.kernel.org/en/developers/process

Brian