[PATCH 0/4] Check Rust signatures at compile time

[PATCH 0/4] Check Rust signatures at compile time

[Alice Ryhl]

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Alice Ryhl (4):
      rust: fix signature of rust_fmt_argument
      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            | 11 ++++++-----
 rust/macros/export.rs           | 25 +++++++++++++++++++++++++
 rust/macros/helpers.rs          | 19 ++++++++++++++++++-
 rust/macros/lib.rs              | 18 ++++++++++++++++++
 rust/macros/quote.rs            | 21 +++++++++++++++++++--
 12 files changed, 112 insertions(+), 21 deletions(-)
---
base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
change-id: 20250227-export-macro-9aa9f1016d8c

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

[PATCH 1/4] 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.

Fixes: 787983da7718 ("vsprintf: add new `%pA` format specifier")
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 lib/vsprintf.c       | 2 +-
 rust/kernel/print.rs | 8 ++++----
 2 files changed, 5 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..8551631dedf1 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -6,13 +6,13 @@
 //!
 //! Reference: <https://docs.kernel.org/core-api/printk-basics.html>
 
-use core::{
+use core::fmt;
+
+use crate::{
     ffi::{c_char, c_void},
-    fmt,
+    str::RawFormatter,
 };
 
-use crate::str::RawFormatter;
-
 // Called from `vsprintf` with format specifier `%pA`.
 #[expect(clippy::missing_safety_doc)]
 #[no_mangle]

-- 
2.48.1.658.g4767266eb4-goog

[PATCH 2/4] rust: add #[export] macro

[Alice Ryhl]

This macro behaves like #[no_mangle], but also performs an assertion
that the Rust function has the same signature as what is declared in the
C header.

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}`

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/prelude.rs |  2 +-
 rust/macros/export.rs  | 25 +++++++++++++++++++++++++
 rust/macros/helpers.rs | 19 ++++++++++++++++++-
 rust/macros/lib.rs     | 18 ++++++++++++++++++
 rust/macros/quote.rs   | 21 +++++++++++++++++++--
 5 files changed, 81 insertions(+), 4 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..3398e1655124
--- /dev/null
+++ b/rust/macros/export.rs
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use crate::helpers::function_name;
+use proc_macro::TokenStream;
+
+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();
+    };
+
+    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..3cbf7705c4c1 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,23 @@ pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
     vtable::vtable(attr, ts)
 }
 
+/// Export a function so that C code can call it.
+///
+/// This macro has the following effect:
+///
+/// * Disables name mangling for this function.
+/// * Verifies at compile-time that the function signature matches what's in the header file.
+///
+/// This macro requires that the function is mentioned in a C header file, and that the header file
+/// is included in `rust/bindings/bindings_helper.h`.
+///
+/// This macro is *not* the same as the C macro `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
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.658.g4767266eb4-goog

[PATCH 3/4] 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.

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 8551631dedf1..e1a5ff3f34a9 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -10,12 +10,13 @@
 
 use crate::{
     ffi::{c_char, c_void},
+    prelude::*,
     str::RawFormatter,
 };
 
 // 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.658.g4767266eb4-goog

[PATCH 4/4] 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}`

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.658.g4767266eb4-goog

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

[Greg Kroah-Hartman]

On Thu, Feb 27, 2025 at 05:01:58PM +0000, Alice Ryhl wrote:
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

It's a bit odd to sign off on a 0/X email with no patch or description
:)

Seriously, nice work!  This resolves the issues I had, and it looks like
you found a needed fix already where things were not quite defined
properly.

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

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

[Boqun Feng]

On Thu, Feb 27, 2025 at 05:02:02PM +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

Is there a way to improve this error message? I vaguely remember there
are ways to do customized error message.

Regards,
Boqun

>    --> <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}`
> 
> 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.658.g4767266eb4-goog
> 

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

[Alice Ryhl]

On Thu, Feb 27, 2025 at 10:29 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Thu, Feb 27, 2025 at 05:02:02PM +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
>
> Is there a way to improve this error message? I vaguely remember there
> are ways to do customized error message.

The if/else error message is super nice because it shows the two types
next to each other where it's very easy to read the signatures and
spot the difference. But if you want to investigate other constructs
that potentially have better error messages, feel free. I'm happy to
update if you find a better construct.

Alice

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

[Alice Ryhl]

On Thu, Feb 27, 2025 at 8:31 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Feb 27, 2025 at 05:01:58PM +0000, Alice Ryhl wrote:
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>
> It's a bit odd to sign off on a 0/X email with no patch or description
> :)

What b4 does, I do. ;)

> Seriously, nice work!  This resolves the issues I had, and it looks like
> you found a needed fix already where things were not quite defined
> properly.
>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks!

