* [PATCH v18 4/7] rust: module: use a reference in macros::module::module
From: Andreas Hindborg @ 2025-09-24 12:39 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Daniel Gomez, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
In-Reply-To: <20250924-module-params-v3-v18-0-bf512c35d910@kernel.org>
When we add parameter support to the module macro, we want to be able to
pass a reference to `ModuleInfo` to a helper function. That is not possible
when we move out of the local `modinfo`. So change the function to access
the local via reference rather than value.
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/macros/module.rs | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 5ee54a00c0b65..cbf3ac0a8f7ba 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -176,23 +176,23 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
// Rust does not allow hyphens in identifiers, use underscore instead.
let ident = info.name.replace('-', "_");
let mut modinfo = ModInfoBuilder::new(ident.as_ref());
- if let Some(authors) = info.authors {
+ if let Some(authors) = &info.authors {
for author in authors {
- modinfo.emit("author", &author);
+ modinfo.emit("author", author);
}
}
- if let Some(description) = info.description {
- modinfo.emit("description", &description);
+ if let Some(description) = &info.description {
+ modinfo.emit("description", description);
}
modinfo.emit("license", &info.license);
- if let Some(aliases) = info.alias {
+ if let Some(aliases) = &info.alias {
for alias in aliases {
- modinfo.emit("alias", &alias);
+ modinfo.emit("alias", alias);
}
}
- if let Some(firmware) = info.firmware {
+ if let Some(firmware) = &info.firmware {
for fw in firmware {
- modinfo.emit("firmware", &fw);
+ modinfo.emit("firmware", fw);
}
}
--
2.47.2
^ permalink raw reply related
* [PATCH v18 1/7] rust: sync: add `SetOnce`
From: Andreas Hindborg @ 2025-09-24 12:39 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Daniel Gomez, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
In-Reply-To: <20250924-module-params-v3-v18-0-bf512c35d910@kernel.org>
Introduce the `SetOnce` type, a container that can only be written once.
The container uses an internal atomic to synchronize writes to the internal
value.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/sync.rs | 2 +
rust/kernel/sync/set_once.rs | 125 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 127 insertions(+)
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index cf5b638a097d9..3f957ad3a9f0a 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -20,6 +20,7 @@
pub mod poll;
pub mod rcu;
mod refcount;
+mod set_once;
pub use arc::{Arc, ArcBorrow, UniqueArc};
pub use completion::Completion;
@@ -29,6 +30,7 @@
pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard};
pub use locked_by::LockedBy;
pub use refcount::Refcount;
+pub use set_once::SetOnce;
/// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
#[repr(transparent)]
diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
new file mode 100644
index 0000000000000..bdba601807d8b
--- /dev/null
+++ b/rust/kernel/sync/set_once.rs
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! A container that can be initialized at most once.
+
+use super::atomic::{
+ ordering::{Acquire, Relaxed, Release},
+ Atomic,
+};
+use core::{cell::UnsafeCell, mem::MaybeUninit};
+
+/// A container that can be populated at most once. Thread safe.
+///
+/// Once the a [`SetOnce`] is populated, it remains populated by the same object for the
+/// lifetime `Self`.
+///
+/// # Invariants
+///
+/// - `init` may only increase in value.
+/// - `init` may only assume values in the range `0..=2`.
+/// - `init == 0` if and only if `value` is uninitialized.
+/// - `init == 1` if and only if there is exactly one thread with exclusive
+/// access to `self.value`.
+/// - `init == 2` if and only if `value` is initialized and valid for shared
+/// access.
+///
+/// # Example
+///
+/// ```
+/// # use kernel::sync::SetOnce;
+/// let value = SetOnce::new();
+/// assert_eq!(None, value.as_ref());
+///
+/// let status = value.populate(42u8);
+/// assert_eq!(true, status);
+/// assert_eq!(Some(&42u8), value.as_ref());
+/// assert_eq!(Some(42u8), value.copy());
+///
+/// let status = value.populate(101u8);
+/// assert_eq!(false, status);
+/// assert_eq!(Some(&42u8), value.as_ref());
+/// assert_eq!(Some(42u8), value.copy());
+/// ```
+pub struct SetOnce<T> {
+ init: Atomic<u32>,
+ value: UnsafeCell<MaybeUninit<T>>,
+}
+
+impl<T> Default for SetOnce<T> {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl<T> SetOnce<T> {
+ /// Create a new [`SetOnce`].
+ ///
+ /// The returned instance will be empty.
+ pub const fn new() -> Self {
+ // INVARIANT: The container is empty and we initialize `init` to `0`.
+ Self {
+ value: UnsafeCell::new(MaybeUninit::uninit()),
+ init: Atomic::new(0),
+ }
+ }
+
+ /// Get a reference to the contained object.
+ ///
+ /// Returns [`None`] if this [`SetOnce`] is empty.
+ pub fn as_ref(&self) -> Option<&T> {
+ if self.init.load(Acquire) == 2 {
+ // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
+ // is initialized and valid for shared access.
+ Some(unsafe { &*self.value.get().cast() })
+ } else {
+ None
+ }
+ }
+
+ /// Populate the [`SetOnce`].
+ ///
+ /// Returns `true` if the [`SetOnce`] was successfully populated.
+ pub fn populate(&self, value: T) -> bool {
+ // INVARIANT: If the swap succeeds:
+ // - We increase `init`.
+ // - We write the valid value `1` to `init`.
+ // - Only one thread can succeed in this write, so we have exclusive access after the
+ // write.
+ if let Ok(0) = self.init.cmpxchg(0, 1, Relaxed) {
+ // SAFETY: By the type invariants of `Self`, the fact that we succeeded in writing `1`
+ // to `self.init` means we obtained exclusive access to `self.value`.
+ unsafe { core::ptr::write(self.value.get().cast(), value) };
+ // INVARIANT:
+ // - We increase `init`.
+ // - We write the valid value `2` to `init`.
+ // - We release our exclusive access to `self.value` and it is now valid for shared
+ // access.
+ self.init.store(2, Release);
+ true
+ } else {
+ false
+ }
+ }
+
+ /// Get a copy of the contained object.
+ ///
+ /// Returns [`None`] if the [`SetOnce`] is empty.
+ pub fn copy(&self) -> Option<T>
+ where
+ T: Copy,
+ {
+ self.as_ref().copied()
+ }
+}
+
+impl<T> Drop for SetOnce<T> {
+ fn drop(&mut self) {
+ if *self.init.get_mut() == 2 {
+ let value = self.value.get_mut();
+ // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
+ // contains a valid value. We have exclusive access, as we hold a `mut` reference to
+ // `self`.
+ unsafe { value.assume_init_drop() };
+ }
+ }
+}
--
2.47.2
^ permalink raw reply related
* [PATCH v18 7/7] modules: add rust modules files to MAINTAINERS
From: Andreas Hindborg @ 2025-09-24 12:39 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Daniel Gomez, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
In-Reply-To: <20250924-module-params-v3-v18-0-bf512c35d910@kernel.org>
The module subsystem people agreed to maintain rust support for modules
[1]. Thus, add entries for relevant files to modules entry in MAINTAINERS.
Link: https://lore.kernel.org/all/0d9e596a-5316-4e00-862b-fd77552ae4b5@suse.com/ [1]
Acked-by: Daniel Gomez <da.gomez@samsung.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index d69021b88aef0..55e3bf16ea0a8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17136,6 +17136,8 @@ F: include/linux/module*.h
F: kernel/module/
F: lib/test_kmod.c
F: lib/tests/module/
+F: rust/kernel/module_param.rs
+F: rust/macros/module.rs
F: scripts/module*
F: tools/testing/selftests/kmod/
F: tools/testing/selftests/module/
--
2.47.2
^ permalink raw reply related
* [PATCH v18 5/7] rust: module: update the module macro with module parameter support
From: Andreas Hindborg @ 2025-09-24 12:39 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Daniel Gomez, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
In-Reply-To: <20250924-module-params-v3-v18-0-bf512c35d910@kernel.org>
Allow module parameters to be declared in the rust `module!` macro.
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/macros/helpers.rs | 25 +++++++
rust/macros/lib.rs | 31 +++++++++
rust/macros/module.rs | 178 ++++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 224 insertions(+), 10 deletions(-)
diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
index e2602be402c10..365d7eb499c08 100644
--- a/rust/macros/helpers.rs
+++ b/rust/macros/helpers.rs
@@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
}
}
+pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
+ let peek = it.clone().next();
+ match peek {
+ Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
+ let _ = it.next();
+ Some(punct.as_char())
+ }
+ _ => None,
+ }
+}
+
pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
if let Some(TokenTree::Literal(literal)) = it.next() {
Some(literal.to_string())
@@ -103,3 +114,17 @@ pub(crate) fn file() -> String {
proc_macro::Span::call_site().file()
}
}
+
+/// Parse a token stream of the form `expected_name: "value",` and return the
+/// string in the position of "value".
+///
+/// # Panics
+///
+/// - On parse error.
+pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
+ assert_eq!(expect_ident(it), expected_name);
+ assert_eq!(expect_punct(it), ':');
+ let string = expect_string(it);
+ assert_eq!(expect_punct(it), ',');
+ string
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index fa847cf3a9b5f..2fb520dc930af 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -28,6 +28,30 @@
/// The `type` argument should be a type which implements the [`Module`]
/// trait. Also accepts various forms of kernel metadata.
///
+/// The `params` field describe module parameters. Each entry has the form
+///
+/// ```ignore
+/// parameter_name: type {
+/// default: default_value,
+/// description: "Description",
+/// }
+/// ```
+///
+/// `type` may be one of
+///
+/// - [`i8`]
+/// - [`u8`]
+/// - [`i8`]
+/// - [`u8`]
+/// - [`i16`]
+/// - [`u16`]
+/// - [`i32`]
+/// - [`u32`]
+/// - [`i64`]
+/// - [`u64`]
+/// - [`isize`]
+/// - [`usize`]
+///
/// C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
///
/// [`Module`]: ../kernel/trait.Module.html
@@ -44,6 +68,12 @@
/// description: "My very own kernel module!",
/// license: "GPL",
/// alias: ["alternate_module_name"],
+/// params: {
+/// my_parameter: i64 {
+/// default: 1,
+/// description: "This parameter has a default of 1",
+/// },
+/// },
/// }
///
/// struct MyModule(i32);
@@ -52,6 +82,7 @@
/// fn init(_module: &'static ThisModule) -> Result<Self> {
/// let foo: i32 = 42;
/// pr_info!("I contain: {}\n", foo);
+/// pr_info!("i32 param is: {}\n", module_parameters::my_parameter.read());
/// Ok(Self(foo))
/// }
/// }
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index cbf3ac0a8f7ba..d62e9c1e2a898 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -26,6 +26,7 @@ struct ModInfoBuilder<'a> {
module: &'a str,
counter: usize,
buffer: String,
+ param_buffer: String,
}
impl<'a> ModInfoBuilder<'a> {
@@ -34,10 +35,11 @@ fn new(module: &'a str) -> Self {
module,
counter: 0,
buffer: String::new(),
+ param_buffer: String::new(),
}
}
- fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
+ fn emit_base(&mut self, field: &str, content: &str, builtin: bool, param: bool) {
let string = if builtin {
// Built-in modules prefix their modinfo strings by `module.`.
format!(
@@ -51,8 +53,14 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
format!("{field}={content}\0")
};
+ let buffer = if param {
+ &mut self.param_buffer
+ } else {
+ &mut self.buffer
+ };
+
write!(
- &mut self.buffer,
+ buffer,
"
{cfg}
#[doc(hidden)]
@@ -75,20 +83,119 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) {
self.counter += 1;
}
- fn emit_only_builtin(&mut self, field: &str, content: &str) {
- self.emit_base(field, content, true)
+ fn emit_only_builtin(&mut self, field: &str, content: &str, param: bool) {
+ self.emit_base(field, content, true, param)
}
- fn emit_only_loadable(&mut self, field: &str, content: &str) {
- self.emit_base(field, content, false)
+ fn emit_only_loadable(&mut self, field: &str, content: &str, param: bool) {
+ self.emit_base(field, content, false, param)
}
fn emit(&mut self, field: &str, content: &str) {
- self.emit_only_builtin(field, content);
- self.emit_only_loadable(field, content);
+ self.emit_internal(field, content, false);
+ }
+
+ fn emit_internal(&mut self, field: &str, content: &str, param: bool) {
+ self.emit_only_builtin(field, content, param);
+ self.emit_only_loadable(field, content, param);
+ }
+
+ fn emit_param(&mut self, field: &str, param: &str, content: &str) {
+ let content = format!("{param}:{content}", param = param, content = content);
+ self.emit_internal(field, &content, true);
+ }
+
+ fn emit_params(&mut self, info: &ModuleInfo) {
+ let Some(params) = &info.params else {
+ return;
+ };
+
+ for param in params {
+ let ops = param_ops_path(¶m.ptype);
+
+ // Note: The spelling of these fields is dictated by the user space
+ // tool `modinfo`.
+ self.emit_param("parmtype", ¶m.name, ¶m.ptype);
+ self.emit_param("parm", ¶m.name, ¶m.description);
+
+ write!(
+ self.param_buffer,
+ "
+ pub(crate) static {param_name}:
+ ::kernel::module_param::ModuleParamAccess<{param_type}> =
+ ::kernel::module_param::ModuleParamAccess::new({param_default});
+
+ const _: () = {{
+ #[link_section = \"__param\"]
+ #[used]
+ static __{module_name}_{param_name}_struct:
+ ::kernel::module_param::KernelParam =
+ ::kernel::module_param::KernelParam::new(
+ ::kernel::bindings::kernel_param {{
+ name: if ::core::cfg!(MODULE) {{
+ ::kernel::c_str!(\"{param_name}\").as_bytes_with_nul()
+ }} else {{
+ ::kernel::c_str!(\"{module_name}.{param_name}\")
+ .as_bytes_with_nul()
+ }}.as_ptr(),
+ // SAFETY: `__this_module` is constructed by the kernel at load
+ // time and will not be freed until the module is unloaded.
+ #[cfg(MODULE)]
+ mod_: unsafe {{
+ core::ptr::from_ref(&::kernel::bindings::__this_module)
+ .cast_mut()
+ }},
+ #[cfg(not(MODULE))]
+ mod_: ::core::ptr::null_mut(),
+ ops: core::ptr::from_ref(&{ops}),
+ perm: 0, // Will not appear in sysfs
+ level: -1,
+ flags: 0,
+ __bindgen_anon_1: ::kernel::bindings::kernel_param__bindgen_ty_1 {{
+ arg: {param_name}.as_void_ptr()
+ }},
+ }}
+ );
+ }};
+ ",
+ module_name = info.name,
+ param_type = param.ptype,
+ param_default = param.default,
+ param_name = param.name,
+ ops = ops,
+ )
+ .unwrap();
+ }
+ }
+}
+
+fn param_ops_path(param_type: &str) -> &'static str {
+ match param_type {
+ "i8" => "::kernel::module_param::PARAM_OPS_I8",
+ "u8" => "::kernel::module_param::PARAM_OPS_U8",
+ "i16" => "::kernel::module_param::PARAM_OPS_I16",
+ "u16" => "::kernel::module_param::PARAM_OPS_U16",
+ "i32" => "::kernel::module_param::PARAM_OPS_I32",
+ "u32" => "::kernel::module_param::PARAM_OPS_U32",
+ "i64" => "::kernel::module_param::PARAM_OPS_I64",
+ "u64" => "::kernel::module_param::PARAM_OPS_U64",
+ "isize" => "::kernel::module_param::PARAM_OPS_ISIZE",
+ "usize" => "::kernel::module_param::PARAM_OPS_USIZE",
+ t => panic!("Unsupported parameter type {}", t),
}
}
+fn expect_param_default(param_it: &mut token_stream::IntoIter) -> String {
+ assert_eq!(expect_ident(param_it), "default");
+ assert_eq!(expect_punct(param_it), ':');
+ let sign = try_sign(param_it);
+ let default = try_literal(param_it).expect("Expected default param value");
+ assert_eq!(expect_punct(param_it), ',');
+ let mut value = sign.map(String::from).unwrap_or_default();
+ value.push_str(&default);
+ value
+}
+
#[derive(Debug, Default)]
struct ModuleInfo {
type_: String,
@@ -98,6 +205,50 @@ struct ModuleInfo {
description: Option<String>,
alias: Option<Vec<String>>,
firmware: Option<Vec<String>>,
+ params: Option<Vec<Parameter>>,
+}
+
+#[derive(Debug)]
+struct Parameter {
+ name: String,
+ ptype: String,
+ default: String,
+ description: String,
+}
+
+fn expect_params(it: &mut token_stream::IntoIter) -> Vec<Parameter> {
+ let params = expect_group(it);
+ assert_eq!(params.delimiter(), Delimiter::Brace);
+ let mut it = params.stream().into_iter();
+ let mut parsed = Vec::new();
+
+ loop {
+ let param_name = match it.next() {
+ Some(TokenTree::Ident(ident)) => ident.to_string(),
+ Some(_) => panic!("Expected Ident or end"),
+ None => break,
+ };
+
+ assert_eq!(expect_punct(&mut it), ':');
+ let param_type = expect_ident(&mut it);
+ let group = expect_group(&mut it);
+ assert_eq!(group.delimiter(), Delimiter::Brace);
+ assert_eq!(expect_punct(&mut it), ',');
+
+ let mut param_it = group.stream().into_iter();
+ let param_default = expect_param_default(&mut param_it);
+ let param_description = expect_string_field(&mut param_it, "description");
+ expect_end(&mut param_it);
+
+ parsed.push(Parameter {
+ name: param_name,
+ ptype: param_type,
+ default: param_default,
+ description: param_description,
+ })
+ }
+
+ parsed
}
impl ModuleInfo {
@@ -112,6 +263,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
"license",
"alias",
"firmware",
+ "params",
];
const REQUIRED_KEYS: &[&str] = &["type", "name", "license"];
let mut seen_keys = Vec::new();
@@ -137,6 +289,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
"license" => info.license = expect_string_ascii(it),
"alias" => info.alias = Some(expect_string_array(it)),
"firmware" => info.firmware = Some(expect_string_array(it)),
+ "params" => info.params = Some(expect_params(it)),
_ => panic!("Unknown key \"{key}\". Valid keys are: {EXPECTED_KEYS:?}."),
}
@@ -199,7 +352,9 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
// Built-in modules also export the `file` modinfo string.
let file =
std::env::var("RUST_MODFILE").expect("Unable to fetch RUST_MODFILE environmental variable");
- modinfo.emit_only_builtin("file", &file);
+ modinfo.emit_only_builtin("file", &file, false);
+
+ modinfo.emit_params(&info);
format!(
"
@@ -363,15 +518,18 @@ unsafe fn __exit() {{
__MOD.assume_init_drop();
}}
}}
-
{modinfo}
}}
}}
+ mod module_parameters {{
+ {params}
+ }}
",
type_ = info.type_,
name = info.name,
ident = ident,
modinfo = modinfo.buffer,
+ params = modinfo.param_buffer,
initcall_section = ".initcall6.init"
)
.parse()
--
2.47.2
^ permalink raw reply related
* [PATCH v18 3/7] rust: introduce module_param module
From: Andreas Hindborg @ 2025-09-24 12:39 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Daniel Gomez, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
In-Reply-To: <20250924-module-params-v3-v18-0-bf512c35d910@kernel.org>
Add types and traits for interfacing the C moduleparam API.
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/lib.rs | 1 +
rust/kernel/module_param.rs | 181 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 182 insertions(+)
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index fef97f2a50984..e34b377d93b2e 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -98,6 +98,7 @@
pub mod list;
pub mod miscdevice;
pub mod mm;
+pub mod module_param;
#[cfg(CONFIG_NET)]
pub mod net;
pub mod of;
diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
new file mode 100644
index 0000000000000..e7d5c930a467d
--- /dev/null
+++ b/rust/kernel/module_param.rs
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for module parameters.
+//!
+//! C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h)
+
+use crate::prelude::*;
+use crate::str::BStr;
+use bindings;
+use kernel::sync::SetOnce;
+
+/// Newtype to make `bindings::kernel_param` [`Sync`].
+#[repr(transparent)]
+#[doc(hidden)]
+pub struct KernelParam(bindings::kernel_param);
+
+impl KernelParam {
+ #[doc(hidden)]
+ pub const fn new(val: bindings::kernel_param) -> Self {
+ Self(val)
+ }
+}
+
+// SAFETY: C kernel handles serializing access to this type. We never access it
+// from Rust module.
+unsafe impl Sync for KernelParam {}
+
+/// Types that can be used for module parameters.
+// NOTE: This trait is `Copy` because drop could produce unsoundness during teardown.
+pub trait ModuleParam: Sized + Copy {
+ /// Parse a parameter argument into the parameter value.
+ fn try_from_param_arg(arg: &BStr) -> Result<Self>;
+}
+
+/// Set the module parameter from a string.
+///
+/// Used to set the parameter value at kernel initialization, when loading
+/// the module or when set through `sysfs`.
+///
+/// See `struct kernel_param_ops.set`.
+///
+/// # Safety
+///
+/// - If `val` is non-null then it must point to a valid null-terminated string that must be valid
+/// for reads for the duration of the call.
+/// - `param` must be a pointer to a `bindings::kernel_param` initialized by the rust module macro.
+/// The pointee must be valid for reads for the duration of the call.
+///
+/// # Note
+///
+/// - The safety requirements are satisfied by C API contract when this function is invoked by the
+/// module subsystem C code.
+/// - Currently, we only support read-only parameters that are not readable from `sysfs`. Thus, this
+/// function is only called at kernel initialization time, or at module load time, and we have
+/// exclusive access to the parameter for the duration of the function.
+///
+/// [`module!`]: macros::module
+unsafe extern "C" fn set_param<T>(val: *const c_char, param: *const bindings::kernel_param) -> c_int
+where
+ T: ModuleParam,
+{
+ // NOTE: If we start supporting arguments without values, val _is_ allowed
+ // to be null here.
+ if val.is_null() {
+ // TODO: Use pr_warn_once available.
+ crate::pr_warn!("Null pointer passed to `module_param::set_param`");
+ return EINVAL.to_errno();
+ }
+
+ // SAFETY: By function safety requirement, val is non-null, null-terminated
+ // and valid for reads for the duration of this function.
+ let arg = unsafe { CStr::from_char_ptr(val) };
+
+ crate::error::from_result(|| {
+ let new_value = T::try_from_param_arg(arg)?;
+
+ // SAFETY: By function safety requirements, this access is safe.
+ let container = unsafe { &*((*param).__bindgen_anon_1.arg.cast::<SetOnce<T>>()) };
+
+ container
+ .populate(new_value)
+ .then_some(0)
+ .ok_or(kernel::error::code::EEXIST)
+ })
+}
+
+macro_rules! impl_int_module_param {
+ ($ty:ident) => {
+ impl ModuleParam for $ty {
+ fn try_from_param_arg(arg: &BStr) -> Result<Self> {
+ <$ty as crate::str::parse_int::ParseInt>::from_str(arg)
+ }
+ }
+ };
+}
+
+impl_int_module_param!(i8);
+impl_int_module_param!(u8);
+impl_int_module_param!(i16);
+impl_int_module_param!(u16);
+impl_int_module_param!(i32);
+impl_int_module_param!(u32);
+impl_int_module_param!(i64);
+impl_int_module_param!(u64);
+impl_int_module_param!(isize);
+impl_int_module_param!(usize);
+
+/// A wrapper for kernel parameters.
+///
+/// This type is instantiated by the [`module!`] macro when module parameters are
+/// defined. You should never need to instantiate this type directly.
+///
+/// Note: This type is `pub` because it is used by module crates to access
+/// parameter values.
+pub struct ModuleParamAccess<T> {
+ value: SetOnce<T>,
+ default: T,
+}
+
+// SAFETY: We only create shared references to the contents of this container,
+// so if `T` is `Sync`, so is `ModuleParamAccess`.
+unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
+
+impl<T> ModuleParamAccess<T> {
+ #[doc(hidden)]
+ pub const fn new(default: T) -> Self {
+ Self {
+ value: SetOnce::new(),
+ default,
+ }
+ }
+
+ /// Get a shared reference to the parameter value.
+ // Note: When sysfs access to parameters are enabled, we have to pass in a
+ // held lock guard here.
+ pub fn value(&self) -> &T {
+ self.value.as_ref().unwrap_or(&self.default)
+ }
+
+ /// Get a mutable pointer to `self`.
+ ///
+ /// NOTE: In most cases it is not safe deref the returned pointer.
+ pub const fn as_void_ptr(&self) -> *mut c_void {
+ core::ptr::from_ref(self).cast_mut().cast()
+ }
+}
+
+#[doc(hidden)]
+/// Generate a static [`kernel_param_ops`](srctree/include/linux/moduleparam.h) struct.
+///
+/// # Examples
+///
+/// ```ignore
+/// make_param_ops!(
+/// /// Documentation for new param ops.
+/// PARAM_OPS_MYTYPE, // Name for the static.
+/// MyType // A type which implements [`ModuleParam`].
+/// );
+/// ```
+macro_rules! make_param_ops {
+ ($ops:ident, $ty:ty) => {
+ #[doc(hidden)]
+ pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
+ flags: 0,
+ set: Some(set_param::<$ty>),
+ get: None,
+ free: None,
+ };
+ };
+}
+
+make_param_ops!(PARAM_OPS_I8, i8);
+make_param_ops!(PARAM_OPS_U8, u8);
+make_param_ops!(PARAM_OPS_I16, i16);
+make_param_ops!(PARAM_OPS_U16, u16);
+make_param_ops!(PARAM_OPS_I32, i32);
+make_param_ops!(PARAM_OPS_U32, u32);
+make_param_ops!(PARAM_OPS_I64, i64);
+make_param_ops!(PARAM_OPS_U64, u64);
+make_param_ops!(PARAM_OPS_ISIZE, isize);
+make_param_ops!(PARAM_OPS_USIZE, usize);
--
2.47.2
^ permalink raw reply related
* [PATCH v18 0/7] rust: extend `module!` macro with integer parameter support
From: Andreas Hindborg @ 2025-09-24 12:39 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Daniel Gomez, Benno Lossin, Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules, Andreas Hindborg
Extend the `module!` macro with support module parameters. Also add some
string to integer parsing functions.
Based on the original module parameter support by Miguel [1],
later extended and generalized by Adam for more types [2][3].
Originally tracked at [4].
Link: https://github.com/Rust-for-Linux/linux/pull/7 [1]
Link: https://github.com/Rust-for-Linux/linux/pull/82 [2]
Link: https://github.com/Rust-for-Linux/linux/pull/87 [3]
Link: https://github.com/Rust-for-Linux/linux/issues/11 [4]
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes in v18:
- Rebase on Rust atomic patches (tip/master).
- Link to v17: https://lore.kernel.org/r/20250711-module-params-v3-v17-0-cf9b10d4923d@kernel.org
Changes in v17:
- Fix drop impl of `SetOnce` so that it works with `UnsafeCell<MaybeUninit<_>>`.
- Slightly reword safety framework in `SetOnce`.
- Rebase on atomic series v6.
- Link to v16: https://lore.kernel.org/r/20250709-module-params-v3-v16-0-4f926bcccb50@kernel.org
Changes in v16:
- Normalize imports in `set_once.rs`.
- Use `UnsafeCell<MaybeUninit<T>>` rather than `Opaque<T>` for `SetOnce`.
- Use regular load in drop of `SetOnce`.
- Update attribution paragraph in cover letter with details from Miguel.
- Remove stray TODO in `set_once.rs`
- Link to v15: https://lore.kernel.org/r/20250707-module-params-v3-v15-0-c1f4269a57b9@kernel.org
Changes in v15:
- Rebase on v6.16-rc5.
- Dedent code in module macro for better formatting.
- Rename `OnceLock` to `SetOnce`.
- Use "being initialized" rather than "being mutably accessed" when
describing initialization state of `OnceLock`.
- Use `Relaxed` ordering when transitioning to exclusive access in
`OnceLock`.
- Add drop implementation for `OnceLock`.
- Re-export `OnceLock` from `kernel::sync` module.
- Improve indentation of in macro code. Prefix `cfg` to `::core::cfg` in
macro code.
- Use `core::ptr::from_ref` rather than `as` casts.
- Hide `KernelParam` instances behind `const _: ()` blocks.
- Rename `ModuleParamAccess::get` to `ModuleParamAccess::value`.
- Rename `RacyKernelParam` to `KernelParam`.
- Remove `ModuleParam::Value`.
- Move `copy` implementation of `OnceLock`.
- Update safety comments and invariants of `OnceLock`.
- Link to v14: https://lore.kernel.org/r/20250702-module-params-v3-v14-0-5b1cc32311af@kernel.org
Note: This series now depends on the atomics series [1].
[1] https://lore.kernel.org/all/20250618164934.19817-1-boqun.feng@gmail.com
Changes in v14:
- Remove unnecessary `crate::` prefix from `module_param::set_param`.
- Make `FromStrRadix` safe again by moving unsafe blocks to macro implementation (thanks Benno).
- Use `core::ptr::write` in `set_param` and drop safety requirement regarding initialization.
- Add a TODO to use `SyncUnsafeCell` for `ModuleParamAccess` when available.
- Add a NOTE regarding `Copy` bound on `ModuleParam`.
- Remove `'static` lifetime qualifier from `ModuleParam::try_from_param_arg` argument.
- Fix a typo in the safety requirements for `set_param`.
- Remove unused `#[macro_export]` attribute.
- Remove obsolete documentation for `ModuleParam::try_from_param_arg`.
- Make `RacyKernelParam` tuple field private.
- Introduce `OnceLock` and use that to synchronize population of parameter values.
- Link to v13: https://lore.kernel.org/r/20250612-module-params-v3-v13-0-bc219cd1a3f8@kernel.org
Changes in v13:
- remove absolute path for `ffi` types.
- Split patch 2 into 4 separate patches.
- Overhaul safety framework for `set_param`.
- Remove generated docs for `kernel_param_ops`.
- Move `parse_int` to separate file.
- Rebase on v6.16-rc1
- Link to v12: https://lore.kernel.org/r/20250506-module-params-v3-v12-0-c04d80c8a2b1@kernel.org
Changes in v12:
- Assign through pointer rather than using `core::ptr::replace`.
- Prevent a potential use-after-free during module teardown.
- Link to v11: https://lore.kernel.org/r/20250502-module-params-v3-v11-0-6096875a2b78@kernel.org
Changes in v11:
- Apply a few nits from Miguel.
- Link to v10: https://lore.kernel.org/r/20250501-module-params-v3-v10-0-4da485d343d5@kernel.org
Changes in v10:
- Apply fixups from Miguel:
- Add integer type suffixes to `assert!` in tests.
- Fix links to docs.kernel.org.
- Applyy markdown and intra-doc links where possible.
- Change to `///` for `mod` docs.
- Slightly reword a comment.
- Pluralize "Examples" section name.
- Hide `use`s in example.
- Removed `#[expect]` for the `rusttest` target.
- Link to v9: https://lore.kernel.org/r/20250321-module-params-v3-v9-0-28b905f2e345@kernel.org
Changes in v9:
- Remove UB when parsing the minimum integer values.
- Make `FromStr` trait unsafe, since wrong implementations can cause UB.
- Drop patches that were applied to rust-next.
- Link to v8: https://lore.kernel.org/r/20250227-module-params-v3-v8-0-ceeee85d9347@kernel.org
Changes in v8:
- Change print statement in sample to better communicate parameter name.
- Use imperative mode in commit messages.
- Remove prefix path from `EINVAL`.
- Change `try_from_param_arg` to accept `&BStr` rather than `&[u8]`.
- Parse integers without 128 bit integer types.
- Seal trait `FromStrRadix`.
- Strengthen safety requirement of `set_param`.
- Remove comment about Display and `PAGE_SIZE`.
- Add note describing why `ModuleParamAccess` is pub.
- Typo and grammar fixes for documentation.
- Update MAINTAINERS with rust module files.
- Link to v7: https://lore.kernel.org/r/20250218-module-params-v3-v7-0-5e1afabcac1b@kernel.org
Changes in v7:
- Remove dependency on `pr_warn_once` patches, replace with TODO.
- Rework `ParseInt::from_str` to avoid allocating.
- Add a comment explaining how we parse "0".
- Change trait bound on `Index` impl for `BStr` to match std library approach.
- Link to v6: https://lore.kernel.org/r/20250211-module-params-v3-v6-0-24b297ddc43d@kernel.org
Changes in v6:
- Fix a bug that prevented parsing of negative default values for
parameters in the `module!` macro.
- Fix a bug that prevented parsing zero in `strip_radix`. Also add a
test case for this.
- Add `AsRef<BStr>` for `[u8]` and `BStr`.
- Use `impl AsRef<BStr>` as type of prefix in `BStr::strip_prefix`.
- Link to v5: https://lore.kernel.org/r/20250204-module-params-v3-v5-0-bf5ec2041625@kernel.org
Changes in v5:
- Fix a typo in a safety comment in `set_param`.
- Use a match statement in `parse_int::strip_radix`.
- Add an implementation of `Index` for `BStr`.
- Fix a logic inversion bug where parameters would not be parsed.
- Use `kernel::ffi::c_char` in `set_param` rather than the one in `core`.
- Use `kernel::c_str!` rather than `c"..."` literal in module macro.
- Rebase on v6.14-rc1.
- Link to v4: https://lore.kernel.org/r/20250109-module-params-v3-v4-0-c208bcfbe11f@kernel.org
Changes in v4:
- Add module maintainers to Cc list (sorry)
- Add a few missing [`doc_links`]
- Add panic section to `expect_string_field`
- Fix a typo in safety requirement of `module_params::free`
- Change `assert!` to `pr_warn_once!` in `module_params::set_param`
- Remove `module_params::get_param` and install null pointer instead
- Remove use of the unstable feature `sync_unsafe_cell`
- Link to v3: https://lore.kernel.org/r/20241213-module-params-v3-v3-0-485a015ac2cf@kernel.org
Changes in v3:
- use `SyncUnsafeCell` rather than `static mut` and simplify parameter access
- remove `Display` bound from `ModuleParam`
- automatically generate documentation for `PARAM_OPS_.*`
- remove `as *const _ as *mut_` phrasing
- inline parameter name in struct instantiation in `emit_params`
- move `RacyKernelParam` out of macro template
- use C string literals rather than byte string literals with explicit null
- template out `__{name}_{param_name}` in `emit_param`
- indent template in `emit_params`
- use let-else expression in `emit_params` to get rid of an indentation level
- document `expect_string_field`
- move invication of `impl_int_module_param` to be closer to macro def
- move attributes after docs in `make_param_ops`
- rename `impl_module_param` to impl_int_module_param`
- use `ty` instead of `ident` in `impl_parse_int`
- use `BStr` instead of `&str` for string manipulation
- move string parsing functions to seperate patch and add examples, fix bugs
- degrade comment about future support from doc comment to regular comment
- remove std lib path from `Sized` marker
- update documentation for `trait ModuleParam`
- Link to v2: https://lore.kernel.org/all/20240819133345.3438739-1-nmi@metaspace.dk/
Changes in v2:
- Remove support for params without values (`NOARG_ALLOWED`).
- Improve documentation for `try_from_param_arg`.
- Use prelude import.
- Refactor `try_from_param_arg` to return `Result`.
- Refactor `ParseInt::from_str` to return `Result`.
- Move C callable functions out of `ModuleParam` trait.
- Rename literal string field parser to `expect_string_field`.
- Move parameter parsing from generation to parsing stage.
- Use absolute type paths in macro code.
- Inline `kparam`and `read_func` values.
- Resolve TODO regarding alignment attributes.
- Remove unnecessary unsafe blocks in macro code.
- Improve error message for unrecognized parameter types.
- Do not use `self` receiver when reading parameter value.
- Add parameter documentation to `module!` macro.
- Use empty `enum` for parameter type.
- Use `addr_of_mut` to get address of parameter value variable.
- Enabled building of docs for for `module_param` module.
- Link to v1: https://lore.kernel.org/rust-for-linux/20240705111455.142790-1-nmi@metaspace.dk/
---
Andreas Hindborg (7):
rust: sync: add `SetOnce`
rust: str: add radix prefixed integer parsing functions
rust: introduce module_param module
rust: module: use a reference in macros::module::module
rust: module: update the module macro with module parameter support
rust: samples: add a module parameter to the rust_minimal sample
modules: add rust modules files to MAINTAINERS
MAINTAINERS | 2 +
rust/kernel/lib.rs | 1 +
rust/kernel/module_param.rs | 181 ++++++++++++++++++++++++++++++++++++++++
rust/kernel/str.rs | 2 +
rust/kernel/str/parse_int.rs | 148 +++++++++++++++++++++++++++++++++
rust/kernel/sync.rs | 2 +
rust/kernel/sync/set_once.rs | 125 ++++++++++++++++++++++++++++
rust/macros/helpers.rs | 25 ++++++
rust/macros/lib.rs | 31 +++++++
rust/macros/module.rs | 194 +++++++++++++++++++++++++++++++++++++++----
samples/rust/rust_minimal.rs | 10 +++
11 files changed, 703 insertions(+), 18 deletions(-)
---
base-commit: 103265a1a936cfe910c9ac0f0ab153f7dac818ba
change-id: 20241211-module-params-v3-ae7e5c8d8b5a
Best regards,
--
Andreas Hindborg <a.hindborg@kernel.org>
^ permalink raw reply
* Re: [PATCH 1/4] PCI: Support FIXUP quirks in modules
From: Petr Pavlu @ 2025-09-24 7:48 UTC (permalink / raw)
To: Brian Norris
Cc: Bjorn Helgaas, Luis Chamberlain, Daniel Gomez, linux-pci,
David Gow, Rae Moar, linux-kselftest, linux-kernel, linux-modules,
Johannes Berg, Sami Tolvanen, Richard Weinberger, Wei Liu,
Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <aNLb9g0AbBXZCJ4m@google.com>
On 9/23/25 7:42 PM, Brian Norris wrote:
> Hi Petr,
>
> On Tue, Sep 23, 2025 at 02:55:34PM +0200, Petr Pavlu wrote:
>> On 9/13/25 12:59 AM, Brian Norris wrote:
>>> @@ -259,6 +315,12 @@ void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev)
>>> return;
>>> }
>>> pci_do_fixups(dev, start, end);
>>> +
>>> + struct pci_fixup_arg arg = {
>>> + .dev = dev,
>>> + .pass = pass,
>>> + };
>>> + module_for_each_mod(pci_module_fixup, &arg);
>>
>> The function module_for_each_mod() walks not only modules that are LIVE,
>> but also those in the COMING and GOING states. This means that this code
>> can potentially execute a PCI fixup from a module before its init
>> function is invoked, and similarly, a fixup can be executed after the
>> exit function has already run. Is this intentional?
>
> Thanks for the callout. I didn't really give this part much thought
> previously.
>
> Per the comments, COMING means "Full formed, running module_init". I
> believe that is a good thing, actually; specifically for controller
> drivers, module_init() might be probing the controller and enumerating
> child PCI devices to which we should apply these FIXUPs. That is a key
> case to support.
>
> GOING is not clearly defined in the header comments, but it seems like
> it's a relatively narrow window between determining there are no module
> refcounts (and transition to GOING) and starting to really tear it down
> (transitioning to UNFORMED before any significant teardown).
> module_exit() runs in the GOING phase.
>
> I think it does not make sense to execute FIXUPs on a GOING module; I'll
> make that change.
Note that when walking the modules list using module_for_each_mod(),
the delete_module() operation can concurrently transition a module to
MODULE_STATE_GOING. If you are thinking about simply having
pci_module_fixup() check that mod->state isn't MODULE_STATE_GOING,
I believe this won't quite work.
>
> Re-quoting one piece:
>> This means that this code
>> can potentially execute a PCI fixup from a module before its init
>> function is invoked,
>
> IIUC, this part is not true? A module is put into COMING state before
> its init function is invoked.
When loading a module, the load_module() function calls
complete_formation(), which puts the module into the COMING state. At
this point, the new code in pci_fixup_device() can see the new module
and potentially attempt to invoke its PCI fixups. However, such a module
has still a bit of way to go before its init function is called from
do_init_module(). The module hasn't yet had its arguments parsed, is not
linked in sysfs, isn't fully registered with codetag support, and hasn't
invoked its constructors (needed for gcov/kasan support).
I don't know enough about PCI fixups and what is allowable in them, but
I suspect it would be better to ensure that no fixup can be invoked from
the module during this period.
If the above makes sense, I think using module_for_each_mod() might not
be the right approach. Alternative options include registering a module
notifier or having modules explicitly register their PCI fixups in their
init function.
--
Cheers,
Petr
^ permalink raw reply
* Re: [PATCH v8 8/8] kbuild: vmlinux.unstripped should always depend on .vmlinux.export.o
From: Nicolas Schier @ 2025-09-24 6:39 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild
In-Reply-To: <0e63a9c7741fe8217e4fd7c60afcf057ffa2ef5a.1758182101.git.legion@kernel.org>
On Thu, Sep 18, 2025 at 10:05:52AM +0200, Alexey Gladkov wrote:
> Since .vmlinux.export.c is used to add generated by modpost modaliases
> for builtin modules the .vmlinux.export.o is no longer optional and
> should always be created. The generation of this file is not dependent
> on CONFIG_MODULES.
>
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
> ---
> scripts/Makefile.vmlinux | 9 ++-------
> scripts/link-vmlinux.sh | 5 +----
> 2 files changed, 3 insertions(+), 11 deletions(-)
>
Reviewed-by: Nicolas Schier <nsc@kernel.org>
--
Nicolas
^ permalink raw reply
* Re: [PATCH v8 7/8] modpost: Create modalias for builtin modules
From: Nicolas Schier @ 2025-09-24 6:38 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Masahiro Yamada, Stephen Rothwell
In-Reply-To: <28d4da3b0e3fc8474142746bcf469e03752c3208.1758182101.git.legion@kernel.org>
On Thu, Sep 18, 2025 at 10:05:51AM +0200, Alexey Gladkov wrote:
> For some modules, modalias is generated using the modpost utility and
> the section is added to the module file.
>
> When a module is added inside vmlinux, modpost does not generate
> modalias for such modules and the information is lost.
>
> As a result kmod (which uses modules.builtin.modinfo in userspace)
> cannot determine that modalias is handled by a builtin kernel module.
>
> $ cat /sys/devices/pci0000:00/0000:00:14.0/modalias
> pci:v00008086d0000A36Dsv00001043sd00008694bc0Csc03i30
>
> $ modinfo xhci_pci
> name: xhci_pci
> filename: (builtin)
> license: GPL
> file: drivers/usb/host/xhci-pci
> description: xHCI PCI Host Controller Driver
>
> Missing modalias "pci:v*d*sv*sd*bc0Csc03i30*" which will be generated by
> modpost if the module is built separately.
>
> To fix this it is necessary to generate the same modalias for vmlinux as
> for the individual modules. Fortunately '.vmlinux.export.o' is already
> generated from which '.modinfo' can be extracted in the same way as for
> vmlinux.o.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
> Tested-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
> include/linux/module.h | 4 ----
> scripts/Makefile.vmlinux | 4 +++-
> scripts/mksysmap | 3 +++
> scripts/mod/file2alias.c | 19 ++++++++++++++++++-
> scripts/mod/modpost.c | 15 +++++++++++++++
> scripts/mod/modpost.h | 2 ++
> 6 files changed, 41 insertions(+), 6 deletions(-)
>
Reviewed-by: Nicolas Schier <nsc@kernel.org>
--
Nicolas
^ permalink raw reply
* Re: [PATCH v8 6/8] modpost: Add modname to mod_device_table alias
From: Nicolas Schier @ 2025-09-24 6:31 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Miguel Ojeda, Andreas Hindborg, Danilo Krummrich, Alex Gaynor,
rust-for-linux
In-Reply-To: <1a0d0bd87a4981d465b9ed21e14f4e78eaa03ded.1758182101.git.legion@kernel.org>
On Thu, Sep 18, 2025 at 10:05:50AM +0200, Alexey Gladkov wrote:
> At this point, if a symbol is compiled as part of the kernel,
> information about which module the symbol belongs to is lost.
>
> To save this it is possible to add the module name to the alias name.
> It's not very pretty, but it's possible for now.
>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Andreas Hindborg <a.hindborg@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Alex Gaynor <alex.gaynor@gmail.com>
> Cc: rust-for-linux@vger.kernel.org
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> ---
> include/linux/module.h | 14 +++++++++++++-
> rust/kernel/device_id.rs | 8 ++++----
> scripts/mod/file2alias.c | 15 ++++++++++++---
> 3 files changed, 29 insertions(+), 8 deletions(-)
>
Acked-by: Nicolas Schier <nsc@kernel.org>
--
Nicolas
^ permalink raw reply
* Re: [PATCH v8 3/8] kbuild: keep .modinfo section in vmlinux.unstripped
From: Nicolas Schier @ 2025-09-24 6:29 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Nathan Chancellor, Petr Pavlu, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, linux-kernel, linux-modules, linux-kbuild,
Masahiro Yamada
In-Reply-To: <aaf67c07447215463300fccaa758904bac42f992.1758182101.git.legion@kernel.org>
On Thu, Sep 18, 2025 at 10:05:47AM +0200, Alexey Gladkov wrote:
> From: Masahiro Yamada <masahiroy@kernel.org>
>
> Keep the .modinfo section during linking, but strip it from the final
> vmlinux.
>
> Adjust scripts/mksysmap to exclude modinfo symbols from kallsyms.
>
> This change will allow the next commit to extract the .modinfo section
> from the vmlinux.unstripped intermediate.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
> ---
> include/asm-generic/vmlinux.lds.h | 2 +-
> scripts/Makefile.vmlinux | 7 +++++--
> scripts/mksysmap | 3 +++
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
Thanks!
Reviewed-by: Nicolas Schier <nsc@kernel.org>
--
Nicolas
^ permalink raw reply
* Re: [PATCH 1/4] PCI: Support FIXUP quirks in modules
From: Brian Norris @ 2025-09-23 17:42 UTC (permalink / raw)
To: Petr Pavlu
Cc: Bjorn Helgaas, Luis Chamberlain, Daniel Gomez, linux-pci,
David Gow, Rae Moar, linux-kselftest, linux-kernel, linux-modules,
Johannes Berg, Sami Tolvanen, Richard Weinberger, Wei Liu,
Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <c84d6952-7977-47cd-8f09-6ea223217337@suse.com>
Hi Petr,
On Tue, Sep 23, 2025 at 02:55:34PM +0200, Petr Pavlu wrote:
> On 9/13/25 12:59 AM, Brian Norris wrote:
> > @@ -259,6 +315,12 @@ void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev)
> > return;
> > }
> > pci_do_fixups(dev, start, end);
> > +
> > + struct pci_fixup_arg arg = {
> > + .dev = dev,
> > + .pass = pass,
> > + };
> > + module_for_each_mod(pci_module_fixup, &arg);
>
> The function module_for_each_mod() walks not only modules that are LIVE,
> but also those in the COMING and GOING states. This means that this code
> can potentially execute a PCI fixup from a module before its init
> function is invoked, and similarly, a fixup can be executed after the
> exit function has already run. Is this intentional?
Thanks for the callout. I didn't really give this part much thought
previously.
Per the comments, COMING means "Full formed, running module_init". I
believe that is a good thing, actually; specifically for controller
drivers, module_init() might be probing the controller and enumerating
child PCI devices to which we should apply these FIXUPs. That is a key
case to support.
GOING is not clearly defined in the header comments, but it seems like
it's a relatively narrow window between determining there are no module
refcounts (and transition to GOING) and starting to really tear it down
(transitioning to UNFORMED before any significant teardown).
module_exit() runs in the GOING phase.
I think it does not make sense to execute FIXUPs on a GOING module; I'll
make that change.
Re-quoting one piece:
> This means that this code
> can potentially execute a PCI fixup from a module before its init
> function is invoked,
IIUC, this part is not true? A module is put into COMING state before
its init function is invoked.
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -2702,6 +2702,32 @@ static int find_module_sections(struct module *mod, struct load_info *info)
> > sizeof(*mod->kunit_init_suites),
> > &mod->num_kunit_init_suites);
> > #endif
> > +#ifdef CONFIG_PCI_QUIRKS
> > + mod->pci_fixup_early = section_objs(info, ".pci_fixup_early",
> > + sizeof(*mod->pci_fixup_early),
> > + &mod->pci_fixup_early_size);
> > + mod->pci_fixup_header = section_objs(info, ".pci_fixup_header",
> > + sizeof(*mod->pci_fixup_header),
> > + &mod->pci_fixup_header_size);
> > + mod->pci_fixup_final = section_objs(info, ".pci_fixup_final",
> > + sizeof(*mod->pci_fixup_final),
> > + &mod->pci_fixup_final_size);
> > + mod->pci_fixup_enable = section_objs(info, ".pci_fixup_enable",
> > + sizeof(*mod->pci_fixup_enable),
> > + &mod->pci_fixup_enable_size);
> > + mod->pci_fixup_resume = section_objs(info, ".pci_fixup_resume",
> > + sizeof(*mod->pci_fixup_resume),
> > + &mod->pci_fixup_resume_size);
> > + mod->pci_fixup_suspend = section_objs(info, ".pci_fixup_suspend",
> > + sizeof(*mod->pci_fixup_suspend),
> > + &mod->pci_fixup_suspend_size);
> > + mod->pci_fixup_resume_early = section_objs(info, ".pci_fixup_resume_early",
> > + sizeof(*mod->pci_fixup_resume_early),
> > + &mod->pci_fixup_resume_early_size);
> > + mod->pci_fixup_suspend_late = section_objs(info, ".pci_fixup_suspend_late",
> > + sizeof(*mod->pci_fixup_suspend_late),
> > + &mod->pci_fixup_suspend_late_size);
> > +#endif
> >
> > mod->extable = section_objs(info, "__ex_table",
> > sizeof(*mod->extable), &mod->num_exentries);
>
> Nit: I suggest writing the object_size argument passed to section_objs()
> here directly as "1" instead of using sizeof(*mod->pci_fixup_...) =
> sizeof(void). This makes the style consistent with the other code in
> find_module_sections().
Ack.
Thanks,
Brian
^ permalink raw reply
* Re: [PATCH 0/4] PCI: Add support and tests for FIXUP quirks in modules
From: Manivannan Sadhasivam @ 2025-09-23 16:20 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Brian Norris, Bjorn Helgaas, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, linux-pci, David Gow, Rae Moar, linux-kselftest,
linux-kernel, linux-modules, Johannes Berg, Sami Tolvanen,
Richard Weinberger, Wei Liu, Brendan Higgins, kunit-dev,
Anton Ivanov, linux-um
In-Reply-To: <aNGR0x185VGHxSde@infradead.org>
On Mon, Sep 22, 2025 at 11:13:39AM -0700, Christoph Hellwig wrote:
> On Mon, Sep 15, 2025 at 11:41:37AM -0700, Brian Norris wrote:
> > I see fixups in controller drivers here:
> >
> > drivers/pci/controller/dwc/pci-imx6.c
> > drivers/pci/controller/dwc/pci-keystone.c
> > drivers/pci/controller/dwc/pcie-qcom.c
> > drivers/pci/controller/pci-loongson.c
> > drivers/pci/controller/pci-tegra.c
> > drivers/pci/controller/pcie-iproc-bcma.c
> > drivers/pci/controller/pcie-iproc.c
> >
> > Are any of those somehow wrong?
>
> When did we allow modular
> controller drivers anyway? That feels like a somewhat bad idea, too.
>
Why not? We currently only restrict the controller drivers implementing the
irqchip controller from being *removed* because of the IRQ disposal concern.
Other than that, I don't see why kernel should restrict building them as
modules.
- Mani
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply
* Re: [PATCH 1/4] PCI: Support FIXUP quirks in modules
From: Petr Pavlu @ 2025-09-23 12:55 UTC (permalink / raw)
To: Brian Norris
Cc: Bjorn Helgaas, Luis Chamberlain, Daniel Gomez, linux-pci,
David Gow, Rae Moar, linux-kselftest, linux-kernel, linux-modules,
Johannes Berg, Sami Tolvanen, Richard Weinberger, Wei Liu,
Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <20250912230208.967129-2-briannorris@chromium.org>
On 9/13/25 12:59 AM, Brian Norris wrote:
> The PCI framework supports "quirks" for PCI devices via several
> DECLARE_PCI_FIXUP_*() macros. These macros allow arch or driver code to
> match device IDs to provide customizations or workarounds for broken
> devices.
>
> This mechanism is generally used in code that can only be built into the
> kernel, but there are a few occasions where this mechanism is used in
> drivers that can be modules. For example, see commit 574f29036fce ("PCI:
> iproc: Apply quirk_paxc_bridge() for module as well as built-in").
>
> The PCI fixup mechanism only works for built-in code, however, because
> pci_fixup_device() only scans the ".pci_fixup_*" linker sections found
> in the main kernel; it never touches modules.
>
> Extend the fixup approach to modules.
>
> I don't attempt to be clever here; the algorithm here scales with the
> number of modules in the system.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>
> drivers/pci/quirks.c | 62 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/module.h | 18 ++++++++++++
> kernel/module/main.c | 26 ++++++++++++++++++
> 3 files changed, 106 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index d97335a40193..db5e0ac82ed7 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -207,6 +207,62 @@ extern struct pci_fixup __end_pci_fixups_suspend_late[];
>
> static bool pci_apply_fixup_final_quirks;
>
> +struct pci_fixup_arg {
> + struct pci_dev *dev;
> + enum pci_fixup_pass pass;
> +};
> +
> +static int pci_module_fixup(struct module *mod, void *parm)
> +{
> + struct pci_fixup_arg *arg = parm;
> + void *start;
> + unsigned int size;
> +
> + switch (arg->pass) {
> + case pci_fixup_early:
> + start = mod->pci_fixup_early;
> + size = mod->pci_fixup_early_size;
> + break;
> + case pci_fixup_header:
> + start = mod->pci_fixup_header;
> + size = mod->pci_fixup_header_size;
> + break;
> + case pci_fixup_final:
> + start = mod->pci_fixup_final;
> + size = mod->pci_fixup_final_size;
> + break;
> + case pci_fixup_enable:
> + start = mod->pci_fixup_enable;
> + size = mod->pci_fixup_enable_size;
> + break;
> + case pci_fixup_resume:
> + start = mod->pci_fixup_resume;
> + size = mod->pci_fixup_resume_size;
> + break;
> + case pci_fixup_suspend:
> + start = mod->pci_fixup_suspend;
> + size = mod->pci_fixup_suspend_size;
> + break;
> + case pci_fixup_resume_early:
> + start = mod->pci_fixup_resume_early;
> + size = mod->pci_fixup_resume_early_size;
> + break;
> + case pci_fixup_suspend_late:
> + start = mod->pci_fixup_suspend_late;
> + size = mod->pci_fixup_suspend_late_size;
> + break;
> + default:
> + return 0;
> + }
> +
> + if (!size)
> + return 0;
> +
> + pci_do_fixups(arg->dev, start, start + size);
> +
> + return 0;
> +}
> +
> void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev)
> {
> struct pci_fixup *start, *end;
> @@ -259,6 +315,12 @@ void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev)
> return;
> }
> pci_do_fixups(dev, start, end);
> +
> + struct pci_fixup_arg arg = {
> + .dev = dev,
> + .pass = pass,
> + };
> + module_for_each_mod(pci_module_fixup, &arg);
The function module_for_each_mod() walks not only modules that are LIVE,
but also those in the COMING and GOING states. This means that this code
can potentially execute a PCI fixup from a module before its init
function is invoked, and similarly, a fixup can be executed after the
exit function has already run. Is this intentional?
> }
> EXPORT_SYMBOL(pci_fixup_device);
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 3319a5269d28..7faa8987b9eb 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -539,6 +539,24 @@ struct module {
> int num_kunit_suites;
> struct kunit_suite **kunit_suites;
> #endif
> +#ifdef CONFIG_PCI_QUIRKS
> + void *pci_fixup_early;
> + unsigned int pci_fixup_early_size;
> + void *pci_fixup_header;
> + unsigned int pci_fixup_header_size;
> + void *pci_fixup_final;
> + unsigned int pci_fixup_final_size;
> + void *pci_fixup_enable;
> + unsigned int pci_fixup_enable_size;
> + void *pci_fixup_resume;
> + unsigned int pci_fixup_resume_size;
> + void *pci_fixup_suspend;
> + unsigned int pci_fixup_suspend_size;
> + void *pci_fixup_resume_early;
> + unsigned int pci_fixup_resume_early_size;
> + void *pci_fixup_suspend_late;
> + unsigned int pci_fixup_suspend_late_size;
> +#endif
>
>
> #ifdef CONFIG_LIVEPATCH
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b26184936..50a80c875adc 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2702,6 +2702,32 @@ static int find_module_sections(struct module *mod, struct load_info *info)
> sizeof(*mod->kunit_init_suites),
> &mod->num_kunit_init_suites);
> #endif
> +#ifdef CONFIG_PCI_QUIRKS
> + mod->pci_fixup_early = section_objs(info, ".pci_fixup_early",
> + sizeof(*mod->pci_fixup_early),
> + &mod->pci_fixup_early_size);
> + mod->pci_fixup_header = section_objs(info, ".pci_fixup_header",
> + sizeof(*mod->pci_fixup_header),
> + &mod->pci_fixup_header_size);
> + mod->pci_fixup_final = section_objs(info, ".pci_fixup_final",
> + sizeof(*mod->pci_fixup_final),
> + &mod->pci_fixup_final_size);
> + mod->pci_fixup_enable = section_objs(info, ".pci_fixup_enable",
> + sizeof(*mod->pci_fixup_enable),
> + &mod->pci_fixup_enable_size);
> + mod->pci_fixup_resume = section_objs(info, ".pci_fixup_resume",
> + sizeof(*mod->pci_fixup_resume),
> + &mod->pci_fixup_resume_size);
> + mod->pci_fixup_suspend = section_objs(info, ".pci_fixup_suspend",
> + sizeof(*mod->pci_fixup_suspend),
> + &mod->pci_fixup_suspend_size);
> + mod->pci_fixup_resume_early = section_objs(info, ".pci_fixup_resume_early",
> + sizeof(*mod->pci_fixup_resume_early),
> + &mod->pci_fixup_resume_early_size);
> + mod->pci_fixup_suspend_late = section_objs(info, ".pci_fixup_suspend_late",
> + sizeof(*mod->pci_fixup_suspend_late),
> + &mod->pci_fixup_suspend_late_size);
> +#endif
>
> mod->extable = section_objs(info, "__ex_table",
> sizeof(*mod->extable), &mod->num_exentries);
Nit: I suggest writing the object_size argument passed to section_objs()
here directly as "1" instead of using sizeof(*mod->pci_fixup_...) =
sizeof(void). This makes the style consistent with the other code in
find_module_sections().
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 0/6] module: enable force unloading of modules that have crashed during init
From: Petr Pavlu @ 2025-09-23 7:21 UTC (permalink / raw)
To: julian-lagattuta
Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, linux-modules,
linux-kernel
In-Reply-To: <20250918201109.24620-2-julian.lagattuta@gmail.com>
On 9/18/25 10:11 PM, julian-lagattuta wrote:
> Running a module that encounters a fatal error during the initcall leaves
> the module in a state where it cannot be forcefully unloaded since it is
> "being used" despite there being no reason it couldn't be unloaded.
> This means that unloading the crashed module requires rebooting.
>
> This patch allows modules that have crashed during initialization to be
> forcefully unloaded with CONFIG_MODULE_FORCE_UNLOAD enabled.
Could you please explain the motivation for doing this in more detail?
I think we shouldn't attempt to do anything clever with modules that
crashed during initialization. Such a module can already leave the
system in an unstable state and trying to recover can cause even more
problems. For instance, I don't see how it is safe to call the module's
exit function.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 0/4] PCI: Add support and tests for FIXUP quirks in modules
From: Brian Norris @ 2025-09-22 18:48 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Bjorn Helgaas, Luis Chamberlain, Petr Pavlu, Daniel Gomez,
linux-pci, David Gow, Rae Moar, linux-kselftest, linux-kernel,
linux-modules, Johannes Berg, Sami Tolvanen, Richard Weinberger,
Wei Liu, Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <aNGR0x185VGHxSde@infradead.org>
On Mon, Sep 22, 2025 at 11:13:39AM -0700, Christoph Hellwig wrote:
> Controller drivers are a special case I guess, but I'd rather still
> not open it up to any random driver.
I don't really see why this particular thing should develop restrictions
beyond "can it work in modules?", but if you have an idea for how to do
that reasonably, my ears are open.
> When did we allow modular
> controller drivers anyway?
An approximate count:
$ git grep tristate ./drivers/pci/controller/ | wc -l
39
There's been a steady trickle of module-related changes over the years.
And several modular controller drivers predate the
drivers/pci/controller/ creation in 2018 at commit 6e0832fa432e ("PCI:
Collect all native drivers under drivers/pci/controller/").
> That feels like a somewhat bad idea, too.
Any particular reason behind that feeling? Most other bus frameworks I'm
familiar with support modular drivers.
Brian
^ permalink raw reply
* Re: [PATCH 0/4] PCI: Add support and tests for FIXUP quirks in modules
From: Christoph Hellwig @ 2025-09-22 18:13 UTC (permalink / raw)
To: Brian Norris
Cc: Christoph Hellwig, Bjorn Helgaas, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, linux-pci, David Gow, Rae Moar, linux-kselftest,
linux-kernel, linux-modules, Johannes Berg, Sami Tolvanen,
Richard Weinberger, Wei Liu, Brendan Higgins, kunit-dev,
Anton Ivanov, linux-um
In-Reply-To: <aMhd4REssOE-AlYw@google.com>
On Mon, Sep 15, 2025 at 11:41:37AM -0700, Brian Norris wrote:
> I see fixups in controller drivers here:
>
> drivers/pci/controller/dwc/pci-imx6.c
> drivers/pci/controller/dwc/pci-keystone.c
> drivers/pci/controller/dwc/pcie-qcom.c
> drivers/pci/controller/pci-loongson.c
> drivers/pci/controller/pci-tegra.c
> drivers/pci/controller/pcie-iproc-bcma.c
> drivers/pci/controller/pcie-iproc.c
>
> Are any of those somehow wrong?
Controller drivers are a special case I guess, but I'd rather still
not open it up to any random driver. When did we allow modular
controller drivers anyway? That feels like a somewhat bad idea, too.
^ permalink raw reply
* Re: [RFC PATCH 00/10] scalable symbol flags with __kflagstab
From: Petr Pavlu @ 2025-09-22 11:41 UTC (permalink / raw)
To: Sid Nayyar
Cc: Nathan Chancellor, Luis Chamberlain, Sami Tolvanen,
Nicolas Schier, Arnd Bergmann, linux-kbuild, linux-arch,
linux-modules, linux-kernel, Giuliano Procida,
Matthias Männich
In-Reply-To: <CA+OvW8bhWK7prmyQMMJ_VYBeGMbn_mNiamHhUgYuCsnht+LFtA@mail.gmail.com>
On 9/15/25 5:53 PM, Sid Nayyar wrote:
> On Mon, Sep 8, 2025 at 11:09 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>> This sounds reasonable to me. Do you have any numbers on hand that would
>> show the impact of extending __ksymtab?
>
> I did performance analysis for module loading. The kflagstab
> optimizes symbol search, which accounts for less than 2% of the
> average module load time. Therefore, this change does not translate
> into any meaningful gains (or losses) in module loading performance.
>
> On the binary size side, the on-disk size for vmlinux is somewhat
> inflated due to extra entries in .symtab and .strtab. Since these
> sections are not part of the final Image, the only increase in the
> in-memory size of the kernel is for the kflagstab itself. This new
> table occupies 1 byte for each symbol in the ksymtab.
This is useful information. However, I was specifically interested in
the impact of having the new flags field present as part of __ksymtab
(kernel_symbol), compared to keeping it in a separate section. Sorry for
not being clear.
I ran a small test to get a better understanding of the different sizes.
I used v6.17-rc6 together with the openSUSE x86_64 config [1], which is
fairly large. The resulting vmlinux.bin (no debuginfo) had an on-disk
size of 58 MiB, and included 5937 + 6589 (GPL-only) exported symbols.
The following table summarizes my measurements and calculations
regarding the sizes of all sections related to exported symbols:
| HAVE_ARCH_PREL32_RELOCATIONS | !HAVE_ARCH_PREL32_RELOCATIONS
Section | Base [B] | Ext. [B] | Sep. [B] | Base [B] | Ext. [B] | Sep. [B]
----------------------------------------------------------------------------------------
__ksymtab | 71244 | 200416 | 150312 | 142488 | 400832 | 300624
__ksymtab_gpl | 79068 | NA | NA | 158136 | NA | NA
__kcrctab | 23748 | 50104 | 50104 | 23748 | 50104 | 50104
__kcrctab_gpl | 26356 | NA | NA | 26356 | NA | NA
__ksymtab_strings | 253628 | 253628 | 253628 | 253628 | 253628 | 253628
__kflagstab | NA | NA | 12526 | NA | NA | 12526
----------------------------------------------------------------------------------------
Total | 454044 | 504148 | 466570 | 604356 | 704564 | 616882
Increase to base [%] | NA | 11.0 | 2.8 | NA | 16.6 | 2.1
The column "HAVE_ARCH_PREL32_RELOCATIONS -> Base" contains the numbers
that I measured. The rest of the values are calculated. The "Ext."
column represents the variant of extending __ksymtab, and the "Sep."
column represents the variant of having a separate __kflagstab. With
HAVE_ARCH_PREL32_RELOCATIONS, each kernel_symbol is 12 B in size and is
extended to 16 B. With !HAVE_ARCH_PREL32_RELOCATIONS, it is 24 B,
extended to 32 B. Note that this does not include the metadata needed to
relocate __ksymtab*, which is freed after the initial processing.
The base export data in this case totals 0.43 MiB. About 50% is used for
storing the names of exported symbols.
Adding __kflagstab as a separate section has a negligible impact, as
expected. When extending __ksymtab (kernel_symbol) instead, the worst
case with !HAVE_ARCH_PREL32_RELOCATIONS increases the export data size
by 16.6%.
Based on the above, I think introducing __kflagstab makes senses, as the
added complexity is minimal, although I feel we could probably also get
away with extending kernel_symbol.
>
>>> The Android Common Kernel source is compiled into what we call
>>> GKI (Generic Kernel Image), which consists of a kernel and a
>>> number of modules. We maintain a stable interface (based on CRCs and
>>> types) between the GKI components and vendor-specific modules
>>> (compiled by device manufacturers, e.g., for hardware-specific
>>> drivers) for the lifetime of a given GKI version.
>>>
>>> This interface is intentionally restricted to the minimal set of
>>> symbols required by the union of all vendor modules; our partners
>>> declare their requirements in symbol lists. Any additions to these
>>> lists are reviewed to ensure kernel internals are not overly exposed.
>>> For example, we restrict drivers from having the ability to open and
>>> read arbitrary files. This ABI boundary also allows us to evolve
>>> internal kernel types that are not exposed to vendor modules, for
>>> example, when a security fix requires a type to change.
>>>
>>> The mechanism we use for this is CONFIG_TRIM_UNUSED_KSYMS and
>>> CONFIG_UNUSED_KSYMS_WHITELIST. This results in a ksymtab
>>> containing two kinds of exported symbols: those explicitly required
>>> by vendors ("vendor-listed") and those only required by GKI modules
>>> ("GKI use only").
>>>
>>> On top of this, we have implemented symbol import protection
>>> (covered in patches 9/10 and 10/10). This feature prevents vendor
>>> modules from using symbols that are not on the vendor-listed
>>> whitelist. It is built on top of CONFIG_MODULE_SIG. GKI modules are
>>> signed with a specific key, while vendor modules are unsigned and thus
>>> treated as untrusted. This distinction allows signed GKI modules to
>>> use any symbol in the ksymtab, while unsigned vendor modules can only
>>> access the declared subset. This provides a significant layer of
>>> defense and security against potentially exploitable vendor module
>>> code.
>>
>> If I understand correctly, this is similar to the recently introduced
>> EXPORT_SYMBOL_FOR_MODULES() macro, but with a coarser boundary.
>>
>> I think that if the goal is to control the kABI scope and limit the use
>> of certain symbols only to GKI modules, then having the protection
>> depend on whether the module is signed is somewhat odd. It doesn't give
>> me much confidence if vendor modules are unsigned in the Android
>> ecosystem. I would expect that you want to improve this in the long
>> term.
>
> GKI modules are the only modules built in the same Kbuild as the
> kernel image, which Google builds and provides to partners. In
> contrast, vendor modules are built and packaged entirely by partners.
>
> Google signs GKI modules with ephemeral keys. Since partners do
> not have these keys, vendor modules are treated as unsigned by
> the kernel.
>
> To ensure the authenticity of these unsigned modules, partners
> package them into a separate image that becomes one of the boot
> partitions. This entire image is signed, and its signature is
> verified by the bootloader at boot time.
>
>> It would then make more sense to me if the protection was determined by
>> whether the module is in-tree (the "intree" flag in modinfo) or,
>> alternatively, if it is signed by a built-in trusted key. I feel this
>> way the feature could be potentially useful for other distributions that
>> care about the kABI scope and have ecosystems where vendor modules are
>> properly signed with some key. However, I'm not sure if this would still
>> work in your case.
>
> Partners can produce both in-tree and out-of-tree modules. We do not
> trust either type regarding symbol exposure, as there is no way to know
> exactly what sources were used. Furthermore, symbols exported via
> EXPORT_SYMBOL_FOR_MODULES can be accessed by any vendor module that
> mimics the GKI module name.
>
> Therefore, neither the in-tree flag nor the EXPORT_SYMBOL_FOR_MODULES
> mechanism provides a strong enough guarantee for the Android kernel to
> identify GKI modules.
>
> Only module signatures are sufficient to allow a module to access the
> full set of exported symbols. Unsigned vendor modules may only access
> the symbol subset declared ahead of time by partners.
This seems to answer why the in-tree flag is not sufficient for you.
However, I also suggested an alternative that the symbol protection
could be determined by whether the module is signed by a key from the
.builtin_trusted_keys keyring, as opposed to being signed by another key
reachable from the .secondary_trusted_keys keyring or being completely
unsigned.
Distributions can require that external modules be signed and allow
additional keys to be added as Machine Owner Keys, which can be made
reachable from .secondary_trusted_keys. Nonetheless, such distributions
might be still interested in limiting the number of symbols that such
external modules can use.
I think this option is worth considering, as it could potentially make
this symbol protection useful for other distributions as well.
>
> In case such symbol protection is not useful for the Linux community, I
> am happy to keep this as an Android-specific feature. However, I would
> urge you to at least accept the kflagstab, as it allows us (and
> potentially other Linux distributions) to easily introduce additional
> flags for symbols. It is also a simplification/clean-up of the module
> loader code.
I'm personally ok with adding the kflagstab support. I think it
introduces minimal complexity and, as you point out, simplifies certain
aspects. Additionally, if we add it, I believe that adding the proposed
symbol protection is simple enough to be included as well, at least from
my perspective.
[1] https://github.com/openSUSE/kernel-source/blob/307f149d9100a0e229eb94cbb997ae61187995c3/config/x86_64/default
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH v4 3/5] ALSA: usb-audio: improve module param quirk_flags
From: Takashi Iwai @ 2025-09-19 12:47 UTC (permalink / raw)
To: cryolitia
Cc: Jaroslav Kysela, Takashi Iwai, Jonathan Corbet, Luis Chamberlain,
Petr Pavlu, Daniel Gomez, Sami Tolvanen, linux-sound, linux-usb,
linux-kernel, linux-doc, Mingcong Bai, Kexy Biscuit, Nie Cheng,
Zhan Jun, Feng Yuan, qaqland, kernel, linux-modules, Takashi Iwai
In-Reply-To: <20250918-sound-v4-3-82cf8123d61c@uniontech.com>
On Thu, 18 Sep 2025 11:24:32 +0200,
Cryolitia PukNgae via B4 Relay wrote:
>
> From: Cryolitia PukNgae <cryolitia@uniontech.com>
>
> For apply and unapply quirk flags more flexibly though param
>
> Co-developed-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
> ---
> sound/usb/card.c | 9 ++--
> sound/usb/quirks.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> sound/usb/quirks.h | 3 +-
> sound/usb/usbaudio.h | 3 ++
> 4 files changed, 126 insertions(+), 8 deletions(-)
>
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index 0265206a8e8cf31133e8463c98fe0497d8ace89e..5837677effa1787ef9d7d02714c3ae43b1a8bc79 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -73,8 +73,8 @@ static bool lowlatency = true;
> static char *quirk_alias[SNDRV_CARDS];
> static char *delayed_register[SNDRV_CARDS];
> static bool implicit_fb[SNDRV_CARDS];
> -static unsigned int quirk_flags[SNDRV_CARDS];
>
> +char *quirk_flags[SNDRV_CARDS];
My preference is to keep this static, but pass the string value to a
function. That is, define snd_usb_init_quirk_flags() in main.c:
static void snd_usb_init_quirk_flags(struct snd_usb_audio *chip, int indx)
{
/* old style option found: the position-based integer value */
if (quirk_flags[idx] &&
!kstrtou32(quirk_flags[idx], 0, &chip->quirk_flags)) {
usb_audio_dbg(chip,
"Set quirk flags 0x%x from param based on position %d for device %04x:%04x\n",
chip->quirk_flags, idx,
USB_ID_VENDOR(chip->usb_id),
USB_ID_PRODUCT(chip->usb_id));
return;
}
/* take the default quirk from the quirk table */
snd_usb_init_quirk_flags_table(chip);
/* add or correct quirk bits from options */
for (i = 0; i < ARRAY_SIZE(quirk_flags); i++) {
char *val __free(kfree) = NULL;
if (!quirk_flags[i] || !*quirk_flags[i])
continue;
val = kstrdup(quirk_flags[i], GFP_KERNEL);
if (!val)
return;
snd_usb_parse_quirk_string(chip, val);
}
}
static int snd_usb_audio_create(....)
{
....
snd_usb_init_quirk_flags(chip, idx);
....
}
The function snd_usb_parse_quirk_string() is defined in quirks.c,
void snd_usb_parse_quirk_string(struct snd_usb_audio *chip, char *val)
{
for (p = val; p && *p;) {
/* Each entry consists of VID:PID:flags */
field = strsep(&p, ":");
if (!field)
break;
.....
}
}
thanks,
Takashi
^ permalink raw reply
* Re: [PATCH v4 2/5] param: export param_array related functions
From: Takashi Iwai @ 2025-09-19 12:37 UTC (permalink / raw)
To: cryolitia
Cc: Jaroslav Kysela, Takashi Iwai, Jonathan Corbet, Luis Chamberlain,
Petr Pavlu, Daniel Gomez, Sami Tolvanen, linux-sound, linux-usb,
linux-kernel, linux-doc, Mingcong Bai, Kexy Biscuit, Nie Cheng,
Zhan Jun, Feng Yuan, qaqland, kernel, linux-modules
In-Reply-To: <20250918-sound-v4-2-82cf8123d61c@uniontech.com>
On Thu, 18 Sep 2025 11:24:31 +0200,
Cryolitia PukNgae via B4 Relay wrote:
>
> From: Cryolitia PukNgae <cryolitia@uniontech.com>
>
> - int param_array_set(const char *val, const struct kernel_param *kp);
> - int param_array_get(char *buffer, const struct kernel_param *kp);
> - void param_array_free(void *arg);
>
> It would be helpful for the new module param we designed in
> snd_usb_audio, in order to run additional custom codes when params
> are set in runtime, and re-use the extisted codes in param.c
>
> Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
Can we do just like below?
static int param_set_quirkp(const char *val, const struct kernel_param *kp)
{
guard(mutex)(&quirk_flags_mutex);
return param_set_charp(val, kp);
}
static const struct kernel_param_ops param_ops_quirkp = {
.set = param_set_quirkp,
.get = param_get_charp,
.free = param_free_charp,
};
#define param_check_quirkp param_check_charp
modules_param_parray(quirk_flags, quirkp, NULL, 0644);
Then mutex is locked at each time a parameter is set.
Optionally, the string value can be verified in param_set_quirkp()
before passing to param_set_charp(), too.
thanks,
Takashi
^ permalink raw reply
* Re: [PATCH v4 1/5] ALSA: usb-audio: add two-way convert between name and bit for QUIRK_FLAG_*
From: Takashi Iwai @ 2025-09-19 12:32 UTC (permalink / raw)
To: cryolitia
Cc: Jaroslav Kysela, Takashi Iwai, Jonathan Corbet, Luis Chamberlain,
Petr Pavlu, Daniel Gomez, Sami Tolvanen, linux-sound, linux-usb,
linux-kernel, linux-doc, Mingcong Bai, Kexy Biscuit, Nie Cheng,
Zhan Jun, Feng Yuan, qaqland, kernel, linux-modules
In-Reply-To: <20250918-sound-v4-1-82cf8123d61c@uniontech.com>
On Thu, 18 Sep 2025 11:24:30 +0200,
Cryolitia PukNgae via B4 Relay wrote:
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -2446,6 +2446,62 @@ static const struct usb_audio_quirk_flags_table quirk_flags_table[] = {
> {} /* terminator */
> };
>
> +static const char *const snd_usb_audio_quirk_flag_names[] = {
> + "get_sample_rate",
> + "share_media_device",
I think it's worth to add a comment that this each entry corresponds
to QUIRK_FLAG_XXX. Or another idea would be to define enums like:
enum {
QUIRK_TYPE_GET_SAMPLE_RATE,
QUIRK_TYPE_SHARE_MEDIA_DEVICE,
....
};
then redefine QUIRK_FLAG_* like:
#define QUIRK_FLAG_GET_SAMPLE_RATE BIT_U32(QUIRK_TYPE_GET_SAMPLE_RATE)
#define QUIRK_FLAG_SHARE_MEDIA_DEVICE BIT_U32(QUIRK_TYPE_SHARE_MEDIA_DEVICE)
....
or
#define QUIRK_FLAG(x) BIT_U32(QUIRK_TYPE_ ## x)
and use like QUIRK_FLAG(GET_SAMPLE_RATE).
With those changes, the above can be defined more safely like
static const char *const snd_usb_audio_quirk_flag_names[] = {
[QUIRK_TYPE_GET_SAMPLE_RATE] = "get_sample_rate",
....
or even more drastically by defining some macro for each entry like:
#define QUIRK_STRING_ENTRY(x) \
[QUIRK_TYPE_ ## x] = __stringify(x)
and put like:
static const char *const snd_usb_audio_quirk_flag_names[] = {
QUIRK_STRING_ENTRY(GET_SAMPLE_RATE),
....
};
In this case, it'll become upper letters, so the parse would need to
deal with the case-insensitive comparison, though.
> +u32 snd_usb_quirk_flags_from_name(char *name)
Use const char *.
> +{
> + u32 flag = 0;
> + u32 i;
The iterator can be simple int.
> + if (!name || !*name)
> + return 0;
> +
> + for (i = 0; snd_usb_audio_quirk_flag_names[i]; i++) {
> + if (strcmp(name, snd_usb_audio_quirk_flag_names[i]) == 0) {
> + flag = (1U << i);
Use BIT_U32(i)
> + break;
We can return the value directly, so flag variable can be dropped.
> void snd_usb_init_quirk_flags(struct snd_usb_audio *chip)
> {
> const struct usb_audio_quirk_flags_table *p;
> @@ -2454,10 +2510,28 @@ void snd_usb_init_quirk_flags(struct snd_usb_audio *chip)
> if (chip->usb_id == p->id ||
> (!USB_ID_PRODUCT(p->id) &&
> USB_ID_VENDOR(chip->usb_id) == USB_ID_VENDOR(p->id))) {
> - usb_audio_dbg(chip,
> - "Set quirk_flags 0x%x for device %04x:%04x\n",
> - p->flags, USB_ID_VENDOR(chip->usb_id),
> - USB_ID_PRODUCT(chip->usb_id));
> + unsigned long flags = p->flags;
> + unsigned long bit;
> +
> + for_each_set_bit(bit, &flags,
> + BYTES_TO_BITS(sizeof(p->flags))) {
> + const char *name =
> + snd_usb_audio_quirk_flag_names[bit];
> +
> + if (name)
> + usb_audio_dbg(chip,
> + "Set quirk flag %s for device %04x:%04x\n",
> + name,
> + USB_ID_VENDOR(chip->usb_id),
> + USB_ID_PRODUCT(chip->usb_id));
> + else
> + usb_audio_warn(chip,
> + "Set unknown quirk flag 0x%lx for device %04x:%04x\n",
> + bit,
> + USB_ID_VENDOR(chip->usb_id),
> + USB_ID_PRODUCT(chip->usb_id));
> + }
This could be better factored out as a function.
thanks,
Takashi
^ permalink raw reply
* Re: [PATCH v2 1/1] module: replace use of system_wq with system_dfl_wq
From: Petr Pavlu @ 2025-09-19 8:18 UTC (permalink / raw)
To: Marco Crivellari
Cc: Tejun Heo, Lai Jiangshan, Frederic Weisbecker,
Sebastian Andrzej Siewior, Michal Hocko, Luis Chamberlain,
linux-kernel, linux-modules
In-Reply-To: <20250918085525.122429-2-marco.crivellari@suse.com>
On 9/18/25 10:55 AM, Marco Crivellari wrote:
> Currently if a user enqueue a work item using schedule_delayed_work() the
> used wq is "system_wq" (per-cpu wq) while queue_delayed_work() use
> WORK_CPU_UNBOUND (used when a cpu is not specified). The same applies to
> schedule_work() that is using system_wq and queue_work(), that makes use
> again of WORK_CPU_UNBOUND.
>
> This lack of consistentcy cannot be addressed without refactoring the API.
>
> This specific patch replace system_wq with system_dfl_wq, the new unbound
> workqueue, because the users does not benefit from a per-cpu wq.
>
> Suggested-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH v4 5/5] ALSA: doc: add docs about improved quirk_flags in snd-usb-audio
From: Cryolitia PukNgae @ 2025-09-19 1:43 UTC (permalink / raw)
To: Randy Dunlap, Jaroslav Kysela, Takashi Iwai, Jonathan Corbet,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen
Cc: linux-sound, linux-usb, linux-kernel, linux-doc, Mingcong Bai,
Kexy Biscuit, Nie Cheng, Zhan Jun, Feng Yuan, qaqland, kernel,
linux-modules
In-Reply-To: <dcbd2c62-5db8-4eb5-aa3a-532b33baaa61@infradead.org>
Thanks for your review. I'll waiting some reviews on other patches and resend
a new version about it.
On 19/09/2025 04.21, Randy Dunlap wrote:
> Hi--
>
> On 9/18/25 2:24 AM, Cryolitia PukNgae via B4 Relay wrote:
>> From: Cryolitia PukNgae <cryolitia@uniontech.com>
>>
>> Just briefly described about the option.
>>
>> Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
>> ---
>> Documentation/sound/alsa-configuration.rst | 108 ++++++++++++++++++++---------
>> 1 file changed, 75 insertions(+), 33 deletions(-)
>>
>> diff --git a/Documentation/sound/alsa-configuration.rst b/Documentation/sound/alsa-configuration.rst
>> index a2fb8ed251dd0294e7a62209ca15d5c32c6adfae..efffe3d534beeddcb6a47ac27a24defb6879f534 100644
>> --- a/Documentation/sound/alsa-configuration.rst
>> +++ b/Documentation/sound/alsa-configuration.rst
>> @@ -2297,39 +2297,81 @@ skip_validation
>> of the unit descriptor instead of a driver probe error, so that we
>> can check its details.
>> quirk_flags
>> - Contains the bit flags for various device specific workarounds.
>> - Applied to the corresponding card index.
>> -
>> - * bit 0: Skip reading sample rate for devices
>> - * bit 1: Create Media Controller API entries
>> - * bit 2: Allow alignment on audio sub-slot at transfer
>> - * bit 3: Add length specifier to transfers
>> - * bit 4: Start playback stream at first in implement feedback mode
>> - * bit 5: Skip clock selector setup
>> - * bit 6: Ignore errors from clock source search
>> - * bit 7: Indicates ITF-USB DSD based DACs
>> - * bit 8: Add a delay of 20ms at each control message handling
>> - * bit 9: Add a delay of 1-2ms at each control message handling
>> - * bit 10: Add a delay of 5-6ms at each control message handling
>> - * bit 11: Add a delay of 50ms at each interface setup
>> - * bit 12: Perform sample rate validations at probe
>> - * bit 13: Disable runtime PM autosuspend
>> - * bit 14: Ignore errors for mixer access
>> - * bit 15: Support generic DSD raw U32_BE format
>> - * bit 16: Set up the interface at first like UAC1
>> - * bit 17: Apply the generic implicit feedback sync mode
>> - * bit 18: Don't apply implicit feedback sync mode
>> - * bit 19: Don't closed interface during setting sample rate
>> - * bit 20: Force an interface reset whenever stopping & restarting
>> - a stream
>> - * bit 21: Do not set PCM rate (frequency) when only one rate is
>> - available for the given endpoint.
>> - * bit 22: Set the fixed resolution 16 for Mic Capture Volume
>> - * bit 23: Set the fixed resolution 384 for Mic Capture Volume
>> - * bit 24: Set minimum volume control value as mute for devices
>> - where the lowest playback value represents muted state instead
>> - of minimum audible volume
>> - * bit 25: Be similar to bit 24 but for capture streams
>> + The option provides a refined and flexible control for applying quirk
>> + flags. It allows to specify the quirk flags for each device, and could
>
> and may
> or: and can
>
>> + be modified dynamically via sysfs.
>> + The old usage accepts an array of integers, each of which apply quirk
>
> applies
>
>> + flags on the device in the order of probing.
>> + e.g. ``quirk_flags=0x01,0x02`` applies get_sample_rate to the first
>
> E.g.,
>
>> + device, and share_media_device to the second device.
>> + The new usage accepts a string in the format of
>> + ``VID1:PID1:FLAGS1;VID2:PID2:FLAGS2;...``, where ``VIDx`` and ``PIDx``
>> + specify the device, and ``FLAGSx`` specify the flags to be applied.
>> + ``VIDx`` and ``PIDx`` are 4-digit hexadecimal numbers, and could be
>
> s/could/may/
>
>> + specified as ``*`` to match any value. ``FLAGSx`` could be a set of
>
> s/could/may/
>
>> + flags given by name, separated by ``|``, or a hexadecimal number
>> + representing the bit flags. The available flag names are listed above.
>
> s/above/below/ ?
>
>> + An exclamation mark could be prefixed to a flag name to negate the flag.
> s/could/may/
>
>> + For example, ``1234:abcd:mixer_playback_min_mute|!ignore_ctl_error;*:*:0x01;``
>
> What happens if the trailing (ending) ';' is omitted?
This is where I found something strange when I was testing it myself. When I use
`echo '*:*:mixer_playback_min_mute` > /sys/module/snd_usb_audio/parameters/quirk_flags`,
the string received by the driver contains a lot of trailing spaces. I am not familiar
with the kernel and don't know how to handle this situation. Simply adding a semicolon
to the input will make the trailing spaces after the semicolon be ignored.
>
>> + applies the ``mixer_playback_min_mute`` flag and clears the
>> + ``ignore_ctl_error`` flag for the device 1234:abcd, and applies the
>> + ``skip_sample_rate`` flag for all devices.
>> +
>> + * bit 0: ``get_sample_rate``
>> + Skip reading sample rate for devices
>
> get vs Skip is a little confusing.
This part is copied from Takashi Iwai[1]. It does a little confusing.
>> + * bit 1: ``share_media_device``
>> + Create Media Controller API entries
>> + * bit 2: ``align_transfer``
>> + Allow alignment on audio sub-slot at transfer
>> + * bit 3: ``tx_length``
>> + Add length specifier to transfers
>> + * bit 4: ``playback_first``
>> + Start playback stream at first in implement feedback mode
>> + * bit 5: ``skip_clock_selector``
>> + Skip clock selector setup
>> + * bit 6: ``ignore_clock_source``
>> + Ignore errors from clock source search
>> + * bit 7: ``itf_usb_dsd_dac``
>> + Indicates ITF-USB DSD based DACs
>
> DSD-based
>
>> + * bit 8: ``ctl_msg_delay``
>> + Add a delay of 20ms at each control message handling
>> + * bit 9: ``ctl_msg_delay_1m``
>> + Add a delay of 1-2ms at each control message handling
>> + * bit 10: ``ctl_msg_delay_5m``
>> + Add a delay of 5-6ms at each control message handling
>> + * bit 11: ``iface_delay``
>> + Add a delay of 50ms at each interface setup
>> + * bit 12: ``validate_rates``
>> + Perform sample rate validations at probe
>> + * bit 13: ``disable_autosuspend``
>> + Disable runtime PM autosuspend
>> + * bit 14: ``ignore_ctl_error``
>> + Ignore errors for mixer access
>> + * bit 15: ``dsd_raw``
>> + Support generic DSD raw U32_BE format
>> + * bit 16: ``set_iface_first``
>> + Set up the interface at first like UAC1
>> + * bit 17: ``generic_implicit_fb``
>> + Apply the generic implicit feedback sync mode
>> + * bit 18: ``skip_implicit_fb``
>> + Don't apply implicit feedback sync mode
>> + * bit 19: ``iface_skip_close``
>> + Don't closed interface during setting sample rate
>
> close
>
>> + * bit 20: ``force_iface_reset``
>> + Force an interface reset whenever stopping & restarting a stream
>> + * bit 21: ``fixed_rate``
>> + Do not set PCM rate (frequency) when only one rate is available
>> + for the given endpoint
>> + * bit 22: ``mic_res_16``
>> + Set the fixed resolution 16 for Mic Capture Volume
>> + * bit 23: ``mic_res_384``
>> + Set the fixed resolution 384 for Mic Capture Volume
>> + * bit 24: ``mixer_playback_min_mute``
>> + Set minimum volume control value as mute for devices where the
>> + lowest playback value represents muted state instead of minimum
>> + audible volume
>> + * bit 25: ``mixer_capture_min_mute``
>> + Be similar to bit 24 but for capture streams
>
> Similar to
>
>>
>> This module supports multiple devices, autoprobe and hotplugging.
>>
>> Are all of these quirks used on various devices or are some of these
> just implemented just in case they are needed in the future?thanks.
I believe in that all of them are used in quirk_flags_table in sound/usb/quirks.c
1. https://lore.kernel.org/all/20210729073855.19043-2-tiwai@suse.de/
Best regards,
Cryolitia
^ permalink raw reply
* Re: [PATCH v4 5/5] ALSA: doc: add docs about improved quirk_flags in snd-usb-audio
From: Randy Dunlap @ 2025-09-18 20:21 UTC (permalink / raw)
To: cryolitia, Jaroslav Kysela, Takashi Iwai, Jonathan Corbet,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen
Cc: linux-sound, linux-usb, linux-kernel, linux-doc, Mingcong Bai,
Kexy Biscuit, Nie Cheng, Zhan Jun, Feng Yuan, qaqland, kernel,
linux-modules
In-Reply-To: <20250918-sound-v4-5-82cf8123d61c@uniontech.com>
Hi--
On 9/18/25 2:24 AM, Cryolitia PukNgae via B4 Relay wrote:
> From: Cryolitia PukNgae <cryolitia@uniontech.com>
>
> Just briefly described about the option.
>
> Signed-off-by: Cryolitia PukNgae <cryolitia@uniontech.com>
> ---
> Documentation/sound/alsa-configuration.rst | 108 ++++++++++++++++++++---------
> 1 file changed, 75 insertions(+), 33 deletions(-)
>
> diff --git a/Documentation/sound/alsa-configuration.rst b/Documentation/sound/alsa-configuration.rst
> index a2fb8ed251dd0294e7a62209ca15d5c32c6adfae..efffe3d534beeddcb6a47ac27a24defb6879f534 100644
> --- a/Documentation/sound/alsa-configuration.rst
> +++ b/Documentation/sound/alsa-configuration.rst
> @@ -2297,39 +2297,81 @@ skip_validation
> of the unit descriptor instead of a driver probe error, so that we
> can check its details.
> quirk_flags
> - Contains the bit flags for various device specific workarounds.
> - Applied to the corresponding card index.
> -
> - * bit 0: Skip reading sample rate for devices
> - * bit 1: Create Media Controller API entries
> - * bit 2: Allow alignment on audio sub-slot at transfer
> - * bit 3: Add length specifier to transfers
> - * bit 4: Start playback stream at first in implement feedback mode
> - * bit 5: Skip clock selector setup
> - * bit 6: Ignore errors from clock source search
> - * bit 7: Indicates ITF-USB DSD based DACs
> - * bit 8: Add a delay of 20ms at each control message handling
> - * bit 9: Add a delay of 1-2ms at each control message handling
> - * bit 10: Add a delay of 5-6ms at each control message handling
> - * bit 11: Add a delay of 50ms at each interface setup
> - * bit 12: Perform sample rate validations at probe
> - * bit 13: Disable runtime PM autosuspend
> - * bit 14: Ignore errors for mixer access
> - * bit 15: Support generic DSD raw U32_BE format
> - * bit 16: Set up the interface at first like UAC1
> - * bit 17: Apply the generic implicit feedback sync mode
> - * bit 18: Don't apply implicit feedback sync mode
> - * bit 19: Don't closed interface during setting sample rate
> - * bit 20: Force an interface reset whenever stopping & restarting
> - a stream
> - * bit 21: Do not set PCM rate (frequency) when only one rate is
> - available for the given endpoint.
> - * bit 22: Set the fixed resolution 16 for Mic Capture Volume
> - * bit 23: Set the fixed resolution 384 for Mic Capture Volume
> - * bit 24: Set minimum volume control value as mute for devices
> - where the lowest playback value represents muted state instead
> - of minimum audible volume
> - * bit 25: Be similar to bit 24 but for capture streams
> + The option provides a refined and flexible control for applying quirk
> + flags. It allows to specify the quirk flags for each device, and could
and may
or: and can
> + be modified dynamically via sysfs.
> + The old usage accepts an array of integers, each of which apply quirk
applies
> + flags on the device in the order of probing.
> + e.g. ``quirk_flags=0x01,0x02`` applies get_sample_rate to the first
E.g.,
> + device, and share_media_device to the second device.
> + The new usage accepts a string in the format of
> + ``VID1:PID1:FLAGS1;VID2:PID2:FLAGS2;...``, where ``VIDx`` and ``PIDx``
> + specify the device, and ``FLAGSx`` specify the flags to be applied.
> + ``VIDx`` and ``PIDx`` are 4-digit hexadecimal numbers, and could be
s/could/may/
> + specified as ``*`` to match any value. ``FLAGSx`` could be a set of
s/could/may/
> + flags given by name, separated by ``|``, or a hexadecimal number
> + representing the bit flags. The available flag names are listed above.
s/above/below/ ?
> + An exclamation mark could be prefixed to a flag name to negate the flag.
s/could/may/
> + For example, ``1234:abcd:mixer_playback_min_mute|!ignore_ctl_error;*:*:0x01;``
What happens if the trailing (ending) ';' is omitted?
> + applies the ``mixer_playback_min_mute`` flag and clears the
> + ``ignore_ctl_error`` flag for the device 1234:abcd, and applies the
> + ``skip_sample_rate`` flag for all devices.
> +
> + * bit 0: ``get_sample_rate``
> + Skip reading sample rate for devices
get vs Skip is a little confusing.
> + * bit 1: ``share_media_device``
> + Create Media Controller API entries
> + * bit 2: ``align_transfer``
> + Allow alignment on audio sub-slot at transfer
> + * bit 3: ``tx_length``
> + Add length specifier to transfers
> + * bit 4: ``playback_first``
> + Start playback stream at first in implement feedback mode
> + * bit 5: ``skip_clock_selector``
> + Skip clock selector setup
> + * bit 6: ``ignore_clock_source``
> + Ignore errors from clock source search
> + * bit 7: ``itf_usb_dsd_dac``
> + Indicates ITF-USB DSD based DACs
DSD-based
> + * bit 8: ``ctl_msg_delay``
> + Add a delay of 20ms at each control message handling
> + * bit 9: ``ctl_msg_delay_1m``
> + Add a delay of 1-2ms at each control message handling
> + * bit 10: ``ctl_msg_delay_5m``
> + Add a delay of 5-6ms at each control message handling
> + * bit 11: ``iface_delay``
> + Add a delay of 50ms at each interface setup
> + * bit 12: ``validate_rates``
> + Perform sample rate validations at probe
> + * bit 13: ``disable_autosuspend``
> + Disable runtime PM autosuspend
> + * bit 14: ``ignore_ctl_error``
> + Ignore errors for mixer access
> + * bit 15: ``dsd_raw``
> + Support generic DSD raw U32_BE format
> + * bit 16: ``set_iface_first``
> + Set up the interface at first like UAC1
> + * bit 17: ``generic_implicit_fb``
> + Apply the generic implicit feedback sync mode
> + * bit 18: ``skip_implicit_fb``
> + Don't apply implicit feedback sync mode
> + * bit 19: ``iface_skip_close``
> + Don't closed interface during setting sample rate
close
> + * bit 20: ``force_iface_reset``
> + Force an interface reset whenever stopping & restarting a stream
> + * bit 21: ``fixed_rate``
> + Do not set PCM rate (frequency) when only one rate is available
> + for the given endpoint
> + * bit 22: ``mic_res_16``
> + Set the fixed resolution 16 for Mic Capture Volume
> + * bit 23: ``mic_res_384``
> + Set the fixed resolution 384 for Mic Capture Volume
> + * bit 24: ``mixer_playback_min_mute``
> + Set minimum volume control value as mute for devices where the
> + lowest playback value represents muted state instead of minimum
> + audible volume
> + * bit 25: ``mixer_capture_min_mute``
> + Be similar to bit 24 but for capture streams
Similar to
>
> This module supports multiple devices, autoprobe and hotplugging.
>
> Are all of these quirks used on various devices or are some of these
just implemented just in case they are needed in the future?thanks.
--
~Randy
^ permalink raw reply
* [PATCH 6/6] module: comment describing new codepath
From: julian-lagattuta @ 2025-09-18 20:11 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu
Cc: Sami Tolvanen, Daniel Gomez, linux-modules, linux-kernel,
julian-lagattuta
In-Reply-To: <20250918201109.24620-2-julian.lagattuta@gmail.com>
added comment so future authors know about this codepath
Signed-off-by: julian-lagattuta <julian.lagattuta@gmail.com>
---
kernel/module/main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 256e30259bcf..f4ce431163fa 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3220,6 +3220,11 @@ static int module_patient_check_exists(const char *name,
*/
if (old && old->state == MODULE_STATE_LIVE)
return -EEXIST;
+
+ /*
+ * Can occur if the module was forcefully unloaded after
+ * its initcall crashed.
+ */
return -EBUSY;
}
--
2.45.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox