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

[Brian Norris <briannorris@chromium.org>]

Hi Jakub,

On Mon, Dec 7, 2020 at 12:10 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.

I'm not sure it's as self-evident as you claim. Similar arguments
could be made of debugfs (it's even typically mounted under /sys, so
one could accidentally think it *is* sysfs!), except that somewhere
along the line it has been decreed to not be a stable interface.

But anyway, I can acknowledge it's a "widely accepted truth [in some
circles]" and act accordingly (e.g., closer review on their
introduction). I'll also maintain my counter-acknowledgment, that this
approach is not universal. Taking another subystem (fs/) as an
example, I didn't have to look far for similar approaches, where
module parameters were removed as features became obsolete, handled
automatically, etc.:

d3df14535f4a ext4: mballoc: make mb_debug() implementation to use pr_debug()
1565bdad59e9 fscrypt: remove struct fscrypt_ctx
73d03931be2f erofs: kill use_vmap module parameter

Although to be fair, I did find at least one along the way where the
author made a special attempt at a "deprecation notice" while handling
it gracefully:

791205e3ec60 pstore/ram: Introduce max_reason and convert dump_oops

I wouldn't be surprised if that module parameter disappears eventually
though, with little fanfare.

> > 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'll highlight (and agree with) Emmanuel's notice that debugfs is not
a suitable replacement in some cases. I'd still agree debugfs is
better where possible though, because it's clearer about the
(in)stability guarantees, and harder for users to "set and forget"
(per your below notes).

> > 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.
...
> 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.

That's a valid concern. I'm not sure what to do about that, beyond
documentation (which such users probably fail to read) or maybe loud
WARN() prints (which such users could easily ignore).

> 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.

I'm not sure I understand this sentence; it's perfectly easy to handle
all manner of changed/dropped module params through staged rollout. If
a param is deleted, one can keep it in their modprobe.d configs until
it's fully phased out. If it's renamed with a slightly different
purpose, encode both purposes in your configs. Et cetera.

Brian

> Otherwise I'd think
> debugfs is indeed a better place for them.