As suggested by Andreas Hindborg, 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.
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
rust/kernel/dma.rs | 54 +++++++++++++++++++++++-----------------
samples/rust/rust_dma.rs | 21 ++++++----------
2 files changed, 38 insertions(+), 37 deletions(-)
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index d3f448868457..24a6f10370c4 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)
- }
+ (|| -> Result<_> {
+ 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) }
+ (|| -> Result {
+ 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)
- }
+ (|| -> Result {
+ 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 908acd34b8db..cc09d49f2056 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -54,13 +54,9 @@ fn probe(pdev: &mut pci::Device, _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,10 @@ 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() {
+ assert_eq!(kernel::dma_read!(self.ca[i].h).unwrap(), value.0);
+ assert_eq!(kernel::dma_read!(self.ca[i].b).unwrap(), value.1);
+ }
}
}
--
2.43.0"Abdiel Janulgue" <abdiel.janulgue@gmail.com> writes: > As suggested by Andreas Hindborg, 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. > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org> Best regards, Andreas Hindborg
On Wed Mar 26, 2025 at 9:11 PM CET, Abdiel Janulgue wrote:
> As suggested by Andreas Hindborg, 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.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
> rust/kernel/dma.rs | 54 +++++++++++++++++++++++-----------------
> samples/rust/rust_dma.rs | 21 ++++++----------
> 2 files changed, 38 insertions(+), 37 deletions(-)
>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index d3f448868457..24a6f10370c4 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)
> - }
> + (|| -> Result<_> {
Please use `::core::result::Result<_, _>` instead. If someone uses this
macro in a place with a different `Result` than the one from the kernel
crate, then this will result in a compile error. (also in the instances
below)
You might want to use `::core::result::Result<_, $crate::error::Error>`
instead though if the type inference can't figure out the error type.
> + 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)*)
> };
> }
>
> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> index 908acd34b8db..cc09d49f2056 100644
> --- a/samples/rust/rust_dma.rs
> +++ b/samples/rust/rust_dma.rs
> @@ -54,13 +54,9 @@ fn probe(pdev: &mut pci::Device, _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,10 @@ 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() {
> + assert_eq!(kernel::dma_read!(self.ca[i].h).unwrap(), value.0);
> + assert_eq!(kernel::dma_read!(self.ca[i].b).unwrap(), value.1);
> + }
This changes the behavior from before: now an error will result in a
panic where before it was just ignored. Not sure what to do here since
it's a sample, but if you intend the functional change, I would mention
it in the commit message.
---
Cheers,
Benno
> }
> }
>
"Benno Lossin" <benno.lossin@proton.me> writes:
> On Wed Mar 26, 2025 at 9:11 PM CET, Abdiel Janulgue wrote:
>> As suggested by Andreas Hindborg, 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.
>>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>> ---
>> rust/kernel/dma.rs | 54 +++++++++++++++++++++++-----------------
>> samples/rust/rust_dma.rs | 21 ++++++----------
>> 2 files changed, 38 insertions(+), 37 deletions(-)
>>
>> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
>> index d3f448868457..24a6f10370c4 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)
>> - }
>> + (|| -> Result<_> {
>
> Please use `::core::result::Result<_, _>` instead. If someone uses this
> macro in a place with a different `Result` than the one from the kernel
> crate, then this will result in a compile error. (also in the instances
> below)
>
> You might want to use `::core::result::Result<_, $crate::error::Error>`
> instead though if the type inference can't figure out the error type.
>
>> + 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)*)
>> };
>> }
>>
>
>> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
>> index 908acd34b8db..cc09d49f2056 100644
>> --- a/samples/rust/rust_dma.rs
>> +++ b/samples/rust/rust_dma.rs
>> @@ -54,13 +54,9 @@ fn probe(pdev: &mut pci::Device, _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,10 @@ 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() {
>> + assert_eq!(kernel::dma_read!(self.ca[i].h).unwrap(), value.0);
>> + assert_eq!(kernel::dma_read!(self.ca[i].b).unwrap(), value.1);
>> + }
>
> This changes the behavior from before: now an error will result in a
> panic where before it was just ignored. Not sure what to do here since
> it's a sample, but if you intend the functional change, I would mention
> it in the commit message.
There is two sides to this. If we want this as a nice example that
people should copy in their drivers, using unwrap is bad. But for
testing and demonstration purposes, I think the unwrap is mandated.
But the `assert_eq!` would panic anyway if comparison fails, right?
Best regards,
Andreas Hindborg
On Tue, Apr 8, 2025 at 2:40 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> But the `assert_eq!` would panic anyway if comparison fails, right?
Previously the `?` generated by the macro would return out of the
closure written by the sample, and thus it wouldn't reach the
`assert_eq!`.
Expanded:
let _ = (|| -> Result {
for (i, value) in TEST_VALUES.into_iter().enumerate() {
match (
&{
let item = ...::item_from_index(&self.ca, i)?;
unsafe { ... }
},
&value.0,
) {
(left_val, right_val) => {
if !(*left_val == *right_val) {
...::assert_failed(...);
}
}
};
}
Ok(())
})();
Cheers,
Miguel
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes: > On Tue, Apr 8, 2025 at 2:40 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> But the `assert_eq!` would panic anyway if comparison fails, right? > > Previously the `?` generated by the macro would return out of the > closure written by the sample, and thus it wouldn't reach the > `assert_eq!`. Right, I see. So the question is whether we want to have the assert panic here or not, of we get an Err. I vote yes. Best regards, Andreas Hindborg
On Tue Apr 8, 2025 at 9:46 PM CEST, Andreas Hindborg wrote: > "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes: >> On Tue, Apr 8, 2025 at 2:40 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: >>> >>> But the `assert_eq!` would panic anyway if comparison fails, right? >> >> Previously the `?` generated by the macro would return out of the >> closure written by the sample, and thus it wouldn't reach the >> `assert_eq!`. > > Right, I see. So the question is whether we want to have the assert > panic here or not, of we get an Err. I vote yes. The assert wouldn't be the source of the panic though, it would be the `.unwrap()`, but I think it's better to report the error. Although I think it would be nicer if the example could use better error handling, but this code is in a `drop` function, so no way to bubble up a result... --- Cheers, Benno
On Tue, Apr 08, 2025 at 09:54:13PM +0000, Benno Lossin wrote: > On Tue Apr 8, 2025 at 9:46 PM CEST, Andreas Hindborg wrote: > > "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes: > >> On Tue, Apr 8, 2025 at 2:40 PM Andreas Hindborg <a.hindborg@kernel.org> wrote: > >>> > >>> But the `assert_eq!` would panic anyway if comparison fails, right? > >> > >> Previously the `?` generated by the macro would return out of the > >> closure written by the sample, and thus it wouldn't reach the > >> `assert_eq!`. > > > > Right, I see. So the question is whether we want to have the assert > > panic here or not, of we get an Err. I vote yes. > > The assert wouldn't be the source of the panic though, it would be the > `.unwrap()`, but I think it's better to report the error. Although I > think it would be nicer if the example could use better error handling, > but this code is in a `drop` function, so no way to bubble up a > result... We could be more explicit and assert is_ok() instead and subsequently do the assert_eq.
On Wed, Mar 26, 2025 at 9:13 PM Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote: > > As suggested by Andreas Hindborg, 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. > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com> I would suggest: Suggested-by: Andreas Hindborg <a.hindborg@kernel.org> Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/ Maybe also consider if Co-developed-by etc. makes sense since he provided the diff. Thanks! Cheers, Miguel
On 26/03/2025 22:48, Miguel Ojeda wrote: > On Wed, Mar 26, 2025 at 9:13 PM Abdiel Janulgue > <abdiel.janulgue@gmail.com> wrote: >> >> As suggested by Andreas Hindborg, 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. >> >> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com> > > I would suggest: > > Suggested-by: Andreas Hindborg <a.hindborg@kernel.org> > Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/ > > Maybe also consider if Co-developed-by etc. makes sense since he > provided the diff. Thanks! Andreas, would you be okay if I add the Co-developed-by tag? Regards, Abdiel
"Abdiel Janulgue" <abdiel.janulgue@gmail.com> writes: > On 26/03/2025 22:48, Miguel Ojeda wrote: >> On Wed, Mar 26, 2025 at 9:13 PM Abdiel Janulgue >> <abdiel.janulgue@gmail.com> wrote: >>> >>> As suggested by Andreas Hindborg, 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. >>> >>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com> >> >> I would suggest: >> >> Suggested-by: Andreas Hindborg <a.hindborg@kernel.org> >> Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/ >> >> Maybe also consider if Co-developed-by etc. makes sense since he >> provided the diff. > > Thanks! Andreas, would you be okay if I add the Co-developed-by tag? Yes, fine with me 👍 Best regards, Andreas Hindborg
© 2016 - 2025 Red Hat, Inc.