We could do better here by having the macros return `Result`,
so that we don't have to wrap these calls in a closure for
validation which is confusing.
Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Suggested-by: Andreas Hindborg <a.hindborg@kernel.org>
Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
rust/kernel/dma.rs | 54 +++++++++++++++++++++++-----------------
samples/rust/rust_dma.rs | 25 ++++++++-----------
2 files changed, 42 insertions(+), 37 deletions(-)
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index d3f448868457..a61da5eeb017 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -328,20 +328,22 @@ unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {}
#[macro_export]
macro_rules! dma_read {
($dma:expr, $idx: expr, $($field:tt)*) => {{
- let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
- // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
- // dereferenced. The compiler also further validates the expression on whether `field`
- // is a member of `item` when expanded by the macro.
- unsafe {
- let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
- $crate::dma::CoherentAllocation::field_read(&$dma, ptr_field)
- }
+ (|| -> ::core::result::Result<_, $crate::error::Error> {
+ let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+ // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
+ // dereferenced. The compiler also further validates the expression on whether `field`
+ // is a member of `item` when expanded by the macro.
+ unsafe {
+ let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
+ ::core::result::Result::Ok($crate::dma::CoherentAllocation::field_read(&$dma, ptr_field))
+ }
+ })()
}};
($dma:ident [ $idx:expr ] $($field:tt)* ) => {
- $crate::dma_read!($dma, $idx, $($field)*);
+ $crate::dma_read!($dma, $idx, $($field)*)
};
($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {
- $crate::dma_read!($($dma).*, $idx, $($field)*);
+ $crate::dma_read!($($dma).*, $idx, $($field)*)
};
}
@@ -368,24 +370,30 @@ macro_rules! dma_read {
#[macro_export]
macro_rules! dma_write {
($dma:ident [ $idx:expr ] $($field:tt)*) => {{
- $crate::dma_write!($dma, $idx, $($field)*);
+ $crate::dma_write!($dma, $idx, $($field)*)
}};
($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {{
- $crate::dma_write!($($dma).*, $idx, $($field)*);
+ $crate::dma_write!($($dma).*, $idx, $($field)*)
}};
($dma:expr, $idx: expr, = $val:expr) => {
- let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
- // SAFETY: `item_from_index` ensures that `item` is always a valid item.
- unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) }
+ (|| -> ::core::result::Result<_, $crate::error::Error> {
+ let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+ // SAFETY: `item_from_index` ensures that `item` is always a valid item.
+ unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) }
+ ::core::result::Result::Ok(())
+ })()
};
($dma:expr, $idx: expr, $(.$field:ident)* = $val:expr) => {
- let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
- // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
- // dereferenced. The compiler also further validates the expression on whether `field`
- // is a member of `item` when expanded by the macro.
- unsafe {
- let ptr_field = ::core::ptr::addr_of_mut!((*item) $(.$field)*);
- $crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val)
- }
+ (|| -> ::core::result::Result<_, $crate::error::Error> {
+ let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+ // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
+ // dereferenced. The compiler also further validates the expression on whether `field`
+ // is a member of `item` when expanded by the macro.
+ unsafe {
+ let ptr_field = ::core::ptr::addr_of_mut!((*item) $(.$field)*);
+ $crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val)
+ }
+ ::core::result::Result::Ok(())
+ })()
};
}
diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 874c2c964afa..1e610545e100 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -54,13 +54,9 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
let ca: CoherentAllocation<MyStruct> =
CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
- || -> Result {
- for (i, value) in TEST_VALUES.into_iter().enumerate() {
- kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
- }
-
- Ok(())
- }()?;
+ for (i, value) in TEST_VALUES.into_iter().enumerate() {
+ kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
+ }
let drvdata = KBox::new(
Self {
@@ -78,13 +74,14 @@ impl Drop for DmaSampleDriver {
fn drop(&mut self) {
dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
- let _ = || -> Result {
- for (i, value) in TEST_VALUES.into_iter().enumerate() {
- assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
- assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
- }
- Ok(())
- }();
+ for (i, value) in TEST_VALUES.into_iter().enumerate() {
+ let val0 = kernel::dma_read!(self.ca[i].h);
+ let val1 = kernel::dma_read!(self.ca[i].b);
+ assert!(val0.is_ok());
+ assert!(val1.is_ok());
+ assert_eq!(val0.unwrap(), value.0);
+ assert_eq!(val1.unwrap(), value.1);
+ }
}
}
--
2.43.0On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote:
> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> index 874c2c964afa..1e610545e100 100644
> --- a/samples/rust/rust_dma.rs
> +++ b/samples/rust/rust_dma.rs
> @@ -54,13 +54,9 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
> let ca: CoherentAllocation<MyStruct> =
> CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
>
> - || -> Result {
> - for (i, value) in TEST_VALUES.into_iter().enumerate() {
> - kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
> - }
> -
> - Ok(())
> - }()?;
> + for (i, value) in TEST_VALUES.into_iter().enumerate() {
> + kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
> + }
>
> let drvdata = KBox::new(
> Self {
> @@ -78,13 +74,14 @@ impl Drop for DmaSampleDriver {
> fn drop(&mut self) {
> dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
>
> - let _ = || -> Result {
> - for (i, value) in TEST_VALUES.into_iter().enumerate() {
> - assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
> - assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
> - }
> - Ok(())
> - }();
> + for (i, value) in TEST_VALUES.into_iter().enumerate() {
> + let val0 = kernel::dma_read!(self.ca[i].h);
> + let val1 = kernel::dma_read!(self.ca[i].b);
> + assert!(val0.is_ok());
> + assert!(val1.is_ok());
> + assert_eq!(val0.unwrap(), value.0);
> + assert_eq!(val1.unwrap(), value.1);
Maybe use if-let to avoid the unwrap?
if let Ok(val0) = val0 {
assert_eq!(val0, value.0);
}
I know it's a bit pointless, since we know it must be ok, but the educational
message of the example should be to check and not to unwrap, so maybe that's
better.
On Thu Apr 10, 2025 at 1:54 PM CEST, Danilo Krummrich wrote:
> On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote:
>> @@ -78,13 +74,14 @@ impl Drop for DmaSampleDriver {
>> fn drop(&mut self) {
>> dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
>>
>> - let _ = || -> Result {
>> - for (i, value) in TEST_VALUES.into_iter().enumerate() {
>> - assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
>> - assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
>> - }
>> - Ok(())
>> - }();
>> + for (i, value) in TEST_VALUES.into_iter().enumerate() {
>> + let val0 = kernel::dma_read!(self.ca[i].h);
>> + let val1 = kernel::dma_read!(self.ca[i].b);
>> + assert!(val0.is_ok());
>> + assert!(val1.is_ok());
>> + assert_eq!(val0.unwrap(), value.0);
>> + assert_eq!(val1.unwrap(), value.1);
>
> Maybe use if-let to avoid the unwrap?
>
> if let Ok(val0) = val0 {
> assert_eq!(val0, value.0);
> }
>
> I know it's a bit pointless, since we know it must be ok, but the educational
> message of the example should be to check and not to unwrap, so maybe that's
> better.
The if-let will silently ignore any errors, so I don't think that it's
fit for example code either.
---
Cheers,
Benno
On Thu, Apr 10, 2025 at 03:11:01PM +0000, Benno Lossin wrote:
> On Thu Apr 10, 2025 at 1:54 PM CEST, Danilo Krummrich wrote:
> > On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote:
> >> @@ -78,13 +74,14 @@ impl Drop for DmaSampleDriver {
> >> fn drop(&mut self) {
> >> dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
> >>
> >> - let _ = || -> Result {
> >> - for (i, value) in TEST_VALUES.into_iter().enumerate() {
> >> - assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
> >> - assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
> >> - }
> >> - Ok(())
> >> - }();
> >> + for (i, value) in TEST_VALUES.into_iter().enumerate() {
> >> + let val0 = kernel::dma_read!(self.ca[i].h);
> >> + let val1 = kernel::dma_read!(self.ca[i].b);
> >> + assert!(val0.is_ok());
> >> + assert!(val1.is_ok());
> >> + assert_eq!(val0.unwrap(), value.0);
> >> + assert_eq!(val1.unwrap(), value.1);
> >
> > Maybe use if-let to avoid the unwrap?
> >
> > if let Ok(val0) = val0 {
> > assert_eq!(val0, value.0);
> > }
> >
> > I know it's a bit pointless, since we know it must be ok, but the educational
> > message of the example should be to check and not to unwrap, so maybe that's
> > better.
>
> The if-let will silently ignore any errors, so I don't think that it's
> fit for example code either.
Yes, but we still have the assert!() before, so the full sequence would be:
assert!(val0.is_ok());
if let Ok(val0) = val0 {
assert_eq!(val0, value.0);
}
The intention would be to avoid patterns that shouldn't be used in "real" code;
assert!() should be obvious not to use for real code.
On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote: > We could do better here by having the macros return `Result`, > so that we don't have to wrap these calls in a closure for > validation which is confusing. > > Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org> > Suggested-by: Andreas Hindborg <a.hindborg@kernel.org> > Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/ > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org> I think you can drop this and the Suggested-by tag, since Andreas is also a co-author.
© 2016 - 2025 Red Hat, Inc.