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(-)
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 {
--- base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8 change-id: 20250408-rust_arm_fix_fw_abstaction-4c3a89d75e29
Best regards,
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 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 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 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 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 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
On Sat, Apr 12, 2025 at 10:01:22AM +0000, Benno Lossin wrote:
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.
Transparent newtypes for all integers would be super inconvenient.
Alice
On Mon Apr 14, 2025 at 4:05 PM CEST, Alice Ryhl wrote:
On Sat, Apr 12, 2025 at 10:01:22AM +0000, Benno Lossin wrote:
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.
Transparent newtypes for all integers would be super inconvenient.
Yeah I noticed that too when trying... We often assign integer literals to the ffi types.
--- Cheers, Benno
linux-stable-mirror@lists.linaro.org
-
Alice Ryhl -
Benno Lossin -
Christian Schrefl -
Danilo Krummrich -
Miguel Ojeda