[PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Christian Schrefl]

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(-)

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 {

---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250408-rust_arm_fix_fw_abstaction-4c3a89d75e29

Best regards,
-- 
Christian Schrefl <chrisi.schrefl@gmail.com>

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Miguel Ojeda]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Danilo Krummrich]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Christian Schrefl]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Miguel Ojeda]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Miguel Ojeda]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Danilo Krummrich]

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?

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Christian Schrefl]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Danilo Krummrich]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Miguel Ojeda]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Christian Schrefl]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Miguel Ojeda]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Christian Schrefl]

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

Re: [PATCH v2] rust: Use `ffi::c_char` type in firmware abstraction `FwFunc`

[Miguel Ojeda]

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