Re: [PATCH v8 07/10] rust: io: introduce `IntoIoVal` trait and single-argument `write_val`

["Alexandre Courbot" <acourbot@nvidia.com>]

On Wed Mar 11, 2026 at 1:34 AM JST, Danilo Krummrich wrote:
> On Mon Mar 9, 2026 at 4:14 PM CET, Alexandre Courbot wrote:
>> +    /// Generic infallible write of value bearing its location, with compile-time bounds check.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// Tuples carrying a location and a value can be used with this method:
>> +    ///
>> +    /// ```no_run
>> +    /// use kernel::io::{Io, Mmio};
>> +    ///
>> +    /// fn do_writes(io: &Mmio) {
>> +    ///     // 32-bit write of value `1` at address `0x10`.
>> +    ///     io.write_val((0x10, 1u32));
>> +    /// }
>> +    /// ```
>> +    #[inline(always)]
>> +    fn write_val<T, L, V>(&self, value: V)
>
> Still not super satisfied with the name write_val() plus I have some more
> considerations, sorry. :)
>
> Let's look at this:
>
> 	let reg = bar.read(regs::NV_PMC_BOOT_0);
>
> 	// Pass around, do some modifications, etc.
>
> 	bar.write_val(reg);
>
> In this case read() returns an object that encodes the absolute location and
> value, i.e. read() pairs with write_val(), which is a bit odd.
>
> In this example:
>
> 	let reg = bar.read(regs::NV_PFALCON_FALCON_RM::of::<E>());
>
> 	// Pass around, do some modifications, etc.
>
> 	bar.write(WithBase::of::<E>(), reg);
>
> read() pairs with write(), since the object returned by read() does only encode
> a relative location, so we have to supply the base location through the first
> argument of write().
>
> Well, technically it also pairs with write_val(), as we could also pass this as
> tuple to write_val().
>
> 	bar.write_val((WithBase::of::<E>(), reg));
>
> However, my point is that this is a bit inconsistent and I think a user would
> expect something more symmetric.
>
> For instance, let's say write() would become the single argument one, then
> read() could return an impl of IntoIoVal, so we would have
>
> 	let reg = bar.read(regs::NV_PMC_BOOT_0);
> 	bar.write(reg);
>
> 	let reg = bar.read(regs::NV_PFALCON_FALCON_RM::of::<E>());
> 	bar.write(reg);
>
> and additionally we can have
>
> 	let val = bar.read_val(regs::NV_PFALCON_FALCON_RM::of::<E>());
> 	bar.write_val(WithBase::of::<E>(), val);
>
> The same goes for
>
> 	let val = bar.read_val(regs::NV_PMC_BOOT_0);
> 	bar.write_val(regs::NV_PMC_BOOT_0, val);
>
> which for complex values does not achieve anything, but makes sense for the FIFO
> register use-case, as val could also be a primitive, e.g. u32.
>
> With a dynamic base, and a single field we could just do the following to
> support such primitives:
>
> 	let val = bar.read_val(regs::NV_PFALCON_FALCON_RM::of::<E>());
> 	bar.write_val(regs::NV_PFALCON_FALCON_RM::of::<E>(), val);
>
> I think adding this symmetry should be trivial, as most of this already compiles
> with the current code.

Not quite, if I understood your idea correctly.

The new `read` would need to return some new type (except for fixed
offset registers) that contains the corresponding `IoLoc`, creating
overhead for the default I/O methods (so at the very least, I think they
should be the ones with the suffix). Accessing the value of that new
type would require some indirection, and probably the return of
closures. And having two pairs of read/write methods that work eerily
similarly but are marginally distinct sounds to me like it could itself
become a source of confusion.

>
> Also, let's not get into a discussion whether we name on or the other write()
> vs.  write_foo(). I just turned it around as I think with the specific name
> write_val() it makes more sense this way around.
>
> But I'm perfectly happy either way as long as we find descriptive names that
> actually make sense.
>
> Unfortunately, I can't really think of a good name for write_val() with its
> current semantics. If we flip it around as in my examples above, the _val()
> prefix seems fine. Maybe write_reg() and read_reg()?

The fact is, there is a symmetry between `read` and `write`:

- `read` takes a location and returns a value,
- `write` takes a location and a value.

`write_val` is really nothing but a convenience shortcut that has
technically no strong reason to exist. As Gary pointed out, the
counterpart of

    let reg = bar.read(regs::NV_PMC_BOOT_0);

is

    bar.write(regs::NV_PMC_BOOT_0, reg);

... and we introduced `write_val` for those cases where the value
in itself provides its location, as `NV_PMC_BOOT_0` is redundant. But
the above statement could also be written:

    bar.write((), reg);

which is exactly the same length as the `write_val` equivalent - it's
just that you need to remember that `()` can be used in this case. But
if you can remember that your register type can be used with
`write_val`, then why not this? This actually makes me doubt that
`write_val` is needed at all, and if we get rid of it, then we have a
symmetric API.

We were so focused on this single issue for the last few revisions that
the single-argument write variant sounded like the only way to handle
this properly, but the `()` use proposed by Gary actually fulfills the
same role and doesn't introduce more burden when you think of it.

So why not try without `write_val` at first? We can always add it later
if we feel the need (and the same applies to a `(location, value)`
symmetric read/write API).

And most importantly, that way we also don't have to worry about its
name. :)