Re: [RFC PATCH 7/8] rust: add firmware abstractions
On Fri, 7 Jun 2024 19:55:49 +0200 Danilo Krummrich 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.
Re: [RFC PATCH 7/8] rust: add firmware abstractions
Hi,
On Fri, 31 May 2024 11:59:47 +0200
Danilo Krummrich 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
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
Signed-off-by: Wedson Almeida Filho
Signed-off-by: Danilo Krummrich
Co-developed-by: FUJITA Tomonori
Signed-off-by: FUJITA Tomonori
Acked-by: Greg Kroah-Hartman
---
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 ..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);
+
+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::();
+// SAFETY: by the function requirements the pointer is valid and we
have unique access for
+// the duration of `'a`.
+unsafe { *ptr }
+}
+
+/// Returns the raw pointer to the device.
+pub(crate) fn as_raw() -> *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
Re: [RFC PATCH 7/8] rust: add firmware abstractions
On Thu, 30 May 2024 08:47:25 +0200
Danilo Krummrich 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 ..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);
>> >> >> +
>> >> >> +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-d...@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() {
>> +// 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) {
>> +// 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
Re: [RFC PATCH 7/8] rust: add firmware abstractions
Hi,
On Thu, 30 May 2024 04:01:39 +0200
Danilo Krummrich 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 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 ..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);
>> >> +
>> >> +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-d...@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() {
+// 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) {
+// 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 {
+/// 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() }
+}
Re: [RFC PATCH 7/8] rust: add firmware abstractions
Hi,
On Wed, 29 May 2024 21:57:03 +0200
Greg KH 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 ..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);
>> +
>> +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.tomon...@gmail.com/
Re: [RFC PATCH 7/8] rust: add firmware abstractions
On Tue, 28 May 2024 14:19:24 +0200
Danilo Krummrich 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 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.
Re: [RFC PATCH 7/8] rust: add firmware abstractions
Hi,
On Tue, 28 May 2024 14:45:02 +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-d...@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 ..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);
+
+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::();
+// SAFETY: by the function requirements the pointer is valid and we
have unique access for
+// the duration of `'a`.
+unsafe { *ptr }
+}
+
+/// Returns the raw pointer to the device.
+pub fn as_raw() -> *mut bindings::device {
+self.0.get()
+}
+}
Re: [RFC PATCH 7/8] rust: add firmware abstractions
On Mon, 27 May 2024 21:22:47 +0200
Danilo Krummrich 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.
Re: [RFC PATCH 7/8] rust: add firmware abstractions
Hi, On Tue, 28 May 2024 08:40:20 + 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 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?
Re: [RFC PATCH 7/8] rust: add firmware abstractions
Hi,
On Wed, 22 May 2024 09:37:30 +0200
Philipp Stanner 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-6f55e4d8e...@google.com/
pub struct Page {
page: NonNull,
}
Re: [RFC PATCH 7/8] rust: add firmware abstractions
Hi,
Thanks for working on the firmware API!
On Mon, 20 May 2024 19:24:19 +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
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> new file mode 100644
> index ..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?
[PATCH 01/11] swiotlb: Expose swiotlb_nr_tlb function to modules
On Wed, 19 Oct 2011 18:19:22 -0400 Konrad Rzeszutek Wilk wrote: > As a mechanism to detect whether SWIOTLB is enabled or not. > We also fix the spelling - it was swioltb instead of > swiotlb. > > CC: FUJITA Tomonori > [v1: Ripped out swiotlb_enabled] > Signed-off-by: Konrad Rzeszutek Wilk > --- > drivers/xen/swiotlb-xen.c |2 +- > include/linux/swiotlb.h |2 +- > lib/swiotlb.c |5 +++-- > 3 files changed, 5 insertions(+), 4 deletions(-) Acked-by: FUJITA Tomonori
Re: [PATCH 01/11] swiotlb: Expose swiotlb_nr_tlb function to modules
On Wed, 19 Oct 2011 18:19:22 -0400 Konrad Rzeszutek Wilk konrad.w...@oracle.com wrote: As a mechanism to detect whether SWIOTLB is enabled or not. We also fix the spelling - it was swioltb instead of swiotlb. CC: FUJITA Tomonori fujita.tomon...@lab.ntt.co.jp [v1: Ripped out swiotlb_enabled] Signed-off-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com --- drivers/xen/swiotlb-xen.c |2 +- include/linux/swiotlb.h |2 +- lib/swiotlb.c |5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) Acked-by: FUJITA Tomonori fujita.tomon...@lab.ntt.co.jp ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/9] swiotlb: Expose swiotlb_nr_tlb function to modules
On Thu, 29 Sep 2011 16:33:47 -0400 Konrad Rzeszutek Wilk konrad.w...@oracle.com wrote: As a mechanism to detect whether SWIOTLB is enabled or not. We also fix the spelling - it was swioltb instead of swiotlb. CC: FUJITA Tomonori fujita.tomon...@lab.ntt.co.jp [v1: Ripped out swiotlb_enabled] Signed-off-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com --- drivers/xen/swiotlb-xen.c |2 +- include/linux/swiotlb.h |2 +- lib/swiotlb.c |5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) Acked-by: FUJITA Tomonori fujita.tomon...@lab.ntt.co.jp ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/9] swiotlb: Expose swiotlb_nr_tlb function to modules
On Thu, 29 Sep 2011 16:33:47 -0400 Konrad Rzeszutek Wilk wrote: > As a mechanism to detect whether SWIOTLB is enabled or not. > We also fix the spelling - it was swioltb instead of > swiotlb. > > CC: FUJITA Tomonori > [v1: Ripped out swiotlb_enabled] > Signed-off-by: Konrad Rzeszutek Wilk > --- > drivers/xen/swiotlb-xen.c |2 +- > include/linux/swiotlb.h |2 +- > lib/swiotlb.c |5 +++-- > 3 files changed, 5 insertions(+), 4 deletions(-) Acked-by: FUJITA Tomonori
Re: [PATCH 4/7] swiotlb: Expose swiotlb_nr_tlb function to modules as swiotlb_enabled
On Tue, 13 Sep 2011 10:12:47 -0400 Konrad Rzeszutek Wilk konrad.w...@oracle.com wrote: As a mechanism to detect whether SWIOTLB is enabled or not. And as such, we might as well wrap it within an 'swiotlb_enabled()' function that will call the swiotlb_nr_tlb. We also fix the spelling - it was swioltb instead of swiotlb. CC: FUJITA Tomonori fujita.tomon...@lab.ntt.co.jp Signed-off-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com --- drivers/xen/swiotlb-xen.c |2 +- include/linux/swiotlb.h |7 ++- lib/swiotlb.c |5 +++-- 3 files changed, 10 insertions(+), 4 deletions(-) Can we just use swiotlb_nr_tbl() rather than inventing a new function that only wraps swiotlb_nr_tbl()? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/7] swiotlb: Expose swiotlb_nr_tlb function to modules as swiotlb_enabled
On Tue, 13 Sep 2011 10:12:47 -0400 Konrad Rzeszutek Wilk wrote: > As a mechanism to detect whether SWIOTLB is enabled or not. > And as such, we might as well wrap it within an 'swiotlb_enabled()' > function that will call the swiotlb_nr_tlb. > > We also fix the spelling - it was swioltb instead of > swiotlb. > > CC: FUJITA Tomonori > Signed-off-by: Konrad Rzeszutek Wilk > --- > drivers/xen/swiotlb-xen.c |2 +- > include/linux/swiotlb.h |7 ++- > lib/swiotlb.c |5 +++-- > 3 files changed, 10 insertions(+), 4 deletions(-) Can we just use swiotlb_nr_tbl() rather than inventing a new function that only wraps swiotlb_nr_tbl()?
Re: Problems with alpha/pci + radeon/ttm
On Thu, 24 Jun 2010 21:51:40 +1200 Michael Cree mc...@orcon.net.nz wrote: Is this a regression (what kernel version worked)? Seems that the IOMMU can't find 128 pages. It's likely due to: - out of the IOMMU space (possibly someone doesn't free the IOMMU space). or - the mapping parameters (such as align) aren't appropriate so the IOMMU can't find space. I don't think KMS drivers have ever worked on alpha so its not a regression, they are working fine on x86 + powerpc and sparc has been run at least once. KMS on the console boot up has worked since about 2.6.32, but starting up the X server has always failed and, in my case, the system becomes unstable and eventually OOPs. I suspect we are simply hitting the limits of the iommu, how big an address space does it handle? since generally graphics drivers try to bind a lot of things to the GART. No idea on the address space limit. I applied the patch of Fujita that logs all IOMMU allocations, and also inserted some extra printks in the ttm kernel code so that I could see which routines failed and the error code returned. Running the radeon test on boot exhibits the following: [ 238.712768] [drm] Tested GTT-VRAM and VRAM-GTT copy for GTT offset 0x1a312000 [ 239.281127] [drm] Tested GTT-VRAM and VRAM-GTT copy for GTT offset 0x1a412000 [ 239.281127] ttm_tt_bind belched -12 [ 239.282104] ttm_bo_handle_move_mem belched -12 [ 239.282104] ttm_bo_move_buffer belched -12 [ 239.282104] ttm_bo_validate belched -12 [ 239.282104] radeon :01:00.0: object_init failed for (1048576, 0x0002) err=-12 [ 239.282104] [drm:radeon_test_moves] *ERROR* Failed to create GTT object 419 [ 239.399291] Error while testing BO move. Note that no IOMMU allocations are printed while radeon_test_moves is running so iommu_arena_alloc doesn't appear to be called. Also the error code returned up to radeon_test_moves is -12 which is ENOMEM. So does appear to be some memory limit. Hmm, not related with IOMMU? looks like ttm_tt_populate could return ENOMEM too. Can we locate where we hit ENOMEM first? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Problems with alpha/pci + radeon/ttm
On Thu, 24 Jun 2010 21:51:40 +1200 Michael Cree wrote: > >> Is this a regression (what kernel version worked)? > >> > >> Seems that the IOMMU can't find 128 pages. It's likely due to: > >> > >> - out of the IOMMU space (possibly someone doesn't free the IOMMU > >> space). > >> > >> or > >> > >> - the mapping parameters (such as align) aren't appropriate so the > >> IOMMU can't find space. > > > > I don't think KMS drivers have ever worked on alpha so its not a > > regression, they are working fine on x86 + powerpc and sparc has been > > run at least once. > > KMS on the console boot up has worked since about 2.6.32, but starting > up the X server has always failed and, in my case, the system becomes > unstable and eventually OOPs. > > > I suspect we are simply hitting the limits of the iommu, how big an > > address space does it handle? since generally graphics drivers try to > > bind a lot of things to the GART. > > No idea on the address space limit. I applied the patch of Fujita that > logs all IOMMU allocations, and also inserted some extra printks in the > ttm kernel code so that I could see which routines failed and the error > code returned. Running the radeon test on boot exhibits the following: > > [ 238.712768] [drm] Tested GTT->VRAM and VRAM->GTT copy for GTT offset > 0x1a312000 > [ 239.281127] [drm] Tested GTT->VRAM and VRAM->GTT copy for GTT offset > 0x1a412000 > [ 239.281127] ttm_tt_bind belched -12 > [ 239.282104] ttm_bo_handle_move_mem belched -12 > [ 239.282104] ttm_bo_move_buffer belched -12 > [ 239.282104] ttm_bo_validate belched -12 > [ 239.282104] radeon :01:00.0: object_init failed for (1048576, > 0x0002) err=-12 > [ 239.282104] [drm:radeon_test_moves] *ERROR* Failed to create GTT > object 419 > [ 239.399291] Error while testing BO move. > > Note that no IOMMU allocations are printed while radeon_test_moves is > running so iommu_arena_alloc doesn't appear to be called. Also the > error code returned up to radeon_test_moves is -12 which is ENOMEM. So > does appear to be some memory limit. Hmm, not related with IOMMU? looks like ttm_tt_populate could return ENOMEM too. Can we locate where we hit ENOMEM first?
Problems with alpha/pci + radeon/ttm
On Thu, 24 Jun 2010 10:53:52 -0400
Matt Turner wrote:
> > Seems that the IOMMU can't find 128 pages. It's likely due to:
> >
> > - out of the IOMMU space (possibly someone doesn't free the IOMMU
> > ?space).
> >
> > or
> >
> > - the mapping parameters (such as align) aren't appropriate so the
> > ?IOMMU can't find space.
> >
> >
> >> Is this the cause of the bug we're seeing in the report [1]?
> >>
> >> Anyone know what's going wrong here?
> >
> >
> > I've attached a patch to print the debug info about the mapping
> > parameters.
> >
> >
> > diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c
> > index d1dbd9a..17cf0d8 100644
> > --- a/arch/alpha/kernel/pci_iommu.c
> > +++ b/arch/alpha/kernel/pci_iommu.c
> > @@ -187,6 +187,10 @@ iommu_arena_alloc(struct device *dev, struct
> > pci_iommu_arena *arena, long n,
> > ? ? ? ?/* Search for N empty ptes */
> > ? ? ? ?ptes = arena->ptes;
> > ? ? ? ?mask = max(align, arena->align_entry) - 1;
> > +
> > + ? ? ? printk("%s: %p, %p, %d, %ld, %lx, %u\n", __func__, dev, arena,
> > arena->size,
> > + ? ? ? ? ? ? ?n, mask, align);
> > +
> > ? ? ? ?p = iommu_arena_find_pages(dev, arena, n, mask);
> > ? ? ? ?if (p < 0) {
> > ? ? ? ? ? ? ? ?spin_unlock_irqrestore(>lock, flags);
>
> Using this patch, I log the attached output.
Your system has 1GB iommu address space. I guess that it's enough for
KSM?
The parameters in the log looks good. But you got this log before you
started X?
Problems with alpha/pci + radeon/ttm
On Mon, 21 Jun 2010 17:19:43 -0400
Matt Turner wrote:
> Michael Cree and I have been debugging FDO bug 26403 [1]. I tried
> booting with `radeon.test=1` and found this, which I think is related:
>
> > [drm] Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x202000
> > [drm] Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x302000
> [snip]
> > [drm] Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0xfd02000
> > [drm] Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0xfe02000
> > pci_map_single failed: could not allocate dma page tables
> > [drm:radeon_ttm_backend_bind] *ERROR* failed to bind 128 pages at 0x0FF02000
> > [TTM] Couldn't bind backend.
> > radeon :00:07.0: object_init failed for (1048576, 0x0002)
> > [drm:radeon_test_moves] *ERROR* Failed to create GTT object 253
> > Error while testing BO move.
>
> From what I can see, the call chain is
> radeon_test_moves
> (radeon_ttm_backend_bind called through callback function)
> - radeon_ttm.c:radeon_ttm_backend_bind calls radeon_gart_bind
> - radeon_gart.c:radeon_gart_bind calls pci_map_page
>- pci_map_page is alpha_pci_map_page, which calls...
> - alpha_pci_map_page calls pci_iommu.c:pci_map_single_1
> - pci_map_single_1 calls iommu_arena_alloc
> - iommu_arena_alloc calls iommu_arena_find_pages
>- iommu_arena_find_pages returns non-0
> - iommu_arena_alloc returns non-0
> - pci_map_single_1 returns 0 after printing
>"could not allocate dma page tables" error
> - alpha_pci_map_page returns 0 from pci_map_single_1
> - radeon_gart_bind returns non-0, error path prints
> "*ERROR* failed to bind 128 pages at 0x0FF02000"
This happens in the latest git, right?
Is this a regression (what kernel version worked)?
Seems that the IOMMU can't find 128 pages. It's likely due to:
- out of the IOMMU space (possibly someone doesn't free the IOMMU
space).
or
- the mapping parameters (such as align) aren't appropriate so the
IOMMU can't find space.
> Is this the cause of the bug we're seeing in the report [1]?
>
> Anyone know what's going wrong here?
I've attached a patch to print the debug info about the mapping
parameters.
diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c
index d1dbd9a..17cf0d8 100644
--- a/arch/alpha/kernel/pci_iommu.c
+++ b/arch/alpha/kernel/pci_iommu.c
@@ -187,6 +187,10 @@ iommu_arena_alloc(struct device *dev, struct
pci_iommu_arena *arena, long n,
/* Search for N empty ptes */
ptes = arena->ptes;
mask = max(align, arena->align_entry) - 1;
+
+ printk("%s: %p, %p, %d, %ld, %lx, %u\n", __func__, dev, arena,
arena->size,
+ n, mask, align);
+
p = iommu_arena_find_pages(dev, arena, n, mask);
if (p < 0) {
spin_unlock_irqrestore(>lock, flags);
Re: Problems with alpha/pci + radeon/ttm
On Mon, 21 Jun 2010 17:19:43 -0400
Matt Turner matts...@gmail.com wrote:
Michael Cree and I have been debugging FDO bug 26403 [1]. I tried
booting with `radeon.test=1` and found this, which I think is related:
[drm] Tested GTT-VRAM and VRAM-GTT copy for GTT offset 0x202000
[drm] Tested GTT-VRAM and VRAM-GTT copy for GTT offset 0x302000
[snip]
[drm] Tested GTT-VRAM and VRAM-GTT copy for GTT offset 0xfd02000
[drm] Tested GTT-VRAM and VRAM-GTT copy for GTT offset 0xfe02000
pci_map_single failed: could not allocate dma page tables
[drm:radeon_ttm_backend_bind] *ERROR* failed to bind 128 pages at 0x0FF02000
[TTM] Couldn't bind backend.
radeon :00:07.0: object_init failed for (1048576, 0x0002)
[drm:radeon_test_moves] *ERROR* Failed to create GTT object 253
Error while testing BO move.
From what I can see, the call chain is
radeon_test_moves
(radeon_ttm_backend_bind called through callback function)
- radeon_ttm.c:radeon_ttm_backend_bind calls radeon_gart_bind
- radeon_gart.c:radeon_gart_bind calls pci_map_page
- pci_map_page is alpha_pci_map_page, which calls...
- alpha_pci_map_page calls pci_iommu.c:pci_map_single_1
- pci_map_single_1 calls iommu_arena_alloc
- iommu_arena_alloc calls iommu_arena_find_pages
- iommu_arena_find_pages returns non-0
- iommu_arena_alloc returns non-0
- pci_map_single_1 returns 0 after printing
could not allocate dma page tables error
- alpha_pci_map_page returns 0 from pci_map_single_1
- radeon_gart_bind returns non-0, error path prints
*ERROR* failed to bind 128 pages at 0x0FF02000
This happens in the latest git, right?
Is this a regression (what kernel version worked)?
Seems that the IOMMU can't find 128 pages. It's likely due to:
- out of the IOMMU space (possibly someone doesn't free the IOMMU
space).
or
- the mapping parameters (such as align) aren't appropriate so the
IOMMU can't find space.
Is this the cause of the bug we're seeing in the report [1]?
Anyone know what's going wrong here?
I've attached a patch to print the debug info about the mapping
parameters.
diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c
index d1dbd9a..17cf0d8 100644
--- a/arch/alpha/kernel/pci_iommu.c
+++ b/arch/alpha/kernel/pci_iommu.c
@@ -187,6 +187,10 @@ iommu_arena_alloc(struct device *dev, struct
pci_iommu_arena *arena, long n,
/* Search for N empty ptes */
ptes = arena-ptes;
mask = max(align, arena-align_entry) - 1;
+
+ printk(%s: %p, %p, %d, %ld, %lx, %u\n, __func__, dev, arena,
arena-size,
+ n, mask, align);
+
p = iommu_arena_find_pages(dev, arena, n, mask);
if (p 0) {
spin_unlock_irqrestore(arena-lock, flags);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