Alice

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

[Andreas Hindborg]

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

> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

What is going on with the cover letter of this one?


Best regards,
Andreas Hindborg

Re: [PATCH 2/4] rust: add #[export] macro

[Andreas Hindborg]

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

> This macro behaves like #[no_mangle], but also performs an assertion
> that the Rust function has the same signature as what is declared in the
> C header.
>
> 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}`
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/prelude.rs |  2 +-
>  rust/macros/export.rs  | 25 +++++++++++++++++++++++++
>  rust/macros/helpers.rs | 19 ++++++++++++++++++-
>  rust/macros/lib.rs     | 18 ++++++++++++++++++
>  rust/macros/quote.rs   | 21 +++++++++++++++++++--
>  5 files changed, 81 insertions(+), 4 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..3398e1655124
> --- /dev/null
> +++ b/rust/macros/export.rs
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use crate::helpers::function_name;
> +use proc_macro::TokenStream;
> +
> +pub(crate) fn export(_attr: TokenStream, ts: TokenStream) -> TokenStream {

This function is documented in macros/lib.rs. Could you insert a
docstring with a link to the function that carries the docs?

Please describe how the function operates and what mechanics it uses to
achieve its goal in a implementation detail comment.

> +    let Some(name) = function_name(ts.clone()) else {
> +        return "::core::compile_error!(\"The #[export] attribute must be used on a function.\");"
> +            .parse::<TokenStream>()
> +            .unwrap();
> +    };
> +
> +    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> {

It would be great with a few tests for this function.

> +    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..3cbf7705c4c1 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,23 @@ pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
>      vtable::vtable(attr, ts)
>  }
>
> +/// Export a function so that C code can call it.
> +///
> +/// This macro has the following effect:
> +///
> +/// * Disables name mangling for this function.
> +/// * Verifies at compile-time that the function signature matches what's in the header file.
> +///
> +/// This macro requires that the function is mentioned in a C header file, and that the header file
> +/// is included in `rust/bindings/bindings_helper.h`.
> +///
> +/// This macro is *not* the same as the C macro `EXPORT_SYMBOL*`, since all Rust symbols are
> +/// currently automatically exported with `EXPORT_SYMBOL_GPL`.

Perhaps add the following:

This macro is useful when rust code is providing a function symbol whose
signature is dictated by a C header file.

> +#[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
> 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)*);

The update to `impl ToTokens for TokenTree` should be split out in a
separate patch and should carry some explanation of the change.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> 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.
>
> Fixes: 787983da7718 ("vsprintf: add new `%pA` format specifier")
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  lib/vsprintf.c       | 2 +-
>  rust/kernel/print.rs | 8 ++++----
>  2 files changed, 5 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..8551631dedf1 100644
> --- a/rust/kernel/print.rs
> +++ b/rust/kernel/print.rs
> @@ -6,13 +6,13 @@
>  //!
>  //! Reference: <https://docs.kernel.org/core-api/printk-basics.html>
>
> -use core::{
> +use core::fmt;
> +
> +use crate::{
>      ffi::{c_char, c_void},
> -    fmt,
> +    str::RawFormatter,
>  };
>
> -use crate::str::RawFormatter;
> -
>  // Called from `vsprintf` with format specifier `%pA`.
>  #[expect(clippy::missing_safety_doc)]
>  #[no_mangle]

The changes in this last hunk is not mentioned in the commit message.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> 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.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>


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


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

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

It would indeed be nice if there was a way to mark some functions to be
emitted as safe by bindgen.

>
> 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}`
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>


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


Best regards,
Andreas Hindborg

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

[Alice Ryhl]

On Fri, Feb 28, 2025 at 9:20 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > 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.
> >
> > Fixes: 787983da7718 ("vsprintf: add new `%pA` format specifier")
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  lib/vsprintf.c       | 2 +-
> >  rust/kernel/print.rs | 8 ++++----
> >  2 files changed, 5 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..8551631dedf1 100644
> > --- a/rust/kernel/print.rs
> > +++ b/rust/kernel/print.rs
> > @@ -6,13 +6,13 @@
> >  //!
> >  //! Reference: <https://docs.kernel.org/core-api/printk-basics.html>
> >
> > -use core::{
> > +use core::fmt;
> > +
> > +use crate::{
> >      ffi::{c_char, c_void},
> > -    fmt,
> > +    str::RawFormatter,
> >  };
> >
> > -use crate::str::RawFormatter;
> > -
> >  // Called from `vsprintf` with format specifier `%pA`.
> >  #[expect(clippy::missing_safety_doc)]
> >  #[no_mangle]
>
> The changes in this last hunk is not mentioned in the commit message.

The diff is rendered pretty poorly, but this is just importing
integers from crate::ffi instead of core::ffi, and I do believe that
the commit message makes it clear that this is needed.

Alice

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

[Alice Ryhl]

On Fri, Feb 28, 2025 at 8:19 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>
> What is going on with the cover letter of this one?

It's empty.

Alice

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

[Alice Ryhl]

On Thu, Feb 27, 2025 at 8:31 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Feb 27, 2025 at 05:01:58PM +0000, Alice Ryhl wrote:
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>
> It's a bit odd to sign off on a 0/X email with no patch or description
> :)
>
> Seriously, nice work!  This resolves the issues I had, and it looks like
> you found a needed fix already where things were not quite defined
> properly.
>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Greg, did you have any input on the choice of header file in the third patch?

