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

Alice Ryhl
2025-02-28T17:08:53Z

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