Re: [TECH TOPIC] Rust for Linux

Wedson Almeida Filho
2021-07-19T18:06:42Z

On Mon, Jul 19, 2021 at 06:02:06PM +0200, Vegard Nossum wrote:
>
> On 7/19/21 3:15 PM, Wedson Almeida Filho wrote:
> > On Mon, Jul 19, 2021 at 01:24:49PM +0100, Wedson Almeida Filho wrote:
> >> On Fri, Jul 09, 2021 at 12:13:25AM +0200, Linus Walleij wrote:
> >>> I have seen that QEMU has a piece of code for the Arm PrimeCell
> >>> PL061 GPIO block which corresponds to drivers/gpio/gpio-pl061.c
> >>> Note that this hardware apart from being used in all Arm reference
> >>> designs is used on ARMv4T systems that are not supported by
> >>> LLVM but only GCC, which might complicate things.
> >>
> >> Here is a working PL061 driver in Rust (converted form the C one):
> >> https://raw.githubusercontent.com/wedsonaf/linux/pl061/drivers/gpio/gpio_pl061_rust.rs
> >
> > I'm also attaching an html rending of the C and Rust versions side by side where
> > I try to line the definitions up to make it easier to contrast the two
> > implementations.
> >
>
> This is really cool :-) As a Rust noob, I have a few questions:
>
> 1. I'm curious about some of the writeb() vs. try_writeb() calls:
>
> fn direction_output(data: &Ref<DeviceData>, offset: u32, value: bool) ->
> Result {
> let woffset = bit(offset + 2).into();
> let _guard = data.lock();
> let pl061 = data.resources().ok_or(Error::ENXIO)?;
> pl061.base.try_writeb((value as u8) << offset, woffset)?;
> let mut gpiodir = pl061.base.readb(GPIODIR);
> gpiodir |= bit(offset);
> pl061.base.writeb(gpiodir, GPIODIR);
>
> // gpio value is set again, because pl061 doesn't allow to set
> value of a gpio pin before
> // configuring it in OUT mode.
> pl061.base.try_writeb((value as u8) << offset, woffset)?;
> Ok(())
> }
>
> Here you have try_writeb() (and error return) where there was just a
> writeb() without any error handling in the C version. Is this what
> Miguel was answering a bit down the thread where the address is computed
> ((value as u8) << offset) so it _needs_ to use the try_() version?

The `writeb` variant only works when we know at compile-time that the offset is
within bounds (the compiler will reject the code otherwise). When the value is
computed at runtime we use a `try` version that checks before performing the
write. We need this to guarantee memory safety.

> If offset can be anything but a "correct" value here, should there be a
> check for that somewhere else and then the computed value can be
> subsequently treated as safe (i.e. there's a second try_writeb() in the
> function that now presumably does the runtime check a second time,
> redundantly)?

Oh, that's a neat idea. We can certainly implement something like this:

let woffset = pl061.base.vet_offsetb(bit(offset + 2))?;

Then woffset would be passed to writeb variants that are guaranteed to succeed.
(Rust helps us ensure that woffset cannot change without checks, which would be
harder to do in C.)

> 2. In many places you have the C code:
>
> struct pl061 *pl061 = dev_get_drvdata(dev);
>
> with the equivalent Rust code as:
>
> let pl061 = data.resources().ok_or(Error::ENXIO)?;
>
> Why doesn't the C code need to check for errors here? Or put
> differently, why can the Rust version fail?

There are two aspecs worth noting here:
1. In C there is cast from void * to struct pl061 * without really knowing if
the stored pointer is of the right type. For example, if I simply change the
struct type to say `struct mutex` in the code above, it will still compile,
though it will be clearly wrong. In Rust we prevent this by not exposing drvdata
directly to drivers, and using type-specialised functions to set/get drvdata, so
it *knows* that the type is right. So in this sense Rust is better because it
offers type guarantees without additional runtime cost. (In Rust, if you change
the type of the function to say `&Mutex`, it won't compile.

2. The extra check we have here is because of a feature that the C code doesn't
have: revocable resources. If we didn't want to have this, we could do say
`data.base.writeb(...)` directly, but then we could have situations where `base`
is used after the device was removed. By having these checks we guarantee that
anyone can hold a reference to device state, but they can no longer use hw
resources after the device is removed.

> 3. In probe() you have:
>
> data.resources().ok_or(Error::ENXIO)?.base.writeb(0, GPIOIE); // disable
> irqs
> data.registrations()
> .ok_or(Error::ENXIO)?
> .gpio_chip
> .register_with_irq(PL061_GPIO_NR, None, dev, data.clone(), irq)?;
>
> So here, if .register_with_irq() or any of the other ?-marked calls
> fail, then the function returns and the local variable "data" and all
> its members are freed/deallocated using destructors like in C++ RAII?

Yes, everything is freed a la C++ RAII destructors on failure. Additionally, the
same mechanism is used on success when the refcount on `data` goes to zero. No
need for chained goto-based exit paths nor devm-like bookkeeping.

Cheers,
-Wedson