[PATCH v2 0/5] Check Rust signatures at compile time

[PATCH v2 0/5] Check Rust signatures at compile time

[Alice Ryhl]

Rust has two different tools for generating function declarations to
call across the FFI boundary:

* bindgen. Generates Rust declarations from a C header.
* cbindgen. Generates C headers from Rust declarations.

In the kernel, we only use bindgen. This is because cbindgen assumes a
cargo-based buildsystem, so it is not compatible with the kernel's build
system. This means that when C code calls a Rust function by name, its
signature must be duplicated in both Rust code and a C header, and the
signature needs to be kept in sync manually.

To eliminate this manual checking, introduce a new macro that verifies
at compile time that the two function declarations use the same
signature. The idea is to run the C declaration through bindgen, and
then have rustc verify that the function pointers have the same type.

The main commit of this series is "rust: add #[export] macro". Please
see its commit message for more details.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v2:
- Various improvements to documentation.
- Split out quote! changes into its own commit.
- Link to v1: https://lore.kernel.org/r/20250227-export-macro-v1-0-948775fc37aa@google.com

---
Alice Ryhl (5):
      rust: fix signature of rust_fmt_argument
      rust: macros: support additional tokens in quote!
      rust: add #[export] macro
      print: use new #[export] macro for rust_fmt_argument
      panic_qr: use new #[export] macro

 drivers/gpu/drm/drm_panic.c     |  5 -----
 drivers/gpu/drm/drm_panic_qr.rs | 15 +++++++++++----
 include/drm/drm_panic.h         |  7 +++++++
 include/linux/sprintf.h         |  3 +++
 lib/vsprintf.c                  |  3 ---
 rust/bindings/bindings_helper.h |  4 ++++
 rust/kernel/prelude.rs          |  2 +-
 rust/kernel/print.rs            | 10 +++++-----
 rust/macros/export.rs           | 28 ++++++++++++++++++++++++++++
 rust/macros/helpers.rs          | 19 ++++++++++++++++++-
 rust/macros/lib.rs              | 24 ++++++++++++++++++++++++
 rust/macros/quote.rs            | 21 +++++++++++++++++++--
 12 files changed, 120 insertions(+), 21 deletions(-)
---
base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
change-id: 20250227-export-macro-9aa9f1016d8c

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

[PATCH v2 1/5] rust: fix signature of rust_fmt_argument

[Alice Ryhl]

Without this change, the rest of this series will emit the following
error message:

error[E0308]: `if` and `else` have incompatible types
  --> <linux>/rust/kernel/print.rs:22:22
   |
21 | #[export]
   | --------- expected because of this
22 | unsafe extern "C" fn rust_fmt_argument(
   |                      ^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
   |
   = note: expected fn item `unsafe extern "C" fn(*mut u8, *mut u8, *mut c_void) -> *mut u8 {bindings::rust_fmt_argument}`
              found fn item `unsafe extern "C" fn(*mut i8, *mut i8, *const c_void) -> *mut i8 {print::rust_fmt_argument}`

The error may be different depending on the architecture.

To fix this, change the void pointer argument to use a const pointer,
and change the imports to use crate::ffi instead of core::ffi for
integer types.

Fixes: 787983da7718 ("vsprintf: add new `%pA` format specifier")
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 lib/vsprintf.c       | 2 +-
 rust/kernel/print.rs | 7 +++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 56fe96319292..a8ac4c4fffcf 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2285,7 +2285,7 @@ int __init no_hash_pointers_enable(char *str)
 early_param("no_hash_pointers", no_hash_pointers_enable);
 
 /* Used for Rust formatting ('%pA'). */
-char *rust_fmt_argument(char *buf, char *end, void *ptr);
+char *rust_fmt_argument(char *buf, char *end, const void *ptr);
 
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index b19ee490be58..61ee36c5e5f5 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -6,12 +6,11 @@
 //!
 //! Reference: <https://docs.kernel.org/core-api/printk-basics.html>
 
-use core::{
+use crate::{
     ffi::{c_char, c_void},
-    fmt,
+    str::RawFormatter,
 };
-
-use crate::str::RawFormatter;
+use core::fmt;
 
 // Called from `vsprintf` with format specifier `%pA`.
 #[expect(clippy::missing_safety_doc)]

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 2/5] rust: macros: support additional tokens in quote!

[Alice Ryhl]

This gives the quote! macro support for the following additional tokens:

* The = token.
* The _ token.
* Using #my_var with variables of type Ident.

Additionally, some type annotations are added to allow cases where
groups are empty. For example, quote! does support () in the input, but
only when it is *not* empty. When it is empty, the compiler cannot infer
the item type of `tokens`.

These additional quote! features are used by a new proc macro that
generates code looking like this:

	const _: () = {
	    if true {
	        ::kernel::bindings::#name
	    } else {
	        #name
	    };
	};

where #name has type Ident.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/macros/quote.rs | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/rust/macros/quote.rs b/rust/macros/quote.rs
index 33a199e4f176..c18960a91082 100644
--- a/rust/macros/quote.rs
+++ b/rust/macros/quote.rs
@@ -20,6 +20,12 @@ fn to_tokens(&self, tokens: &mut TokenStream) {
     }
 }
 