Alice

Re: [PATCH 2/4] rust: add #[export] macro

[Alice Ryhl]

On Fri, Feb 28, 2025 at 9:20 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > This macro behaves like #[no_mangle], but also performs an assertion
> > that the Rust function has the same signature as what is declared in the
> > C header.
> >
> > 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}`
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/prelude.rs |  2 +-
> >  rust/macros/export.rs  | 25 +++++++++++++++++++++++++
> >  rust/macros/helpers.rs | 19 ++++++++++++++++++-
> >  rust/macros/lib.rs     | 18 ++++++++++++++++++
> >  rust/macros/quote.rs   | 21 +++++++++++++++++++--
> >  5 files changed, 81 insertions(+), 4 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..3398e1655124
> > --- /dev/null
> > +++ b/rust/macros/export.rs
> > @@ -0,0 +1,25 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +use crate::helpers::function_name;
> > +use proc_macro::TokenStream;
> > +
> > +pub(crate) fn export(_attr: TokenStream, ts: TokenStream) -> TokenStream {
>
> This function is documented in macros/lib.rs. Could you insert a
> docstring with a link to the function that carries the docs?

These functions are not visible in the docs, and no other macro does that.

> Please describe how the function operates and what mechanics it uses to
> achieve its goal in a implementation detail comment.
>
> > +    let Some(name) = function_name(ts.clone()) else {
> > +        return "::core::compile_error!(\"The #[export] attribute must be used on a function.\");"
> > +            .parse::<TokenStream>()
> > +            .unwrap();
> > +    };
> > +
> > +    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> {
>
> It would be great with a few tests for this function.

I don't think we have a mechanism for tests in the macro crate?

> > +    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..3cbf7705c4c1 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,23 @@ pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
> >      vtable::vtable(attr, ts)
> >  }
> >
> > +/// Export a function so that C code can call it.
> > +///
> > +/// This macro has the following effect:
> > +///
> > +/// * Disables name mangling for this function.
> > +/// * Verifies at compile-time that the function signature matches what's in the header file.
> > +///
> > +/// This macro requires that the function is mentioned in a C header file, and that the header file
> > +/// is included in `rust/bindings/bindings_helper.h`.
> > +///
> > +/// This macro is *not* the same as the C macro `EXPORT_SYMBOL*`, since all Rust symbols are
> > +/// currently automatically exported with `EXPORT_SYMBOL_GPL`.
>
> Perhaps add the following:
>
> This macro is useful when rust code is providing a function symbol whose
> signature is dictated by a C header file.

I do think this could use more info about when to use it. E.g., you
don't use it if C calls it via a vtable, but only if C calls it via a
declaration in a header file. I'll add more info.

> > +#[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
> > 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)*);
>
> The update to `impl ToTokens for TokenTree` should be split out in a
> separate patch and should carry some explanation of the change.

I think this case is borderline for whether it's necessary to split
up, but okay.

Alice

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

[Andreas Hindborg]

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

> On Fri, Feb 28, 2025 at 9:20 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > 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.
>> >
>> > Fixes: 787983da7718 ("vsprintf: add new `%pA` format specifier")
>> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> > ---
>> >  lib/vsprintf.c       | 2 +-
>> >  rust/kernel/print.rs | 8 ++++----
>> >  2 files changed, 5 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..8551631dedf1 100644
>> > --- a/rust/kernel/print.rs
>> > +++ b/rust/kernel/print.rs
>> > @@ -6,13 +6,13 @@
>> >  //!
>> >  //! Reference: <https://docs.kernel.org/core-api/printk-basics.html>
>> >
>> > -use core::{
>> > +use core::fmt;
>> > +
>> > +use crate::{
>> >      ffi::{c_char, c_void},
>> > -    fmt,
>> > +    str::RawFormatter,
>> >  };
>> >
>> > -use crate::str::RawFormatter;
>> > -
>> >  // Called from `vsprintf` with format specifier `%pA`.
>> >  #[expect(clippy::missing_safety_doc)]
>> >  #[no_mangle]
>>
>> The changes in this last hunk is not mentioned in the commit message.
>
> The diff is rendered pretty poorly, but this is just importing
> integers from crate::ffi instead of core::ffi, and I do believe that
> the commit message makes it clear that this is needed.

I see, thanks for clarifying. Consider including the clarification in
the commit message.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On Fri, Feb 28, 2025 at 8:19 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>
>> What is going on with the cover letter of this one?
>
> It's empty.

I can see that 😆 Would you consider making it not empty?


Best regards,
Andreas Hindborg

Re: [PATCH 2/4] rust: add #[export] macro

[Andreas Hindborg]

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

> On Fri, Feb 28, 2025 at 9:20 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > This macro behaves like #[no_mangle], but also performs an assertion
>> > that the Rust function has the same signature as what is declared in the
>> > C header.
>> >
>> > 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}`
>> >
>> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> > ---
>> >  rust/kernel/prelude.rs |  2 +-
>> >  rust/macros/export.rs  | 25 +++++++++++++++++++++++++
>> >  rust/macros/helpers.rs | 19 ++++++++++++++++++-
>> >  rust/macros/lib.rs     | 18 ++++++++++++++++++
>> >  rust/macros/quote.rs   | 21 +++++++++++++++++++--
>> >  5 files changed, 81 insertions(+), 4 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..3398e1655124
>> > --- /dev/null
>> > +++ b/rust/macros/export.rs
>> > @@ -0,0 +1,25 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +
>> > +use crate::helpers::function_name;
>> > +use proc_macro::TokenStream;
>> > +
>> > +pub(crate) fn export(_attr: TokenStream, ts: TokenStream) -> TokenStream {
>>
>> This function is documented in macros/lib.rs. Could you insert a
>> docstring with a link to the function that carries the docs?
>
> These functions are not visible in the docs, and no other macro does that.

I do realize that. I don't think it is ever too late to start improving.
Don't you think it would be overall net positive to have

/// Please see [`crate::export`] for documentation.

attached to this function?

>
>> Please describe how the function operates and what mechanics it uses to
>> achieve its goal in a implementation detail comment.
>>
>> > +    let Some(name) = function_name(ts.clone()) else {
>> > +        return "::core::compile_error!(\"The #[export] attribute must be used on a function.\");"
>> > +            .parse::<TokenStream>()
>> > +            .unwrap();
>> > +    };
>> > +
>> > +    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> {
>>
>> It would be great with a few tests for this function.
>
> I don't think we have a mechanism for tests in the macro crate?

Ah, I didn't realize. I'll create an issue for that if it is so.

>
>> > +    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..3cbf7705c4c1 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,23 @@ pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
>> >      vtable::vtable(attr, ts)
>> >  }
>> >
>> > +/// Export a function so that C code can call it.
>> > +///
>> > +/// This macro has the following effect:
>> > +///
>> > +/// * Disables name mangling for this function.
>> > +/// * Verifies at compile-time that the function signature matches what's in the header file.
>> > +///
>> > +/// This macro requires that the function is mentioned in a C header file, and that the header file
>> > +/// is included in `rust/bindings/bindings_helper.h`.
>> > +///
>> > +/// This macro is *not* the same as the C macro `EXPORT_SYMBOL*`, since all Rust symbols are
>> > +/// currently automatically exported with `EXPORT_SYMBOL_GPL`.
>>
>> Perhaps add the following:
>>
>> This macro is useful when rust code is providing a function symbol whose
>> signature is dictated by a C header file.
>
> I do think this could use more info about when to use it. E.g., you
> don't use it if C calls it via a vtable, but only if C calls it via a
> declaration in a header file. I'll add more info.
>
>> > +#[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
>> > 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)*);
>>
>> The update to `impl ToTokens for TokenTree` should be split out in a
>> separate patch and should carry some explanation of the change.
>
> I think this case is borderline for whether it's necessary to split
> up, but okay.

Thanks!


Best regards,
Andreas Hindborg

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

[Andy Shevchenko]

On Thu, Feb 27, 2025 at 05:01:58PM +0000, Alice Ryhl wrote:

This is unfortunate. What does this mean?
Can you, please, provide a meaningful cover letter?

> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Feb 28, 2025 at 09:46:32AM +0100, Alice Ryhl wrote:
> On Fri, Feb 28, 2025 at 8:19 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >
> > "Alice Ryhl" <aliceryhl@google.com> writes:
> >
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> >
> > What is going on with the cover letter of this one?
> 
> It's empty.

So, it means nothing to review, sorry.

-- 
With Best Regards,
Andy Shevchenko