[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87pld98zg3.fsf@kernel.org>
Date: Tue, 05 Aug 2025 19:12:44 +0200
From: Andreas Hindborg <a.hindborg@...sung.com>
To: Boqun Feng <boqun.feng@...il.com>, Danilo Krummrich <dakr@...hat.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, mcgrof@...nel.org,
russ.weight@...ux.dev, ojeda@...nel.org, alex.gaynor@...il.com,
wedsonaf@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
benno.lossin@...ton.me, aliceryhl@...gle.com, airlied@...il.com,
fujita.tomonori@...il.com, pstanner@...hat.com, ajanulgu@...hat.com,
lyude@...hat.com, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] rust: add firmware abstractions
Boqun Feng <boqun.feng@...il.com> writes:
> On Mon, Jun 17, 2024 at 10:29:41PM +0200, Danilo Krummrich wrote:
>> Add an abstraction around the kernels firmware API to request firmware
>> images. The abstraction provides functions to access the firmware's size
>> and backing buffer.
>>
>> The firmware is released once the abstraction instance is dropped.
>>
>> Signed-off-by: Danilo Krummrich <dakr@...hat.com>
..
>> +/// # Examples
>> +///
>> +/// ```
>> +/// # use kernel::{c_str, device::Device, firmware::Firmware};
>> +///
>> +/// # // SAFETY: *NOT* safe, just for the example to get an `ARef<Device>` instance
>> +/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) };
>> +///
>> +/// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev).unwrap();
>> +/// let blob = fw.data();
>> +/// ```
>> +pub struct Firmware(NonNull<bindings::firmware>);
>> +
>
> I feel like eventually we need a very simple smart pointer type for
> these case, for example:
>
> /// A smart pointer owns the underlying data.
> pub struct Owned<T: Ownable> {
> ptr: NonNull<T>,
> }
>
> impl<T: Ownable> Owned<T> {
> /// # Safety
> /// `ptr` needs to be a valid pointer, and it should be the
> /// unique owner to the object, in other words, no one can touch
> /// or free the underlying data.
> pub unsafe to_owned(ptr: *mut T) -> Self {
> // SAFETY: Per function safety requirement.
> Self { ptr: unsafe { NonNull::new_unchecked(ptr) } }
> }
>
> /// other safe constructors are available if a initializer (impl
> /// Init) is provided
> }
>
> /// A Ownable type is a type that can be put into `Owned<T>`, and
> /// when `Owned<T>` drops, `ptr_drop` will be called.
> pub unsafe trait Ownable {
> /// # Safety
> /// This could only be called in the `Owned::drop` function.
> unsafe fn ptr_drop(ptr: *mut Self);
> }
>
> impl<T: Ownable> Drop for Owned<T> {
> fn drop(&mut self) {
> /// SAFETY: In Owned<T>::drop.
> unsafe {
> <T as Ownable>::ptr_drop(self.as_mut_ptr());
> }
> }
> }
>
> we can implement Deref and DerefMut easily on `Owned<T>`. And then we
> could define Firmware as
>
> #[repr(transparent)]
> pub struct Firmware(Opaque<bindings::firmware>);
>
> and
>
> unsafe impl Ownable for Firmware {
> unsafe fn ptr_drop(ptr: *mut Self) {
> // SAFETY: Per function safety, this is called in
> // Owned::drop(), so `ptr` is a unique pointer to object,
> // it's safe to release the firmware.
> unsafe { bindings::release_firmware(ptr.cast()); }
> }
> }
>
> and the request_*() will return a `Result<Owned<Self>>`.
>
> Alice mentioned the need of this in page as well:
>
> https://lore.kernel.org/rust-for-linux/CAH5fLgjrt0Ohj1qBv=GrqZumBTMQ1jbsKakChmxmG2JYDJEM8w@mail.gmail.com
>
> Just bring it up while we are (maybe not? ;-)) at it. Also I would like
> to hear whether this would work for Firmware in the longer-term ;-) But
> yes, I'm not that worried about merging it as it is if others are all
> OK.
Please see [1] for an attempt at this pattern.
Best regards,
Andreas Hindborg
[1] https://lore.kernel.org/r/20250618-unique-ref-v11-0-49eadcdc0aa6@pm.me
Powered by blists - more mailing lists