| Message ID | 20240520172422.181763-4-dakr@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM Rust abstractions and Nova | expand |
On Mon, 20 May 2024 19:24:19 +0200 Danilo Krummrich <dakr@redhat.com> wrote: > Add an abstraction around the kernels firmware API to request firmware > images. The abstraction provides functions to access the firmware > buffer and / or copy it to a new buffer allocated with a given > allocator backend. > Was playing with firmware extractions based on this patch. Unfortunately I ended up with a lot of pointer operations, unsafe statements. As we know many vendors have a C headers for the definitions of the firwmare content, the driver extract the data by applying a struct pointer on it. But in rust, I feel it would nice that we can also have a common firmware extractor for drivers, that can wrap the pointer operations, take a list of the firmware struct members that converted from C headers as the input, offer the driver some common ABI methods to query them. Maybe that would ease the pain a lot. Thanks, Zhi. > The firmware is released once the abstraction is dropped. > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > --- > rust/bindings/bindings_helper.h | 1 + > rust/kernel/firmware.rs | 74 > +++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | > 1 + 3 files changed, 76 insertions(+) > create mode 100644 rust/kernel/firmware.rs > > diff --git a/rust/bindings/bindings_helper.h > b/rust/bindings/bindings_helper.h index b245db8d5a87..e4ffc47da5ec > 100644 --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -14,6 +14,7 @@ > #include <kunit/test.h> > #include <linux/errname.h> > #include <linux/ethtool.h> > +#include <linux/firmware.h> > #include <linux/jiffies.h> > #include <linux/mdio.h> > #include <linux/pci.h> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > new file mode 100644 > index 000000000000..700504fb3c9c > --- /dev/null > +++ b/rust/kernel/firmware.rs > @@ -0,0 +1,74 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Firmware abstraction > +//! > +//! C header: > [`include/linux/firmware.h`](../../../../include/linux/firmware.h") + > +use crate::{bindings, device::Device, error::Error, error::Result, > str::CStr, types::Opaque}; + > +/// Abstraction around a C firmware struct. > +/// > +/// This is a simple abstraction around the C firmware API. Just > like with the C API, firmware can +/// be requested. Once requested > the abstraction provides direct access to the firmware buffer as +/// > `&[u8]`. Alternatively, the firmware can be copied to a new buffer > using `Firmware::copy`. The +/// firmware is released once > [`Firmware`] is dropped. +/// +/// # Examples > +/// > +/// ``` > +/// let fw = Firmware::request("path/to/firmware.bin", > dev.as_ref())?; +/// driver_load_firmware(fw.data()); > +/// ``` > +pub struct Firmware(Opaque<*const bindings::firmware>); > + > +impl Firmware { > + /// Send a firmware request and wait for it. See also > `bindings::request_firmware`. > + pub fn request(name: &CStr, dev: &Device) -> Result<Self> { > + let fw = Opaque::uninit(); > + > + let ret = unsafe { bindings::request_firmware(fw.get(), > name.as_char_ptr(), dev.as_raw()) }; > + if ret != 0 { > + return Err(Error::from_errno(ret)); > + } > + > + Ok(Firmware(fw)) > + } > + > + /// Send a request for an optional fw module. See also > `bindings::request_firmware_nowarn`. > + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> > { > + let fw = Opaque::uninit(); > + > + let ret = unsafe { > + bindings::firmware_request_nowarn(fw.get(), > name.as_char_ptr(), dev.as_raw()) > + }; > + if ret != 0 { > + return Err(Error::from_errno(ret)); > + } > + > + Ok(Firmware(fw)) > + } > + > + /// Returns the size of the requested firmware in bytes. > + pub fn size(&self) -> usize { > + unsafe { (*(*self.0.get())).size } > + } > + > + /// Returns the requested firmware as `&[u8]`. > + pub fn data(&self) -> &[u8] { > + unsafe { > core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) } > + } > +} > + > +impl Drop for Firmware { > + fn drop(&mut self) { > + unsafe { bindings::release_firmware(*self.0.get()) }; > + } > +} > + > +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, > which is safe to be used from any +// thread. > +unsafe impl Send for Firmware {} > + > +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, > references to which are safe to +// be used from any thread. > +unsafe impl Sync for Firmware {} > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 6415968ee3b8..ed97d131661a 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -35,6 +35,7 @@ > #[cfg(CONFIG_DRM = "y")] > pub mod drm; > pub mod error; > +pub mod firmware; > pub mod init; > pub mod ioctl; > #[cfg(CONFIG_KUNIT)]
Hi, Thanks for working on the firmware API! On Mon, 20 May 2024 19:24:19 +0200 Danilo Krummrich <dakr@redhat.com> wrote: > Add an abstraction around the kernels firmware API to request firmware > images. The abstraction provides functions to access the firmware > buffer and / or copy it to a new buffer allocated with a given allocator > backend. > > The firmware is released once the abstraction is dropped. > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > --- > rust/bindings/bindings_helper.h | 1 + > rust/kernel/firmware.rs | 74 +++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 3 files changed, 76 insertions(+) > create mode 100644 rust/kernel/firmware.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index b245db8d5a87..e4ffc47da5ec 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -14,6 +14,7 @@ > #include <kunit/test.h> > #include <linux/errname.h> > #include <linux/ethtool.h> > +#include <linux/firmware.h> > #include <linux/jiffies.h> > #include <linux/mdio.h> > #include <linux/pci.h> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > new file mode 100644 > index 000000000000..700504fb3c9c > --- /dev/null > +++ b/rust/kernel/firmware.rs > @@ -0,0 +1,74 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Firmware abstraction > +//! > +//! C header: [`include/linux/firmware.h`](../../../../include/linux/firmware.h") > + > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr, types::Opaque}; > + > +/// Abstraction around a C firmware struct. > +/// > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The > +/// firmware is released once [`Firmware`] is dropped. > +/// > +/// # Examples > +/// > +/// ``` > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?; > +/// driver_load_firmware(fw.data()); > +/// ``` > +pub struct Firmware(Opaque<*const bindings::firmware>); Wrapping a raw pointer is not the intended use of Qpaque type?
On Wed, 2024-05-22 at 08:53 +0900, FUJITA Tomonori wrote: > Hi, > Thanks for working on the firmware API! > > On Mon, 20 May 2024 19:24:19 +0200 > Danilo Krummrich <dakr@redhat.com> wrote: > > > Add an abstraction around the kernels firmware API to request > > firmware > > images. The abstraction provides functions to access the firmware > > buffer and / or copy it to a new buffer allocated with a given > > allocator > > backend. > > > > The firmware is released once the abstraction is dropped. > > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > > --- > > rust/bindings/bindings_helper.h | 1 + > > rust/kernel/firmware.rs | 74 > > +++++++++++++++++++++++++++++++++ > > rust/kernel/lib.rs | 1 + > > 3 files changed, 76 insertions(+) > > create mode 100644 rust/kernel/firmware.rs > > > > diff --git a/rust/bindings/bindings_helper.h > > b/rust/bindings/bindings_helper.h > > index b245db8d5a87..e4ffc47da5ec 100644 > > --- a/rust/bindings/bindings_helper.h > > +++ b/rust/bindings/bindings_helper.h > > @@ -14,6 +14,7 @@ > > #include <kunit/test.h> > > #include <linux/errname.h> > > #include <linux/ethtool.h> > > +#include <linux/firmware.h> > > #include <linux/jiffies.h> > > #include <linux/mdio.h> > > #include <linux/pci.h> > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > > new file mode 100644 > > index 000000000000..700504fb3c9c > > --- /dev/null > > +++ b/rust/kernel/firmware.rs > > @@ -0,0 +1,74 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Firmware abstraction > > +//! > > +//! C header: > > [`include/linux/firmware.h`](../../../../include/linux/firmware.h") > > + > > +use crate::{bindings, device::Device, error::Error, error::Result, > > str::CStr, types::Opaque}; > > + > > +/// Abstraction around a C firmware struct. > > +/// > > +/// This is a simple abstraction around the C firmware API. Just > > like with the C API, firmware can > > +/// be requested. Once requested the abstraction provides direct > > access to the firmware buffer as > > +/// `&[u8]`. Alternatively, the firmware can be copied to a new > > buffer using `Firmware::copy`. The > > +/// firmware is released once [`Firmware`] is dropped. > > +/// > > +/// # Examples > > +/// > > +/// ``` > > +/// let fw = Firmware::request("path/to/firmware.bin", > > dev.as_ref())?; > > +/// driver_load_firmware(fw.data()); > > +/// ``` > > +pub struct Firmware(Opaque<*const bindings::firmware>); > > Wrapping a raw pointer is not the intended use of Qpaque type? > What is the intended use? As I see it, all uses wrapp some binding::* – but a rawpointer to a binding shouldn't be wrapped by it? Maybe we can add something to the docu in kernel/types.rs: /// Stores an opaque value. /// /// This is meant to be used with FFI objects that are never interpreted by Rust code. #[repr(transparent)] pub struct Opaque<T> { P.
Hi, On Wed, 22 May 2024 09:37:30 +0200 Philipp Stanner <pstanner@redhat.com> wrote: >> > +/// Abstraction around a C firmware struct. >> > +/// >> > +/// This is a simple abstraction around the C firmware API. Just >> > like with the C API, firmware can >> > +/// be requested. Once requested the abstraction provides direct >> > access to the firmware buffer as >> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new >> > buffer using `Firmware::copy`. The >> > +/// firmware is released once [`Firmware`] is dropped. >> > +/// >> > +/// # Examples >> > +/// >> > +/// ``` >> > +/// let fw = Firmware::request("path/to/firmware.bin", >> > dev.as_ref())?; >> > +/// driver_load_firmware(fw.data()); >> > +/// ``` >> > +pub struct Firmware(Opaque<*const bindings::firmware>); >> >> Wrapping a raw pointer is not the intended use of Qpaque type? >> > > What is the intended use? > As I see it, all uses wrapp some binding::* – but a rawpointer to a > binding shouldn't be wrapped by it? If I understand correctly, it's for embedding C's struct in Rust's struct (as all the usage in the tree do). It gives the tricks for initialization and a pointer to the embedded object. The C's firmware API gives a pointer to an initialized object so no reason to use Opaque. With such C API, Rust's struct simply uses raw pointers in old rust branch. For example, https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/chrdev.rs#L28 struct Cdev(*mut bindings::cdev); Another choice that I know is NonNull: https://lore.kernel.org/lkml/20240415-alice-mm-v5-4-6f55e4d8ef51@google.com/ pub struct Page { page: NonNull<bindings::page>, }
On Thu, May 23, 2024 at 08:15:13AM +0900, FUJITA Tomonori wrote: > Hi, > > On Wed, 22 May 2024 09:37:30 +0200 > Philipp Stanner <pstanner@redhat.com> wrote: > > >> > +/// Abstraction around a C firmware struct. > >> > +/// > >> > +/// This is a simple abstraction around the C firmware API. Just > >> > like with the C API, firmware can > >> > +/// be requested. Once requested the abstraction provides direct > >> > access to the firmware buffer as > >> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new > >> > buffer using `Firmware::copy`. The > >> > +/// firmware is released once [`Firmware`] is dropped. > >> > +/// > >> > +/// # Examples > >> > +/// > >> > +/// ``` > >> > +/// let fw = Firmware::request("path/to/firmware.bin", > >> > dev.as_ref())?; > >> > +/// driver_load_firmware(fw.data()); > >> > +/// ``` > >> > +pub struct Firmware(Opaque<*const bindings::firmware>); > >> > >> Wrapping a raw pointer is not the intended use of Qpaque type? > >> > > > > What is the intended use? > > As I see it, all uses wrapp some binding::* – but a rawpointer to a > > binding shouldn't be wrapped by it? > Thank you Tomo for calling this out! And yes, using `Opaque` on a pointer is weird. A `Opaque<T>` is `UnsafeCell<MayUninit<T>>`, `UnsafeCell` means the inner `T` may be accessed by C code at anytime, and `MayUninit` means that the inner `T` may not be properly initialized by the C code. So as the doc says: This is meant to be used with FFI objects that are never interpreted by Rust code. that is, Rust code should never create a `&T` or `&mut T`, it should only be accessed with `Opaque::get()` or its friends (i.e. accessing it via a raw pointer), much like a black box to Rust code in some sense. Hence putting `Opaque` on a raw pointer is not what you want to do. > If I understand correctly, it's for embedding C's struct in Rust's > struct (as all the usage in the tree do). It gives the tricks for > initialization and a pointer to the embedded object. > > The C's firmware API gives a pointer to an initialized object so no > reason to use Opaque. > > With such C API, Rust's struct simply uses raw pointers in old rust > branch. For example, > > https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/chrdev.rs#L28 > > struct Cdev(*mut bindings::cdev); > > > Another choice that I know is NonNull: > > https://lore.kernel.org/lkml/20240415-alice-mm-v5-4-6f55e4d8ef51@google.com/ > > pub struct Page { > page: NonNull<bindings::page>, > } Both are reasonable for temporary use, although, we could generify the "wrapping on pointer which owns the underlying object" in the future. Regards, Boqun
On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote: > On Mon, 20 May 2024 19:24:19 +0200 > Danilo Krummrich <dakr@redhat.com> wrote: > > > Add an abstraction around the kernels firmware API to request firmware > > images. The abstraction provides functions to access the firmware > > buffer and / or copy it to a new buffer allocated with a given > > allocator backend. > > > > Was playing with firmware extractions based on this patch. > Unfortunately I ended up with a lot of pointer operations, unsafe > statements. > > As we know many vendors have a C headers for the definitions of the > firwmare content, the driver extract the data by applying a struct > pointer on it. > > But in rust, I feel it would nice that we can also have a common > firmware extractor for drivers, that can wrap the pointer operations, > take a list of the firmware struct members that converted from C headers > as the input, offer the driver some common ABI methods to query them. > Maybe that would ease the pain a lot. So, you mean some abstraction that takes a list of types, offsets in the firmware and a reference to the firmware itself and provides references to the corresponding objects? I agree it might be helpful to have some common infrastructure for this, but the operations on it would still be unsafe, since ultimately it involves dereferencing pointers. > > Thanks, > Zhi. > > > The firmware is released once the abstraction is dropped. > > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > > --- > > rust/bindings/bindings_helper.h | 1 + > > rust/kernel/firmware.rs | 74 > > +++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | > > 1 + 3 files changed, 76 insertions(+) > > create mode 100644 rust/kernel/firmware.rs > > > > diff --git a/rust/bindings/bindings_helper.h > > b/rust/bindings/bindings_helper.h index b245db8d5a87..e4ffc47da5ec > > 100644 --- a/rust/bindings/bindings_helper.h > > +++ b/rust/bindings/bindings_helper.h > > @@ -14,6 +14,7 @@ > > #include <kunit/test.h> > > #include <linux/errname.h> > > #include <linux/ethtool.h> > > +#include <linux/firmware.h> > > #include <linux/jiffies.h> > > #include <linux/mdio.h> > > #include <linux/pci.h> > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > > new file mode 100644 > > index 000000000000..700504fb3c9c > > --- /dev/null > > +++ b/rust/kernel/firmware.rs > > @@ -0,0 +1,74 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Firmware abstraction > > +//! > > +//! C header: > > [`include/linux/firmware.h`](../../../../include/linux/firmware.h") + > > +use crate::{bindings, device::Device, error::Error, error::Result, > > str::CStr, types::Opaque}; + > > +/// Abstraction around a C firmware struct. > > +/// > > +/// This is a simple abstraction around the C firmware API. Just > > like with the C API, firmware can +/// be requested. Once requested > > the abstraction provides direct access to the firmware buffer as +/// > > `&[u8]`. Alternatively, the firmware can be copied to a new buffer > > using `Firmware::copy`. The +/// firmware is released once > > [`Firmware`] is dropped. +/// +/// # Examples > > +/// > > +/// ``` > > +/// let fw = Firmware::request("path/to/firmware.bin", > > dev.as_ref())?; +/// driver_load_firmware(fw.data()); > > +/// ``` > > +pub struct Firmware(Opaque<*const bindings::firmware>); > > + > > +impl Firmware { > > + /// Send a firmware request and wait for it. See also > > `bindings::request_firmware`. > > + pub fn request(name: &CStr, dev: &Device) -> Result<Self> { > > + let fw = Opaque::uninit(); > > + > > + let ret = unsafe { bindings::request_firmware(fw.get(), > > name.as_char_ptr(), dev.as_raw()) }; > > + if ret != 0 { > > + return Err(Error::from_errno(ret)); > > + } > > + > > + Ok(Firmware(fw)) > > + } > > + > > + /// Send a request for an optional fw module. See also > > `bindings::request_firmware_nowarn`. > > + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> > > { > > + let fw = Opaque::uninit(); > > + > > + let ret = unsafe { > > + bindings::firmware_request_nowarn(fw.get(), > > name.as_char_ptr(), dev.as_raw()) > > + }; > > + if ret != 0 { > > + return Err(Error::from_errno(ret)); > > + } > > + > > + Ok(Firmware(fw)) > > + } > > + > > + /// Returns the size of the requested firmware in bytes. > > + pub fn size(&self) -> usize { > > + unsafe { (*(*self.0.get())).size } > > + } > > + > > + /// Returns the requested firmware as `&[u8]`. > > + pub fn data(&self) -> &[u8] { > > + unsafe { > > core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) } > > + } > > +} > > + > > +impl Drop for Firmware { > > + fn drop(&mut self) { > > + unsafe { bindings::release_firmware(*self.0.get()) }; > > + } > > +} > > + > > +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, > > which is safe to be used from any +// thread. > > +unsafe impl Send for Firmware {} > > + > > +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, > > references to which are safe to +// be used from any thread. > > +unsafe impl Sync for Firmware {} > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > index 6415968ee3b8..ed97d131661a 100644 > > --- a/rust/kernel/lib.rs > > +++ b/rust/kernel/lib.rs > > @@ -35,6 +35,7 @@ > > #[cfg(CONFIG_DRM = "y")] > > pub mod drm; > > pub mod error; > > +pub mod firmware; > > pub mod init; > > pub mod ioctl; > > #[cfg(CONFIG_KUNIT)] >
On Wed, May 22, 2024 at 08:53:34AM +0900, FUJITA Tomonori wrote: > Hi, > Thanks for working on the firmware API! > > On Mon, 20 May 2024 19:24:19 +0200 > Danilo Krummrich <dakr@redhat.com> wrote: > > > Add an abstraction around the kernels firmware API to request firmware > > images. The abstraction provides functions to access the firmware > > buffer and / or copy it to a new buffer allocated with a given allocator > > backend. > > > > The firmware is released once the abstraction is dropped. > > > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > > --- > > rust/bindings/bindings_helper.h | 1 + > > rust/kernel/firmware.rs | 74 +++++++++++++++++++++++++++++++++ > > rust/kernel/lib.rs | 1 + > > 3 files changed, 76 insertions(+) > > create mode 100644 rust/kernel/firmware.rs > > > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > > index b245db8d5a87..e4ffc47da5ec 100644 > > --- a/rust/bindings/bindings_helper.h > > +++ b/rust/bindings/bindings_helper.h > > @@ -14,6 +14,7 @@ > > #include <kunit/test.h> > > #include <linux/errname.h> > > #include <linux/ethtool.h> > > +#include <linux/firmware.h> > > #include <linux/jiffies.h> > > #include <linux/mdio.h> > > #include <linux/pci.h> > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > > new file mode 100644 > > index 000000000000..700504fb3c9c > > --- /dev/null > > +++ b/rust/kernel/firmware.rs > > @@ -0,0 +1,74 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Firmware abstraction > > +//! > > +//! C header: [`include/linux/firmware.h`](../../../../include/linux/firmware.h") > > + > > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr, types::Opaque}; > > + > > +/// Abstraction around a C firmware struct. > > +/// > > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can > > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as > > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The > > +/// firmware is released once [`Firmware`] is dropped. > > +/// > > +/// # Examples > > +/// > > +/// ``` > > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?; > > +/// driver_load_firmware(fw.data()); > > +/// ``` > > +pub struct Firmware(Opaque<*const bindings::firmware>); > > Wrapping a raw pointer is not the intended use of Qpaque type? > Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of the boilerplate in the 'request' functions to some common 'request_internal' one.
On 27/05/2024 22.18, Danilo Krummrich wrote: > External email: Use caution opening links or attachments > > > On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote: >> On Mon, 20 May 2024 19:24:19 +0200 >> Danilo Krummrich <dakr@redhat.com> wrote: >> >>> Add an abstraction around the kernels firmware API to request firmware >>> images. The abstraction provides functions to access the firmware >>> buffer and / or copy it to a new buffer allocated with a given >>> allocator backend. >>> >> >> Was playing with firmware extractions based on this patch. >> Unfortunately I ended up with a lot of pointer operations, unsafe >> statements. >> >> As we know many vendors have a C headers for the definitions of the >> firwmare content, the driver extract the data by applying a struct >> pointer on it. >> >> But in rust, I feel it would nice that we can also have a common >> firmware extractor for drivers, that can wrap the pointer operations, >> take a list of the firmware struct members that converted from C headers >> as the input, offer the driver some common ABI methods to query them. >> Maybe that would ease the pain a lot. > > So, you mean some abstraction that takes a list of types, offsets in the > firmware and a reference to the firmware itself and provides references to the > corresponding objects? > > I agree it might be helpful to have some common infrastructure for this, but the > operations on it would still be unsafe, since ultimately it involves > dereferencing pointers. > I think the goal is to 1) concentrate the "unsafe" operations in a abstraction so the driver doesn't have to write explanation of unsafe operations here and there, improve the readability of the driver that interacts with vendor-firmware buffer. 2) ease the driver maintenance burden. Some solutions I saw in different projects written in rust for de-referencing a per-defined struct: 1) Take the vendor firmware buffer as a whole, invent methods like read/write with offset for operating the buffer. In this scheme, the driver is responsible for taking care of each data member in a vendor firmware struct and also its valid offset. The abstraction only covers the boundary of the whole firmware buffer. The cons is the readability of the operation of the vendor firmware buffer in the driver is not good comparing with C code. Hate to think a lot of xxx = vendor_firmware_struct.read(offset); // reading item A in the code. 2) Define the firmware struct in rust (might need to find a better way to handle "union" in the definition of the vendor firmware struct). Use macros to generate methods of accessing each data member for the vendor firmware struct. Then the code in the driver will be like xxx = vendor_firmware_struct.item_a(); in the driver. In this scheme, the abstraction and the generated methods covers the boundary check. The "unsafe" statement can stay in the generated struct-access methods. This might even be implemented as a more generic rust feature in the kernel. The cons is still the driver might need to sync between the C-version definition and rust-version definition. 3) Also re-using C bindings generation in the kernel came to my mind when thinking of this problem, since it allows the rust code to access the C struct, but they don't have the boundary check. Still need another layer/macros to wrap it. >> >> Thanks, >> Zhi. >> >>> The firmware is released once the abstraction is dropped. >>> >>> Signed-off-by: Danilo Krummrich <dakr@redhat.com> >>> --- >>> rust/bindings/bindings_helper.h | 1 + >>> rust/kernel/firmware.rs | 74 >>> +++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | >>> 1 + 3 files changed, 76 insertions(+) >>> create mode 100644 rust/kernel/firmware.rs >>> >>> diff --git a/rust/bindings/bindings_helper.h >>> b/rust/bindings/bindings_helper.h index b245db8d5a87..e4ffc47da5ec >>> 100644 --- a/rust/bindings/bindings_helper.h >>> +++ b/rust/bindings/bindings_helper.h >>> @@ -14,6 +14,7 @@ >>> #include <kunit/test.h> >>> #include <linux/errname.h> >>> #include <linux/ethtool.h> >>> +#include <linux/firmware.h> >>> #include <linux/jiffies.h> >>> #include <linux/mdio.h> >>> #include <linux/pci.h> >>> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs >>> new file mode 100644 >>> index 000000000000..700504fb3c9c >>> --- /dev/null >>> +++ b/rust/kernel/firmware.rs >>> @@ -0,0 +1,74 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + >>> +//! Firmware abstraction >>> +//! >>> +//! C header: >>> [`include/linux/firmware.h`](../../../../include/linux/firmware.h") + >>> +use crate::{bindings, device::Device, error::Error, error::Result, >>> str::CStr, types::Opaque}; + >>> +/// Abstraction around a C firmware struct. >>> +/// >>> +/// This is a simple abstraction around the C firmware API. Just >>> like with the C API, firmware can +/// be requested. Once requested >>> the abstraction provides direct access to the firmware buffer as +/// >>> `&[u8]`. Alternatively, the firmware can be copied to a new buffer >>> using `Firmware::copy`. The +/// firmware is released once >>> [`Firmware`] is dropped. +/// +/// # Examples >>> +/// >>> +/// ``` >>> +/// let fw = Firmware::request("path/to/firmware.bin", >>> dev.as_ref())?; +/// driver_load_firmware(fw.data()); >>> +/// ``` >>> +pub struct Firmware(Opaque<*const bindings::firmware>); >>> + >>> +impl Firmware { >>> + /// Send a firmware request and wait for it. See also >>> `bindings::request_firmware`. >>> + pub fn request(name: &CStr, dev: &Device) -> Result<Self> { >>> + let fw = Opaque::uninit(); >>> + >>> + let ret = unsafe { bindings::request_firmware(fw.get(), >>> name.as_char_ptr(), dev.as_raw()) }; >>> + if ret != 0 { >>> + return Err(Error::from_errno(ret)); >>> + } >>> + >>> + Ok(Firmware(fw)) >>> + } >>> + >>> + /// Send a request for an optional fw module. See also >>> `bindings::request_firmware_nowarn`. >>> + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> >>> { >>> + let fw = Opaque::uninit(); >>> + >>> + let ret = unsafe { >>> + bindings::firmware_request_nowarn(fw.get(), >>> name.as_char_ptr(), dev.as_raw()) >>> + }; >>> + if ret != 0 { >>> + return Err(Error::from_errno(ret)); >>> + } >>> + >>> + Ok(Firmware(fw)) >>> + } >>> + >>> + /// Returns the size of the requested firmware in bytes. >>> + pub fn size(&self) -> usize { >>> + unsafe { (*(*self.0.get())).size } >>> + } >>> + >>> + /// Returns the requested firmware as `&[u8]`. >>> + pub fn data(&self) -> &[u8] { >>> + unsafe { >>> core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) } >>> + } >>> +} >>> + >>> +impl Drop for Firmware { >>> + fn drop(&mut self) { >>> + unsafe { bindings::release_firmware(*self.0.get()) }; >>> + } >>> +} >>> + >>> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, >>> which is safe to be used from any +// thread. >>> +unsafe impl Send for Firmware {} >>> + >>> +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, >>> references to which are safe to +// be used from any thread. >>> +unsafe impl Sync for Firmware {} >>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >>> index 6415968ee3b8..ed97d131661a 100644 >>> --- a/rust/kernel/lib.rs >>> +++ b/rust/kernel/lib.rs >>> @@ -35,6 +35,7 @@ >>> #[cfg(CONFIG_DRM = "y")] >>> pub mod drm; >>> pub mod error; >>> +pub mod firmware; >>> pub mod init; >>> pub mod ioctl; >>> #[cfg(CONFIG_KUNIT)] >> >
Hi, On Tue, 28 May 2024 08:40:20 +0000 Zhi Wang <zhiw@nvidia.com> wrote: > On 27/05/2024 22.18, Danilo Krummrich wrote: >> External email: Use caution opening links or attachments >> >> >> On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote: >>> On Mon, 20 May 2024 19:24:19 +0200 >>> Danilo Krummrich <dakr@redhat.com> wrote: >>> >>>> Add an abstraction around the kernels firmware API to request firmware >>>> images. The abstraction provides functions to access the firmware >>>> buffer and / or copy it to a new buffer allocated with a given >>>> allocator backend. >>>> >>> >>> Was playing with firmware extractions based on this patch. >>> Unfortunately I ended up with a lot of pointer operations, unsafe >>> statements. >>> >>> As we know many vendors have a C headers for the definitions of the >>> firwmare content, the driver extract the data by applying a struct >>> pointer on it. >>> >>> But in rust, I feel it would nice that we can also have a common >>> firmware extractor for drivers, that can wrap the pointer operations, >>> take a list of the firmware struct members that converted from C headers >>> as the input, offer the driver some common ABI methods to query them. >>> Maybe that would ease the pain a lot. >> >> So, you mean some abstraction that takes a list of types, offsets in the >> firmware and a reference to the firmware itself and provides references to the >> corresponding objects? >> >> I agree it might be helpful to have some common infrastructure for this, but the >> operations on it would still be unsafe, since ultimately it involves >> dereferencing pointers. >> > > I think the goal is to 1) concentrate the "unsafe" operations in a > abstraction so the driver doesn't have to write explanation of unsafe > operations here and there, improve the readability of the driver that > interacts with vendor-firmware buffer. 2) ease the driver maintenance > burden. > > Some solutions I saw in different projects written in rust for > de-referencing a per-defined struct: Aren't there other abstractions that require that functionality, not just the firmware abstractions?
On 28/05/2024 13.17, FUJITA Tomonori wrote: > External email: Use caution opening links or attachments > > > Hi, > > On Tue, 28 May 2024 08:40:20 +0000 > Zhi Wang <zhiw@nvidia.com> wrote: > >> On 27/05/2024 22.18, Danilo Krummrich wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote: >>>> On Mon, 20 May 2024 19:24:19 +0200 >>>> Danilo Krummrich <dakr@redhat.com> wrote: >>>> >>>>> Add an abstraction around the kernels firmware API to request firmware >>>>> images. The abstraction provides functions to access the firmware >>>>> buffer and / or copy it to a new buffer allocated with a given >>>>> allocator backend. >>>>> >>>> >>>> Was playing with firmware extractions based on this patch. >>>> Unfortunately I ended up with a lot of pointer operations, unsafe >>>> statements. >>>> >>>> As we know many vendors have a C headers for the definitions of the >>>> firwmare content, the driver extract the data by applying a struct >>>> pointer on it. >>>> >>>> But in rust, I feel it would nice that we can also have a common >>>> firmware extractor for drivers, that can wrap the pointer operations, >>>> take a list of the firmware struct members that converted from C headers >>>> as the input, offer the driver some common ABI methods to query them. >>>> Maybe that would ease the pain a lot. >>> >>> So, you mean some abstraction that takes a list of types, offsets in the >>> firmware and a reference to the firmware itself and provides references to the >>> corresponding objects? >>> >>> I agree it might be helpful to have some common infrastructure for this, but the >>> operations on it would still be unsafe, since ultimately it involves >>> dereferencing pointers. >>> >> >> I think the goal is to 1) concentrate the "unsafe" operations in a >> abstraction so the driver doesn't have to write explanation of unsafe >> operations here and there, improve the readability of the driver that >> interacts with vendor-firmware buffer. 2) ease the driver maintenance >> burden. >> >> Some solutions I saw in different projects written in rust for >> de-referencing a per-defined struct: > > Aren't there other abstractions that require that functionality, not > just the firmware abstractions? Sure, but they might implement it in a different way due to their different scale and requirements of maintenance. I am more interested in what is the better option for firmware abstractions based on its scale and requirements. 1) how to improve the readability of the driver, meanwhile keep the operations safe. 2) there has been C-version vendor-firmware definitions, what would be the best for the rust driver to leverage it based on the routines that the rust kernel has already had. 3) how to avoid syncing the headers of vendor firmware between C and rust version, as the definition can scale up due to HW generations or ABI changes. Thanks, Zhi.
On Mon, 27 May 2024 21:22:47 +0200 Danilo Krummrich <dakr@redhat.com> wrote: >> > +/// Abstraction around a C firmware struct. >> > +/// >> > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can >> > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as >> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The >> > +/// firmware is released once [`Firmware`] is dropped. >> > +/// >> > +/// # Examples >> > +/// >> > +/// ``` >> > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?; >> > +/// driver_load_firmware(fw.data()); >> > +/// ``` >> > +pub struct Firmware(Opaque<*const bindings::firmware>); >> >> Wrapping a raw pointer is not the intended use of Qpaque type? >> > > Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of > the boilerplate in the 'request' functions to some common 'request_internal' one. You might need to add 'Invariants' comment on Firmware struct. BTW, what merge window are you aiming for? As I wrote before, I have a driver that needs the firmware abstractions (the minimum device abstractions is enough; Device::as_raw() and as_ref()). So the sooner, the better for me.
Hi Luis and Russ, I just noted I forgot to add you to this patch, sorry for that. Please find the full series and previous discussions in [1]. - Danilo [1] https://lore.kernel.org/rust-for-linux/20240520172059.181256-1-dakr@redhat.com/ On Mon, May 20, 2024 at 07:24:19PM +0200, Danilo Krummrich wrote: > Add an abstraction around the kernels firmware API to request firmware > images. The abstraction provides functions to access the firmware > buffer and / or copy it to a new buffer allocated with a given allocator > backend. > > The firmware is released once the abstraction is dropped. > > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > --- > rust/bindings/bindings_helper.h | 1 + > rust/kernel/firmware.rs | 74 +++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 3 files changed, 76 insertions(+) > create mode 100644 rust/kernel/firmware.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index b245db8d5a87..e4ffc47da5ec 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -14,6 +14,7 @@ > #include <kunit/test.h> > #include <linux/errname.h> > #include <linux/ethtool.h> > +#include <linux/firmware.h> > #include <linux/jiffies.h> > #include <linux/mdio.h> > #include <linux/pci.h> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > new file mode 100644 > index 000000000000..700504fb3c9c > --- /dev/null > +++ b/rust/kernel/firmware.rs > @@ -0,0 +1,74 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Firmware abstraction > +//! > +//! C header: [`include/linux/firmware.h`](../../../../include/linux/firmware.h") > + > +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr, types::Opaque}; > + > +/// Abstraction around a C firmware struct. > +/// > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The > +/// firmware is released once [`Firmware`] is dropped. > +/// > +/// # Examples > +/// > +/// ``` > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?; > +/// driver_load_firmware(fw.data()); > +/// ``` > +pub struct Firmware(Opaque<*const bindings::firmware>); > + > +impl Firmware { > + /// Send a firmware request and wait for it. See also `bindings::request_firmware`. > + pub fn request(name: &CStr, dev: &Device) -> Result<Self> { > + let fw = Opaque::uninit(); > + > + let ret = unsafe { bindings::request_firmware(fw.get(), name.as_char_ptr(), dev.as_raw()) }; > + if ret != 0 { > + return Err(Error::from_errno(ret)); > + } > + > + Ok(Firmware(fw)) > + } > + > + /// Send a request for an optional fw module. See also `bindings::request_firmware_nowarn`. > + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> { > + let fw = Opaque::uninit(); > + > + let ret = unsafe { > + bindings::firmware_request_nowarn(fw.get(), name.as_char_ptr(), dev.as_raw()) > + }; > + if ret != 0 { > + return Err(Error::from_errno(ret)); > + } > + > + Ok(Firmware(fw)) > + } > + > + /// Returns the size of the requested firmware in bytes. > + pub fn size(&self) -> usize { > + unsafe { (*(*self.0.get())).size } > + } > + > + /// Returns the requested firmware as `&[u8]`. > + pub fn data(&self) -> &[u8] { > + unsafe { core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) } > + } > +} > + > +impl Drop for Firmware { > + fn drop(&mut self) { > + unsafe { bindings::release_firmware(*self.0.get()) }; > + } > +} > + > +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, which is safe to be used from any > +// thread. > +unsafe impl Send for Firmware {} > + > +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, references to which are safe to > +// be used from any thread. > +unsafe impl Sync for Firmware {} > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 6415968ee3b8..ed97d131661a 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -35,6 +35,7 @@ > #[cfg(CONFIG_DRM = "y")] > pub mod drm; > pub mod error; > +pub mod firmware; > pub mod init; > pub mod ioctl; > #[cfg(CONFIG_KUNIT)] > -- > 2.45.1 >
On Tue, May 28, 2024 at 08:01:26PM +0900, FUJITA Tomonori wrote: > On Mon, 27 May 2024 21:22:47 +0200 > Danilo Krummrich <dakr@redhat.com> wrote: > > >> > +/// Abstraction around a C firmware struct. > >> > +/// > >> > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can > >> > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as > >> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The > >> > +/// firmware is released once [`Firmware`] is dropped. > >> > +/// > >> > +/// # Examples > >> > +/// > >> > +/// ``` > >> > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?; > >> > +/// driver_load_firmware(fw.data()); > >> > +/// ``` > >> > +pub struct Firmware(Opaque<*const bindings::firmware>); > >> > >> Wrapping a raw pointer is not the intended use of Qpaque type? > >> > > > > Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of > > the boilerplate in the 'request' functions to some common 'request_internal' one. > > You might need to add 'Invariants' comment on Firmware struct. Which ones do you think should be documented? > > BTW, what merge window are you aiming for? As I wrote before, I have a > driver that needs the firmware abstractions (the minimum device > abstractions is enough; Device::as_raw() and as_ref()). So the sooner, > the better for me. I'm not aiming this on a specific merge window. 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@redhat.com/ >
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@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@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 >
On Tue, May 28, 2024 at 08:40:20AM +0000, Zhi Wang wrote: > On 27/05/2024 22.18, Danilo Krummrich wrote: > > External email: Use caution opening links or attachments > > > > > > On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote: > >> On Mon, 20 May 2024 19:24:19 +0200 > >> Danilo Krummrich <dakr@redhat.com> wrote: > >> > >>> Add an abstraction around the kernels firmware API to request firmware > >>> images. The abstraction provides functions to access the firmware > >>> buffer and / or copy it to a new buffer allocated with a given > >>> allocator backend. > >>> > >> > >> Was playing with firmware extractions based on this patch. > >> Unfortunately I ended up with a lot of pointer operations, unsafe > >> statements. > >> > >> As we know many vendors have a C headers for the definitions of the > >> firwmare content, the driver extract the data by applying a struct > >> pointer on it. > >> > >> But in rust, I feel it would nice that we can also have a common > >> firmware extractor for drivers, that can wrap the pointer operations, > >> take a list of the firmware struct members that converted from C headers > >> as the input, offer the driver some common ABI methods to query them. > >> Maybe that would ease the pain a lot. > > > > So, you mean some abstraction that takes a list of types, offsets in the > > firmware and a reference to the firmware itself and provides references to the > > corresponding objects? > > > > I agree it might be helpful to have some common infrastructure for this, but the > > operations on it would still be unsafe, since ultimately it involves > > dereferencing pointers. > > > > I think the goal is to 1) concentrate the "unsafe" operations in a > abstraction so the driver doesn't have to write explanation of unsafe > operations here and there, improve the readability of the driver that > interacts with vendor-firmware buffer. 2) ease the driver maintenance > burden. With a generic abstraction, as in 1), at lest some of the abstraction's functions would be unsafe themselves, since they have to rely on the layout (or offset) passed by the driver being correct. What if I pass a wrong layout / offset for a structure that contains a pointer? This might still result in an invalid pointer dereference. Am I missing something? > > Some solutions I saw in different projects written in rust for > de-referencing a per-defined struct: > > 1) Take the vendor firmware buffer as a whole, invent methods like > read/write with offset for operating the buffer. > > In this scheme, the driver is responsible for taking care of each data > member in a vendor firmware struct and also its valid offset. The > abstraction only covers the boundary of the whole firmware buffer. > > The cons is the readability of the operation of the vendor firmware > buffer in the driver is not good comparing with C code. > > Hate to think a lot of xxx = vendor_firmware_struct.read(offset); // > reading item A in the code. > > 2) Define the firmware struct in rust (might need to find a better way > to handle "union" in the definition of the vendor firmware struct). Use > macros to generate methods of accessing each data member for the vendor > firmware struct. > > Then the code in the driver will be like xxx = > vendor_firmware_struct.item_a(); in the driver. > > In this scheme, the abstraction and the generated methods covers the > boundary check. The "unsafe" statement can stay in the generated > struct-access methods. > > This might even be implemented as a more generic rust feature in the kernel. This sounds more like a driver specific abstraction to me, which, as you write, we can probably come up with some macros that help implementing it. But again, what if the offsets are within the boundary, but still at a wrong offset? What if the data obtained from a wrong offset leads to other safety implications when further processing them? I think no generic abstraction can ever cover the safety parts of this (entirely). I think there are always semanic parts to this the driver has to cover. Generally, I think we should aim for some generalization, but I think we should not expect it to cover all the safety aspects. > > The cons is still the driver might need to sync between the C-version > definition and rust-version definition. What do you mean with the driver needs to sync between a C and a Rust version of structure definitions? > > 3) Also re-using C bindings generation in the kernel came to my mind > when thinking of this problem, since it allows the rust code to access > the C struct, but they don't have the boundary check. Still need another > layer/macros to wrap it. I think we should have the structure definitions in Rust directly. - Danilo > > > >> > >> Thanks, > >> Zhi. > >>
On 28/05/2024 17.18, Danilo Krummrich wrote: > External email: Use caution opening links or attachments > > > On Tue, May 28, 2024 at 08:40:20AM +0000, Zhi Wang wrote: >> On 27/05/2024 22.18, Danilo Krummrich wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On Tue, May 21, 2024 at 08:32:31AM +0300, Zhi Wang wrote: >>>> On Mon, 20 May 2024 19:24:19 +0200 >>>> Danilo Krummrich <dakr@redhat.com> wrote: >>>> >>>>> Add an abstraction around the kernels firmware API to request firmware >>>>> images. The abstraction provides functions to access the firmware >>>>> buffer and / or copy it to a new buffer allocated with a given >>>>> allocator backend. >>>>> >>>> >>>> Was playing with firmware extractions based on this patch. >>>> Unfortunately I ended up with a lot of pointer operations, unsafe >>>> statements. >>>> >>>> As we know many vendors have a C headers for the definitions of the >>>> firwmare content, the driver extract the data by applying a struct >>>> pointer on it. >>>> >>>> But in rust, I feel it would nice that we can also have a common >>>> firmware extractor for drivers, that can wrap the pointer operations, >>>> take a list of the firmware struct members that converted from C headers >>>> as the input, offer the driver some common ABI methods to query them. >>>> Maybe that would ease the pain a lot. >>> >>> So, you mean some abstraction that takes a list of types, offsets in the >>> firmware and a reference to the firmware itself and provides references to the >>> corresponding objects? >>> >>> I agree it might be helpful to have some common infrastructure for this, but the >>> operations on it would still be unsafe, since ultimately it involves >>> dereferencing pointers. >>> >> >> I think the goal is to 1) concentrate the "unsafe" operations in a >> abstraction so the driver doesn't have to write explanation of unsafe >> operations here and there, improve the readability of the driver that >> interacts with vendor-firmware buffer. 2) ease the driver maintenance >> burden. > > With a generic abstraction, as in 1), at lest some of the abstraction's > functions would be unsafe themselves, since they have to rely on the layout > (or offset) passed by the driver being correct. What if I pass a wrong layout / > offset for a structure that contains a pointer? This might still result in an > invalid pointer dereference. Am I missing something? > >> >> Some solutions I saw in different projects written in rust for >> de-referencing a per-defined struct: >> >> 1) Take the vendor firmware buffer as a whole, invent methods like >> read/write with offset for operating the buffer. >> >> In this scheme, the driver is responsible for taking care of each data >> member in a vendor firmware struct and also its valid offset. The >> abstraction only covers the boundary of the whole firmware buffer. >> >> The cons is the readability of the operation of the vendor firmware >> buffer in the driver is not good comparing with C code. >> >> Hate to think a lot of xxx = vendor_firmware_struct.read(offset); // >> reading item A in the code. >> >> 2) Define the firmware struct in rust (might need to find a better way >> to handle "union" in the definition of the vendor firmware struct). Use >> macros to generate methods of accessing each data member for the vendor >> firmware struct. >> >> Then the code in the driver will be like xxx = >> vendor_firmware_struct.item_a(); in the driver. >> >> In this scheme, the abstraction and the generated methods covers the >> boundary check. The "unsafe" statement can stay in the generated >> struct-access methods. >> >> This might even be implemented as a more generic rust feature in the kernel. > > This sounds more like a driver specific abstraction to me, which, as you write, > we can probably come up with some macros that help implementing it. > > But again, what if the offsets are within the boundary, but still at a wrong > offset? What if the data obtained from a wrong offset leads to other safety > implications when further processing them? I think no generic abstraction can > ever cover the safety parts of this (entirely). I think there are always semanic > parts to this the driver has to cover. > I was thinking of a proc_macro that takes a vender-firmware struct definition. As it can get the type and the name of each member, then it can generate methods like xxx::member_a() that returns the value from the "unsafe {*(type *)(pointer + offset)}". Thus, the unsafe stuff stays in the generated methods. For offset, I was hoping the macro can automatically calculate it based on the member offset (the vendor-firmware definition) when generating xxx::member_x(). (Maybe it can also take alignment into account) As the return value has a rust type, rust should catch it if the caller wants to do something crazy there. > Generally, I think we should aim for some generalization, but I think we should > not expect it to cover all the safety aspects. > I agree. I was mostly thinking how to ease the pain of driver and see how the best we can do for it. >> >> The cons is still the driver might need to sync between the C-version >> definition and rust-version definition. > > What do you mean with the driver needs to sync between a C and a Rust version of > structure definitions? > Let's say a C driver maintains quite some headers and support some not very new HWs. A new rust driver maintains some headers that written in rust, it needs those headers as well. Now the firmware updates, both the C headers and rust headers needs to be updated accordingly due to ABI changes. I was thinking if that process can be optimized, at least trying to avoid the sync process, which might be painful if the amount of the headers are huge. >> >> 3) Also re-using C bindings generation in the kernel came to my mind >> when thinking of this problem, since it allows the rust code to access >> the C struct, but they don't have the boundary check. Still need another >> layer/macros to wrap it. > > I think we should have the structure definitions in Rust directly. > > - Danilo > >> >> >>>> >>>> Thanks, >>>> Zhi. >>>> >
Hi, On Tue, 28 May 2024 14:45:02 +0200 Greg KH <gregkh@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@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 Tue, 28 May 2024 14:19:24 +0200 Danilo Krummrich <dakr@redhat.com> wrote: > On Tue, May 28, 2024 at 08:01:26PM +0900, FUJITA Tomonori wrote: >> On Mon, 27 May 2024 21:22:47 +0200 >> Danilo Krummrich <dakr@redhat.com> wrote: >> >> >> > +/// Abstraction around a C firmware struct. >> >> > +/// >> >> > +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can >> >> > +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as >> >> > +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The >> >> > +/// firmware is released once [`Firmware`] is dropped. >> >> > +/// >> >> > +/// # Examples >> >> > +/// >> >> > +/// ``` >> >> > +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?; >> >> > +/// driver_load_firmware(fw.data()); >> >> > +/// ``` >> >> > +pub struct Firmware(Opaque<*const bindings::firmware>); >> >> >> >> Wrapping a raw pointer is not the intended use of Qpaque type? >> >> >> > >> > Indeed, will fix this in v2 and use NonNull instead. I'll also offload most of >> > the boilerplate in the 'request' functions to some common 'request_internal' one. >> >> You might need to add 'Invariants' comment on Firmware struct. > > Which ones do you think should be documented? Something like the comment for struct Page looks fine to me. But the Rust reviewers might have a different opinion. /// The pointer is valid, and has ownership over the page.
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@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@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
Hi, On Wed, 29 May 2024 21:57:03 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: >> 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. Great, thanks a lot! Danilo and Wedson, are there any concerns about pushing this patch [1] for the firmware abstractions? I you prefer to be the author of the patch, please let me know. Who the author is doesn't matter to me. Otherwise, I'll add Co-developed-by tag. [1] https://lore.kernel.org/rust-for-linux/20240529.092821.1593412345609718860.fujita.tomonori@gmail.com/
On Thu, May 30, 2024 at 08:28:24AM +0900, FUJITA Tomonori wrote: > Hi, > > On Wed, 29 May 2024 21:57:03 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > >> 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. > > Great, thanks a lot! > > Danilo and Wedson, are there any concerns about pushing this patch [1] > for the firmware abstractions? Well, if everyone is fine with this one I don't see why we can't we go with [1] directly? AFAICS, we'd only need the following fix: -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h) +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h) [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/ > > I you prefer to be the author of the patch, please let me know. Who > the author is doesn't matter to me. Otherwise, I'll add > Co-developed-by tag. > > [1] https://lore.kernel.org/rust-for-linux/20240529.092821.1593412345609718860.fujita.tomonori@gmail.com/ >
Hi, On Thu, 30 May 2024 04:01:39 +0200 Danilo Krummrich <dakr@redhat.com> wrote: > On Thu, May 30, 2024 at 08:28:24AM +0900, FUJITA Tomonori wrote: >> Hi, >> >> On Wed, 29 May 2024 21:57:03 +0200 >> Greg KH <gregkh@linuxfoundation.org> wrote: >> >> >> 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. >> >> Great, thanks a lot! >> >> Danilo and Wedson, are there any concerns about pushing this patch [1] >> for the firmware abstractions? > > Well, if everyone is fine with this one I don't see why we can't we go with [1] > directly? AFAICS, we'd only need the following fix: > > -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h) > +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h) > > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/ The difference is that your patch touches the reference count of a struct device. My patch doesn't. The following part in your patch allows the rust code to freely play with the reference count of a struct device. Your Rust drm driver interact with the reference count in a different way than C's drm drivers if I understand correctly. I'm not sure that Greg will be convinenced with that approach. +// SAFETY: Instances of `Device` are always ref-counted. +unsafe impl crate::types::AlwaysRefCounted for Device { + fn inc_ref(&self) { + // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero. + unsafe { bindings::get_device(self.as_raw()) }; + } + + unsafe fn dec_ref(obj: ptr::NonNull<Self>) { + // SAFETY: The safety requirements guarantee that the refcount is nonzero. + unsafe { bindings::put_device(obj.cast().as_ptr()) } + } +} The following comments give the impression that Rust abstractions wrongly interact with the reference count; callers check out the reference counter. Nobody should do that. + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. + pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> { + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for + /// the entire duration when the returned reference exists. + pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self { + // SAFETY: Guaranteed by the safety requirements of the function. + unsafe { &*ptr.cast() } + }
On Thu, May 30, 2024 at 01:24:33PM +0900, FUJITA Tomonori wrote: > Hi, > > On Thu, 30 May 2024 04:01:39 +0200 > Danilo Krummrich <dakr@redhat.com> wrote: > > > On Thu, May 30, 2024 at 08:28:24AM +0900, FUJITA Tomonori wrote: > >> Hi, > >> > >> On Wed, 29 May 2024 21:57:03 +0200 > >> Greg KH <gregkh@linuxfoundation.org> wrote: > >> > >> >> 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. > >> > >> Great, thanks a lot! > >> > >> Danilo and Wedson, are there any concerns about pushing this patch [1] > >> for the firmware abstractions? > > > > Well, if everyone is fine with this one I don't see why we can't we go with [1] > > directly? AFAICS, we'd only need the following fix: > > > > -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h) > > +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h) > > > > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/ > > The difference is that your patch touches the reference count of a > struct device. My patch doesn't. > > The following part in your patch allows the rust code to freely play > with the reference count of a struct device. Your Rust drm driver > interact with the reference count in a different way than C's drm > drivers if I understand correctly. I'm not sure that Greg will be > convinenced with that approach. I don't see how this is different than what we do in C. If you (for whatever reason) want to keep a pointer to a struct device you should make sure to increase the reference count of this device, such that it can't get freed for the time being. This is a 1:1 representation of that and conceptually identical. > > +// SAFETY: Instances of `Device` are always ref-counted. > +unsafe impl crate::types::AlwaysRefCounted for Device { > + fn inc_ref(&self) { > + // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero. > + unsafe { bindings::get_device(self.as_raw()) }; > + } > + > + unsafe fn dec_ref(obj: ptr::NonNull<Self>) { > + // SAFETY: The safety requirements guarantee that the refcount is nonzero. > + unsafe { bindings::put_device(obj.cast().as_ptr()) } > + } > +} > > The following comments give the impression that Rust abstractions > wrongly interact with the reference count; callers check out the > reference counter. Nobody should do that. No, saying that the caller must ensure that the device "has a non-zero reference count" is a perfectly valid requirement. It doensn't mean that the calling code has to peek the actual reference count, but it means that it must be ensured that while we try to increase the reference count it can't drop to zero unexpectedly. Your patch, as a subset of this one, has the same requirements as listed below. > > + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. > + pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> { > > + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for > + /// the entire duration when the returned reference exists. > + pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self { > + // SAFETY: Guaranteed by the safety requirements of the function. > + unsafe { &*ptr.cast() } > + } >
On Thu, 30 May 2024 08:47:25 +0200 Danilo Krummrich <dakr@redhat.com> wrote: >> >> >> 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. >> >> >> >> Great, thanks a lot! >> >> >> >> Danilo and Wedson, are there any concerns about pushing this patch [1] >> >> for the firmware abstractions? >> > >> > Well, if everyone is fine with this one I don't see why we can't we go with [1] >> > directly? AFAICS, we'd only need the following fix: >> > >> > -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h) >> > +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h) >> > >> > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/ >> >> The difference is that your patch touches the reference count of a >> struct device. My patch doesn't. >> >> The following part in your patch allows the rust code to freely play >> with the reference count of a struct device. Your Rust drm driver >> interact with the reference count in a different way than C's drm >> drivers if I understand correctly. I'm not sure that Greg will be >> convinenced with that approach. > > I don't see how this is different than what we do in C. If you (for whatever > reason) want to keep a pointer to a struct device you should make sure to > increase the reference count of this device, such that it can't get freed for > the time being. > > This is a 1:1 representation of that and conceptually identical. A drm driver does such? If a drm driver allocates two types of driver-specific data and keep a pointer to the pci device, then the driver calls get_device() twice to increase the reference count of the pci's device? At the very least, network device drivers don't. a driver doesn't play with the reference count. The network subsystem does. And, the network subsystem doesn't care about how many pointers to a pci device a driver keeps. With the patch [1], a driver plays with the reference count of a device and directly calls get/put_device. It's a different than C drivers for me (not sure about drm subsystem though). But I might be misunderstanding that Greg isn't convinced with this reference count thing. We need to wait for his response. >> +// SAFETY: Instances of `Device` are always ref-counted. >> +unsafe impl crate::types::AlwaysRefCounted for Device { >> + fn inc_ref(&self) { >> + // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero. >> + unsafe { bindings::get_device(self.as_raw()) }; >> + } >> + >> + unsafe fn dec_ref(obj: ptr::NonNull<Self>) { >> + // SAFETY: The safety requirements guarantee that the refcount is nonzero. >> + unsafe { bindings::put_device(obj.cast().as_ptr()) } >> + } >> +} >> >> The following comments give the impression that Rust abstractions >> wrongly interact with the reference count; callers check out the >> reference counter. Nobody should do that. > > No, saying that the caller must ensure that the device "has a non-zero reference > count" is a perfectly valid requirement. > > It doensn't mean that the calling code has to peek the actual reference count, > but it means that it must be ensured that while we try to increase the reference > count it can't drop to zero unexpectedly. Yeah, the same requirements but expressed differently. But again, I might be misunderstanding. Greg might have a different reason. > Your patch, as a subset of this one, has the same requirements as listed below. > >> >> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. >> + pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> { >> >> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for >> + /// the entire duration when the returned reference exists. >> + pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self { >> + // SAFETY: Guaranteed by the safety requirements of the function. >> + unsafe { &*ptr.cast() } >> + } >> > >
On Fri, May 31, 2024 at 04:50:32PM +0900, FUJITA Tomonori wrote: > On Thu, 30 May 2024 08:47:25 +0200 > Danilo Krummrich <dakr@redhat.com> wrote: > > >> >> >> 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. > >> >> > >> >> Great, thanks a lot! > >> >> > >> >> Danilo and Wedson, are there any concerns about pushing this patch [1] > >> >> for the firmware abstractions? > >> > > >> > Well, if everyone is fine with this one I don't see why we can't we go with [1] > >> > directly? AFAICS, we'd only need the following fix: > >> > > >> > -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h) > >> > +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h) > >> > > >> > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/ > >> > >> The difference is that your patch touches the reference count of a > >> struct device. My patch doesn't. > >> > >> The following part in your patch allows the rust code to freely play > >> with the reference count of a struct device. Your Rust drm driver > >> interact with the reference count in a different way than C's drm > >> drivers if I understand correctly. I'm not sure that Greg will be > >> convinenced with that approach. > > > > I don't see how this is different than what we do in C. If you (for whatever > > reason) want to keep a pointer to a struct device you should make sure to > > increase the reference count of this device, such that it can't get freed for > > the time being. > > > > This is a 1:1 representation of that and conceptually identical. > > A drm driver does such? If a drm driver allocates two types of > driver-specific data and keep a pointer to the pci device, then the > driver calls get_device() twice to increase the reference count of the > pci's device? Think about it more generically. If you store a pointer to a device in some structure (driver private data, generic subsystem structure, etc.), can you guarantee that without acquiring a reference that the device' reference count doesn't drop to zero while your structure is alive? There are cases where you can't, e.g. with Arc<device::Data>. How do you guarantee (generically for every driver) the last reference of your device data is dropped with the driver's remove callback? If we make it the driver's responsibility to care about this the whole thing is unsafe as in C. In some cases you can, mostly the ones where you free a structure in the driver's remove callback. But again, then there is no advantage over what we do in C. What if you change the lifetime of your structure later on, then you may introduce a bug. > > At the very least, network device drivers don't. a driver doesn't play > with the reference count. The network subsystem does. And, the network A quick 'grep' shows 23 occurrences of get_device() in network drivers. > subsystem doesn't care about how many pointers to a pci device a > driver keeps. If none of the drivers has structures storing a pointer to a pci device that out live the driver's remove callback that's fine. > > With the patch [1], a driver plays with the reference count of a > device and directly calls get/put_device. It's a different than C > drivers for me (not sure about drm subsystem though). It's not different. Again, when a C driver stores a device pointer in a structure whose lifetime is not bound a special boundary, like the device remove callback, it must increase the reference count. It's just that most of the time we are moving within this boundary and in C we just rely on that. In Rust we can only get this safe by taking a reference for every pointer we store. And making it safe to use seems to be the goal. Generally, the whole point of a reference count is that everyone who owns a pointer to this shared structure increases / decreases the reference count accordingly. > > But I might be misunderstanding that Greg isn't convinced with this > reference count thing. We need to wait for his response. > > > >> +// SAFETY: Instances of `Device` are always ref-counted. > >> +unsafe impl crate::types::AlwaysRefCounted for Device { > >> + fn inc_ref(&self) { > >> + // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero. > >> + unsafe { bindings::get_device(self.as_raw()) }; > >> + } > >> + > >> + unsafe fn dec_ref(obj: ptr::NonNull<Self>) { > >> + // SAFETY: The safety requirements guarantee that the refcount is nonzero. > >> + unsafe { bindings::put_device(obj.cast().as_ptr()) } > >> + } > >> +} > >> > >> The following comments give the impression that Rust abstractions > >> wrongly interact with the reference count; callers check out the > >> reference counter. Nobody should do that. > > > > No, saying that the caller must ensure that the device "has a non-zero reference > > count" is a perfectly valid requirement. > > > > It doensn't mean that the calling code has to peek the actual reference count, > > but it means that it must be ensured that while we try to increase the reference > > count it can't drop to zero unexpectedly. > > Yeah, the same requirements but expressed differently. Well, instead of "ensure that `ptr` is valid, non-null, and has a non-zero reference count" you propose "the pointer must point at a valid `device`". When I ask you to specify what "pointer to valid device" means, what would be the answer? Since we're still discussing this in the thread of the firmware abstraction, if you have any further concerns, let's discuss them in the thread for the corresponding patch. Once we get to a conclusion I can send a series with only the device and firmare abstractions such that we can get them in outside of the scope of the reset of both series to get your driver going. - Danilo > > But again, I might be misunderstanding. Greg might have a different reason. > > > Your patch, as a subset of this one, has the same requirements as listed below. > > > >> > >> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count. > >> + pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> { > >> > >> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for > >> + /// the entire duration when the returned reference exists. > >> + pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self { > >> + // SAFETY: Guaranteed by the safety requirements of the function. > >> + unsafe { &*ptr.cast() } > >> + } > >> > > > > >
Hi, On Fri, 31 May 2024 11:59:47 +0200 Danilo Krummrich <dakr@redhat.com> wrote: > Once we get to a conclusion I can send a series with only the device and firmare > abstractions such that we can get them in outside of the scope of the reset of > both series to get your driver going. Since your discussion with Greg seems to continue for a while, let me include the following patch that Greg approved with the next version of the PHY driver patchset. You have the new version of the firmware patch? The unused functions will not be merged so only request_firmware() and release_firmware() can be included. If not, I can include my firmware patch that I wrote before. = From: Danilo Krummrich <dakr@redhat.com> Date: Fri, 7 Jun 2024 20:14:59 +0900 Subject: [PATCH] add abstraction for struct device Add abstraction for struct device. This implements only the minimum necessary functions required for the abstractions of firmware API; that is, wrapping C's pointer to a device object with Rust struct only during a caller knows the pointer is valid (e.g., the probe callback). Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> Signed-off-by: Danilo Krummrich <dakr@redhat.com> Co-developed-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- rust/kernel/device.rs | 31 +++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 2 files changed, 32 insertions(+) create mode 100644 rust/kernel/device.rs diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs new file mode 100644 index 000000000000..55ec4f364628 --- /dev/null +++ b/rust/kernel/device.rs @@ -0,0 +1,31 @@ +// 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; + +/// Wraps the kernel's `struct task_struct`. +#[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(crate) fn as_raw(&self) -> *mut bindings::device { + self.0.get() + } +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index fbd91a48ff8b..dd1207f1a873 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -28,6 +28,7 @@ pub mod alloc; mod build_assert; +pub mod device; pub mod error; pub mod init; pub mod ioctl;
On Fri, Jun 07, 2024 at 09:11:32PM +0900, FUJITA Tomonori wrote: > Hi, > > On Fri, 31 May 2024 11:59:47 +0200 > Danilo Krummrich <dakr@redhat.com> wrote: > > > Once we get to a conclusion I can send a series with only the device and firmare > > abstractions such that we can get them in outside of the scope of the reset of > > both series to get your driver going. > > Since your discussion with Greg seems to continue for a while, let me > include the following patch that Greg approved with the next version > of the PHY driver patchset. Yes, please take this one now. We can build on it from there. I had a meeting yesterday with a lot of rust kernel and userspace people at Microsoft and talked a bunch about this and how to move forward. I think we need to take much smaller "steps" and even encourage most drivers to start out as a mix of c and rust as there is no real "requirement" that a driver be "pure" rust at all. This should both make the interface logic simpler to start with, AND provide a base so that people can just write the majority of their driver logic in rust, which is where the language "safety" issues are most needed, not in the lifecycle rules involving the internal driver model infrastructure. Anyway, that's all hand-wavy right now, sorry, to get back to the point here, again, let's take this, which will allow the firmware bindings to be resubmitted and hopefully accepted, and we can move forward from there to "real" things like a USB or PCI or even platform device and driver binding stuff. thanks, greg k-h
On Fri, Jun 07, 2024 at 09:11:32PM +0900, FUJITA Tomonori wrote: > Hi, > > On Fri, 31 May 2024 11:59:47 +0200 > Danilo Krummrich <dakr@redhat.com> wrote: > > > Once we get to a conclusion I can send a series with only the device and firmare > > abstractions such that we can get them in outside of the scope of the reset of > > both series to get your driver going. > > Since your discussion with Greg seems to continue for a while, let me > include the following patch that Greg approved with the next version > of the PHY driver patchset. This all doesn't make a lot of sense. If that's the case, Greg approved something the he keeps arguing about in the discussion of the original patch. Please see the discussion about the NULL pointer check [1]. Besides that, I really don't think it's the correct approach to just (partially) pick up a patch from the mailing list that is actively discussed and submit versions that are slightly altered in the points that are actively discussed. This really just complicates the situation and does not help getting to an agreement. > > You have the new version of the firmware patch? The unused functions > will not be merged so only request_firmware() and release_firmware() > can be included. If not, I can include my firmware patch that I wrote > before. > Please find the updated firmware patch in [2]. However, I propose to just send a new series with just the device abstraction and this firmware patch and proceed from there. [1] https://lore.kernel.org/rust-for-linux/Zl8_bXqK-T24y1kp@cassiopeiae/ [2] https://gitlab.freedesktop.org/drm/nova/-/commit/0683e186929c4922d565e5315525925aa2d8d686 NAK for the patch below, for the reasons above. Please also see comments inline. > = > From: Danilo Krummrich <dakr@redhat.com> > Date: Fri, 7 Jun 2024 20:14:59 +0900 > Subject: [PATCH] add abstraction for struct device > > Add abstraction for struct device. This implements only the minimum > necessary functions required for the abstractions of firmware API; > that is, wrapping C's pointer to a device object with Rust struct only > during a caller knows the pointer is valid (e.g., the probe callback). > > Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > Signed-off-by: Danilo Krummrich <dakr@redhat.com> > Co-developed-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > rust/kernel/device.rs | 31 +++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 2 files changed, 32 insertions(+) > create mode 100644 rust/kernel/device.rs > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs > new file mode 100644 > index 000000000000..55ec4f364628 > --- /dev/null > +++ b/rust/kernel/device.rs > @@ -0,0 +1,31 @@ > +// 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; > + > +/// Wraps the kernel's `struct task_struct`. > +#[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`. The original patch says: "Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for the entire duration when the returned reference exists." This is way more precise than just saying "For the duration of 'a, the pointer must point at a valid `device`.", hence we should keep the original comment. > + 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 } Why not just + // SAFETY: Guaranteed by the safety requirements of the function. + unsafe { &*ptr.cast() } as in the original commit? > + } > + > + /// Returns the raw pointer to the device. > + pub(crate) fn as_raw(&self) -> *mut bindings::device { > + self.0.get() > + } > +} > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index fbd91a48ff8b..dd1207f1a873 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -28,6 +28,7 @@ > > pub mod alloc; > mod build_assert; > +pub mod device; > pub mod error; > pub mod init; > pub mod ioctl; > -- > 2.34.1 >
On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote: > On Fri, Jun 07, 2024 at 09:11:32PM +0900, FUJITA Tomonori wrote: > > Hi, > > > > On Fri, 31 May 2024 11:59:47 +0200 > > Danilo Krummrich <dakr@redhat.com> wrote: > > > > > Once we get to a conclusion I can send a series with only the device and firmare > > > abstractions such that we can get them in outside of the scope of the reset of > > > both series to get your driver going. > > > > Since your discussion with Greg seems to continue for a while, let me > > include the following patch that Greg approved with the next version > > of the PHY driver patchset. > > Yes, please take this one now. We can build on it from there. This patch still contains the points *you* are discussing on the original one. Why do I spend my time on this discussion if those points don't seem to matter for you now? Also, why would we want to have this patch (and the firmware one) in two separate submissions? If we urgently want to land the firmware abstractions I can send a separate series with just the device and firmware abstraction patches. > > I had a meeting yesterday with a lot of rust kernel and userspace people > at Microsoft and talked a bunch about this and how to move forward. I > think we need to take much smaller "steps" and even encourage most > drivers to start out as a mix of c and rust as there is no real > "requirement" that a driver be "pure" rust at all. This should both > make the interface logic simpler to start with, AND provide a base so > that people can just write the majority of their driver logic in rust, > which is where the language "safety" issues are most needed, not in the > lifecycle rules involving the internal driver model infrastructure. What do you mean by "drivers to start out as mix of C and Rust". I don' think this is desireable. Writing abstractions for C core infrastructure already is a lot of effort and sometimes rather difficult in certain aspects, (e.g. align lifetimes). An immediate concern I have is that this is an invitation for drivers to write their own abstractions, as in interfacing with C core infrastructure in C and and do a driver specific abstraction. > > Anyway, that's all hand-wavy right now, sorry, to get back to the point > here, again, let's take this, which will allow the firmware bindings to > be resubmitted and hopefully accepted, and we can move forward from > there to "real" things like a USB or PCI or even platform device and > driver binding stuff. The abstractions for that are on the list and in the sense of what you say above for "smaller steps, they are quite minimal. I don't see how we could break this down even further. > > thanks, > > greg k-h >
On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote: > Anyway, that's all hand-wavy right now, sorry, to get back to the point > here, again, let's take this, which will allow the firmware bindings to > be resubmitted and hopefully accepted, and we can move forward from > there to "real" things like a USB or PCI or even platform device and > driver binding stuff. In order to continue I propose to send out the following series: 1) minimal device and firmware abstractions only 2) v2 of all other device / driver, devres and PCI driver abstractions 3) v2 of basic DRM driver abstractions and Nova The v2 series would contain static driver allocation (in case of DRM even const) and quite a few more simplifications around `driver::Registration` and `device::Data`. Does that make sense? - Danilo > > thanks, > > greg k-h >
On Fri, Jun 07, 2024 at 03:33:39PM +0200, Danilo Krummrich wrote: > On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote: > > Anyway, that's all hand-wavy right now, sorry, to get back to the point > > here, again, let's take this, which will allow the firmware bindings to > > be resubmitted and hopefully accepted, and we can move forward from > > there to "real" things like a USB or PCI or even platform device and > > driver binding stuff. > > In order to continue I propose to send out the following series: > > 1) minimal device and firmware abstractions only Sounds good. But after this, I don't want to take ANY driver core rust code that is not able to live in the "normal" part of the Linux kernel tree. It's just unsustainable to have it all in one directory sorry. If this deadline makes that kbuild work actually happen faster, all the more reason to have it. If that kbuild work is somehow stalled due to other issues, please let me know what that is. > 2) v2 of all other device / driver, devres and PCI driver abstractions I will be glad to review this. > 3) v2 of basic DRM driver abstractions and Nova I love it how one of the most complex driver subsystems we have (drm) is attempting to be the "first real" driver use for the rust apis. Bold move :) > The v2 series would contain static driver allocation (in case of DRM even const) > and quite a few more simplifications around `driver::Registration` and > `device::Data`. > > Does that make sense? Yes, but note, I'm going to probably push back on the "driver::" stuff. But will do so with more constructive criticism as I want this to work very much and I agree that I think we are both talking past each other in different ways. Mostly probably due to my total lack of Rust experience, my apologies, thanks for your patience with me for this. thanks, greg k-h
On Fri, Jun 07, 2024 at 05:41:11PM +0200, Greg KH wrote: > On Fri, Jun 07, 2024 at 03:33:39PM +0200, Danilo Krummrich wrote: > > On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote: > > > Anyway, that's all hand-wavy right now, sorry, to get back to the point > > > here, again, let's take this, which will allow the firmware bindings to > > > be resubmitted and hopefully accepted, and we can move forward from > > > there to "real" things like a USB or PCI or even platform device and > > > driver binding stuff. > > > > In order to continue I propose to send out the following series: > > > > 1) minimal device and firmware abstractions only > > Sounds good. Just a heads-up, I'll probably send this one quite a bit earlier than the other two to make sure to unblock Fujita on their PHY driver. > > But after this, I don't want to take ANY driver core rust code that is > not able to live in the "normal" part of the Linux kernel tree. It's > just unsustainable to have it all in one directory sorry. If this > deadline makes that kbuild work actually happen faster, all the more > reason to have it. If that kbuild work is somehow stalled due to other > issues, please let me know what that is. > > > 2) v2 of all other device / driver, devres and PCI driver abstractions > > I will be glad to review this. Glad to hear that! > > > 3) v2 of basic DRM driver abstractions and Nova > > I love it how one of the most complex driver subsystems we have (drm) is > attempting to be the "first real" driver use for the rust apis. Bold > move :) I'd argue that as one of the most complex driver subsystems we have a lot of need for the advantages Rust provides to us. :) > > > The v2 series would contain static driver allocation (in case of DRM even const) > > and quite a few more simplifications around `driver::Registration` and > > `device::Data`. > > > > Does that make sense? > > Yes, but note, I'm going to probably push back on the "driver::" stuff. That's OK, I'm happy to convince you of its usefulness. :) > But will do so with more constructive criticism as I want this to work > very much and I agree that I think we are both talking past each other > in different ways. Mostly probably due to my total lack of Rust > experience, my apologies, thanks for your patience with me for this. Very much appreciated! Please don't hesitate to ask for more explanation on certain things if they're unclear. I'm happy trying my best to provide useful answers. - Danilo > > thanks, > > greg k-h >
On Fri, 7 Jun 2024 19:55:49 +0200 Danilo Krummrich <dakr@redhat.com> wrote: > On Fri, Jun 07, 2024 at 05:41:11PM +0200, Greg KH wrote: >> On Fri, Jun 07, 2024 at 03:33:39PM +0200, Danilo Krummrich wrote: >> > On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote: >> > > Anyway, that's all hand-wavy right now, sorry, to get back to the point >> > > here, again, let's take this, which will allow the firmware bindings to >> > > be resubmitted and hopefully accepted, and we can move forward from >> > > there to "real" things like a USB or PCI or even platform device and >> > > driver binding stuff. >> > >> > In order to continue I propose to send out the following series: >> > >> > 1) minimal device and firmware abstractions only >> >> Sounds good. > > Just a heads-up, I'll probably send this one quite a bit earlier than the other > two to make sure to unblock Fujita on their PHY driver. Please. The sooner, the better. I need to send the PHY driver with these patchse to netdev. I'm not sure what the above "minimal device" means. If you send the original patch again instead of the patch that Greg already approved and the discussion continues, then I proceed with the approved patch.
On Sat, Jun 08, 2024 at 08:28:06AM +0900, FUJITA Tomonori wrote: > On Fri, 7 Jun 2024 19:55:49 +0200 > Danilo Krummrich <dakr@redhat.com> wrote: > > > On Fri, Jun 07, 2024 at 05:41:11PM +0200, Greg KH wrote: > >> On Fri, Jun 07, 2024 at 03:33:39PM +0200, Danilo Krummrich wrote: > >> > On Fri, Jun 07, 2024 at 02:36:50PM +0200, Greg KH wrote: > >> > > Anyway, that's all hand-wavy right now, sorry, to get back to the point > >> > > here, again, let's take this, which will allow the firmware bindings to > >> > > be resubmitted and hopefully accepted, and we can move forward from > >> > > there to "real" things like a USB or PCI or even platform device and > >> > > driver binding stuff. > >> > > >> > In order to continue I propose to send out the following series: > >> > > >> > 1) minimal device and firmware abstractions only > >> > >> Sounds good. > > > > Just a heads-up, I'll probably send this one quite a bit earlier than the other > > two to make sure to unblock Fujita on their PHY driver. > > Please. The sooner, the better. I need to send the PHY driver with > these patchse to netdev. Why do you want to send those patches to netdev? I think nothing prevents you from sending your PHY driver to netdev. Just add a note to your series that it depends on those two patches. How and through which trees things are merged in the end can be figured out by the corresponding maintainers in the end. > > I'm not sure what the above "minimal device" means. If you send the > original patch again instead of the patch that Greg already approved > and the discussion continues, then I proceed with the approved patch. > I'm honestly getting a bit tired of this... 1) I fundamentally disagree that it's a good thing to fork off patches that are actively discussed and reviewed on the mailing list with the objective to bypass the discussion and the review process. Especially without agreement of all involved parties. 2) It's at least questionable to claim that your forked-off patch can be considered to be "approved". 3) I really try to help and already confirmed to send out a separate series with only the patches you need as well to accelerate things for you. If you really want to help with that, you are very welcome to get involved in the discussion and review process. If you don't want to, that is fine too. But please stop adding confusion to those series by forking off patches. Besides that, I also don't appreciate your attitude, trying to put some kind of "ultimatum" on me. - Danilo
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index b245db8d5a87..e4ffc47da5ec 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -14,6 +14,7 @@ #include <kunit/test.h> #include <linux/errname.h> #include <linux/ethtool.h> +#include <linux/firmware.h> #include <linux/jiffies.h> #include <linux/mdio.h> #include <linux/pci.h> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs new file mode 100644 index 000000000000..700504fb3c9c --- /dev/null +++ b/rust/kernel/firmware.rs @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Firmware abstraction +//! +//! C header: [`include/linux/firmware.h`](../../../../include/linux/firmware.h") + +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr, types::Opaque}; + +/// Abstraction around a C firmware struct. +/// +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as +/// `&[u8]`. Alternatively, the firmware can be copied to a new buffer using `Firmware::copy`. The +/// firmware is released once [`Firmware`] is dropped. +/// +/// # Examples +/// +/// ``` +/// let fw = Firmware::request("path/to/firmware.bin", dev.as_ref())?; +/// driver_load_firmware(fw.data()); +/// ``` +pub struct Firmware(Opaque<*const bindings::firmware>); + +impl Firmware { + /// Send a firmware request and wait for it. See also `bindings::request_firmware`. + pub fn request(name: &CStr, dev: &Device) -> Result<Self> { + let fw = Opaque::uninit(); + + let ret = unsafe { bindings::request_firmware(fw.get(), name.as_char_ptr(), dev.as_raw()) }; + if ret != 0 { + return Err(Error::from_errno(ret)); + } + + Ok(Firmware(fw)) + } + + /// Send a request for an optional fw module. See also `bindings::request_firmware_nowarn`. + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result<Self> { + let fw = Opaque::uninit(); + + let ret = unsafe { + bindings::firmware_request_nowarn(fw.get(), name.as_char_ptr(), dev.as_raw()) + }; + if ret != 0 { + return Err(Error::from_errno(ret)); + } + + Ok(Firmware(fw)) + } + + /// Returns the size of the requested firmware in bytes. + pub fn size(&self) -> usize { + unsafe { (*(*self.0.get())).size } + } + + /// Returns the requested firmware as `&[u8]`. + pub fn data(&self) -> &[u8] { + unsafe { core::slice::from_raw_parts((*(*self.0.get())).data, self.size()) } + } +} + +impl Drop for Firmware { + fn drop(&mut self) { + unsafe { bindings::release_firmware(*self.0.get()) }; + } +} + +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, which is safe to be used from any +// thread. +unsafe impl Send for Firmware {} + +// SAFETY: `Firmware` only holds a pointer to a C firmware struct, references to which are safe to +// be used from any thread. +unsafe impl Sync for Firmware {} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 6415968ee3b8..ed97d131661a 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -35,6 +35,7 @@ #[cfg(CONFIG_DRM = "y")] pub mod drm; pub mod error; +pub mod firmware; pub mod init; pub mod ioctl; #[cfg(CONFIG_KUNIT)]
Add an abstraction around the kernels firmware API to request firmware images. The abstraction provides functions to access the firmware buffer and / or copy it to a new buffer allocated with a given allocator backend. The firmware is released once the abstraction is dropped. Signed-off-by: Danilo Krummrich <dakr@redhat.com> --- rust/bindings/bindings_helper.h | 1 + rust/kernel/firmware.rs | 74 +++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 3 files changed, 76 insertions(+) create mode 100644 rust/kernel/firmware.rs