# Meeting 2023-10-25
<!-- Leave your topic starting with ### in the relevant sections below -->

## Critical
<!-- bugs, soundness issues, urgent patches/reviews etc. -->

### How do you do a "data-race-free" load (or even store)?

[Wedson's usage](https://lore.kernel.org/rust-for-linux/[e-mail]/)

other potential [usage](https://github.com/rust-lang/unsafe-code-guidelines/issues/348#issuecomment-1218795242)

Our options:

1. using read_volatile and write_volatile and ignoring the data race warning (later on we try to fix the Rust doc/spec of them)
* Or maybe they actually won't data race with accesses outside Rust? See below
2. proposing the [arch::load, arch::store](https://github.com/rust-lang/unsafe-code-guidelines/issues/321).
3. implementing our own version of read/write with inline asm in Rust
4. using C's READ_ONCE, WRITE_ONCE to implement read/write in Rust.
5. Or we explicitly say, we assume `read_volatile` and `write_volatile` have the same guarantee regarding UB as `READ_ONCE` and `WRITE_ONCE`.

In [doc of `read_volatile`](https://doc.rust-lang.org/std/ptr/fn.read_volatile.html):

> In particular, a race between a read_volatile and any write operation to the same location is undefined behavior.

But maybe "any write operation" actually means "any write operation in Rust (Abstract Machine)", in other words, if a `read_volatile` race with a write in C or by hardware, it's not a UB? One of the reasons that makes me believe this is: if `read_volatile` is used to read a hardware register, there is no possible way for Rust compiler to know whether the hardware will modify the memory or not, therefore compilers won't miss-compile anything (or use any assumption to optimize).

Consensus: Add functions `read_once`/`write_once` that use `{read,write}_volatile` under the hood, but document that explicitly.

Miguel (and others?): also creating an issue somewhere on the Rust project after we have the new functions properly documented or linking it in the proper place. Add it also to the #2 issue.

## Status Reports
<!-- You want to report on something you are working on/request reviews.
Or you want to know the status of something someone else is doing -->

### Safety Standard

I have started on writing the safety standard: https://github.com/y86-dev/linux/tree/safety-standard
It is not yet ready for review (feel free to take a look regardless), but just wanted to inform you guys.

https://github.com/y86-dev/linux/commit/00d1980957976130d59078ebc3082be0a9e46087

## Discussion Questions
<!-- Anything that requires lengthy discussion/more general questions also fit here -->

### `InPlaceModule`

Benno: @Wedson is your patch for `InPlaceModule` ready to be upstreamed? I think it could be used by Tomo's PHY abstractions. See [here](https://lore.kernel.org/rust-for-linux/[e-mail]/).

Gary: We have `impl<T, E> Init<T, E> for T`, if we have `impl<T, E> Init<T, E> for Result<T, E>` then every `Module` is by-default `InPlaceModule`.

Consensus: Wedson to send the patch while continue to explore a more ergonomic way to avoid having two traits.

### `#[vtable]` documentation

What do people think of [this](https://lore.kernel.org/rust-for-linux/[e-mail]/)?:

For a trait method to be optional, it must have a default implementation.
This is also the case for traits annotated with `#[vtable]`, but in this
case the default implementation will never be executed. The reason for this
is that the functions will be called through function pointers installed in
C side vtables. When an optional method is not implemented on a `#[vtable]`
trait, a NULL entry is installed in the vtable. Thus the default
implementation is never called. Since these traits are not designed to be
used on the Rust side, it should not be possible to call the default
implementation. It is not possible to replicate the default behavior from C
in Rust, since that is not maintainable. The default implementaiton should
therefore call `kernel::build_error`, thus preventing calls to this
function at compile time:

Alice: How about?

9,12c9,12
< implementation. It is not possible to replicate the default behavior from C
< in Rust, since that is not maintainable. The default implementaiton should
< therefore call `kernel::build_error`, thus preventing calls to this
< function at compile time:
---
> implementation. This is done to ensure that we call the vtable methods
> through the C vtable, and not through the Rust vtable. Therefore, the
> default implementation should call `kernel::build_error`, which preventing
> calls to this function at compile time:

Consensus: Use Alice's diff.

### Locking Patterns

In the PHY driver review I encountered the following function:

fn write(&mut self, regnum: u16, val: u16) -> Result {
    let phydev = self.0.get();
        // SAFETY: `phydev` is pointing to a valid object by the type
        // invariant of `Self`. So an FFI call with a valid pointer.
        to_result(unsafe {
            bindings::mdiobus_write(
                (*phydev).mdio.bus,
                (*phydev).mdio.addr,
                regnum.into(),
                val
            )
        })
    }
}

Note that this function is called with the `phy_device->lock` held. The `mdiobus_write` function will then also lock the `mdio.bus->mdio_lock`. The question is, should this function be `&self` or `&mut self`? And more generally what do we do with this pattern?

https://lore.kernel.org/rust-for-linux/[e-mail]/

## Miscellaneous
<!-- stuff that does not fit into other categories -->