Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org E9E866049C
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "briannorris\@chromium.org" <briannorris@chromium.org>,
        "g.schlmm\@googlemail.com" <g.schlmm@googlemail.com>
Subject: Re: [PATCH 3/6] rtw88: use a module parameter to control LPS enter
References: <20191025093345.22643-4-yhchuang@realtek.com>
        <20191031075911.3CCB86079C@smtp.codeaurora.org>
        <F7CD281DE3E379468C6D07993EA72F84D1914F4C@RTITMBSVM04.realtek.com.tw>
Date:   Fri, 01 Nov 2019 10:30:57 +0200
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D1914F4C@RTITMBSVM04.realtek.com.tw>
        (Tony Chuang's message of "Thu, 31 Oct 2019 08:17:37 +0000")
Message-ID: <87imo40zi6.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Tony Chuang <yhchuang@realtek.com> writes:

> From: Kalle Valo
>> <yhchuang@realtek.com> wrote:
>> 
>> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>> >
>> > If the number of packets is less than the LPS threshold, driver
>> > can then enter LPS mode.
>> > And driver used to take RTW_LPS_THRESHOLD as the threshold. As
>> > the macro can not be changed after compiled, use a parameter
>> > instead.
>> >
>> > The larger of the threshold, the more traffic required to leave
>> > power save mode, responsive time could be longer, but also the
>> > power consumption could be lower.
>> >
>> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>> > Reviewed-by: Chris Chiu <chiu@endlessm.com>
>> 
>> I don't think a module parameter should be used to control power save
>> level, instead there should be a generic interface for that. Also the commit
>> log does not give any explanation why this needs to be a module parameter.
>> 
>> Tony, there's a high barrier for adding new module parameters. It's a
>> common
>> phrase for me to say "module parameters are not windows .ini files". And to
>> make it
>> easier for everyone always submit controversial patches separately, do not
>> hide
>> within a bigger patchset.
>> 
>
> Alright, I was thinking module parameters as a convenient tool for driver to
> control the behavior for debugging or out-of-band adjusting. But it seems like
> you treat it more carefully.
>
> Actually this is just going to allow us to set different default values for different
> use cases. So is there a better way to control it. Or I should just change the
> value to a better one. By our experience, set this to 50 is a more reasonable
> value, such that some web surfing or background traffic wouldn't make the
> driver to leave PS mode.

I recall having a similar discussion something like 10 years ago. (Yes,
I have been here for way too long). I think at the time recommendation
was to use latency value from the QoS framework to make it possible for
user space to change wireless power save aggressiveness. But I don't
know if anyone really used that.

I was feeling nostalgic and decided to find some pointers:

https://lore.kernel.org/linux-wireless/1271850458-32437-2-git-send-email-juuso.oikarinen@nokia.com/

And it seems the patch was even applied:

195e294d21e8 mac80211: Determine dynamic PS timeout based on ps-qos network latency

This is for mac80211 dynamic ps feature, but I imagine we could somehow
extend it to driver settings like the LPS threshold here. Something like
this would be much more acceptable than having custom module parameters
for each driver.

-- 
Kalle Valo