Message ID | 20201203185732.9CFA5C433ED@smtp.codeaurora.org |
---|---|
State | New |
Headers | show |
Series | pull-request: wireless-drivers-next-2020-12-03 | expand |
On Thu, 3 Dec 2020 18:57:32 +0000 (UTC) Kalle Valo wrote: > wireless-drivers-next patches for v5.11 > > First set of patches for v5.11. rtw88 getting improvements to work > better with Bluetooth and other driver also getting some new features. > mhi-ath11k-immutable branch was pulled from mhi tree to avoid > conflicts with mhi tree. Pulled, but there are a lot of fixes in here which look like they should have been part of the other PR, if you ask me. There's also a patch which looks like it renames a module parameter. Module parameters are considered uAPI.
Jakub Kicinski <kuba@kernel.org> writes: > On Thu, 3 Dec 2020 18:57:32 +0000 (UTC) Kalle Valo wrote: >> wireless-drivers-next patches for v5.11 >> >> First set of patches for v5.11. rtw88 getting improvements to work >> better with Bluetooth and other driver also getting some new features. >> mhi-ath11k-immutable branch was pulled from mhi tree to avoid >> conflicts with mhi tree. > > Pulled, but there are a lot of fixes in here which look like they > should have been part of the other PR, if you ask me. Yeah, I'm actually on purpose keeping the bar high for patches going to wireless-drivers (ie. the fixes going to -rc releases). This is just to keep things simple for me and avoiding the number of conflicts between the trees. > 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. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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. 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). 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. Brian
On Tue, Dec 8, 2020 at 7:01 AM Edward Cree <ecree.xilinx@gmail.com> wrote: > My understanding is that it's because users can have them in their > modprobe.conf, which causes breakage if an update removes the param. > I think the module insert fails if there are unrecognised parameters > there. That's a nice understanding, but I believe it's an incorrect one: # echo 'options rtw88_pci doesnotexist=helloworld' >> /etc/modprobe.d/rtw.conf # modprobe rtw88_pci; echo $? 0 In fact, while I was already quite aware about the removal Jakub is highlighting (in the rtw88 driver), I was a user of the parameter, and was quite happy to see it die (because now the driver does the Right Thing automatically). I still left the option in my modprobe.conf, while I finished staging upgrades of all my systems. I ran into no problems, and now that the migration is done, I killed the modprobe.conf entry. Brian
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.