Re: pull-request: wireless-drivers-next-2020-12-03

[Emmanuel Grumbach <egrumbach@gmail.com>]

On Mon, Dec 7, 2020 at 10:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 7 Dec 2020 11:35:53 -0800 Brian Norris wrote:
> > On Mon, Dec 7, 2020 at 2:42 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> > > Jakub Kicinski <kuba@kernel.org> writes:
> > > > On Thu,  3 Dec 2020 18:57:32 +0000 (UTC) Kalle Valo wrote:
> > > > There's also a patch which looks like it renames a module parameter.
> > > > Module parameters are considered uAPI.
> > >
> > > Ah, I have been actually wondering that if they are part of user space
> > > API or not, good to know that they are. I'll keep an eye of this in the
> > > future so that we are not breaking the uAPI with module parameter
> > > changes.
> >
> > Is there some reference for this rule (e.g., dictate from on high; or
> > some explanation of reasons)? Or limitations on it? Because as-is,
> > this sounds like one could never drop a module parameter, or remove
> > obsolete features.
>
> TBH its one of those "widely accepted truth" in networking which was
> probably discussed before I started compiling kernels so I don't know
> the full background. But it seems pretty self-evident even without
> knowing the casus that made us institute the rule.
>
> Module parameters are certainly userspace ABI, since user space can
> control them either when loading the module or via sysfs.
>
> > It also suggests that debug-related knobs (which
> > can benefit from some amount of flexibility over time) should go
> > exclusively in debugfs (where ABI guarantees are explicitly not made),
> > even at the expense of usability (dropping a line into
> > /etc/modprobe.d/ is hard to beat).
>
> Indeed, debugfs seems more appropriate.

I don't think that a module parameter and a debugfs knob are
technically equivalent and the only difference would be whether it is
considered ABI or not. The usability of a module parameter is hard to
beat as Brian said, but I think the difference goes beyond usability
~= ease of use. A debugfs hook can't be available at the very start of
the module. You first have to register your debugfs knobs to the
parent dir. And if you want your parent dir to belong to the subsystem
you register to, then you first need to register to the subsystem
which means that a fair amount of code has been running already. A
debugfs hook won't allow you to parametrize this piece of code.

>
> > That's not to say I totally disagree with the original claim, but I'm
> > just interested in knowing precisely what it means.
> >
> > And to put a precise spin on this: what would this rule say about the following?
> >
> > http://git.kernel.org/linus/f06021a18fcf8d8a1e79c5e0a8ec4eb2b038e153
> > iwlwifi: remove lar_disable module parameter
> >
> > Should that parameter have never been introduced in the first place,
> > never be removed, or something else? I think I've seen this sort of
> > pattern before, where features get phased in over time, with module
> > parameters as either escape hatches or as opt-in mechanisms.
> > Eventually, they stabilize, and there's no need (or sometimes, it's
> > actively harmful) to keep the knob around.
> >
> > Or the one that might (?) be in question here:
> > fc3ac64a3a28 rtw88: decide lps deep mode from firmware feature.
> >
> > The original module parameter was useful for enabling new power-saving
> > features, because the driver didn't yet know which chip(s)/firmware(s)
> > were stable with which power features. Now, the driver has learned how
> > to figure out the optimal power settings, so it's dropping the old
> > param and adding an "escape hatch", in case there are problems.
> >
> > I'd say this one is a bit more subtle than the lar_disable example,
> > but I'm still not sure that really qualifies as a "user-visible"
> > change.
>
> If I'm reading this right the pattern seems to be that module
> parameters are used as chicken bits. It's an interesting problem,
> I'm not sure this use case was discussed. My concern would be that
> there is no guarantee users will in fact report the new feature
> fails for them, and therefore grow to depend on the chicken bits.
>
> Since updating software is so much easier than re-etching silicon
> I'd personally not use chicken bits in software, especially with
> growing adoption of staggered update roll outs. Otherwise I'd think
> debugfs is indeed a better place for them.

In this specific case, having put the lar_disable functionality under
debugfs would have meant that the user would have had to:
1) Load the driver
2) write the debugfs hook
3) take the interface down
4) take the interface up

to make the configuration take effect which is hard because typically
the user doesn't control the interface directly but it is controlled
by the wpa_supplicant.
Don't get me wrong, I'm not saying the choice made for this module
parameter was right or wrong, I don't even want to get into that
discussion, I'm just saying that debugfs is not an infra that allows
you to do what you'd do with a module parameter.
In a sense, I guess I'm having the same discussion with our System
guys for whom module parameter = registry key in windows :)