Re: [TECH TOPIC] Rust for Linux

Laurent Pinchart
2021-07-19T19:38:05Z

On Mon, Jul 19, 2021 at 07:06:36PM +0100, Wedson Almeida Filho wrote:
> 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.

If the driver reached a code path with an I/O write after .remove()
returns, the game is likely over already. It would be more interesting
to see how we could prevent that from happening in the first place.
Checking individual I/O writes at runtime will not only add additional
CPU costs, but will also produce code paths that are not well tested. It
feels that we're inventing a problem just to be able to showcase the
solution :-)

> > 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.

--
Regards,

Laurent Pinchart