| Message ID | 20250411-rust_arm_fix_fw_abstaction-v1-1-0a9e598451c6@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | rust: fix building firmware abstraction on 32bit arm | expand |
On Fri Apr 11, 2025 at 9:14 AM CEST, Christian Schrefl wrote: > When trying to build the rust firmware abstractions on 32 bit arm the > following build error occures: > > ``` [...] > ``` > > To fix this error the char pointer type in `FwFunc` is converted to > `ffi::c_char`. > > Fixes: de6582833db0 ("rust: add firmware abstractions") > Cc: stable@vger.kernel.org # Backport only to 6.15 needed > > Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com> Reviewed-by: Benno Lossin <benno.lossin@proton.me> --- Cheers, Benno > --- > rust/kernel/firmware.rs | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-)
On Fri, Apr 11, 2025 at 09:14:48AM +0200, Christian Schrefl wrote: > When trying to build the rust firmware abstractions on 32 bit arm the > following build error occures: > > ``` > error[E0308]: mismatched types > --> rust/kernel/firmware.rs:20:14 > | > 20 | Self(bindings::request_firmware) > | ---- ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found fn item > | | > | arguments to this function are incorrect > | > = note: expected fn pointer `unsafe extern "C" fn(_, *const i8, _) -> _` > found fn item `unsafe extern "C" fn(_, *const u8, _) -> _ {request_firmware}` This looks like you have local changes in your tree, running in this error. I get the exact same errors when I apply the following diff: diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index f04b058b09b2..a67047e3aa6b 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -12,7 +12,7 @@ /// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`, /// `bindings::firmware_request_platform`, `bindings::request_firmware_direct`. struct FwFunc( - unsafe extern "C" fn(*mut *const bindings::firmware, *const u8, *mut bindings::device) -> i32, + unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32, ); > note: tuple struct defined here > --> rust/kernel/firmware.rs:14:8 > | > 14 | struct FwFunc( > | ^^^^^^ > > error[E0308]: mismatched types > --> rust/kernel/firmware.rs:24:14 > | > 24 | Self(bindings::firmware_request_nowarn) > | ---- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found fn item > | | > | arguments to this function are incorrect > | > = note: expected fn pointer `unsafe extern "C" fn(_, *const i8, _) -> _` > found fn item `unsafe extern "C" fn(_, *const u8, _) -> _ {firmware_request_nowarn}` > note: tuple struct defined here > --> rust/kernel/firmware.rs:14:8 > | > 14 | struct FwFunc( > | ^^^^^^ > > error[E0308]: mismatched types > --> rust/kernel/firmware.rs:64:45 > | > 64 | let ret = unsafe { func.0(pfw as _, name.as_char_ptr(), dev.as_raw()) }; > | ------ ^^^^^^^^^^^^^^^^^^ expected `*const i8`, found `*const u8` > | | > | arguments to this function are incorrect > | > = note: expected raw pointer `*const i8` > found raw pointer `*const u8` > > error: aborting due to 3 previous errors > ``` I did a test build with multi_v7_defconfig and I can't reproduce this issue. I think the kernel does always use -funsigned-char, as also documented in commit 1bae8729e50a ("rust: map `long` to `isize` and `char` to `u8`")? > > To fix this error the char pointer type in `FwFunc` is converted to > `ffi::c_char`. > > Fixes: de6582833db0 ("rust: add firmware abstractions") > Cc: stable@vger.kernel.org # Backport only to 6.15 needed > > Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com> > --- > rust/kernel/firmware.rs | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > index f04b058b09b2d2397e26344d0e055b3aa5061432..1d6284316f2a4652ef3f76272670e5e29b0ff924 100644 > --- a/rust/kernel/firmware.rs > +++ b/rust/kernel/firmware.rs > @@ -5,14 +5,18 @@ > //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h) > > use crate::{bindings, device::Device, error::Error, error::Result, str::CStr}; > -use core::ptr::NonNull; > +use core::{ffi, ptr::NonNull}; The change itself seems to be fine anyways, but I think we should use crate::ffi instead.
On Fri Apr 11, 2025 at 9:14 AM CEST, Christian Schrefl wrote: > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > index f04b058b09b2d2397e26344d0e055b3aa5061432..1d6284316f2a4652ef3f76272670e5e29b0ff924 100644 > --- a/rust/kernel/firmware.rs > +++ b/rust/kernel/firmware.rs > @@ -5,14 +5,18 @@ > //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h) > > use crate::{bindings, device::Device, error::Error, error::Result, str::CStr}; > -use core::ptr::NonNull; > +use core::{ffi, ptr::NonNull}; Ah I overlooked this, you should be using `kernel::ffi` (or `crate::ffi`) instead of `core`. (for `c_char` it doesn't matter, but we shouldn't be using `core::ffi`, since we have our own mappings). --- Cheers, Benno
On 11.04.25 12:35 PM, Danilo Krummrich wrote: > On Fri, Apr 11, 2025 at 09:14:48AM +0200, Christian Schrefl wrote: >> When trying to build the rust firmware abstractions on 32 bit arm the >> following build error occures: >> >> ``` >> error[E0308]: mismatched types >> --> rust/kernel/firmware.rs:20:14 >> | >> 20 | Self(bindings::request_firmware) >> | ---- ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found fn item >> | | >> | arguments to this function are incorrect >> | >> = note: expected fn pointer `unsafe extern "C" fn(_, *const i8, _) -> _` >> found fn item `unsafe extern "C" fn(_, *const u8, _) -> _ {request_firmware}` > > This looks like you have local changes in your tree, running in this error. I > get the exact same errors when I apply the following diff: > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > index f04b058b09b2..a67047e3aa6b 100644 > --- a/rust/kernel/firmware.rs > +++ b/rust/kernel/firmware.rs > @@ -12,7 +12,7 @@ > /// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`, > /// `bindings::firmware_request_platform`, `bindings::request_firmware_direct`. > struct FwFunc( > - unsafe extern "C" fn(*mut *const bindings::firmware, *const u8, *mut bindings::device) -> i32, > + unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32, > ); > >> note: tuple struct defined here >> --> rust/kernel/firmware.rs:14:8 >> | >> 14 | struct FwFunc( >> | ^^^^^^ >> >> error[E0308]: mismatched types >> --> rust/kernel/firmware.rs:24:14 >> | >> 24 | Self(bindings::firmware_request_nowarn) >> | ---- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found fn item >> | | >> | arguments to this function are incorrect >> | >> = note: expected fn pointer `unsafe extern "C" fn(_, *const i8, _) -> _` >> found fn item `unsafe extern "C" fn(_, *const u8, _) -> _ {firmware_request_nowarn}` >> note: tuple struct defined here >> --> rust/kernel/firmware.rs:14:8 >> | >> 14 | struct FwFunc( >> | ^^^^^^ >> >> error[E0308]: mismatched types >> --> rust/kernel/firmware.rs:64:45 >> | >> 64 | let ret = unsafe { func.0(pfw as _, name.as_char_ptr(), dev.as_raw()) }; >> | ------ ^^^^^^^^^^^^^^^^^^ expected `*const i8`, found `*const u8` >> | | >> | arguments to this function are incorrect >> | >> = note: expected raw pointer `*const i8` >> found raw pointer `*const u8` >> >> error: aborting due to 3 previous errors >> ``` > > I did a test build with multi_v7_defconfig and I can't reproduce this issue. > Interesting, I've it seems this is only an issue on 6.13 with my arm patches applied. It seems that it works on v6.14 and v6.15-rc1 but the error occurs on ffd294d346d1 (tag: v6.13) with my 32-bit arm patches applied. > I think the kernel does always use -funsigned-char, as also documented in commit > 1bae8729e50a ("rust: map `long` to `isize` and `char` to `u8`")? > >> >> To fix this error the char pointer type in `FwFunc` is converted to >> `ffi::c_char`. >> >> Fixes: de6582833db0 ("rust: add firmware abstractions") >> Cc: stable@vger.kernel.org # Backport only to 6.15 needed >> >> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com> >> --- >> rust/kernel/firmware.rs | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs >> index f04b058b09b2d2397e26344d0e055b3aa5061432..1d6284316f2a4652ef3f76272670e5e29b0ff924 100644 >> --- a/rust/kernel/firmware.rs >> +++ b/rust/kernel/firmware.rs >> @@ -5,14 +5,18 @@ >> //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h) >> >> use crate::{bindings, device::Device, error::Error, error::Result, str::CStr}; >> -use core::ptr::NonNull; >> +use core::{ffi, ptr::NonNull}; > > The change itself seems to be fine anyways, but I think we should use crate::ffi > instead. Right, I just did what RA recommended without thinking about it much. I guess this patch isn't really needed. Should I still send a V2 using `crate::ffi`? Cheers, Christian
On Fri, Apr 11, 2025 at 2:46 PM Benno Lossin <benno.lossin@proton.me> wrote: > > Ah I overlooked this, you should be using `kernel::ffi` (or > `crate::ffi`) instead of `core`. (for `c_char` it doesn't matter, but we > shouldn't be using `core::ffi`, since we have our own mappings). In 6.6, C `char` changed to unsigned, but `core::ffi::c_char` is signed (in x86_64 at least). We should just never use `core::ffi` (except in `rust/ffi.rs`, of course) -- I think we should just add the C types to the prelude (which we discussed in the past) so that it is easy to avoid the mistake (something like the patch attached as the end result, but tested and across a kernel cycle or two) and mention it in the Coding Guidelines. Thoughts? I tried to use Clippy's `disallowed-types` too: disallowed-types = [ { path = "core::ffi::c_void", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_char", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_schar", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_uchar", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_short", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_ushort", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_int", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_uint", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_long", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_ulong", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_longlong", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_ulonglong", reason = "the `kernel::ffi` types should be used instead" }, ] But it goes across aliases. Cheers, Miguel
On Fri, Apr 11, 2025 at 03:47:28PM +0200, Christian Schrefl wrote: > On 11.04.25 12:35 PM, Danilo Krummrich wrote: > > On Fri, Apr 11, 2025 at 09:14:48AM +0200, Christian Schrefl wrote: > > I did a test build with multi_v7_defconfig and I can't reproduce this issue. > > > Interesting, I've it seems this is only an issue on 6.13 with my arm patches applied. > > It seems that it works on v6.14 and v6.15-rc1 but the error occurs on ffd294d346d1 (tag: v6.13) > with my 32-bit arm patches applied. That makes sense, commit 1bae8729e50a ("rust: map `long` to `isize` and `char` to `u8`") changed FwFunc to take a *const u8, which previously was *const i8. > >> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > >> index f04b058b09b2d2397e26344d0e055b3aa5061432..1d6284316f2a4652ef3f76272670e5e29b0ff924 100644 > >> --- a/rust/kernel/firmware.rs > >> +++ b/rust/kernel/firmware.rs > >> @@ -5,14 +5,18 @@ > >> //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h) > >> > >> use crate::{bindings, device::Device, error::Error, error::Result, str::CStr}; > >> -use core::ptr::NonNull; > >> +use core::{ffi, ptr::NonNull}; > > > > The change itself seems to be fine anyways, but I think we should use crate::ffi > > instead. > Right, I just did what RA recommended without thinking about it much. > > I guess this patch isn't really needed. Should I still send a V2 using `crate::ffi`? Yes, please. I think it's still an improvement.
On Fri, Apr 11, 2025 at 4:15 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > In 6.6, C `char` changed to unsigned, but `core::ffi::c_char` is > signed (in x86_64 at least). By 6.6, I meant since 6.6.y LTS (since that is the one I remember due to backports), the actual change happened earlier but after 6.1.y LTS. Cheers, Miguel
On Fri Apr 11, 2025 at 4:15 PM CEST, Miguel Ojeda wrote: > On Fri, Apr 11, 2025 at 2:46 PM Benno Lossin <benno.lossin@proton.me> wrote: >> >> Ah I overlooked this, you should be using `kernel::ffi` (or >> `crate::ffi`) instead of `core`. (for `c_char` it doesn't matter, but we >> shouldn't be using `core::ffi`, since we have our own mappings). > > In 6.6, C `char` changed to unsigned, but `core::ffi::c_char` is > signed (in x86_64 at least). > > We should just never use `core::ffi` (except in `rust/ffi.rs`, of > course) -- I think we should just add the C types to the prelude > (which we discussed in the past) so that it is easy to avoid the > mistake (something like the patch attached as the end result, but > tested and across a kernel cycle or two) and mention it in the Coding > Guidelines. Thoughts? Yeah sounds like a good idea. > I tried to use Clippy's `disallowed-types` too: > > disallowed-types = [ > { path = "core::ffi::c_void", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_char", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_schar", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_uchar", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_short", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_ushort", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_int", reason = "the `kernel::ffi` types > should be used instead" }, > { path = "core::ffi::c_uint", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_long", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_ulong", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_longlong", reason = "the `kernel::ffi` > types should be used instead" }, > { path = "core::ffi::c_ulonglong", reason = "the `kernel::ffi` > types should be used instead" }, > ] > > But it goes across aliases. We could make the types in `ffi` be transparent newtypes. But not sure if that could interfere with kCFI or other stuff. --- Cheers, Benno
diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index f04b058b09b2d2397e26344d0e055b3aa5061432..1d6284316f2a4652ef3f76272670e5e29b0ff924 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -5,14 +5,18 @@ //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h) use crate::{bindings, device::Device, error::Error, error::Result, str::CStr}; -use core::ptr::NonNull; +use core::{ffi, ptr::NonNull}; /// # Invariants /// /// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`, /// `bindings::firmware_request_platform`, `bindings::request_firmware_direct`. struct FwFunc( - unsafe extern "C" fn(*mut *const bindings::firmware, *const u8, *mut bindings::device) -> i32, + unsafe extern "C" fn( + *mut *const bindings::firmware, + *const ffi::c_char, + *mut bindings::device, + ) -> i32, ); impl FwFunc {
When trying to build the rust firmware abstractions on 32 bit arm the following build error occures: ``` error[E0308]: mismatched types --> rust/kernel/firmware.rs:20:14 | 20 | Self(bindings::request_firmware) | ---- ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found fn item | | | arguments to this function are incorrect | = note: expected fn pointer `unsafe extern "C" fn(_, *const i8, _) -> _` found fn item `unsafe extern "C" fn(_, *const u8, _) -> _ {request_firmware}` note: tuple struct defined here --> rust/kernel/firmware.rs:14:8 | 14 | struct FwFunc( | ^^^^^^ error[E0308]: mismatched types --> rust/kernel/firmware.rs:24:14 | 24 | Self(bindings::firmware_request_nowarn) | ---- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found fn item | | | arguments to this function are incorrect | = note: expected fn pointer `unsafe extern "C" fn(_, *const i8, _) -> _` found fn item `unsafe extern "C" fn(_, *const u8, _) -> _ {firmware_request_nowarn}` note: tuple struct defined here --> rust/kernel/firmware.rs:14:8 | 14 | struct FwFunc( | ^^^^^^ error[E0308]: mismatched types --> rust/kernel/firmware.rs:64:45 | 64 | let ret = unsafe { func.0(pfw as _, name.as_char_ptr(), dev.as_raw()) }; | ------ ^^^^^^^^^^^^^^^^^^ expected `*const i8`, found `*const u8` | | | arguments to this function are incorrect | = note: expected raw pointer `*const i8` found raw pointer `*const u8` error: aborting due to 3 previous errors ``` To fix this error the char pointer type in `FwFunc` is converted to `ffi::c_char`. Fixes: de6582833db0 ("rust: add firmware abstractions") Cc: stable@vger.kernel.org # Backport only to 6.15 needed Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com> --- rust/kernel/firmware.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) --- base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 change-id: 20250408-rust_arm_fix_fw_abstaction-4c3a89d75e29 Best regards,