| Message ID | 20250228-export-macro-v2-5-569cc7e8926c@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Check Rust signatures at compile time | expand |
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
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
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 >
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
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;
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(-)