Re: [TECH TOPIC] Rust for Linux

[Wedson Almeida Filho <wedsonaf@google.com>]

On Mon, Jul 19, 2021 at 03:53:31PM +0200, Linus Walleij wrote:
> On Mon, Jul 19, 2021 at 2:24 PM Wedson Almeida Filho
> <wedsonaf@google.com> wrote:
> 
> > 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
> 
> My first impression is that this looks very good compared to other Rust
> code I have seen, and it looks very intuitive and maintainable.

Thanks for taking to time to check it out.

> use kernel::{
>     amba, bit, declare_id_table, device, gpio,
>     io_mem::IoMem,
>     irq::{self, IrqData, LockedIrqData},
>     power,
>     prelude::*,
>     sync::{IrqDisableSpinLock, Ref},
> };
> 
> I think people want to see the code inside these classes as well,
> can we browse it? 

The whole tree is available here: https://github.com/wedsonaf/linux/tree/pl061
-- I should caution you that the two most recent commits are not merged into the
rust tree yet because they're WIP. (I'll clean them up and eventually merge
after the feedback from you all.)

> Especially the gpio class in my case,
> because i want to know what happens when you do
> impl gpio::Chip for PL061Device { ... } I suppose it is just
> a C wrapper to the struct gpio_chip?

`impl gpio::Chip for PL061Device` is just saying that the functions within that
block conform to the ones in `gpio::Chip`. The C wrapper glue is really in
`gpio::Registration::register_with_irq`, called in `probe`: it calls the C
functions and registers "trampolines" that C code can call that routes those to
type-safe Rust code.

> I've got questions but they are mostly due to my lack of understanding
> of Rust idioms I think. For example:
> 
>     fn unmask(data: &Ref<DeviceData>, irq_data: &IrqData) {
>         let mask = bit(irq_data.hwirq() % u64::from(PL061_GPIO_NR));
>         let _guard = data.lock();
>         if let Some(pl061) = data.resources() {
>             let gpioie = pl061.base.readb(GPIOIE) | mask;
>             let _ = pl061.base.try_writeb(gpioie, GPIOIE);
>         }
>     }
> 
> I suppose _guard will be unlocked automatically when we leave
> the function? It's a bit like with Make, it is necessary to know
> what is implicit.

Correct, `guard` will unlock the spinlock automatically when it goes out of
scope. Also note that `pl061` is also a guard, it will unlock RCU read side when
it goes out of scope.

> Then the "_" variable is some kind of idiom for a throw-away
> variable I guess. Can it be named "foo" or is the "_" just specially
> magic?

You can choose any name, however the compiler will issue a warning for an
unused variable. Prefixing it with an underscore silences the warning.

Ordinarily this isn't a problem because you'd access some variable explicitly 
protected by the lock, which isn't the case here.

> It looks like stuff we can even maintain superficially without
> being super versed in Rust. Simple things can be altered
> without knowing exactly how the language works.
> 
> > (I tested iton QEMU through the sysfs interface and also gpio-keys as QEMU
> > uses one of the PL061 pins as the power button.)
> 
> Fair enough.
> 
> > 1. State created on `probe` is ref-counted.
> > 2. Hardware resources (device mem and irq in this case) are "revocable".
> > 3. On `remove`, we automatically revoke access to hardware resources, then free
> > them.
> 
> This is neat. Just generally speaking I like the looks of this
> from a driver subsystem point of view. Unless the code in the
> Kernel::classes is horrible that is.

Once you've had a chance to browse through the code, let me know if there are
concerns.

Cheers,
-Wedson