Re: [PATCH v13 2/7] rust: add dma coherent allocator abstraction.

[Danilo Krummrich <dakr@kernel.org>]

On Fri, Mar 21, 2025 at 03:29:01PM -0300, Jason Gunthorpe wrote:
> On Fri, Mar 21, 2025 at 06:34:53PM +0100, Danilo Krummrich wrote:
> > On Fri, Mar 21, 2025 at 02:23:53PM -0300, Jason Gunthorpe wrote:
> > > On Fri, Mar 07, 2025 at 01:06:19PM +0200, Abdiel Janulgue wrote:
> > > 
> > > > +        // SAFETY: Device pointer is guaranteed as valid by the type invariant on `Device`.
> > > > +        let ret = unsafe {
> > > > +            bindings::dma_alloc_attrs(
> > > > +                dev.as_raw(),
> > > > +                size,
> > > > +                &mut dma_handle,
> > > > +                gfp_flags.as_raw(),
> > > > +                dma_attrs.as_raw(),
> > > > +            )
> > > 
> > > This is not the correct safety statement, the device must have a driver
> > > bound to call this function, a struct device reference is not
> > > sufficient.
> > > 
> > > I belive Danilo was suggesting to ignore this unsafety for now, but if
> > > so it should be documented correctly.
> > 
> > If just landed patches [1], which are the foundation of addressing this issue.
> 
> Those patches say:
> 
>  The context types can be extended as required, e.g. to limit availability  of
>  certain (bus) device functions to probe().
> 
> Which is not an appropriate limitation for dma_alloc_coherent, we
> expect it to be called outside probe in real drivers. Is there more to
> that story?

Yeah, we can also use them to derive specifically typed Device instances from
other entry points of the driver where we know for sure that at this point the
device must (still) be bound to the driver.

For instance, bus callbacks, subsystem callbacks, certain (but not all) IOCTLs,
IRQ handlers, etc.

All those cases can be covered by only the type system, without additional
locks. We could even use this as an optimization to bypass Devres'
try_access() calls when holding a corresponding device instance in those places.

> 
> Regardless, the safety comment should not be merged with incorrect
> information. :\

v15 did land in rust-next, so unfortunately this was overlooked. Since you
caught, mind sending a patch improving the comment?