+impl ToTokens for proc_macro::Ident {
+    fn to_tokens(&self, tokens: &mut TokenStream) {
+        tokens.extend([TokenTree::from(self.clone())]);
+    }
+}
+
 impl ToTokens for TokenTree {
     fn to_tokens(&self, tokens: &mut TokenStream) {
         tokens.extend([self.clone()]);
@@ -40,7 +46,7 @@ fn to_tokens(&self, tokens: &mut TokenStream) {
 /// `quote` crate but provides only just enough functionality needed by the current `macros` crate.
 macro_rules! quote_spanned {
     ($span:expr => $($tt:tt)*) => {{
-        let mut tokens;
+        let mut tokens: ::std::vec::Vec<::proc_macro::TokenTree>;
         #[allow(clippy::vec_init_then_push)]
         {
             tokens = ::std::vec::Vec::new();
@@ -65,7 +71,8 @@ macro_rules! quote_spanned {
         quote_spanned!(@proc $v $span $($tt)*);
     };
     (@proc $v:ident $span:ident ( $($inner:tt)* ) $($tt:tt)*) => {
-        let mut tokens = ::std::vec::Vec::new();
+        #[allow(unused_mut)]
+        let mut tokens = ::std::vec::Vec::<::proc_macro::TokenTree>::new();
         quote_spanned!(@proc tokens $span $($inner)*);
         $v.push(::proc_macro::TokenTree::Group(::proc_macro::Group::new(
             ::proc_macro::Delimiter::Parenthesis,
@@ -136,6 +143,16 @@ macro_rules! quote_spanned {
         ));
         quote_spanned!(@proc $v $span $($tt)*);
     };
+    (@proc $v:ident $span:ident = $($tt:tt)*) => {
+        $v.push(::proc_macro::TokenTree::Punct(
+                ::proc_macro::Punct::new('=', ::proc_macro::Spacing::Alone)
+        ));
+        quote_spanned!(@proc $v $span $($tt)*);
+    };
+    (@proc $v:ident $span:ident _ $($tt:tt)*) => {
+        $v.push(::proc_macro::TokenTree::Ident(::proc_macro::Ident::new("_", $span)));
+        quote_spanned!(@proc $v $span $($tt)*);
+    };
     (@proc $v:ident $span:ident $id:ident $($tt:tt)*) => {
         $v.push(::proc_macro::TokenTree::Ident(::proc_macro::Ident::new(stringify!($id), $span)));
         quote_spanned!(@proc $v $span $($tt)*);

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 3/5] rust: add #[export] macro

[Alice Ryhl]

Rust has two different tools for generating function declarations to
call across the FFI boundary:

* bindgen. Generates Rust declarations from a C header.
* cbindgen. Generates C headers from Rust declarations.

In the kernel, we only use bindgen. This is because cbindgen assumes a
cargo-based buildsystem, so it is not compatible with the kernel's build
system. This means that when C code calls a Rust function by name, its
signature must be duplicated in both Rust code and a C header, and the
signature needs to be kept in sync manually.

To eliminate this manual checking, introduce a new macro that verifies
at compile time that the two function declarations use the same
signature. The idea is to run the C declaration through bindgen, and
then have rustc verify that the function pointers have the same type.

The signature must still be written twice, but at least you can no
longer get it wrong. If the signatures don't match, you will get errors
that look like this:

error[E0308]: `if` and `else` have incompatible types
  --> <linux>/rust/kernel/print.rs:22:22
   |
21 | #[export]
   | --------- expected because of this
22 | unsafe extern "C" fn rust_fmt_argument(
   |                      ^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
   |
   = note: expected fn item `unsafe extern "C" fn(*mut u8, *mut u8, *mut c_void) -> *mut u8 {bindings::rust_fmt_argument}`
              found fn item `unsafe extern "C" fn(*mut i8, *mut i8, *const c_void) -> *mut i8 {print::rust_fmt_argument}`

It is unfortunate that the error message starts out by saying "`if` and
`else` have incompatible types", but I believe the rest of the error
message is reasonably clear and not too confusing.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/prelude.rs |  2 +-
 rust/macros/export.rs  | 28 ++++++++++++++++++++++++++++
 rust/macros/helpers.rs | 19 ++++++++++++++++++-
 rust/macros/lib.rs     | 24 ++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index dde2e0649790..889102f5a81e 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -17,7 +17,7 @@
 pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
 
 #[doc(no_inline)]
-pub use macros::{module, pin_data, pinned_drop, vtable, Zeroable};
+pub use macros::{export, module, pin_data, pinned_drop, vtable, Zeroable};
 
 pub use super::{build_assert, build_error};
 
diff --git a/rust/macros/export.rs b/rust/macros/export.rs
new file mode 100644
index 000000000000..c5ec75f2b07f
--- /dev/null
+++ b/rust/macros/export.rs
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use crate::helpers::function_name;
+use proc_macro::TokenStream;
+
+/// Please see [`crate::export`] for documentation.
+pub(crate) fn export(_attr: TokenStream, ts: TokenStream) -> TokenStream {
+    let Some(name) = function_name(ts.clone()) else {
+        return "::core::compile_error!(\"The #[export] attribute must be used on a function.\");"
+            .parse::<TokenStream>()
+            .unwrap();
+    };
+
+    // This verifies that the function has the same signature as the declaration generated by
+    // bindgen. It makes use of the fact that all branches of an if/else must have the same type.
+    let signature_check = quote!(
+        const _: () = {
+            if true {
+                ::kernel::bindings::#name
+            } else {
+                #name
+            };
+        };
+    );
+
+    let no_mangle = "#[no_mangle]".parse::<TokenStream>().unwrap();
+    TokenStream::from_iter([signature_check, no_mangle, ts])
+}
diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
index 563dcd2b7ace..3e04f8ecfc74 100644
--- a/rust/macros/helpers.rs
+++ b/rust/macros/helpers.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-use proc_macro::{token_stream, Group, TokenStream, TokenTree};
+use proc_macro::{token_stream, Group, Ident, TokenStream, TokenTree};
 
 pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
     if let Some(TokenTree::Ident(ident)) = it.next() {
@@ -215,3 +215,20 @@ pub(crate) fn parse_generics(input: TokenStream) -> (Generics, Vec<TokenTree>) {
         rest,
     )
 }
+
+/// Given a function declaration, finds the name of the function.
+pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
+    let mut input = input.into_iter();
+    while let Some(token) = input.next() {
+        match token {
+            TokenTree::Ident(i) if i.to_string() == "fn" => {
+                if let Some(TokenTree::Ident(i)) = input.next() {
+                    return Some(i);
+                }
+                return None;
+            }
+            _ => continue,
+        }
+    }
+    None
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index d61bc6a56425..fbb2860e991f 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -9,6 +9,7 @@
 #[macro_use]
 mod quote;
 mod concat_idents;
+mod export;
 mod helpers;
 mod module;
 mod paste;
@@ -174,6 +175,29 @@ pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
     vtable::vtable(attr, ts)
 }
 
+/// Export a function so that C code can call it via a header file.
+///
+/// Functions exported using this macro can be called from C code using the declaration in the
+/// appropriate header file. It should only be used in cases where C calls the function through a
+/// header file; cases where C calls into Rust via a function pointer in a vtable (such as
+/// `file_operations`) should not use this macro.
+///
+/// This macro has the following effect:
+///
+/// * Disables name mangling for this function.
+/// * Verifies at compile-time that the function signature matches the declaration in the header
+///   file.
+///
+/// You must declare the signature of the Rust function in a header file that is included by
+/// `rust/bindings/bindings_helper.h`.
+///
+/// This macro is *not* the same as the C macros `EXPORT_SYMBOL_*`, since all Rust symbols are
+/// currently automatically exported with `EXPORT_SYMBOL_GPL`.
+#[proc_macro_attribute]
+pub fn export(attr: TokenStream, ts: TokenStream) -> TokenStream {
+    export::export(attr, ts)
+}
+
 /// Concatenate two identifiers.
 ///
 /// This is useful in macros that need to declare or reference items with names

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 4/5] print: use new #[export] macro for rust_fmt_argument

[Alice Ryhl]

This moves the rust_fmt_argument function over to use the new #[export]
macro, which will verify at compile-time that the function signature
matches what is in the header file.

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
I'm not sure which header file to put this in. Any advice?
---
 include/linux/sprintf.h | 3 +++
 lib/vsprintf.c          | 3 ---
 rust/kernel/print.rs    | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/sprintf.h b/include/linux/sprintf.h
index 33dcbec71925..029ad83efd74 100644
--- a/include/linux/sprintf.h
+++ b/include/linux/sprintf.h
@@ -24,4 +24,7 @@ __scanf(2, 0) int vsscanf(const char *, const char *, va_list);
 extern bool no_hash_pointers;
 int no_hash_pointers_enable(char *str);
 
+/* Used for Rust formatting ('%pA'). */
+char *rust_fmt_argument(char *buf, char *end, const void *ptr);
+
 #endif	/* _LINUX_KERNEL_SPRINTF_H */
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a8ac4c4fffcf..1da61c3e011f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2284,9 +2284,6 @@ int __init no_hash_pointers_enable(char *str)
 }
 early_param("no_hash_pointers", no_hash_pointers_enable);
 
-/* Used for Rust formatting ('%pA'). */
-char *rust_fmt_argument(char *buf, char *end, const void *ptr);
-
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index 61ee36c5e5f5..cf4714242e14 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -8,13 +8,14 @@
 
 use crate::{
     ffi::{c_char, c_void},
+    prelude::*,
     str::RawFormatter,
 };
 use core::fmt;
 
 // Called from `vsprintf` with format specifier `%pA`.
 #[expect(clippy::missing_safety_doc)]
-#[no_mangle]
+#[export]
 unsafe extern "C" fn rust_fmt_argument(
     buf: *mut c_char,
     end: *mut c_char,

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 5/5] panic_qr: use new #[export] macro

[Alice Ryhl]

This validates at compile time that the signatures match what is in the
header file. It highlights one annoyance with the compile-time check,
which is that it can only be used with functions marked unsafe.

If the function is not unsafe, then this error is emitted:

error[E0308]: `if` and `else` have incompatible types
   --> <linux>/drivers/gpu/drm/drm_panic_qr.rs:987:19
    |
986 | #[export]
    | --------- expected because of this
987 | pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found safe fn
    |
    = note: expected fn item `unsafe extern "C" fn(_, _) -> _ {kernel::bindings::drm_panic_qr_max_data_size}`
               found fn item `extern "C" fn(_, _) -> _ {drm_panic_qr_max_data_size}`

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/gpu/drm/drm_panic.c     |  5 -----
 drivers/gpu/drm/drm_panic_qr.rs | 15 +++++++++++----
 include/drm/drm_panic.h         |  7 +++++++
 rust/bindings/bindings_helper.h |  4 ++++
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index f128d345b16d..dee5301dd729 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -486,11 +486,6 @@ static void drm_panic_qr_exit(void)
 	stream.workspace = NULL;
 }
 
-extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
-
-extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
-				u8 *tmp, size_t tmp_size);
-
 static int drm_panic_get_qr_code_url(u8 **qr_image)
 {
 	struct kmsg_dump_iter iter;
diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
index bcf248f69252..d055655aa0cd 100644
--- a/drivers/gpu/drm/drm_panic_qr.rs
+++ b/drivers/gpu/drm/drm_panic_qr.rs
@@ -27,7 +27,10 @@
 //! * <https://github.com/bjguillot/qr>
 
 use core::cmp;
-use kernel::str::CStr;
+use kernel::{
+    prelude::*,
+    str::CStr,
+};
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)]
 struct Version(usize);
@@ -929,7 +932,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
 /// * `tmp` must be valid for reading and writing for `tmp_size` bytes.
 ///
 /// They must remain valid for the duration of the function call.
-#[no_mangle]
+#[export]
 pub unsafe extern "C" fn drm_panic_qr_generate(
     url: *const kernel::ffi::c_char,
     data: *mut u8,
@@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
 /// * If `url_len` > 0, remove the 2 segments header/length and also count the
 ///   conversion to numeric segments.
 /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment.
-#[no_mangle]
-pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
+///
+/// # Safety
+///
+/// Always safe to call.
+#[export]
+pub unsafe extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
     #[expect(clippy::manual_range_contains)]
     if version < 1 || version > 40 {
         return 0;
diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
index f4e1fa9ae607..2a1536e0229a 100644
--- a/include/drm/drm_panic.h
+++ b/include/drm/drm_panic.h
@@ -163,4 +163,11 @@ static inline void drm_panic_unlock(struct drm_device *dev, unsigned long flags)
 
 #endif
 
+#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
+extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
+
+extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
+				u8 *tmp, size_t tmp_size);
+#endif
+
 #endif /* __DRM_PANIC_H__ */
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14..5345aa93fb8a 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -36,6 +36,10 @@
 #include <linux/workqueue.h>
 #include <trace/events/rust_sample.h>
 
+#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
+#include <drm/drm_panic.h>
+#endif
+
 /* `bindgen` gets confused at certain things. */
 const size_t RUST_CONST_HELPER_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
 const size_t RUST_CONST_HELPER_PAGE_SIZE = PAGE_SIZE;

-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 0/5] Check Rust signatures at compile time

[Andy Shevchenko]

On Fri, Feb 28, 2025 at 12:39:29PM +0000, Alice Ryhl wrote:
> Rust has two different tools for generating function declarations to
> call across the FFI boundary:
> 
> * bindgen. Generates Rust declarations from a C header.
> * cbindgen. Generates C headers from Rust declarations.
> 
> In the kernel, we only use bindgen. This is because cbindgen assumes a
> cargo-based buildsystem, so it is not compatible with the kernel's build
> system. This means that when C code calls a Rust function by name, its
> signature must be duplicated in both Rust code and a C header, and the
> signature needs to be kept in sync manually.
> 
> To eliminate this manual checking, introduce a new macro that verifies
> at compile time that the two function declarations use the same
> signature. The idea is to run the C declaration through bindgen, and
> then have rustc verify that the function pointers have the same type.
> 
> The main commit of this series is "rust: add #[export] macro". Please
> see its commit message for more details.

Thanks for the cover letter, makes much more sense now!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/5] Check Rust signatures at compile time

[Miguel Ojeda]

On Fri, Feb 28, 2025 at 1:40 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> This is because cbindgen assumes a
> cargo-based buildsystem, so it is not compatible with the kernel's build
> system.

I don't think this is true (checking is already a very good
justification, so we don't need this to justify the series):

    https://lore.kernel.org/rust-for-linux/CANiq72mHVbnmA_G1Fx3rz06RXA+=K5ERoWKjH-UDJ9cKxvKQ1g@mail.gmail.com/

Cheers,
Miguel

Re: [PATCH v2 0/5] Check Rust signatures at compile time

[Alice Ryhl]

On Fri, Feb 28, 2025 at 2:12 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Feb 28, 2025 at 1:40 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > This is because cbindgen assumes a
> > cargo-based buildsystem, so it is not compatible with the kernel's build
> > system.
>
> I don't think this is true (checking is already a very good
> justification, so we don't need this to justify the series):
>
>     https://lore.kernel.org/rust-for-linux/CANiq72mHVbnmA_G1Fx3rz06RXA+=K5ERoWKjH-UDJ9cKxvKQ1g@mail.gmail.com/

Huh, I thought for sure it did.

Alice

Re: [PATCH v2 1/5] rust: fix signature of rust_fmt_argument

[Tamir Duberstein]

On Fri, Feb 28, 2025 at 7:41 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Without this change, the rest of this series will emit the following
> error message:
>
> error[E0308]: `if` and `else` have incompatible types
>   --> <linux>/rust/kernel/print.rs:22:22
>    |
> 21 | #[export]
>    | --------- expected because of this
> 22 | unsafe extern "C" fn rust_fmt_argument(
>    |                      ^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
>    |
>    = note: expected fn item `unsafe extern "C" fn(*mut u8, *mut u8, *mut c_void) -> *mut u8 {bindings::rust_fmt_argument}`
>               found fn item `unsafe extern "C" fn(*mut i8, *mut i8, *const c_void) -> *mut i8 {print::rust_fmt_argument}`
>
> The error may be different depending on the architecture.
>
> To fix this, change the void pointer argument to use a const pointer,
> and change the imports to use crate::ffi instead of core::ffi for
> integer types.
>
> Fixes: 787983da7718 ("vsprintf: add new `%pA` format specifier")
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  lib/vsprintf.c       | 2 +-
>  rust/kernel/print.rs | 7 +++----
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 56fe96319292..a8ac4c4fffcf 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2285,7 +2285,7 @@ int __init no_hash_pointers_enable(char *str)
>  early_param("no_hash_pointers", no_hash_pointers_enable);
>
>  /* Used for Rust formatting ('%pA'). */
> -char *rust_fmt_argument(char *buf, char *end, void *ptr);
> +char *rust_fmt_argument(char *buf, char *end, const void *ptr);
>
>  /*
>   * Show a '%p' thing.  A kernel extension is that the '%p' is followed
> diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
> index b19ee490be58..61ee36c5e5f5 100644
> --- a/rust/kernel/print.rs
> +++ b/rust/kernel/print.rs
> @@ -6,12 +6,11 @@
>  //!
>  //! Reference: <https://docs.kernel.org/core-api/printk-basics.html>
>
> -use core::{
> +use crate::{
>      ffi::{c_char, c_void},
> -    fmt,
> +    str::RawFormatter,
>  };
> -
> -use crate::str::RawFormatter;
> +use core::fmt;
>
>  // Called from `vsprintf` with format specifier `%pA`.
>  #[expect(clippy::missing_safety_doc)]
>
> --
> 2.48.1.711.g2feabab25a-goog

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v2 2/5] rust: macros: support additional tokens in quote!

[Tamir Duberstein]

On Fri, Feb 28, 2025 at 7:42 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> This gives the quote! macro support for the following additional tokens:
>
> * The = token.
> * The _ token.
> * Using #my_var with variables of type Ident.
>
> Additionally, some type annotations are added to allow cases where
> groups are empty. For example, quote! does support () in the input, but
> only when it is *not* empty. When it is empty, the compiler cannot infer
> the item type of `tokens`.
>
> These additional quote! features are used by a new proc macro that
> generates code looking like this:
>
>         const _: () = {
>             if true {
>                 ::kernel::bindings::#name
>             } else {
>                 #name
>             };
>         };
>
> where #name has type Ident.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/macros/quote.rs | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/rust/macros/quote.rs b/rust/macros/quote.rs
> index 33a199e4f176..c18960a91082 100644
> --- a/rust/macros/quote.rs
> +++ b/rust/macros/quote.rs
> @@ -20,6 +20,12 @@ fn to_tokens(&self, tokens: &mut TokenStream) {
>      }
>  }
>
> +impl ToTokens for proc_macro::Ident {
> +    fn to_tokens(&self, tokens: &mut TokenStream) {
> +        tokens.extend([TokenTree::from(self.clone())]);
> +    }
> +}
> +
>  impl ToTokens for TokenTree {
>      fn to_tokens(&self, tokens: &mut TokenStream) {
>          tokens.extend([self.clone()]);
> @@ -40,7 +46,7 @@ fn to_tokens(&self, tokens: &mut TokenStream) {
>  /// `quote` crate but provides only just enough functionality needed by the current `macros` crate.
>  macro_rules! quote_spanned {
>      ($span:expr => $($tt:tt)*) => {{
> -        let mut tokens;
> +        let mut tokens: ::std::vec::Vec<::proc_macro::TokenTree>;
>          #[allow(clippy::vec_init_then_push)]
>          {
>              tokens = ::std::vec::Vec::new();
> @@ -65,7 +71,8 @@ macro_rules! quote_spanned {
>          quote_spanned!(@proc $v $span $($tt)*);
>      };
>      (@proc $v:ident $span:ident ( $($inner:tt)* ) $($tt:tt)*) => {
> -        let mut tokens = ::std::vec::Vec::new();
> +        #[allow(unused_mut)]

It'd be nice to mention the need for this attribute in the commit
message along with the added type annotations.

> +        let mut tokens = ::std::vec::Vec::<::proc_macro::TokenTree>::new();
>          quote_spanned!(@proc tokens $span $($inner)*);
>          $v.push(::proc_macro::TokenTree::Group(::proc_macro::Group::new(
>              ::proc_macro::Delimiter::Parenthesis,
> @@ -136,6 +143,16 @@ macro_rules! quote_spanned {
>          ));
>          quote_spanned!(@proc $v $span $($tt)*);
>      };
> +    (@proc $v:ident $span:ident = $($tt:tt)*) => {
> +        $v.push(::proc_macro::TokenTree::Punct(
> +                ::proc_macro::Punct::new('=', ::proc_macro::Spacing::Alone)
> +        ));
> +        quote_spanned!(@proc $v $span $($tt)*);
> +    };
> +    (@proc $v:ident $span:ident _ $($tt:tt)*) => {
> +        $v.push(::proc_macro::TokenTree::Ident(::proc_macro::Ident::new("_", $span)));
> +        quote_spanned!(@proc $v $span $($tt)*);
> +    };
>      (@proc $v:ident $span:ident $id:ident $($tt:tt)*) => {
>          $v.push(::proc_macro::TokenTree::Ident(::proc_macro::Ident::new(stringify!($id), $span)));
>          quote_spanned!(@proc $v $span $($tt)*);
>
> --
> 2.48.1.711.g2feabab25a-goog

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v2 5/5] panic_qr: use new #[export] macro

[Andy Shevchenko]

On Fri, Feb 28, 2025 at 12:39:34PM +0000, Alice Ryhl wrote:
> This validates at compile time that the signatures match what is in the
> header file. It highlights one annoyance with the compile-time check,
> which is that it can only be used with functions marked unsafe.
> 
> If the function is not unsafe, then this error is emitted:
> 
> error[E0308]: `if` and `else` have incompatible types
>    --> <linux>/drivers/gpu/drm/drm_panic_qr.rs:987:19
>     |
> 986 | #[export]
>     | --------- expected because of this
> 987 | pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
>     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found safe fn
>     |
>     = note: expected fn item `unsafe extern "C" fn(_, _) -> _ {kernel::bindings::drm_panic_qr_max_data_size}`
>                found fn item `extern "C" fn(_, _) -> _ {drm_panic_qr_max_data_size}`

...

> +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> +extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
> +
> +extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
> +				u8 *tmp, size_t tmp_size);

Is extern needed?

> +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] print: use new #[export] macro for rust_fmt_argument

[Andy Shevchenko]

On Fri, Feb 28, 2025 at 12:39:33PM +0000, Alice Ryhl wrote:
> This moves the rust_fmt_argument function over to use the new #[export]
> macro, which will verify at compile-time that the function signature
> matches what is in the header file.

...

> I'm not sure which header file to put this in. Any advice?

I believe you found the right place.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/5] rust: add #[export] macro

[Tamir Duberstein]

On Fri, Feb 28, 2025 at 7:40 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Rust has two different tools for generating function declarations to
> call across the FFI boundary:
>
> * bindgen. Generates Rust declarations from a C header.
> * cbindgen. Generates C headers from Rust declarations.
>
> In the kernel, we only use bindgen. This is because cbindgen assumes a
> cargo-based buildsystem, so it is not compatible with the kernel's build
> system. This means that when C code calls a Rust function by name, its
> signature must be duplicated in both Rust code and a C header, and the
> signature needs to be kept in sync manually.

This needs an update given Miguel's comments on the cover letter. I
wonder if the code should also justify the choice (over cbindgen).

> To eliminate this manual checking, introduce a new macro that verifies
> at compile time that the two function declarations use the same
> signature. The idea is to run the C declaration through bindgen, and
> then have rustc verify that the function pointers have the same type.
>
> The signature must still be written twice, but at least you can no
> longer get it wrong. If the signatures don't match, you will get errors
> that look like this:
>
> error[E0308]: `if` and `else` have incompatible types
>   --> <linux>/rust/kernel/print.rs:22:22
>    |
> 21 | #[export]
>    | --------- expected because of this
> 22 | unsafe extern "C" fn rust_fmt_argument(
>    |                      ^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
>    |
>    = note: expected fn item `unsafe extern "C" fn(*mut u8, *mut u8, *mut c_void) -> *mut u8 {bindings::rust_fmt_argument}`
>               found fn item `unsafe extern "C" fn(*mut i8, *mut i8, *const c_void) -> *mut i8 {print::rust_fmt_argument}`
>
> It is unfortunate that the error message starts out by saying "`if` and
> `else` have incompatible types", but I believe the rest of the error
> message is reasonably clear and not too confusing.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/prelude.rs |  2 +-
>  rust/macros/export.rs  | 28 ++++++++++++++++++++++++++++
>  rust/macros/helpers.rs | 19 ++++++++++++++++++-
>  rust/macros/lib.rs     | 24 ++++++++++++++++++++++++
>  4 files changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> index dde2e0649790..889102f5a81e 100644
> --- a/rust/kernel/prelude.rs
> +++ b/rust/kernel/prelude.rs
> @@ -17,7 +17,7 @@
>  pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
>
>  #[doc(no_inline)]
> -pub use macros::{module, pin_data, pinned_drop, vtable, Zeroable};
> +pub use macros::{export, module, pin_data, pinned_drop, vtable, Zeroable};
>
>  pub use super::{build_assert, build_error};
>
> diff --git a/rust/macros/export.rs b/rust/macros/export.rs
> new file mode 100644
> index 000000000000..c5ec75f2b07f
> --- /dev/null
> +++ b/rust/macros/export.rs
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use crate::helpers::function_name;
> +use proc_macro::TokenStream;
> +
> +/// Please see [`crate::export`] for documentation.
> +pub(crate) fn export(_attr: TokenStream, ts: TokenStream) -> TokenStream {
> +    let Some(name) = function_name(ts.clone()) else {
> +        return "::core::compile_error!(\"The #[export] attribute must be used on a function.\");"
> +            .parse::<TokenStream>()
> +            .unwrap();
> +    };

Could you also show in the commit message what this error looks like
when the macro is misused?

> +
> +    // This verifies that the function has the same signature as the declaration generated by
> +    // bindgen. It makes use of the fact that all branches of an if/else must have the same type.
> +    let signature_check = quote!(
> +        const _: () = {
> +            if true {
> +                ::kernel::bindings::#name
> +            } else {
> +                #name
> +            };
> +        };
> +    );
> +
> +    let no_mangle = "#[no_mangle]".parse::<TokenStream>().unwrap();

Would this be simpler as `let no_mangle = quote!("#[no_mangle]");`?

> +    TokenStream::from_iter([signature_check, no_mangle, ts])
> +}
> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
> index 563dcd2b7ace..3e04f8ecfc74 100644
> --- a/rust/macros/helpers.rs
> +++ b/rust/macros/helpers.rs
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>
> -use proc_macro::{token_stream, Group, TokenStream, TokenTree};
> +use proc_macro::{token_stream, Group, Ident, TokenStream, TokenTree};
>
>  pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
>      if let Some(TokenTree::Ident(ident)) = it.next() {
> @@ -215,3 +215,20 @@ pub(crate) fn parse_generics(input: TokenStream) -> (Generics, Vec<TokenTree>) {
>          rest,
>      )
>  }
> +
> +/// Given a function declaration, finds the name of the function.
> +pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
> +    let mut input = input.into_iter();
> +    while let Some(token) = input.next() {
> +        match token {
> +            TokenTree::Ident(i) if i.to_string() == "fn" => {
> +                if let Some(TokenTree::Ident(i)) = input.next() {
> +                    return Some(i);
> +                }
> +                return None;
> +            }
> +            _ => continue,
> +        }
> +    }
> +    None
> +}
> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
> index d61bc6a56425..fbb2860e991f 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -9,6 +9,7 @@
>  #[macro_use]
>  mod quote;
>  mod concat_idents;
> +mod export;
>  mod helpers;
>  mod module;
>  mod paste;
> @@ -174,6 +175,29 @@ pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
>      vtable::vtable(attr, ts)
>  }
>
> +/// Export a function so that C code can call it via a header file.
> +///
> +/// Functions exported using this macro can be called from C code using the declaration in the
> +/// appropriate header file. It should only be used in cases where C calls the function through a
> +/// header file; cases where C calls into Rust via a function pointer in a vtable (such as
> +/// `file_operations`) should not use this macro.
> +///
> +/// This macro has the following effect:
> +///
> +/// * Disables name mangling for this function.
> +/// * Verifies at compile-time that the function signature matches the declaration in the header
> +///   file.
> +///
> +/// You must declare the signature of the Rust function in a header file that is included by
> +/// `rust/bindings/bindings_helper.h`.
> +///
> +/// This macro is *not* the same as the C macros `EXPORT_SYMBOL_*`, since all Rust symbols are
> +/// currently automatically exported with `EXPORT_SYMBOL_GPL`.

nit: "since" implies causation, which isn't the case, I think.
Consider if ", since" can be replaced with a semicolon.

> +#[proc_macro_attribute]
> +pub fn export(attr: TokenStream, ts: TokenStream) -> TokenStream {
> +    export::export(attr, ts)
> +}
> +
>  /// Concatenate two identifiers.
>  ///
>  /// This is useful in macros that need to declare or reference items with names
>
> --
> 2.48.1.711.g2feabab25a-goog

Minor comments.

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v2 4/5] print: use new #[export] macro for rust_fmt_argument

[Tamir Duberstein]

On Fri, Feb 28, 2025 at 7:41 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> This moves the rust_fmt_argument function over to use the new #[export]
> macro, which will verify at compile-time that the function signature
> matches what is in the header file.
>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> I'm not sure which header file to put this in. Any advice?
> ---
>  include/linux/sprintf.h | 3 +++
>  lib/vsprintf.c          | 3 ---
>  rust/kernel/print.rs    | 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/sprintf.h b/include/linux/sprintf.h
> index 33dcbec71925..029ad83efd74 100644
> --- a/include/linux/sprintf.h
> +++ b/include/linux/sprintf.h
> @@ -24,4 +24,7 @@ __scanf(2, 0) int vsscanf(const char *, const char *, va_list);
>  extern bool no_hash_pointers;
>  int no_hash_pointers_enable(char *str);
>
> +/* Used for Rust formatting ('%pA'). */
> +char *rust_fmt_argument(char *buf, char *end, const void *ptr);
> +
>  #endif /* _LINUX_KERNEL_SPRINTF_H */
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index a8ac4c4fffcf..1da61c3e011f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2284,9 +2284,6 @@ int __init no_hash_pointers_enable(char *str)
>  }
>  early_param("no_hash_pointers", no_hash_pointers_enable);
>
> -/* Used for Rust formatting ('%pA'). */
> -char *rust_fmt_argument(char *buf, char *end, const void *ptr);
> -
>  /*
>   * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>   * by an extra set of alphanumeric characters that are extended format
> diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
> index 61ee36c5e5f5..cf4714242e14 100644
> --- a/rust/kernel/print.rs
> +++ b/rust/kernel/print.rs
> @@ -8,13 +8,14 @@
>
>  use crate::{
>      ffi::{c_char, c_void},
> +    prelude::*,
>      str::RawFormatter,
>  };
>  use core::fmt;
>
>  // Called from `vsprintf` with format specifier `%pA`.
>  #[expect(clippy::missing_safety_doc)]
> -#[no_mangle]
> +#[export]
>  unsafe extern "C" fn rust_fmt_argument(
>      buf: *mut c_char,
>      end: *mut c_char,
>
> --
> 2.48.1.711.g2feabab25a-goog

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v2 3/5] rust: add #[export] macro

[Miguel Ojeda]

On Fri, Feb 28, 2025 at 4:41 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> This needs an update given Miguel's comments on the cover letter. I
> wonder if the code should also justify the choice (over cbindgen).

`cbindgen` is a longer term thing and more complex, assuming we use it
in the end, so I think it is fine going with this for the time being
-- it is straightforward and a net improvement.

Later on, if needed, we can just make `export` a no-op, right?

It may also be useful to have the exports "explicitly tagged" this way.

Cheers,
Miguel

Re: [PATCH v2 3/5] rust: add #[export] macro

[Tamir Duberstein]

On Fri, Feb 28, 2025 at 10:49 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Feb 28, 2025 at 4:41 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > This needs an update given Miguel's comments on the cover letter. I
> > wonder if the code should also justify the choice (over cbindgen).
>
> `cbindgen` is a longer term thing and more complex, assuming we use it
> in the end, so I think it is fine going with this for the time being
> -- it is straightforward and a net improvement.
>
> Later on, if needed, we can just make `export` a no-op, right?
>
> It may also be useful to have the exports "explicitly tagged" this way.

Sounds reasonable. I was suggesting that we document the rationale -
not that we change the decision.

Re: [PATCH v2 5/5] panic_qr: use new #[export] macro

[Tamir Duberstein]

On Fri, Feb 28, 2025 at 7:41 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> This validates at compile time that the signatures match what is in the
> header file. It highlights one annoyance with the compile-time check,
> which is that it can only be used with functions marked unsafe.
>
> If the function is not unsafe, then this error is emitted:
>
> error[E0308]: `if` and `else` have incompatible types
>    --> <linux>/drivers/gpu/drm/drm_panic_qr.rs:987:19
>     |
> 986 | #[export]
>     | --------- expected because of this
> 987 | pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
>     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found safe fn
>     |
>     = note: expected fn item `unsafe extern "C" fn(_, _) -> _ {kernel::bindings::drm_panic_qr_max_data_size}`
>                found fn item `extern "C" fn(_, _) -> _ {drm_panic_qr_max_data_size}`
>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  drivers/gpu/drm/drm_panic.c     |  5 -----
>  drivers/gpu/drm/drm_panic_qr.rs | 15 +++++++++++----
>  include/drm/drm_panic.h         |  7 +++++++
>  rust/bindings/bindings_helper.h |  4 ++++
>  4 files changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index f128d345b16d..dee5301dd729 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -486,11 +486,6 @@ static void drm_panic_qr_exit(void)
>         stream.workspace = NULL;
>  }
>
> -extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
> -
> -extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
> -                               u8 *tmp, size_t tmp_size);
> -
>  static int drm_panic_get_qr_code_url(u8 **qr_image)
>  {
>         struct kmsg_dump_iter iter;
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index bcf248f69252..d055655aa0cd 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -27,7 +27,10 @@
>  //! * <https://github.com/bjguillot/qr>
>
>  use core::cmp;
> -use kernel::str::CStr;
> +use kernel::{
> +    prelude::*,
> +    str::CStr,
> +};
>
>  #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)]
>  struct Version(usize);
> @@ -929,7 +932,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
>  /// * `tmp` must be valid for reading and writing for `tmp_size` bytes.
>  ///
>  /// They must remain valid for the duration of the function call.
> -#[no_mangle]
> +#[export]
>  pub unsafe extern "C" fn drm_panic_qr_generate(
>      url: *const kernel::ffi::c_char,
>      data: *mut u8,
> @@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
>  /// * If `url_len` > 0, remove the 2 segments header/length and also count the
>  ///   conversion to numeric segments.
>  /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment.
> -#[no_mangle]
> -pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> +///
> +/// # Safety
> +///
> +/// Always safe to call.

This should explain why it's marked unsafe, since it's always safe to call.


> +#[export]
> +pub unsafe extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
>      #[expect(clippy::manual_range_contains)]
>      if version < 1 || version > 40 {
>          return 0;
> diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
> index f4e1fa9ae607..2a1536e0229a 100644
> --- a/include/drm/drm_panic.h
> +++ b/include/drm/drm_panic.h
> @@ -163,4 +163,11 @@ static inline void drm_panic_unlock(struct drm_device *dev, unsigned long flags)
>
>  #endif
>
> +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> +extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
> +
> +extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
> +                               u8 *tmp, size_t tmp_size);
> +#endif
> +
>  #endif /* __DRM_PANIC_H__ */
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 55354e4dec14..5345aa93fb8a 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -36,6 +36,10 @@
>  #include <linux/workqueue.h>
>  #include <trace/events/rust_sample.h>
>
> +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> +#include <drm/drm_panic.h>
> +#endif

Why the guard here?

It'd be nice to have a comment here explaining the atypical need for
this include.

> +
>  /* `bindgen` gets confused at certain things. */
>  const size_t RUST_CONST_HELPER_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
>  const size_t RUST_CONST_HELPER_PAGE_SIZE = PAGE_SIZE;
>
> --
> 2.48.1.711.g2feabab25a-goog

With Andy's comment about extern addressed:

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v2 5/5] panic_qr: use new #[export] macro

[Alice Ryhl]

On Fri, Feb 28, 2025 at 4:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Feb 28, 2025 at 12:39:34PM +0000, Alice Ryhl wrote:
> > This validates at compile time that the signatures match what is in the
> > header file. It highlights one annoyance with the compile-time check,
> > which is that it can only be used with functions marked unsafe.
> >
> > If the function is not unsafe, then this error is emitted:
> >
> > error[E0308]: `if` and `else` have incompatible types
> >    --> <linux>/drivers/gpu/drm/drm_panic_qr.rs:987:19
> >     |
> > 986 | #[export]
> >     | --------- expected because of this
> > 987 | pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> >     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found safe fn
> >     |
> >     = note: expected fn item `unsafe extern "C" fn(_, _) -> _ {kernel::bindings::drm_panic_qr_max_data_size}`
> >                found fn item `extern "C" fn(_, _) -> _ {drm_panic_qr_max_data_size}`
>
> ...
>
> > +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> > +extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
> > +
> > +extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
> > +                             u8 *tmp, size_t tmp_size);
>
> Is extern needed?

I doubt it. I just copied the declaration without changing it.

Alice

Re: [PATCH v2 5/5] panic_qr: use new #[export] macro

[Simona Vetter]

On Fri, Feb 28, 2025 at 12:39:34PM +0000, Alice Ryhl wrote:
> This validates at compile time that the signatures match what is in the
> header file. It highlights one annoyance with the compile-time check,
> which is that it can only be used with functions marked unsafe.
> 
> If the function is not unsafe, then this error is emitted:
> 
> error[E0308]: `if` and `else` have incompatible types
>    --> <linux>/drivers/gpu/drm/drm_panic_qr.rs:987:19
>     |
> 986 | #[export]
>     | --------- expected because of this
> 987 | pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
>     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found safe fn
>     |
>     = note: expected fn item `unsafe extern "C" fn(_, _) -> _ {kernel::bindings::drm_panic_qr_max_data_size}`
>                found fn item `extern "C" fn(_, _) -> _ {drm_panic_qr_max_data_size}`
> 
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

I guess makes most sense to land this all through a rust tree, for that:

Acked-by: Simona Vetter <simona.vetter@ffwll.ch>

The rust macro magic definitely flies above my head, so no revivew from
me.

Cheers, Sima

> ---
>  drivers/gpu/drm/drm_panic.c     |  5 -----
>  drivers/gpu/drm/drm_panic_qr.rs | 15 +++++++++++----
>  include/drm/drm_panic.h         |  7 +++++++
>  rust/bindings/bindings_helper.h |  4 ++++
>  4 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index f128d345b16d..dee5301dd729 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -486,11 +486,6 @@ static void drm_panic_qr_exit(void)
>  	stream.workspace = NULL;
>  }
>  
> -extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
> -
> -extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
> -				u8 *tmp, size_t tmp_size);
> -
>  static int drm_panic_get_qr_code_url(u8 **qr_image)
>  {
>  	struct kmsg_dump_iter iter;
> diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> index bcf248f69252..d055655aa0cd 100644
> --- a/drivers/gpu/drm/drm_panic_qr.rs
> +++ b/drivers/gpu/drm/drm_panic_qr.rs
> @@ -27,7 +27,10 @@
>  //! * <https://github.com/bjguillot/qr>
>  
>  use core::cmp;
> -use kernel::str::CStr;
> +use kernel::{
> +    prelude::*,
> +    str::CStr,
> +};
>  
>  #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)]
>  struct Version(usize);
> @@ -929,7 +932,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
>  /// * `tmp` must be valid for reading and writing for `tmp_size` bytes.
>  ///
>  /// They must remain valid for the duration of the function call.
> -#[no_mangle]
> +#[export]
>  pub unsafe extern "C" fn drm_panic_qr_generate(
>      url: *const kernel::ffi::c_char,
>      data: *mut u8,
> @@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
>  /// * If `url_len` > 0, remove the 2 segments header/length and also count the
>  ///   conversion to numeric segments.
>  /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment.
> -#[no_mangle]
> -pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> +///
> +/// # Safety
> +///
> +/// Always safe to call.
> +#[export]
> +pub unsafe extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
>      #[expect(clippy::manual_range_contains)]
>      if version < 1 || version > 40 {
>          return 0;
> diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
> index f4e1fa9ae607..2a1536e0229a 100644
> --- a/include/drm/drm_panic.h
> +++ b/include/drm/drm_panic.h
> @@ -163,4 +163,11 @@ static inline void drm_panic_unlock(struct drm_device *dev, unsigned long flags)
>  
>  #endif
>  
> +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> +extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
> +
> +extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
> +				u8 *tmp, size_t tmp_size);
> +#endif
> +
>  #endif /* __DRM_PANIC_H__ */
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 55354e4dec14..5345aa93fb8a 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -36,6 +36,10 @@
>  #include <linux/workqueue.h>
>  #include <trace/events/rust_sample.h>
>  
> +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> +#include <drm/drm_panic.h>
> +#endif
> +
>  /* `bindgen` gets confused at certain things. */
>  const size_t RUST_CONST_HELPER_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
>  const size_t RUST_CONST_HELPER_PAGE_SIZE = PAGE_SIZE;
> 
> -- 
> 2.48.1.711.g2feabab25a-goog
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 5/5] panic_qr: use new #[export] macro

[Alice Ryhl]

On Fri, Feb 28, 2025 at 4:55 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Fri, Feb 28, 2025 at 7:41 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > This validates at compile time that the signatures match what is in the
> > header file. It highlights one annoyance with the compile-time check,
> > which is that it can only be used with functions marked unsafe.
> >
> > If the function is not unsafe, then this error is emitted:
> >
> > error[E0308]: `if` and `else` have incompatible types
> >    --> <linux>/drivers/gpu/drm/drm_panic_qr.rs:987:19
> >     |
> > 986 | #[export]
> >     | --------- expected because of this
> > 987 | pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> >     |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found safe fn
> >     |
> >     = note: expected fn item `unsafe extern "C" fn(_, _) -> _ {kernel::bindings::drm_panic_qr_max_data_size}`
> >                found fn item `extern "C" fn(_, _) -> _ {drm_panic_qr_max_data_size}`
> >
> > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  drivers/gpu/drm/drm_panic.c     |  5 -----
> >  drivers/gpu/drm/drm_panic_qr.rs | 15 +++++++++++----
> >  include/drm/drm_panic.h         |  7 +++++++
> >  rust/bindings/bindings_helper.h |  4 ++++
> >  4 files changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> > index f128d345b16d..dee5301dd729 100644
> > --- a/drivers/gpu/drm/drm_panic.c
> > +++ b/drivers/gpu/drm/drm_panic.c
> > @@ -486,11 +486,6 @@ static void drm_panic_qr_exit(void)
> >         stream.workspace = NULL;
> >  }
> >
> > -extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
> > -
> > -extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
> > -                               u8 *tmp, size_t tmp_size);
> > -
> >  static int drm_panic_get_qr_code_url(u8 **qr_image)
> >  {
> >         struct kmsg_dump_iter iter;
> > diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs
> > index bcf248f69252..d055655aa0cd 100644
> > --- a/drivers/gpu/drm/drm_panic_qr.rs
> > +++ b/drivers/gpu/drm/drm_panic_qr.rs
> > @@ -27,7 +27,10 @@
> >  //! * <https://github.com/bjguillot/qr>
> >
> >  use core::cmp;
> > -use kernel::str::CStr;
> > +use kernel::{
> > +    prelude::*,
> > +    str::CStr,
> > +};
> >
> >  #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)]
> >  struct Version(usize);
> > @@ -929,7 +932,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
> >  /// * `tmp` must be valid for reading and writing for `tmp_size` bytes.
> >  ///
> >  /// They must remain valid for the duration of the function call.
> > -#[no_mangle]
> > +#[export]
> >  pub unsafe extern "C" fn drm_panic_qr_generate(
> >      url: *const kernel::ffi::c_char,
> >      data: *mut u8,
> > @@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
> >  /// * If `url_len` > 0, remove the 2 segments header/length and also count the
> >  ///   conversion to numeric segments.
> >  /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment.
> > -#[no_mangle]
> > -pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> > +///
> > +/// # Safety
> > +///
> > +/// Always safe to call.
>
> This should explain why it's marked unsafe, since it's always safe to call.

Safety comments generally do not explain rationale for why they are
the way they are. Where would you like me to put it?

> > +#[export]
> > +pub unsafe extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> >      #[expect(clippy::manual_range_contains)]
> >      if version < 1 || version > 40 {
> >          return 0;
> > diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
> > index f4e1fa9ae607..2a1536e0229a 100644
> > --- a/include/drm/drm_panic.h
> > +++ b/include/drm/drm_panic.h
> > @@ -163,4 +163,11 @@ static inline void drm_panic_unlock(struct drm_device *dev, unsigned long flags)
> >
> >  #endif
> >
> > +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> > +extern size_t drm_panic_qr_max_data_size(u8 version, size_t url_len);
> > +
> > +extern u8 drm_panic_qr_generate(const char *url, u8 *data, size_t data_len, size_t data_size,
> > +                               u8 *tmp, size_t tmp_size);
> > +#endif
> > +
> >  #endif /* __DRM_PANIC_H__ */
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 55354e4dec14..5345aa93fb8a 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -36,6 +36,10 @@
> >  #include <linux/workqueue.h>
> >  #include <trace/events/rust_sample.h>
> >
> > +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> > +#include <drm/drm_panic.h>
> > +#endif
>
> Why the guard here?
>
> It'd be nice to have a comment here explaining the atypical need for
> this include.

It's not necessary. I can drop it.

Alice

Re: [PATCH v2 4/5] print: use new #[export] macro for rust_fmt_argument

[Alice Ryhl]

On Fri, Feb 28, 2025 at 4:37 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Feb 28, 2025 at 12:39:33PM +0000, Alice Ryhl wrote:
> > This moves the rust_fmt_argument function over to use the new #[export]
> > macro, which will verify at compile-time that the function signature
> > matches what is in the header file.
>
> ...
>
> > I'm not sure which header file to put this in. Any advice?
>
> I believe you found the right place.

Thanks for checking!

Alice

Re: [PATCH v2 5/5] panic_qr: use new #[export] macro

[Tamir Duberstein]

On Fri, Feb 28, 2025 at 12:08 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Fri, Feb 28, 2025 at 4:55 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > On Fri, Feb 28, 2025 at 7:41 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > @@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
> > >  /// * If `url_len` > 0, remove the 2 segments header/length and also count the
> > >  ///   conversion to numeric segments.
> > >  /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment.
> > > -#[no_mangle]
> > > -pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> > > +///
> > > +/// # Safety
> > > +///
> > > +/// Always safe to call.
> >
> > This should explain why it's marked unsafe, since it's always safe to call.
>
> Safety comments generally do not explain rationale for why they are
> the way they are. Where would you like me to put it?

Safety comments also generally do not say that the function isn't
really unsafe (with a notable exception in
`samples/rust/rust_print_main.rs` which is similar to this case).

Perhaps "This function is marked unsafe because ... but since a safety
comment is still required:" would flow nicely into the safety section.

> > > @@ -36,6 +36,10 @@
> > >  #include <linux/workqueue.h>
> > >  #include <trace/events/rust_sample.h>
> > >
> > > +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> > > +#include <drm/drm_panic.h>
> > > +#endif
> >
> > Why the guard here?
> >
> > It'd be nice to have a comment here explaining the atypical need for
> > this include.
>
> It's not necessary. I can drop it.

Ok. A comment on the include would still be helpful.

Re: [PATCH v2 2/5] rust: macros: support additional tokens in quote!

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> This gives the quote! macro support for the following additional tokens:
>
> * The = token.
> * The _ token.
> * Using #my_var with variables of type Ident.
>
> Additionally, some type annotations are added to allow cases where
> groups are empty. For example, quote! does support () in the input, but
> only when it is *not* empty. When it is empty, the compiler cannot infer
> the item type of `tokens`.
>
> These additional quote! features are used by a new proc macro that
> generates code looking like this:
>
> 	const _: () = {
> 	    if true {
> 	        ::kernel::bindings::#name
> 	    } else {
> 	        #name
> 	    };
> 	};
>
> where #name has type Ident.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH v2 3/5] rust: add #[export] macro

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> Rust has two different tools for generating function declarations to
> call across the FFI boundary:
>
> * bindgen. Generates Rust declarations from a C header.
> * cbindgen. Generates C headers from Rust declarations.
>
> In the kernel, we only use bindgen. This is because cbindgen assumes a
> cargo-based buildsystem, so it is not compatible with the kernel's build
> system. This means that when C code calls a Rust function by name, its
> signature must be duplicated in both Rust code and a C header, and the
> signature needs to be kept in sync manually.
>
> To eliminate this manual checking, introduce a new macro that verifies
> at compile time that the two function declarations use the same
> signature. The idea is to run the C declaration through bindgen, and
> then have rustc verify that the function pointers have the same type.
>
> The signature must still be written twice, but at least you can no
> longer get it wrong. If the signatures don't match, you will get errors
> that look like this:
>
> error[E0308]: `if` and `else` have incompatible types
>   --> <linux>/rust/kernel/print.rs:22:22
>    |
> 21 | #[export]
>    | --------- expected because of this
> 22 | unsafe extern "C" fn rust_fmt_argument(
>    |                      ^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
>    |
>    = note: expected fn item `unsafe extern "C" fn(*mut u8, *mut u8, *mut c_void) -> *mut u8 {bindings::rust_fmt_argument}`
>               found fn item `unsafe extern "C" fn(*mut i8, *mut i8, *const c_void) -> *mut i8 {print::rust_fmt_argument}`
>
> It is unfortunate that the error message starts out by saying "`if` and
> `else` have incompatible types", but I believe the rest of the error
> message is reasonably clear and not too confusing.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH v2 0/5] Check Rust signatures at compile time

[Greg Kroah-Hartman]

On Fri, Feb 28, 2025 at 12:39:29PM +0000, Alice Ryhl wrote:
> Rust has two different tools for generating function declarations to
> call across the FFI boundary:
> 
> * bindgen. Generates Rust declarations from a C header.
> * cbindgen. Generates C headers from Rust declarations.
> 
> In the kernel, we only use bindgen. This is because cbindgen assumes a
> cargo-based buildsystem, so it is not compatible with the kernel's build
> system. This means that when C code calls a Rust function by name, its
> signature must be duplicated in both Rust code and a C header, and the
> signature needs to be kept in sync manually.
> 
> To eliminate this manual checking, introduce a new macro that verifies
> at compile time that the two function declarations use the same
> signature. The idea is to run the C declaration through bindgen, and
> then have rustc verify that the function pointers have the same type.
> 
> The main commit of this series is "rust: add #[export] macro". Please
> see its commit message for more details.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>


Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v2 2/5] rust: macros: support additional tokens in quote!

[Alice Ryhl]

On Fri, Feb 28, 2025 at 4:26 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Fri, Feb 28, 2025 at 7:42 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > This gives the quote! macro support for the following additional tokens:
> >
> > * The = token.
> > * The _ token.
> > * Using #my_var with variables of type Ident.
> >
> > Additionally, some type annotations are added to allow cases where
> > groups are empty. For example, quote! does support () in the input, but
> > only when it is *not* empty. When it is empty, the compiler cannot infer
> > the item type of `tokens`.
> >
> > These additional quote! features are used by a new proc macro that
> > generates code looking like this:
> >
> >         const _: () = {
> >             if true {
> >                 ::kernel::bindings::#name
> >             } else {
> >                 #name
> >             };
> >         };
> >
> > where #name has type Ident.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/macros/quote.rs | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/macros/quote.rs b/rust/macros/quote.rs
> > index 33a199e4f176..c18960a91082 100644
> > --- a/rust/macros/quote.rs
> > +++ b/rust/macros/quote.rs
> > @@ -20,6 +20,12 @@ fn to_tokens(&self, tokens: &mut TokenStream) {
> >      }
> >  }
> >
> > +impl ToTokens for proc_macro::Ident {
> > +    fn to_tokens(&self, tokens: &mut TokenStream) {
> > +        tokens.extend([TokenTree::from(self.clone())]);
> > +    }
> > +}
> > +
> >  impl ToTokens for TokenTree {
> >      fn to_tokens(&self, tokens: &mut TokenStream) {
> >          tokens.extend([self.clone()]);
> > @@ -40,7 +46,7 @@ fn to_tokens(&self, tokens: &mut TokenStream) {
> >  /// `quote` crate but provides only just enough functionality needed by the current `macros` crate.
> >  macro_rules! quote_spanned {
> >      ($span:expr => $($tt:tt)*) => {{
> > -        let mut tokens;
> > +        let mut tokens: ::std::vec::Vec<::proc_macro::TokenTree>;
> >          #[allow(clippy::vec_init_then_push)]
> >          {
> >              tokens = ::std::vec::Vec::new();
> > @@ -65,7 +71,8 @@ macro_rules! quote_spanned {
> >          quote_spanned!(@proc $v $span $($tt)*);
> >      };
> >      (@proc $v:ident $span:ident ( $($inner:tt)* ) $($tt:tt)*) => {
> > -        let mut tokens = ::std::vec::Vec::new();
> > +        #[allow(unused_mut)]
>
> It'd be nice to mention the need for this attribute in the commit
> message along with the added type annotations.

Adding a note to mention that when the () is empty, not only can't it
infer the item type, it's also never modified.

Alice

Re: [PATCH v2 3/5] rust: add #[export] macro

[Alice Ryhl]

On Fri, Feb 28, 2025 at 4:40 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Fri, Feb 28, 2025 at 7:40 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > Rust has two different tools for generating function declarations to
> > call across the FFI boundary:
> >
> > * bindgen. Generates Rust declarations from a C header.
> > * cbindgen. Generates C headers from Rust declarations.
> >
> > In the kernel, we only use bindgen. This is because cbindgen assumes a
> > cargo-based buildsystem, so it is not compatible with the kernel's build
> > system. This means that when C code calls a Rust function by name, its
> > signature must be duplicated in both Rust code and a C header, and the
> > signature needs to be kept in sync manually.
>
> This needs an update given Miguel's comments on the cover letter. I
> wonder if the code should also justify the choice (over cbindgen).

Will do.

> > +/// Please see [`crate::export`] for documentation.
> > +pub(crate) fn export(_attr: TokenStream, ts: TokenStream) -> TokenStream {
> > +    let Some(name) = function_name(ts.clone()) else {
> > +        return "::core::compile_error!(\"The #[export] attribute must be used on a function.\");"
> > +            .parse::<TokenStream>()
> > +            .unwrap();
> > +    };
>
> Could you also show in the commit message what this error looks like
> when the macro is misused?

It looks like this:

error: The #[export] attribute must be used on a function.
   --> <linux>/rust/kernel/lib.rs:241:1
    |
241 | #[export]
    | ^^^^^^^^^
    |
    = note: this error originates in the attribute macro `export` (in
Nightly builds, run with -Z macro-backtrace for more info)

But I don't really think it's very useful to include this in the commit message.

> > +    let no_mangle = "#[no_mangle]".parse::<TokenStream>().unwrap();
>
> Would this be simpler as `let no_mangle = quote!("#[no_mangle]");`?

I'll do that. It requires adding the # token to the previous commit.

> > +/// This macro is *not* the same as the C macros `EXPORT_SYMBOL_*`, since all Rust symbols are
> > +/// currently automatically exported with `EXPORT_SYMBOL_GPL`.
>
> nit: "since" implies causation, which isn't the case, I think.
> Consider if ", since" can be replaced with a semicolon.

Will reword.

> > +#[proc_macro_attribute]
> > +pub fn export(attr: TokenStream, ts: TokenStream) -> TokenStream {
> > +    export::export(attr, ts)
> > +}
> > +
> >  /// Concatenate two identifiers.
> >  ///
> >  /// This is useful in macros that need to declare or reference items with names
> >
> > --
> > 2.48.1.711.g2feabab25a-goog
>
> Minor comments.
>
> Reviewed-by: Tamir Duberstein <tamird@gmail.com>

Thanks!

Alice

Re: [PATCH v2 5/5] panic_qr: use new #[export] macro

[Alice Ryhl]

On Fri, Feb 28, 2025 at 6:15 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Fri, Feb 28, 2025 at 12:08 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Fri, Feb 28, 2025 at 4:55 PM Tamir Duberstein <tamird@gmail.com> wrote:
> > >
> > > On Fri, Feb 28, 2025 at 7:41 AM Alice Ryhl <aliceryhl@google.com> wrote:
> > > >
> > > > @@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
> > > >  /// * If `url_len` > 0, remove the 2 segments header/length and also count the
> > > >  ///   conversion to numeric segments.
> > > >  /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment.
> > > > -#[no_mangle]
> > > > -pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> > > > +///
> > > > +/// # Safety
> > > > +///
> > > > +/// Always safe to call.
> > >
> > > This should explain why it's marked unsafe, since it's always safe to call.
> >
> > Safety comments generally do not explain rationale for why they are
> > the way they are. Where would you like me to put it?
>
> Safety comments also generally do not say that the function isn't
> really unsafe (with a notable exception in
> `samples/rust/rust_print_main.rs` which is similar to this case).
>
> Perhaps "This function is marked unsafe because ... but since a safety
> comment is still required:" would flow nicely into the safety section.

I added a comment, but I disagree with this claim. The phrase "Always
safe to call." is actually quite common for a "# Safety" section, even
if we have rarely needed it in the kernel specifically.

> > > > @@ -36,6 +36,10 @@
> > > >  #include <linux/workqueue.h>
> > > >  #include <trace/events/rust_sample.h>
> > > >
> > > > +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> > > > +#include <drm/drm_panic.h>
> > > > +#endif
> > >
> > > Why the guard here?
> > >
> > > It'd be nice to have a comment here explaining the atypical need for
> > > this include.
> >
> > It's not necessary. I can drop it.
>
> Ok. A comment on the include would still be helpful.

Added.

Alice