Message-Id: <DC7AQ7LH1EA0.2BFROWRN54A4@kernel.org>
Date: Wed, 20 Aug 2025 15:40:36 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "Alice Ryhl" <aliceryhl@...gle.com>, <akpm@...ux-foundation.org>,
 <ojeda@...nel.org>, <alex.gaynor@...il.com>, <boqun.feng@...il.com>,
 <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>,
 <a.hindborg@...nel.org>, <tmgross@...ch.edu>, <abdiel.janulgue@...il.com>,
 <acourbot@...dia.com>, <jgg@...pe.ca>, <lyude@...hat.com>,
 <robin.murphy@....com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] rust: dma: implement DataDirection

On Wed Aug 20, 2025 at 3:17 PM CEST, Daniel Almeida wrote:
>
>
>> On 18 Aug 2025, at 18:03, Danilo Krummrich <dakr@...nel.org> wrote:
>> 
>> On Mon Aug 18, 2025 at 8:47 PM CEST, Alice Ryhl wrote:
>>> with no warnings and build-failure if out-of-bounds.
>> 
>> +/// DMA data direction.
>> +///
>> +/// Corresponds to the C [`enum dma_data_direction`].
>> +///
>> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
>> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
>> +#[repr(u32)]
>> +pub enum DataDirection {
>> +    /// The DMA mapping is for bidirectional data transfer.
>> +    ///
>> +    /// This is used when the buffer can be both read from and written to by the device.
>> +    /// The cache for the corresponding memory region is both flushed and invalidated.
>> +    Bidirectional = Self::const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),
>> +
>> +    /// The DMA mapping is for data transfer from memory to the device (write).
>> +    ///
>> +    /// The CPU has prepared data in the buffer, and the device will read it.
>> +    /// The cache for the corresponding memory region is flushed.
>> +    ToDevice = Self::const_cast(bindings::dma_data_direction_DMA_TO_DEVICE),
>> +
>> +    /// The DMA mapping is for data transfer from the device to memory (read).
>> +    ///
>> +    /// The device will write data into the buffer for the CPU to read.
>> +    /// The cache for the corresponding memory region is invalidated before CPU access.
>> +    FromDevice = Self::const_cast(bindings::dma_data_direction_DMA_FROM_DEVICE),
>> +
>> +    /// The DMA mapping is not for data transfer.
>> +    ///
>> +    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
>> +    /// will not perform any cache coherency operations.
>> +    None = Self::const_cast(bindings::dma_data_direction_DMA_NONE),
>> +}
>> +
>> +impl DataDirection {
>> +    /// Casts the bindgen-generated enum type to a `u32` at compile time.
>> +    ///
>> +    /// This function will cause a compile-time error if the underlying value of the
>> +    /// C enum is out of bounds for `u32`.
>> +    const fn const_cast(val: bindings::dma_data_direction) -> u32 {
>
> This should be its own generic helper for similar enums, IMHO

In general, I agree, but it may not be exactly straight forward (considering
that it still needs to work from const context).

The function relies on the fact that both the argument and return type are
numeric primitives, which need to fit into an i128. We could probably define a
marker trait for such types, etc.

Yet, I don't know how we could do the casts from one generic type I to another
generic type O. I think we'd need to implement traits for that as well. However,
this would require the unstable const_trait_impl feature.

Hence, working this out (if even possible currently) and improving existing enum
abstractions should be done independent from this series.

>> +        // CAST: The C standard allows compilers to choose different integer types for enums.
>> +        // To safely check the value, we cast it to a wide signed integer type (`i128`)
>> +        // which can hold any standard C integer enum type without truncation.
>> +        let wide_val = val as i128;
>> +
>> +        // Check if the value is outside the valid range for the target type `u32`.
>> +        // CAST: `u32::MAX` is cast to `i128` to match the type of `wide_val` for the comparison.
>> +        if wide_val < 0 || wide_val > u32::MAX as i128 {
>> +            // Trigger a compile-time error in a const context.
>> +            panic!("C enum value is out of bounds for the target type `u32`.");
>> +        }
>> +
>> +        // CAST: This cast is valid because the check above guarantees that `wide_val`
>> +        // is within the representable range of `u32`.
>> +        wide_val as u32
>> +    }
>> +}
>> +
>> +impl From<DataDirection> for bindings::dma_data_direction {
>> +    /// Returns the raw representation of [`enum dma_data_direction`].
>> +    fn from(direction: DataDirection) -> Self {
>> +        // CAST: `direction as u32` gets the underlying representation of our `#[repr(u32)]` enum.
>> +        // The subsequent cast to `Self` (the bindgen type) assumes the C enum is compatible
>> +        // with the enum variants of `DataDirection`, which is a valid assumption given our
>> +        // compile-time checks.
>> +        direction as u32 as Self
>> +    }
>> +}

Powered by blists - more mailing lists