Re: [RFC PATCH v5 2/3] rust: add minimal IIO subsystem abstractions

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 22 Aug 2026 14:26:57 +0800
Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com> wrote:

> Add safe Rust wrappers for the Linux IIO (Industrial I/O) subsystem:
> 
> - IioChanInfo enum wrapping iio_chan_info_enum, with TryFrom<u32> for
>   type-safe dispatch in read_raw. The compiler enforces match
>   exhaustiveness, replacing the previous raw isize approach.
> - IioVal enum with NonZeroI32 for division-by-zero prevention on
>   IIO_VAL_FRACTIONAL.
> - IioDriver trait with read_raw callback (requires Send + Sync).
> - Device<T, State> with typestate (Unregistered -> Registered) to
>   prevent double-registration at compile time.
> - PinnedDrop for guaranteed cleanup sequence:
>     iio_device_unregister -> drop_in_place(T) -> iio_device_free
>   iio_device_unregister() calls cdev_device_del() which drains the
>   kernfs workqueue before returning. All in-flight read_raw callbacks
>   (which go through kernfs sysfs reads) complete before drop_in_place
>   proceeds. This covers the sysfs read path used by this driver.
> - Compile-time const VTABLE (iio_info).
> - C-to-Rust FFI trampoline for read_raw dispatch.
> 
> The abstraction uses iio_device_alloc (not devm_*) so that the Rust
> Drop implementation has full control over the cleanup sequence.
> Module ownership is enforced via __iio_device_register(indio_dev, module).
> 
> Signed-off-by: Muchamad Coirul Anwar <muchamadcoirulanwar@gmail.com>

Hi Muchamad

This looks fine to me subject to a few little things - see inline.

However I didn't take the time to decode every line of rust today so there were bits
I simply didn't understand yet.  So for this to be able to move forward I'm going
to need reviews from rust experts!

Jonathan

> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index a56ba6309594..5dc917d92151 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -86,6 +86,7 @@ macro_rules! declare_err {
>      declare_err!(EIOCBQUEUED, "iocb queued, will get completion event.");
>      declare_err!(ERECALLCONFLICT, "Conflict with recalled state.");
>      declare_err!(ENOGRACE, "NFS file lock reclaim refused.");
> +    declare_err!(ENODATA, "No data available.");

Do we have something says there must be a user in the same patch?
A really generic thing like this in C would definitely be a patch on its
own so that folk who care about maintaining a given file can easily see
it without reviewing the rest of the series.

So unless you can't do otherwise, break this out as a precursor patch.

>  }
>  
>  /// Generic integer kernel error.
> diff --git a/rust/kernel/iio.rs b/rust/kernel/iio.rs
> new file mode 100644
> index 000000000000..f1638160fed1
> --- /dev/null
> +++ b/rust/kernel/iio.rs
...

> +
> +build_iio_enum! {
> +    /// Raw unprocessed value from the channel (`IIO_CHAN_INFO_RAW`).
> +    ///
> +    /// For sensors, this is typically the ADC reading or register value
> +    /// before any scaling or offset correction.
> +    Raw = iio_chan_info_enum_IIO_CHAN_INFO_RAW,

I guess there may be a rust convention for this but from a human trying to
read the code point of view this need a blank line here and in similar places
where you have docs / thing documented repeated back to back.

> +    /// Scale factor to convert raw values to SI units (`IIO_CHAN_INFO_SCALE`).
> +    ///
> +    /// The processed value is `raw * scale`. The unit depends on the channel
> +    /// type (e.g. V for voltage, m/s² for acceleration, rad for angle).
> +    Scale = iio_chan_info_enum_IIO_CHAN_INFO_SCALE,
> +}


> +
> +/// C-compatible trampoline for the `iio_info.read_raw` callback.
> +///
> +/// # Safety
> +///
> +/// This function is only called by the IIO core via the `read_raw` function
> +/// pointer in `iio_info`. The IIO core guarantees:
> +/// - `indio_dev` is a valid `iio_dev` allocated by `iio_device_alloc`.
> +/// - `chan` points to a valid channel spec from the device's channel array.
> +/// - `val` is a valid non-null pointer to a writable `int`.
> +/// - `val2` is a valid non-null pointer to a writable `int`. The IIO core
> +///   always passes stack-allocated storage for both, regardless of whether
> +///   the driver uses `val2` (e.g. `IIO_VAL_INT` only writes `val`; `val2`

That val2 is always a valid pointer smells a bit like the c interface leaking
into the rust.  I'm not necessarily against that being a constraint we take
on but I'm not sure how we document that. Probably add something to the C docs.
Any C driver relying on this today is probably buggy for other reasons.


> +///   is provided but left unread by the caller for that return type).
> +unsafe extern "C" fn read_raw_callback<T: IioDriver>(
> +    indio_dev: *mut iio_dev,
> +    chan: *const iio_chan_spec,
> +    val: *mut c_int,
> +    val2: *mut c_int,
> +    info: isize,
> +) -> c_int {
> +    // SAFETY: `indio_dev` is valid and was allocated with space for `T` in its
> +    // private data area. The `priv_` field was initialized in `Device::build_device()`.
> +    let priv_ptr = unsafe { (*indio_dev).priv_ as *mut T };
> +    // SAFETY: `priv_ptr` points to a valid, initialized instance of `T` that
> +    // lives as long as the `iio_dev` allocation.
> +    let driver = unsafe { &*priv_ptr };
> +
> +    let info_enum = match IioChanInfo::try_from(info as u32) {
> +        Ok(valid) => valid,
> +        Err(e) => return e.to_errno(),
> +    };
> +
> +    match driver.read_raw(chan, info_enum) {
> +        Ok(IioVal::Int(v)) => {
> +            // SAFETY: `val` is valid per the function's Safety contract above.
> +            // `val2` is not written; `IIO_VAL_INT` signals to the IIO core
> +            // that only `val` carries meaningful data.
> +            unsafe {
> +                *val = v;
> +            }
> +            IIO_VAL_INT
> +        }
> +        Ok(IioVal::Fractional(v, v2)) => {
> +            // SAFETY: both `val` and `val2` are valid per the Safety contract.
> +            unsafe {
> +                *val = v;
> +                *val2 = v2.get();

Why get in some places and not others?  May well be a gap in my really limited
rust knowledge.

> +            }
> +            IIO_VAL_FRACTIONAL
> +        }
> +        Ok(IioVal::IntPlusMicro(v, v2)) => {
> +            // SAFETY: both `val` and `val2` are valid per the Safety contract.
> +            unsafe {
> +                *val = v;
> +                *val2 = v2;
> +            }
> +            IIO_VAL_INT_PLUS_MICRO
> +        }
> +        Ok(IioVal::IntPlusNano(v, v2)) => {
> +            // SAFETY: both `val` and `val2` are valid per the Safety contract.
> +            unsafe {
> +                *val = v;
> +                *val2 = v2;
> +            }
> +            IIO_VAL_INT_PLUS_NANO
> +        }
> +        Err(e) => e.to_errno(),
> +    }
> +}