Re: [PATCH v14 06/11] rust: dma: add dma addressing capabilities

[Danilo Krummrich <dakr@kernel.org>]

On Wed, Mar 12, 2025 at 12:37:45PM +0900, Alexandre Courbot wrote:
> On Wed Mar 12, 2025 at 2:48 AM JST, Abdiel Janulgue wrote:
> > @@ -18,7 +18,35 @@
> >  /// The [`Device`] trait should be implemented by bus specific device representations, where the
> >  /// underlying bus has potential support for DMA, such as [`crate::pci::Device`] or
> >  /// [crate::platform::Device].
> > -pub trait Device: AsRef<device::Device> {}
> > +pub trait Device: AsRef<device::Device> {
> > +    /// Inform the kernel about the device's DMA addressing capabilities.
> > +    ///
> > +    /// Set both the DMA mask and the coherent DMA mask to the same value.
> > +    ///
> > +    /// Note that we don't check the return value from the C `dma_set_coherent_mask` as the DMA API
> > +    /// guarantees that the coherent DMA mask can be set to the same or smaller than the streaming
> > +    /// DMA mask.
> > +    fn dma_set_mask_and_coherent(&mut 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_and_coherent(self.as_ref().as_raw(), mask) };
> > +        if ret != 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(())
> > +        }
> > +    }
> > +
> > +    /// Same as [`Self::dma_set_mask_and_coherent`], but set the mask only for streaming mappings.
> > +    fn dma_set_mask(&mut 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_ref().as_raw(), mask) };
> > +        if ret != 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(())
> > +        }
> > +    }
> > +}
> 
> I'm not quite sure why this trait is needed.
> 
> The default implementations of the methods are not overridden in this
> patchset and I don't see any scenario where they would need to be. Why
> not do it the simple way by just adding an implementation block for
> device::Device?
> 
> This also introduces a `&dyn Device` trait object as parameter to the
> coherent allocation methods - not a big deal, but still inelegant IMHO.

There are mainly two reasons:

(1) The dma_set_mask() function famility modifies the underlying struct device
    without a lock. Therefore, we'd need a mutable reference to the
    device::Device. However, we can't give out a mutable reference to a
    device::Device, since it'd be prone to mem::swap().

    Besides that - and I think that's even more important - we can't claim for a
    generic device::Device in general, that we own it to an extend that we can
    modify unprotected fields.

    However, we can claim this for bus specific devices in probe()[1]. Hence the
    trait, such that the DMA mask functions can be easily derived by bus
    specific devices.

(2) Setting a DMA mask only makes sense for devices that sit on a (potentially)
    DMA capable bus. Having this trait implemented for only the applicable
    device types prevents setting the DMA mask and allocating DMA memory for the
    wrong device.

    This isn't necessary for safety reasons, but I think it's still nice to
    catch.

---

[1] Gotcha with bus specific devices.

Currently bus specific devices are implemented as Device<ARef<device::Device>>.

This was a bad idea and is on my list to fix. As convinient as it is, it is
unsound, since this means that driver can always derive a mutable reference to a
bus specific device.

I think that the upcoming Ownable stuff would be a good solution, but maybe I
should wait any longer and fix it right away without Ownable.