Re: [PATCH 2/3] rust: gpio: Add basic consumer abstractions

[Kohei Ito]

Hi Alexandre,

thank you for your review.

> > Due to a bindgen issue that may generate the wrong type for enum types,
> > `gpio/consumer.h` is included at the top of `bindings_helper.h` as a
> > temporary workaround. Once the issue is resolved, it can be moved back
> > to its proper alphabetical position.
> 
> Can you describe what the issue is, and share any relevant link?

`bindgen` can generate the wrong type for the `enum`s when their
forward declarations appear before the actual definitions. The details
are described in [1].

I haven't confirmed that `gpiod_flags` is actually affected. I placed
the include at the top as a precaution, but if it isn't needed I'll drop
the workaround.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8cbc95f983bcec7e042266766ffe0d68980e4290

> > +/// The GPIO descriptor flags to configure its direction and output value.
> > +///
> > +/// Rust abstraction for the C [`enum gpiod_flags`].
> > +///
> > +/// They can be combined with the operators `|`, and `&`.
> 
> The C comment for `gpiod_flags` says "these values cannot be OR'd" so I
> guess this comment isn't true. Besides, there is no `BitOr` impl for
> `GpiodFlags` in the patch so it actually cannot be done.

Good catch! I'll correct it.

> > +    // Always inline to optimize out error path of `build_assert`.
> > +    #[inline(always)]
> > +    const fn new(value: bindings::gpiod_flags) -> Self {
> > +        build_assert!(value as u64 <= bindings::gpiod_flags::MAX as u64);
> 
> Better to not use `build_assert` here as it inserts build-time
> landmines.
> 
> Since you are only using this to build the constants above, you can just
> do `Self(bindings::gpiod_flags_*)` on them. Adding an extra assert for
> an bounded enum type doesn't add any extra protection.

Sure. I'll remove it.


> > +/// A reference-counted gpio descriptor.
> 
> Not really - the GPIO device is reference-counted, but descriptors are
> not. Calling `gpiod_get` a second time returns `EBUSY`.

Sure. I'll correct it.

> > +/// ```
> > +/// use crate::{
> 
> These doctests won't compile as they are supposed to use `kernel::`, not
> `crate::`.
> 
> Please make sure to include the doctests when building
> (`CONFIG_RUST_KERNEL_DOCTESTS` build option), and to also build the
> `rustdoc` target as per the checklist [1].
> 
> [1] https://rust-for-linux.com/contributing#submit-checklist-addendum

Sure. I'll fix it and make sure to run the doctests and build rustdoc.

> > +// SAFETY: It is safe to call `gpiod_put` on another thread than where `gpiod_get` was called.
> > +unsafe impl Send for GpioDesc {}
> 
> We should probably also implement `Sync` so GPIOs can be used in
> interrupt context.

I agree we want `Sync` so that GPIOs can be used from interrupt context.
However, the direction setters are not safe to call concurrently on the
same descriptor: gpiolib changes the hardware direction and then updates
`GPIOD_FLAG_IS_OUT`, so the two can become inconsistent.

I think we can add `Sync` if the direction setters take `&mut self` (or
use the typestate approach). I'll look into this together with the
typestate design.

> > +
> > +impl GpioDesc {
> > +    /// Gets [`GpioDesc`] corresponding to a [`Device`] and a connection id.
> > +    ///
> > +    /// Equivalent to the kernel's [`gpiod_get`] API.
> > +    ///
> > +    /// [`gpiod_get`]: https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_get
> > +    pub fn get(dev: &Device, name: Option<&CStr>, flags: GpiodFlags) -> Result<Self> {
> 
> `dev` here is only used as a lookup key, and the GPIO descriptor can
> outlive the device being unbound (the GPIO can actually even be obtained
> while the device is unbound!). This is because `dev` is not the provider
> of the GPIO, but as the API name implies its consumer - i.e. the device
> on which the GPIO is expected to have an effect.
> 
> This is what the GPIO API expects, but it looks a bit counterintuitive
> when compared to most other Rust subsystems, where an obtained resource
> is typically tied to the device given as parameter being bound. I think
> it's worth mentioning in the comment.

Sure. I'll add a note explaining how `dev` is used.

> > +    /// Get the direction.
> > +    ///
> > +    /// Equivalent to the kernel's [`gpiod_get_direction`] API.
> > +    ///
> > +    /// [`gpiod_get_direction`]:
> > +    /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_get_direction
> > +    #[inline]
> > +    pub fn get_direction(&self) -> Result<LineDirection> {
> > +        // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> > +        // [`gpiod_get_direction`].
> > +        let ret = unsafe { bindings::gpiod_get_direction(self.as_raw()) };
> > +        if ret < 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            LineDirection::try_from(ret)
> > +        }
> > +    }
> 
> IIUC the direction of a GPIO at a given point in the code is always
> statically known, and only a subset of the API really make sense for a
> given direction (e.g. `gpiod_set_raw_value_commit` returns `EPERM` if
> the direction is not output). So this is a prime candidate for using the
> typestate pattern to store the direction in the type.
> 
> I.e. you would have `GpioDesc<Input>`, `GpioDesc<Output>`, and changing
> the direction would consume the descriptor and return the new one with
> the requested direction.
> 
> The regulator Rust API makes use of this pattern, you can check it out
> for an example if needed.

I haven't fully grasped the idea yet. I'll check the regulator Rust API
implementation and explore a typestate implementation for GPIO consumer
APIs.

> > +    /// Test whether the GPIO is active-low or not.
> > +    ///
> > +    /// Equivalent to the kernel's [`gpiod_is_active_low`] API.
> > +    ///
> > +    /// [`gpiod_is_active_low`]:
> > +    /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_is_active_low
> > +    #[inline]
> > +    pub fn is_active_low(&self) -> Result<bool> {
> > +        // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> > +        // [`gpiod_is_active_low`].
> > +        match unsafe { bindings::gpiod_is_active_low(self.as_raw()) } {
> > +            0 => Ok(false),
> > +            1 => Ok(true),
> > +            err => Err(Error::from_errno(err)),
> > +        }
> 
> In C this function cannot fail for a valid descriptor, so the Rust one
> shouldn't either. Anything != 0 can be considered `true`.

Sure. I'll correct it for `GpioDesc`. Should we return Result<bool> for
OptionalGpioDesc? As I understand, NULL GPIO descriptors can't return
the right state.

> > +    /// Report whether gpio value access may sleep or not.
> > +    ///
> > +    /// Equivalent to the kernel's [`gpiod_cansleep`] API.
> > +    ///
> > +    /// [`gpiod_cansleep`]:
> > +    /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_cansleep
> > +    #[inline]
> > +    pub fn cansleep(&self) -> Result<bool> {
> > +        // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> > +        // [`gpiod_cansleep`].
> > +        match unsafe { bindings::gpiod_cansleep(self.as_raw()) } {
> > +            0 => Ok(false),
> > +            1 => Ok(true),
> > +            err => Err(Error::from_errno(err)),
> > +        }
> > +    }
> 
> Same here.

Sure. I'll fix it in the same way.

> Also, as a general guideline, it is good to have a concrete user for new
> Rust abstractions. Do you have a project that will make use of this?

No, I don't have a specific project that will use this. My motivation
is that Rust drivers currently have no way to use GPIO lines, so I
expect that providing a basic set of consumer APIs would make it easier
for such drivers to appear.

That said, I understand the concern about adding APIs without actual
users. If you think it should wait until there is a concrete user,
please let me know.

Best regards,
Kohei Ito