| Message ID | 20250412-rust_arm_fix_fw_abstaction-v2-1-8e6fdf093d71@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc` | expand |
On Sat, Apr 12, 2025 at 12:30 PM Christian Schrefl <chrisi.schrefl@gmail.com> wrote: > > "propper" type for this, so use a `*const kernel::ffi::c_char` pointer > instad. A couple typos -- I usually suggest using e.g. `scripts/checkpatch.pl --codespell`. > This should probably be backported to stable, for people/distros > using Arm 32 patches on stable. Up to the stable team -- I will add Cc: stable when I pick it up since it shouldn't hurt, but it does not change anything there for the supported arches, so they may or may not want to pick it. Thanks! Cheers, Miguel
On Sat, Apr 12, 2025 at 01:59:48PM +0200, Miguel Ojeda wrote: > On Sat, Apr 12, 2025 at 12:30 PM Christian Schrefl > <chrisi.schrefl@gmail.com> wrote: > > > > "propper" type for this, so use a `*const kernel::ffi::c_char` pointer > > instad. > > A couple typos -- I usually suggest using e.g. `scripts/checkpatch.pl > --codespell`. > > > This should probably be backported to stable, for people/distros > > using Arm 32 patches on stable. > > Up to the stable team -- I will add Cc: stable when I pick it up since > it shouldn't hurt, but it does not change anything there for the > supported arches, so they may or may not want to pick it. Typically, firmware patches go through driver-core, but if you like I'm fine with you taking it through the Rust tree. Acked-by: Danilo Krummrich <dakr@kernel.org> I don't think backporting it hurts, but it's also not really necessary. Like Miguel says, up to the stable team. Just note that before v6.13, core::ffi is required, since kernel::ffi does not exist. The firmware abstractions were introduced with v6.11. - Danilo
On 12.04.25 1:59 PM, Miguel Ojeda wrote: > On Sat, Apr 12, 2025 at 12:30 PM Christian Schrefl > <chrisi.schrefl@gmail.com> wrote: >> >> "propper" type for this, so use a `*const kernel::ffi::c_char` pointer >> instad. > > A couple typos -- I usually suggest using e.g. `scripts/checkpatch.pl > --codespell`. Do you want me to send a v3 with that fixed or are you going to do that when applying. (I've now configured my `b4 check` to add `--codespell`) > >> This should probably be backported to stable, for people/distros >> using Arm 32 patches on stable. > > Up to the stable team -- I will add Cc: stable when I pick it up since > it shouldn't hurt, but it does not change anything there for the > supported arches, so they may or may not want to pick it. > > Thanks! > > Cheers, > Miguel
On Sat, Apr 12, 2025 at 2:35 PM Danilo Krummrich <dakr@kernel.org> wrote: > > Typically, firmware patches go through driver-core, but if you like I'm fine > with you taking it through the Rust tree. > > Acked-by: Danilo Krummrich <dakr@kernel.org> Happy either way! If someone else takes it: Acked-by: Miguel Ojeda <ojeda@kernel.org> > Just note that before v6.13, core::ffi is required, since kernel::ffi does not > exist. The firmware abstractions were introduced with v6.11. It exists nowadays in 6.12.y (I backported it with the big `alloc` backport series). Cheers, Miguel
On Sat, Apr 12, 2025 at 3:55 PM Christian Schrefl <chrisi.schrefl@gmail.com> wrote: > > Do you want me to send a v3 with that fixed or are you going to do that > when applying. No, no worries, but thanks :) Cheers, Miguel
On Sat, Apr 12, 2025 at 12:29:48PM +0200, Christian Schrefl wrote: > The `FwFunc` struct contains an function with a char pointer argument, > for which a `*const u8` pointer was used. This is not really the > "propper" type for this, so use a `*const kernel::ffi::c_char` pointer > instad. > > This has no real functionality changes, since `kernel::ffi::c_char` is > a type alias to `u8` anyways. > > This used to cause problems on 6.13 when building for 32 bit arm (with > my patches), since rust mapped c_char to i8 instead. Now that I read this again: Isn't it the other way around? For arm32 c_char was mapped to u8, but FwFunc expected i8 (since that's what c_char was mapped to for all other architectures that are supported in v6.13). Can you please clarify this in the commit message?
On 12.04.25 4:53 PM, Danilo Krummrich wrote: > On Sat, Apr 12, 2025 at 12:29:48PM +0200, Christian Schrefl wrote: >> The `FwFunc` struct contains an function with a char pointer argument, >> for which a `*const u8` pointer was used. This is not really the >> "propper" type for this, so use a `*const kernel::ffi::c_char` pointer >> instad. >> >> This has no real functionality changes, since `kernel::ffi::c_char` is >> a type alias to `u8` anyways. >> >> This used to cause problems on 6.13 when building for 32 bit arm (with >> my patches), since rust mapped c_char to i8 instead. > > Now that I read this again: > > Isn't it the other way around? For arm32 c_char was mapped to u8, but FwFunc > expected i8 (since that's what c_char was mapped to for all other architectures > that are supported in v6.13). > > Can you please clarify this in the commit message? Ah sorry I got confused because by the change in 1bae8729e50a ("rust: map `long` to `isize` and `char` to `u8`"). How about changing that section to: .... This has no real functionality changes, since now `kernel::ffi::c_char` is now a type alias to `u8` anyways, but before commit 1bae8729e50a ("rust: map `long` to `isize` and `char` to `u8`") the concrete type of `core::ffi::c_char` depended on the architecture (However all supported architectures at the time mapped to `i8`). This causes problems on v6.13 when building for 32 bit arm (with my patches), since back then `*const u8` was used in the function argument and the function that bindgen generated used `*const core::ffi::c_char` which Rust mapped to `u8` on 32 bit arm. This caused the following build error: ... Cheers Christian
On Sun, Apr 13, 2025 at 10:55:35AM +0200, Christian Schrefl wrote: > On 12.04.25 4:53 PM, Danilo Krummrich wrote: > > On Sat, Apr 12, 2025 at 12:29:48PM +0200, Christian Schrefl wrote: > >> The `FwFunc` struct contains an function with a char pointer argument, > >> for which a `*const u8` pointer was used. This is not really the > >> "propper" type for this, so use a `*const kernel::ffi::c_char` pointer > >> instad. > >> > >> This has no real functionality changes, since `kernel::ffi::c_char` is > >> a type alias to `u8` anyways. > >> > >> This used to cause problems on 6.13 when building for 32 bit arm (with > >> my patches), since rust mapped c_char to i8 instead. > > > > Now that I read this again: > > > > Isn't it the other way around? For arm32 c_char was mapped to u8, but FwFunc > > expected i8 (since that's what c_char was mapped to for all other architectures > > that are supported in v6.13). > > > > Can you please clarify this in the commit message? > > Ah sorry I got confused because by the change in 1bae8729e50a ("rust: > map `long` to `isize` and `char` to `u8`"). > > How about changing that section to: > .... > This has no real functionality changes, since now `kernel::ffi::c_char` > is now a type alias to `u8` anyways, but before commit 1bae8729e50a ("rust: > map `long` to `isize` and `char` to `u8`") the concrete type of > `core::ffi::c_char` depended on the architecture (However all > supported architectures at the time mapped to `i8`). > > This causes problems on v6.13 when building for 32 bit arm (with my > patches), since back then `*const u8` was used in the function argument I'd put slightly more emphasis on the fact that there's no problem in v6.13 without addition your additional patches to introduce 32 bit arm support. Otherwise, sounds good! Thanks, Danilo
On Sun, Apr 13, 2025 at 10:55 AM Christian Schrefl <chrisi.schrefl@gmail.com> wrote: > > This causes problems on v6.13 when building for 32 bit arm (with my > patches), since back then `*const u8` was used in the function argument In v6.13, the function argument used `i8` -- if it were `u8`, you wouldn't have had an issue, no? Nowadays, in 6.13.y, everything is `u8`, so you shouldn't have a build error with your patches. Cheers, Miguel
On 13.04.25 5:07 PM, Miguel Ojeda wrote: > On Sun, Apr 13, 2025 at 10:55 AM Christian Schrefl > <chrisi.schrefl@gmail.com> wrote: >> >> This causes problems on v6.13 when building for 32 bit arm (with my >> patches), since back then `*const u8` was used in the function argument > > In v6.13, the function argument used `i8` -- if it were `u8`, you > wouldn't have had an issue, no? Yes I've sent a new version of this section in the thread with Danilo. I'll send it out as V3 soon. > > Nowadays, in 6.13.y, everything is `u8`, so you shouldn't have a build > error with your patches. > > Cheers, > Miguel
On Sun, Apr 13, 2025 at 5:12 PM Christian Schrefl <chrisi.schrefl@gmail.com> wrote: > > Yes I've sent a new version of this section in the thread with Danilo. > > I'll send it out as V3 soon. Hmm... the version I quoted is the new one, no? https://lore.kernel.org/rust-for-linux/c04d3ec9-46f8-4ccd-b0ed-52a1adea11b7@gmail.com/ Or do you mean some other thread? Cheers, Miguel
On 13.04.25 5:07 PM, Miguel Ojeda wrote: > On Sun, Apr 13, 2025 at 10:55 AM Christian Schrefl > <chrisi.schrefl@gmail.com> wrote: >> >> This causes problems on v6.13 when building for 32 bit arm (with my >> patches), since back then `*const u8` was used in the function argument > > In v6.13, the function argument used `i8` -- if it were `u8`, you > wouldn't have had an issue, no? > Ah right that`s a typo. Should have said `i8`. > > Nowadays, in 6.13.y, everything is `u8`, so you shouldn't have a build > error with your patches. I'll mention it explicitly that its not a problem in stable 6.13, only in the v6.13 tag. Sorry for the confusion, Thunderbird showed this as a response to the original patch mail, so I thought you were referring to that. Cheers Christian
On Sun, Apr 13, 2025 at 5:32 PM Christian Schrefl <chrisi.schrefl@gmail.com> wrote: > > Sorry for the confusion, Thunderbird showed this as a response to the > original patch mail, so I thought you were referring to that. No worries! Lore shows it as replying to the right message, so I assume it is Thunderbird-related, yeah. Thanks! Cheers, Miguel
diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index f04b058b09b2d2397e26344d0e055b3aa5061432..2494c96e105f3a28af74548d63a44464ba50eae3 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -4,7 +4,7 @@ //! //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h) -use crate::{bindings, device::Device, error::Error, error::Result, str::CStr}; +use crate::{bindings, device::Device, error::Error, error::Result, ffi, str::CStr}; use core::ptr::NonNull; /// # Invariants @@ -12,7 +12,11 @@ /// 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 {
The `FwFunc` struct contains an function with a char pointer argument, for which a `*const u8` pointer was used. This is not really the "propper" type for this, so use a `*const kernel::ffi::c_char` pointer instad. This has no real functionality changes, since `kernel::ffi::c_char` is a type alias to `u8` anyways. This used to cause problems on 6.13 when building for 32 bit arm (with my patches), since rust mapped c_char to i8 instead. Build error for this case: ``` error[E0308]: mismatched types --> rust/kernel/firmware.rs:20:4 | 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 ``` Fixes: de6582833db0 ("rust: add firmware abstractions") Reviewed-by: Benno Lossin <benno.lossin@proton.me> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com> --- This should probably be backported to stable, for people/distros using Arm 32 patches on stable. --- Changes in v2: - Use `kernel::ffi::c_char` instead of `core::ffi::c_char`. (Danilo & Benno) - Reword the commit message. - Link to v1: https://lore.kernel.org/r/20250411-rust_arm_fix_fw_abstaction-v1-1-0a9e598451c6@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,