On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote:> However, if you have a driver that needs the firmware abstractions, I would be > surprised if there were any hesitations to already merge the minimum device > abstractions [1] and this one (once reviewed) without the rest. At least there > aren't any from my side. > > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr at redhat.com/No, the device abstractions are NOT ready for merging just yet, sorry, if for no other reason than a non-RFC has never been posted :) greg k-h
On Tue, May 28, 2024 at 02:45:02PM +0200, Greg KH wrote:> On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote: > > However, if you have a driver that needs the firmware abstractions, I would be > > surprised if there were any hesitations to already merge the minimum device > > abstractions [1] and this one (once reviewed) without the rest. At least there > > aren't any from my side. > > > > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr at redhat.com/ > > No, the device abstractions are NOT ready for merging just yet, sorry, > if for no other reason than a non-RFC has never been posted :)I did not say they are ready. I said that I'd be surprised if we couldn't merge [1] once it is ready even without the rest being ready. :)> > greg k-h >
Hi, On Tue, 28 May 2024 14:45:02 +0200 Greg KH <gregkh at linuxfoundation.org> wrote:> On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote: >> However, if you have a driver that needs the firmware abstractions, I would be >> surprised if there were any hesitations to already merge the minimum device >> abstractions [1] and this one (once reviewed) without the rest. At least there >> aren't any from my side. >> >> [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr at redhat.com/ > > No, the device abstractions are NOT ready for merging just yet, sorry, > if for no other reason than a non-RFC has never been posted :)Indeed, I understand that you aren't convinced. We can start with much simpler device abstractions than the above minimum for the firmware abstractions. All the firmware abstractions need is wrapping C's pointer to a device object with Rust struct only during a caller knows the pointer is valid. No play with the reference count of a struct device. For a Rust PHY driver, you know that you have a valid pointer to C's device object of C's PHY device during the probe callback. The driver creates a Rust device object to wrap the C pointer to the C's device object and passes it to the firmware abstractions. The firmware abstractions gets the C's pointer from the Rust object and calls C's function to load firmware, returns the result. You have concerns about the simple code like the following? diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs new file mode 100644 index 000000000000..6144437984a9 --- /dev/null +++ b/rust/kernel/device.rs @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Generic devices that are part of the kernel's driver model. +//! +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h) + +use crate::types::Opaque; + +#[repr(transparent)] +pub struct Device(Opaque<bindings::device>); + +impl Device { + /// Creates a new [`Device`] instance from a raw pointer. + /// + /// # Safety + /// + /// For the duration of 'a, the pointer must point at a valid `device`. + pub unsafe fn from_raw<'a>(ptr: *mut bindings::device) -> &'a Self { + // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`. + let ptr = ptr.cast::<Self>(); + // SAFETY: by the function requirements the pointer is valid and we have unique access for + // the duration of `'a`. + unsafe { &mut *ptr } + } + + /// Returns the raw pointer to the device. + pub fn as_raw(&self) -> *mut bindings::device { + self.0.get() + } +}
On Wed, May 29, 2024 at 09:28:21AM +0900, FUJITA Tomonori wrote:> Hi, > > On Tue, 28 May 2024 14:45:02 +0200 > Greg KH <gregkh at linuxfoundation.org> wrote: > > > On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote: > >> However, if you have a driver that needs the firmware abstractions, I would be > >> surprised if there were any hesitations to already merge the minimum device > >> abstractions [1] and this one (once reviewed) without the rest. At least there > >> aren't any from my side. > >> > >> [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr at redhat.com/ > > > > No, the device abstractions are NOT ready for merging just yet, sorry, > > if for no other reason than a non-RFC has never been posted :) > > Indeed, I understand that you aren't convinced. > > We can start with much simpler device abstractions than the above > minimum for the firmware abstractions. > > All the firmware abstractions need is wrapping C's pointer to a device > object with Rust struct only during a caller knows the pointer is > valid. No play with the reference count of a struct device.Makes sense, but note that almost no one should be dealing with "raw" struct device pointers :)> For a Rust PHY driver, you know that you have a valid pointer to C's > device object of C's PHY device during the probe callback. The driver > creates a Rust device object to wrap the C pointer to the C's device > object and passes it to the firmware abstractions. The firmware > abstractions gets the C's pointer from the Rust object and calls C's > function to load firmware, returns the result. > > You have concerns about the simple code like the following? > > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs > new file mode 100644 > index 000000000000..6144437984a9 > --- /dev/null > +++ b/rust/kernel/device.rs > @@ -0,0 +1,30 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Generic devices that are part of the kernel's driver model. > +//! > +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h) > + > +use crate::types::Opaque; > + > +#[repr(transparent)] > +pub struct Device(Opaque<bindings::device>); > + > +impl Device { > + /// Creates a new [`Device`] instance from a raw pointer. > + /// > + /// # Safety > + /// > + /// For the duration of 'a, the pointer must point at a valid `device`.If the following rust code does what this comment says, then sure, I'm ok with it for now if it helps you all out with stuff like the firmware interface for the phy rust code. thanks, greg k-h