rust: Add dma_set_mask() and dma_set_coherent_mask() bindings
Subsystems (4)
PATCH 1/2 rust: Add dma_set_mask() and dma_set_coherent_mask() bindings
Add bindings to allow setting the DMA masks for both a generic device and a PCI device.
--- rust/kernel/device.rs | 25 +++++++++++++++++++++++++ rust/kernel/pci.rs | 23 +++++++++++++++++++++++ 2 files changed, 48 insertions(+)
rust/kernel/device.rs +25 -0
@@ -10,6 +10,7 @@ types::{ARef, Opaque}, };use core::{fmt, marker::PhantomData , ptr}; +use kernel::prelude::*;#[cfg( CONFIG_PRINTK )] use crate ::c_str;
@@ -67,6 +68,30 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device { self.0 .get () } +/// Sets the DMA mask for the device. +pub fn dma_set_mask (&self, mask:u64 ) ->Result { +// SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid. +let ret =unsafe { bindings::dma_set_mask (&(*self.as_raw ())as *const _ as *mut _ , mask) }; +if ret !=0 { +Err (Error ::from_errno (ret)) + }else { +Ok (()) + } + } + +/// Sets the coherent DMA mask for the device. +pub fn dma_set_coherent_mask (&self, mask:u64 ) ->Result { +// SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid. +let ret =unsafe { + bindings::dma_set_coherent_mask (&(*self.as_raw ())as *const _ as *mut _ , mask) + }; +if ret !=0 { +Err (Error ::from_errno (ret)) + }else { +Ok (()) + } + } +/// Returns a reference to the parent device, if any. #[cfg_attr(not( CONFIG_AUXILIARY_BUS ), expect(dead_code))] pub (crate )fn parent (&self) ->Option <&Self > {
rust/kernel/pci.rs +23 -0
@@ -369,6 +369,17 @@ fn as_raw(&self) -> *mut bindings::pci_dev { } } +impl <'a,Ctx : device::DeviceContext >From <&'a kernel::pci::Device <Ctx >> +for &'a device::Device <Ctx > +{ +fn from (pdev: &kernel::pci::Device <Ctx >) -> &device::Device <Ctx > { +// SAFETY: The returned reference has the same lifetime as the +// pci::Device which holds a reference on the underlying device +// pointer. +unsafe { device::Device ::as_ref (&(*pdev.as_raw ()).devas *const _ as *mut _ ) } + } +} +impl Device {/// Returns the PCI vendor ID. pub fn vendor_id (&self) ->u16 {
@@ -393,6 +404,18 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {// - by its type invariant `self.as_raw` is always a valid pointer to a `struct pci_dev`. Ok (unsafe { bindings::pci_resource_len (self.as_raw (), bar.try_into ()?) }) } + +/// Set the DMA mask for PCI device. +pub fn dma_set_mask (&self, mask:u64 ) ->Result { +let dev: &device::Device = self.into (); + dev.dma_set_mask (mask) + } + +/// Set the coherent DMA mask for the PCI device. +pub fn dma_set_coherent_mask (&self, mask:u64 ) ->Result { +let dev: &device::Device = self.into (); + dev.dma_set_coherent_mask (mask) + } }impl Device <device::Bound > {
Signature · 1 line
2.47.2
PATCH 2/2 rust: Add several miscellaneous PCI helpers
Add bindings to obtain a PCI device's resource start address, bus/ device function, revision ID and subsystem device and vendor IDs.
--- rust/helpers/pci.c | 10 ++++++++++ rust/kernel/pci.rs | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+)
rust/helpers/pci.c +10 -0
@@ -12,6 +12,16 @@ void *rust_helper_pci_get_drvdata(struct pci_dev *pdev)return pci_get_drvdata (pdev); } +u16 rust_helper_pci_dev_id (struct pci_dev *dev) +{ +return PCI_DEVID (dev->bus->number, dev->devfn); +} + +resource_size_t rust_helper_pci_resource_start (struct pci_dev *pdev,int bar) +{ +return pci_resource_start (pdev, bar); +} +resource_size_t rust_helper_pci_resource_len (struct pci_dev *pdev,int bar) {return pci_resource_len (pdev, bar);
rust/kernel/pci.rs +36 -0
@@ -393,6 +393,42 @@ pub fn device_id(&self) -> u16 {unsafe { (*self.as_raw ()).device } } +/// Returns the PCI revision ID. +pub fn revision_id (&self) ->u8 { +// SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`. +unsafe { (*self.as_raw ()).revision } + } + +/// Returns the PCI bus device/function. +pub fn dev_id (&self) ->u16 { +// SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`. +unsafe { bindings::pci_dev_id (self.as_raw ()) } + } + +/// Returns the PCI subsystem vendor ID. +pub fn subsystem_vendor_id (&self) ->u16 { +// SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`. +unsafe { (*self.as_raw ()).subsystem_vendor } + } + +/// Returns the PCI subsystem device ID. +pub fn subsystem_device_id (&self) ->u16 { +// SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`. +unsafe { (*self.as_raw ()).subsystem_device } + } + +/// Returns the start of the given PCI bar resource. +pub fn resource_start (&self, bar:u32 ) ->Result <bindings::resource_size_t > { +if !Bar ::index_is_valid (bar) { +return Err (EINVAL ); + } + +// SAFETY: +// - `bar` is a valid bar number, as guaranteed by the above call to `Bar::index_is_valid`, +// - by its type invariant `self.as_raw` is always a valid pointer to a `struct pci_dev`. +Ok (unsafe { bindings::pci_resource_start (self.as_raw (), bar.try_into ()?) }) + } +/// Returns the size of the given PCI bar resource. pub fn resource_len (&self, bar:u32 ) ->Result <bindings::resource_size_t > {if !Bar ::index_is_valid (bar) {
Signature · 1 line
2.47.2
Re: [PATCH 2/2] rust: Add several miscellaneous PCI helpers
Quoted 2 lines · Alistair Popple
Add bindings to obtain a PCI device's resource start address, bus/ device function, revision ID and subsystem device and vendor IDs.
FWIW,
Greg Kroah-Hartman
maintainerRe: [PATCH 2/2] rust: Add several miscellaneous PCI helpers
In reply to Alistair PoppleQuoted 2 lines · Alistair Popple
Add bindings to obtain a PCI device's resource start address, bus/ device function, revision ID and subsystem device and vendor IDs.
Do we have a user for these new helpers already?
thanks,
greg k-h
Re: [PATCH 2/2] rust: Add several miscellaneous PCI helpers
Quoted 6 lines · Greg Kroah-Hartman
Quoted 2 lines · Alistair Popple
Add bindings to obtain a PCI device's resource start address, bus/ device function, revision ID and subsystem device and vendor IDs.
Do we have a user for these new helpers already?
We need them for nova-core. I'm still cleaning up the patch series that uses these but figured these particular bindings should be pretty uncontroversial so could be sent ahead of that.
Danilo has requested some changes so I will note that when I send v2.
Thanks.
Quoted 3 lines
thanks,
greg k-h
Danilo Krummrich
maintainerRe: [PATCH 2/2] rust: Add several miscellaneous PCI helpers
In reply to Alistair Popplerust/kernel/pci.rs pub fn device_id(&self) -> u16 { · line 397
Quoted 10 lines · Alistair Popple
rust/kernel/pci.rs +2
-0@@ -393,6 +393,42 @@ pub fn device_id(&self) -> u16 {unsafe { (*self.as_raw ()).device } } +/// Returns the PCI revision ID. +pub fn revision_id (&self) ->u8 {
We should add a compiler hint for those methods to be inlined.
rust/kernel/pci.rs pub fn device_id(&self) -> u16 { · line 398
Quoted 1 line
rust/kernel/pci.rs +1
-0+// SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
Let's refer to the type invariant for the validity of self.as_raw().
rust/kernel/pci.rs pub fn device_id(&self) -> u16 { · line 400
Quoted 2 lines
rust/kernel/pci.rs +2
-0+unsafe { (*self.as_raw ()).revision } + }
Both is also true for the existing methods vendor_id() and device_id(). Can you please fix them up in a separate patch as well?
Also, please add a brief note in the commit message where those will be used (even though I obviously know). :)
Re: [PATCH 2/2] rust: Add several miscellaneous PCI helpers
Quoted 16 lines · Danilo Krummrich
rust/kernel/pci.rs pub fn device_id(&self) -> u16 { · line 397
Quoted 10 lines · Alistair Popple
rust/kernel/pci.rs +2
-0@@ -393,6 +393,42 @@ pub fn device_id(&self) -> u16 {unsafe { (*self.as_raw ()).device } } +/// Returns the PCI revision ID. +pub fn revision_id (&self) ->u8 {We should add a compiler hint for those methods to be inlined.
rust/kernel/pci.rs pub fn device_id(&self) -> u16 { · line 398
Quoted 1 line
rust/kernel/pci.rs +1
-0+// SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
Quoted 7 lines
Let's refer to the type invariant for the validity of
self.as_raw().rust/kernel/pci.rs pub fn device_id(&self) -> u16 { · line 400
Quoted 2 lines
rust/kernel/pci.rs +2
-0+unsafe { (*self.as_raw ()).revision } + }Both is also true for the existing methods
vendor_id()anddevice_id(). Can you please fix them up in a separate patch as well?
Sure.
Quoted 2 lines
Also, please add a brief note in the commit message where those will be used (even though I obviously know). :)
:) Will do.
Alexandre Courbot
reviewerHi Alistair,
Quoted 42 lines · Alistair Popple
Add bindings to allow setting the DMA masks for both a generic device and a PCI device.
Signed-off-by Alistair Popple <[email protected]>Cc Danilo Krummrich <[email protected]>Cc Bjorn Helgaas <[email protected]>Cc "Krzysztof Wilczyński" <[email protected]>Cc Miguel Ojeda <[email protected]>Cc Alex Gaynor <[email protected]>Cc Boqun Feng <[email protected]>Cc Gary Guo <[email protected]>Cc "Björn Roy Baron" <[email protected]>Cc Benno Lossin <[email protected]>Cc Andreas Hindborg <[email protected]>Cc Alice Ryhl <[email protected]>Cc Trevor Gross <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc John Hubbard <[email protected]>Cc Alexandre Courbot <[email protected]>--- rust/kernel/device.rs | 25 +++++++++++++++++++++++++ rust/kernel/pci.rs | 23 +++++++++++++++++++++++ 2 files changed, 48 insertions(+)rust/kernel/device.rs +1
-0@@ -10,6 +10,7 @@ types::{ARef, Opaque}, };use core::{fmt, marker::PhantomData , ptr}; +use kernel::prelude::*;#[cfg( CONFIG_PRINTK )] use crate ::c_str;@@ -67,6 +68,30 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device { self.0 .get () }
rust/kernel/device.rs pub(crate) fn as_raw(&self) -> *mut bindings::device { · line 79
Quoted 10 lines
rust/kernel/device.rs +9
-0+/// Sets the DMA mask for the device. +pub fn dma_set_mask (&self, mask:u64 ) ->Result { +// SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid. +let ret =unsafe { bindings::dma_set_mask (&(*self.as_raw ())as *const _ as *mut _ , mask) }; +if ret !=0 { +Err (Error ::from_errno (ret)) + }else { +Ok (()) + }
I think you want to use kernel::error::to_result() here?
rust/kernel/device.rs pub(crate) fn as_raw(&self) -> *mut bindings::device { · line 92
Quoted 13 lines
rust/kernel/device.rs +13
-0+ } + +/// Sets the coherent DMA mask for the device. +pub fn dma_set_coherent_mask (&self, mask:u64 ) ->Result { +// SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid. +let ret =unsafe { + bindings::dma_set_coherent_mask (&(*self.as_raw ())as *const _ as *mut _ , mask) + }; +if ret !=0 { +Err (Error ::from_errno (ret)) + }else { +Ok (()) + }
And here as well.
Quoted 13 lines
rust/kernel/device.rs +2
-0+ } +/// Returns a reference to the parent device, if any. #[cfg_attr(not( CONFIG_AUXILIARY_BUS ), expect(dead_code))] pub (crate )fn parent (&self) ->Option <&Self > {rust/kernel/pci.rs +0
-0@@ -369,6 +369,17 @@ fn as_raw(&self) -> *mut bindings::pci_dev { } }
rust/kernel/pci.rs fn as_raw(&self) -> *mut bindings::pci_dev { · line 381
Quoted 10 lines
rust/kernel/pci.rs +10
-0+impl <'a,Ctx : device::DeviceContext >From <&'a kernel::pci::Device <Ctx >> +for &'a device::Device <Ctx > +{ +fn from (pdev: &kernel::pci::Device <Ctx >) -> &device::Device <Ctx > { +// SAFETY: The returned reference has the same lifetime as the +// pci::Device which holds a reference on the underlying device +// pointer. +unsafe { device::Device ::as_ref (&(*pdev.as_raw ()).devas *const _ as *mut _ ) } + } +}
IIUC pci::Device has an AsRef<device::Device> implementation, why not use that in the code below?
rust/kernel/pci.rs pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> { · line 411
Quoted 13 lines
rust/kernel/pci.rs +6
-0+impl Device {/// Returns the PCI vendor ID. pub fn vendor_id (&self) ->u16 {@@ -393,6 +404,18 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {// - by its type invariant `self.as_raw` is always a valid pointer to a `struct pci_dev`. Ok (unsafe { bindings::pci_resource_len (self.as_raw (), bar.try_into ()?) }) } + +/// Set the DMA mask for PCI device. +pub fn dma_set_mask (&self, mask:u64 ) ->Result { +let dev: &device::Device = self.into (); + dev.dma_set_mask (mask)
Yup, I have tried and self.as_ref().dma_set_mask(mask) works just fine, so I don't think we need the From implementation above.
Quoted 46 lines · Alexandre Courbot
Hi Alistair,
Quoted 43 lines · Alistair Popple
Add bindings to allow setting the DMA masks for both a generic device and a PCI device.
Signed-off-by Alistair Popple <[email protected]>Cc Danilo Krummrich <[email protected]>Cc Bjorn Helgaas <[email protected]>Cc "Krzysztof Wilczyński" <[email protected]>Cc Miguel Ojeda <[email protected]>Cc Alex Gaynor <[email protected]>Cc Boqun Feng <[email protected]>Cc Gary Guo <[email protected]>Cc "Björn Roy Baron" <[email protected]>Cc Benno Lossin <[email protected]>Cc Andreas Hindborg <[email protected]>Cc Alice Ryhl <[email protected]>Cc Trevor Gross <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc John Hubbard <[email protected]>Cc Alexandre Courbot <[email protected]>--- rust/kernel/device.rs | 25 +++++++++++++++++++++++++ rust/kernel/pci.rs | 23 +++++++++++++++++++++++ 2 files changed, 48 insertions(+)rust/kernel/device.rs +1
-0@@ -10,6 +10,7 @@ types::{ARef, Opaque}, };use core::{fmt, marker::PhantomData , ptr}; +use kernel::prelude::*;#[cfg( CONFIG_PRINTK )] use crate ::c_str;@@ -67,6 +68,30 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device { self.0 .get () }
Quoted 11 lines
rust/kernel/device.rs pub(crate) fn as_raw(&self) -> *mut bindings::device { · line 79
Quoted 9 lines
rust/kernel/device.rs +9
-0+/// Sets the DMA mask for the device. +pub fn dma_set_mask (&self, mask:u64 ) ->Result { +// SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid. +let ret =unsafe { bindings::dma_set_mask (&(*self.as_raw ())as *const _ as *mut _ , mask) }; +if ret !=0 { +Err (Error ::from_errno (ret)) + }else { +Ok (()) + }I think you want to use
kernel::error::to_result()here?
Ok.
Quoted 43 lines
rust/kernel/device.rs pub(crate) fn as_raw(&self) -> *mut bindings::device { · line 92
Quoted 13 lines
rust/kernel/device.rs +13
-0+ } + +/// Sets the coherent DMA mask for the device. +pub fn dma_set_coherent_mask (&self, mask:u64 ) ->Result { +// SAFETY: By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid. +let ret =unsafe { + bindings::dma_set_coherent_mask (&(*self.as_raw ())as *const _ as *mut _ , mask) + }; +if ret !=0 { +Err (Error ::from_errno (ret)) + }else { +Ok (()) + }And here as well.
Quoted 13 lines
rust/kernel/device.rs +2
-0+ } +/// Returns a reference to the parent device, if any. #[cfg_attr(not( CONFIG_AUXILIARY_BUS ), expect(dead_code))] pub (crate )fn parent (&self) ->Option <&Self > {rust/kernel/pci.rs +0
-0@@ -369,6 +369,17 @@ fn as_raw(&self) -> *mut bindings::pci_dev { } }rust/kernel/pci.rs fn as_raw(&self) -> *mut bindings::pci_dev { · line 381
Quoted 10 lines
rust/kernel/pci.rs +10
-0+impl <'a,Ctx : device::DeviceContext >From <&'a kernel::pci::Device <Ctx >> +for &'a device::Device <Ctx > +{ +fn from (pdev: &kernel::pci::Device <Ctx >) -> &device::Device <Ctx > { +// SAFETY: The returned reference has the same lifetime as the +// pci::Device which holds a reference on the underlying device +// pointer. +unsafe { device::Device ::as_ref (&(*pdev.as_raw ()).devas *const _ as *mut _ ) } + } +}IIUC
pci::Devicehas anAsRef<device::Device>implementation, why not use that in the code below?
Quoted 16 lines
rust/kernel/pci.rs pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> { · line 411
Quoted 13 lines
rust/kernel/pci.rs +6
-0+impl Device {/// Returns the PCI vendor ID. pub fn vendor_id (&self) ->u16 {@@ -393,6 +404,18 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {// - by its type invariant `self.as_raw` is always a valid pointer to a `struct pci_dev`. Ok (unsafe { bindings::pci_resource_len (self.as_raw (), bar.try_into ()?) }) } + +/// Set the DMA mask for PCI device. +pub fn dma_set_mask (&self, mask:u64 ) ->Result { +let dev: &device::Device = self.into (); + dev.dma_set_mask (mask)Yup, I have tried and
self.as_ref().dma_set_mask(mask)works just fine, so I don't think we need theFromimplementation above.
Right you are. Will drop the From implementation for the next version, thanks.
Danilo Krummrich
maintainerQuoted 2 lines · Alistair Popple
Add bindings to allow setting the DMA masks for both a generic device and a PCI device.
Nice coincidence, I was about to get back to this. I already implemented this in a previous patch [1], but didn't apply it yet.
I think the approach below is thought a bit too simple:
(1) We want the DMA mask methods to be implemented by a trait in dma.rs. Subsequently, the trait should only be implemented by bus devices where the bus actually supports DMA. Allowing to set the DMA mask on any device doesn't make sense.
(2) We need to consider that with this we do no prevent dma_set_coherent_mask() to concurrently with dma_alloc_coherent() (not even if we'd add a new Probe device context).
(2) is the main reason why I didn't follow up yet. So far I haven't found a nice solution for a sound API that doesn't need unsafe.
One thing I did consider was to have some kind of per device table (similar to the device ID table) for drivers to specify the DMA mask already at compile time. However, I'm pretty sure there are cases where the DMA mask has to derived dynamically from probe().
I think I have to think a bit more about it.
Danilo Krummrich
maintainerQuoted 7 lines · Danilo Krummrich
Quoted 2 lines · Alistair Popple
Add bindings to allow setting the DMA masks for both a generic device and a PCI device.
Nice coincidence, I was about to get back to this. I already implemented this in a previous patch [1], but didn't apply it yet.
Quoted 6 lines
I think the approach below is thought a bit too simple:
(1) We want the DMA mask methods to be implemented by a trait in
dma.rs. Subsequently, the trait should only be implemented by bus devices where the bus actually supports DMA. Allowing to set the DMA mask on any device doesn't make sense.
Forgot to mention, another reason for a trait is that we can also use it as a trait bound on dma::CoherentAllocation::new(), such that people can't pass arbitrary devices to dma::CoherentAllocation::new(), but only those that actually sit on a DMA capable bus.
Quoted 16 lines
(2) We need to consider that with this we do no prevent
dma_set_coherent_mask()to concurrently withdma_alloc_coherent()(not even if we'd add a newProbedevice context).(2) is the main reason why I didn't follow up yet. So far I haven't found a nice solution for a sound API that doesn't need unsafe.
One thing I did consider was to have some kind of per device table (similar to the device ID table) for drivers to specify the DMA mask already at compile time. However, I'm pretty sure there are cases where the DMA mask has to derived dynamically from
probe().I think I have to think a bit more about it.
Danilo Krummrich
maintainerQuoted 15 lines · Danilo Krummrich
Quoted 7 lines · Danilo Krummrich
Quoted 2 lines · Alistair Popple
Add bindings to allow setting the DMA masks for both a generic device and a PCI device.
Nice coincidence, I was about to get back to this. I already implemented this in a previous patch [1], but didn't apply it yet.
Quoted 6 lines
I think the approach below is thought a bit too simple:
(1) We want the DMA mask methods to be implemented by a trait in
dma.rs. Subsequently, the trait should only be implemented by bus devices where the bus actually supports DMA. Allowing to set the DMA mask on any device doesn't make sense.
Quoted 19 lines
Forgot to mention, another reason for a trait is that we can also use it as a trait bound on
dma::CoherentAllocation::new(), such that people can't pass arbitrary devices todma::CoherentAllocation::new(), but only those that actually sit on a DMA capable bus.Quoted 14 lines
(2) We need to consider that with this we do no prevent
dma_set_coherent_mask()to concurrently withdma_alloc_coherent()(not even if we'd add a newProbedevice context).(2) is the main reason why I didn't follow up yet. So far I haven't found a nice solution for a sound API that doesn't need unsafe.
One thing I did consider was to have some kind of per device table (similar to the device ID table) for drivers to specify the DMA mask already at compile time. However, I'm pretty sure there are cases where the DMA mask has to derived dynamically from
probe().I think I have to think a bit more about it.
Ok, there are multiple things to consider in the context of (2) above.
(a) We have to ensure that the dev->dma_mask pointer is properly initialized, which happens when the corresponding bus device is initialized. This is definitely the case when probe() is called, i.e. when the device is bound.
So the solutions here is simple, we just implement the dma::Device trait (which implements dma_set_mask() and dma_set_coherent_mask()) for &Device<Bound>.
(b) When dma_set_mask() or dma_set_coherent_mask() are called concurrently with e.g. dma_alloc_coherent(), there is a data race with dev->dma_mask, dev->coherent_dma_mask and dev->dma_skip_sync (also set by dma_set_mask()).
However, AFAICT, this does not necessarily make the Rust API unsafe in the sense of Rust's requirements. I.e. a potential data race does not lead to undefined behavior on the CPU side of things, but may result into a not properly functioning device.
It would be possible to declare dma_set_mask() and dma_set_coherent_mask() Rust accessors as safe with the caveat that the device may not be able to use the memory concurrently allocated with e.g. dma::CoherentAllocation::new() properly.
The alternative would be to make dma_set_mask() and dma_set_coherent_mask() unsafe to begin with.
I don't think there's a reasonable alternative given that the mask may be derived on runtime in probe() by probing the device itself.
I guess we could do something with type states and cookie values etc., but that's unreasonable overhead for something that is clearly more a theoretical than a practical concern.
My conclusion is that we should just declare dma_set_mask() and dma_set_coherent_mask() as safe functions (with proper documentation on the pitfalls), given that the device is equally malfunctioning if they're not called at all.
@Alistair: If that is fine for you I'll pick up my old patches ([1] and related ones) and re-send them.
If there is more discussion on (b) I'm happy to follow up either here or in the mentioned patches once I re-visited and re-sent them.
Quoted 1 line
Quoted 1 line
Quoted 71 lines · Danilo Krummrich
Quoted 15 lines · Danilo Krummrich
Quoted 7 lines · Danilo Krummrich
Quoted 2 lines · Alistair Popple
Add bindings to allow setting the DMA masks for both a generic device and a PCI device.
Nice coincidence, I was about to get back to this. I already implemented this in a previous patch [1], but didn't apply it yet.
Quoted 6 lines
I think the approach below is thought a bit too simple:
(1) We want the DMA mask methods to be implemented by a trait in
dma.rs. Subsequently, the trait should only be implemented by bus devices where the bus actually supports DMA. Allowing to set the DMA mask on any device doesn't make sense.Quoted 19 lines
Forgot to mention, another reason for a trait is that we can also use it as a trait bound on
dma::CoherentAllocation::new(), such that people can't pass arbitrary devices todma::CoherentAllocation::new(), but only those that actually sit on a DMA capable bus.Quoted 14 lines
(2) We need to consider that with this we do no prevent
dma_set_coherent_mask()to concurrently withdma_alloc_coherent()(not even if we'd add a newProbedevice context).(2) is the main reason why I didn't follow up yet. So far I haven't found a nice solution for a sound API that doesn't need unsafe.
One thing I did consider was to have some kind of per device table (similar to the device ID table) for drivers to specify the DMA mask already at compile time. However, I'm pretty sure there are cases where the DMA mask has to derived dynamically from
probe().I think I have to think a bit more about it.
Ok, there are multiple things to consider in the context of (2) above.
(a) We have to ensure that the
dev->dma_maskpointer is properly initialized, which happens when the corresponding bus device is initialized. This is definitely the case whenprobe()is called, i.e. when the device is bound.So the solutions here is simple, we just implement the
dma::Devicetrait (which implementsdma_set_mask()anddma_set_coherent_mask()) for &Device<Bound>.(b) When
dma_set_mask()ordma_set_coherent_mask()are called concurrently with e.g.dma_alloc_coherent(), there is a data race withdev->dma_mask,dev->coherent_dma_maskanddev->dma_skip_sync(also set bydma_set_mask()).However, AFAICT, this does not necessarily make the Rust API unsafe in the sense of Rust's requirements. I.e. a potential data race does not lead to undefined behavior on the CPU side of things, but may result into a not properly functioning device.
It would be possible to declare
dma_set_mask()anddma_set_coherent_mask()Rust accessors as safe with the caveat that the device may not be able to use the memory concurrently allocated with e.g.dma::CoherentAllocation::new()properly.The alternative would be to make
dma_set_mask()anddma_set_coherent_mask()unsafe to begin with.I don't think there's a reasonable alternative given that the mask may be derived on runtime in
probe()by probing the device itself.I guess we could do something with type states and cookie values etc., but that's unreasonable overhead for something that is clearly more a theoretical than a practical concern.
Quoted 7 lines
My conclusion is that we should just declare
dma_set_mask()anddma_set_coherent_mask()as safe functions (with proper documentation on the pitfalls), given that the device is equally malfunctioning if they're not called at all.@Alistair: If that is fine for you I'll pick up my old patches ([1] and related ones) and re-send them.
Fine with me, and I agree with your conclusion that these should just be declared as safe functions with proper documentation. I think there are cases where the mask is dynamic and in practice I think basically all drivers just set the mask in their probe routine before doing any DMA allocations anyway so it does seem more theoretical.
Quoted 2 lines
If there is more discussion on (b) I'm happy to follow up either here or in the mentioned patches once I re-visited and re-sent them.
Sounds good. My motivation for sending this was simply that we will need it to start initialising the GPU in nova-core.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Danilo Krummrich
maintainerQuoted 51 lines · Danilo Krummrich
Quoted 15 lines · Danilo Krummrich
Quoted 7 lines · Danilo Krummrich
Quoted 2 lines · Alistair Popple
Add bindings to allow setting the DMA masks for both a generic device and a PCI device.
Nice coincidence, I was about to get back to this. I already implemented this in a previous patch [1], but didn't apply it yet.
Quoted 6 lines
I think the approach below is thought a bit too simple:
(1) We want the DMA mask methods to be implemented by a trait in
dma.rs. Subsequently, the trait should only be implemented by bus devices where the bus actually supports DMA. Allowing to set the DMA mask on any device doesn't make sense.Quoted 19 lines
Forgot to mention, another reason for a trait is that we can also use it as a trait bound on
dma::CoherentAllocation::new(), such that people can't pass arbitrary devices todma::CoherentAllocation::new(), but only those that actually sit on a DMA capable bus.Quoted 14 lines
(2) We need to consider that with this we do no prevent
dma_set_coherent_mask()to concurrently withdma_alloc_coherent()(not even if we'd add a newProbedevice context).(2) is the main reason why I didn't follow up yet. So far I haven't found a nice solution for a sound API that doesn't need unsafe.
One thing I did consider was to have some kind of per device table (similar to the device ID table) for drivers to specify the DMA mask already at compile time. However, I'm pretty sure there are cases where the DMA mask has to derived dynamically from
probe().I think I have to think a bit more about it.
Ok, there are multiple things to consider in the context of (2) above.
(a) We have to ensure that the
dev->dma_maskpointer is properly initialized, which happens when the corresponding bus device is initialized. This is definitely the case whenprobe()is called, i.e. when the device is bound.So the solutions here is simple, we just implement the
dma::Devicetrait (which implementsdma_set_mask()anddma_set_coherent_mask()) for &Device<Bound>.(b) When
dma_set_mask()ordma_set_coherent_mask()are called concurrently with e.g.dma_alloc_coherent(), there is a data race withdev->dma_mask,dev->coherent_dma_maskanddev->dma_skip_sync(also set bydma_set_mask()).
Quoted 4 lines
However, AFAICT, this does not necessarily make the Rust API unsafe in the sense of Rust's requirements. I.e. a potential data race does not lead to undefined behavior on the CPU side of things, but may result into a not properly functioning device.
Apparently, this is wrong, and it might indeed result in undefined behavior on the CPU side of things. :(
Quoted 28 lines
It would be possible to declare
dma_set_mask()anddma_set_coherent_mask()Rust accessors as safe with the caveat that the device may not be able to use the memory concurrently allocated with e.g.dma::CoherentAllocation::new()properly.The alternative would be to make
dma_set_mask()anddma_set_coherent_mask()unsafe to begin with.I don't think there's a reasonable alternative given that the mask may be derived on runtime in
probe()by probing the device itself.I guess we could do something with type states and cookie values etc., but that's unreasonable overhead for something that is clearly more a theoretical than a practical concern.
My conclusion is that we should just declare
dma_set_mask()anddma_set_coherent_mask()as safe functions (with proper documentation on the pitfalls), given that the device is equally malfunctioning if they're not called at all.@Alistair: If that is fine for you I'll pick up my old patches ([1] and related ones) and re-send them.
If there is more discussion on (b) I'm happy to follow up either here or in the mentioned patches once I re-visited and re-sent them.
Quoted 1 line
Quoted 1 line