Re: [PATCH 2/4] rust: dma: generalize `dma_{read,write}` macro

[Gary Guo <gary@garyguo.net>]

On 2026-02-14 10:04, Benno Lossin wrote:
> On Sat Feb 14, 2026 at 6:33 AM CET, Gary Guo wrote:
>> +    (@parse [$dma:expr] [$($proj:tt)*] [, $val:expr]) => {
>> +        let ptr = $crate::project_pointer!(
>> +            mut $crate::dma::CoherentAllocation::as_mut_ptr(&$dma), $($proj)*
>> +        );
>> +        let val = $val;
>> +        // SAFETY: pointer created by projection is within DMA region.
>> +        unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, ptr, val) }
> 
> This evaluates `$dma` for a second time (and also places it inside an
> `unsafe` block).

Ah good point. The macro that we have today put `$val` inside unsafe and I've spotted and
lifted it out, but I didn't spot the `$dma` part.

> 
>> +    };
> 
> Missing surrounding `{}` to allow this in expression position?

Yeah, I also spotted this myself after sending the series out.

> 
>> +    (@parse [$dma:expr] [$($proj:tt)*] [.$field:tt $($rest:tt)*]) => {
>> +        $crate::dma_write!(@parse [$dma] [$($proj)* .$field] [$($rest)*])
>> +    };
>> +    (@parse [$dma:expr] [$($proj:tt)*] [[$index:expr]? $($rest:tt)*]) => {
>> +        $crate::dma_write!(@parse [$dma] [$($proj)* [$index]?] [$($rest)*])
>> +    };
>> +    (@parse [$dma:expr] [$($proj:tt)*] [[$index:expr] $($rest:tt)*]) => {
>> +        $crate::dma_write!(@parse [$dma] [$($proj)* [$index]] [$($rest)*])
>> +    };
>> +    ($dma:expr, $($rest:tt)*) => {
>> +        $crate::dma_write!(@parse [$dma] [] [$($rest)*])
>>      };
> 
> I'm wondering if this also works:
>     
>     ($dma:expr, $($(.$field:ident)? $([$index:expr])?)*, $val:expr) => {{
>         let dma = &$dma;
>         let ptr = $crate::project_pointer!(
>             mut $crate::dma::CoherentAllocation::as_mut_ptr(dma),
>             $($(.$field)? $([$index])?)*,
>         );
>         let val = $val;
>         // SAFETY: pointer created by projection is within DMA region.
>         unsafe { $crate::dma::CoherentAllocation::field_write(dma, ptr, val) }
>     }}

Rust would complain that the outer repetition can match empty token tree.

> 
>>  }
>> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
>> index 9c45851c876e..b772ada2c65c 100644
>> --- a/samples/rust/rust_dma.rs
>> +++ b/samples/rust/rust_dma.rs
>> @@ -68,7 +68,7 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
>>                  CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
>>  
>>              for (i, value) in TEST_VALUES.into_iter().enumerate() {
>> -                kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
>> +                kernel::dma_write!(ca, [i]?, MyStruct::new(value.0, value.1));
>>              }
>>  
>>              let size = 4 * page::PAGE_SIZE;
>> @@ -91,17 +91,17 @@ fn drop(self: Pin<&mut Self>) {
>>          dev_info!(self.pdev, "Unload DMA test driver.\n");
>>  
>>          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());
>> +            let result = (|| -> Result<_> {
>> +                let val0 = kernel::dma_read!(self.ca, [i]?.h);
>> +                let val1 = kernel::dma_read!(self.ca, [i]?.b);
>>  
>> -            if let Ok(val0) = val0 {
>>                  assert_eq!(val0, value.0);
>> -            }
>> -            if let Ok(val1) = val1 {
>>                  assert_eq!(val1, value.1);
>> -            }
>> +
>> +                Ok(())
>> +            })();
> 
> I dislike that we have to reintroduce the budget-try block here. Ideally
> we could add something like `try` at the beginning of the macro and then
> automatically add the try block. Feel free to make that a future series.

I don't think this is an issue. It's visible inside the samples because
we are testing the values, but in practice most users would propagate the
errors out.

I also dislike that the budget-try block that we have inside `dma_read!`
currently hard-codes the error type.

Best,
Gary

> 
> Cheers,
> Benno
> 
>> +
>> +            assert!(result.is_ok());
>>          }
>>  
>>          for (i, entry) in self.sgt.iter().enumerate() {