Re: [PATCH v7 05/10] rust: io: add IoLoc and IoWrite types

["Gary Guo" <gary@garyguo.net>]

On Tue Mar 3, 2026 at 2:55 PM GMT, Alexandre Courbot wrote:
> On Tue Mar 3, 2026 at 5:31 PM JST, Alexandre Courbot wrote:
>> On Tue Mar 3, 2026 at 5:14 PM JST, Alexandre Courbot wrote:
>>> On Mon Mar 2, 2026 at 10:39 PM JST, Gary Guo wrote:
>>>> On Mon Mar 2, 2026 at 1:12 PM GMT, Danilo Krummrich wrote:
>>>>> On Mon Mar 2, 2026 at 1:53 PM CET, Gary Guo wrote:
>>>>>> On Mon Mar 2, 2026 at 1:44 AM GMT, Alexandre Courbot wrote:
>>>>>>> That should be doable. Note that we currently support `zeroed` and
>>>>>>> `default` as initializers, so having the same level of coverage would
>>>>>>> require two `write` variants. I'd like to hear what Danilo thinks.
>>>>>>
>>>>>> I wonder if just providing a single version that starts with
>>>>>> `Default::default()` should be sufficient? For most users, zeroed version is the
>>>>>> default version anyway. For those where default is not zero, it perhaps makes
>>>>>> more sense to start with default anyway; if explicitly zeroing is needed they
>>>>>> can always do an explicit `::zeroed()`.
>>>>>
>>>>> I was thinking about this for a while and also thought that we probably only
>>>>> ever need a version that starts with Default::default().
>>>>>
>>>>> What I still dislike is that the common case becomes write_with() instead of
>>>>> just write(). (Just to clarify, the name write_with() is perfectly fine for what
>>>>> the function does, it's more that we need it in the first place.)
>>>>>
>>>>> Also, IIUC, if the value is not created within the closure, we'd still have the
>>>>> following redundancy, right?
>>>>>
>>>>> 	let reg = regs::NV_PFALCON_FALCON_DMATRFMOFFS::of::<E>()
>>>>> 		.try_init(|r| r.try_with_offs(load_offsets.dst_start + pos))?;
>>>>>
>>>>> 	bar.write(regs::NV_PFALCON_FALCON_DMATRFMOFFS::of::<E>(), reg);
>>>>>
>>>>> It's just that this case nicely converts to write_with().
>>>>
>>>> You would have
>>>>
>>>>     let reg = regs::NV_PFALCON_FALCON_DMATRFMOFFS::default()
>>>>         .try_with_offs(load_offsets.dst_start + pos))?;
>>>>
>>>>     bar.write(regs::NV_PFALCON_FALCON_DMATRFMOFFS::of::<E>(), reg);
>>>>
>>>> Note that the `default()` invocation doesn't mention relative base, as it's just
>>>> plain bitfields without offset at that point. [ I like the fact that this
>>>> doesn't need to use closure, as I generally prefer code without them, perhaps I
>>>> am not "rusty" enough :) ]
>>>>
>>>> In my view, if the code is complex enough that you have
>>>>
>>>>     let reg = ...;
>>>>     <some code>
>>>>     bar.write(reg)
>>>>
>>>> then it probably makes sense to have register name mentioned again (this is
>>>> typed checked anyway so you don't need to worry about misnaming it). Otherwise,
>>>> one might read the code and be confused about what register is being written to
>>>> at all.
>>>>
>>>> I think for explicit location parameter makes much more sense for relative
>>>> addressed registers and register arrays.
>>>
>>> I am not too worried either about having to repeat the location in a
>>> write if we needed to store the register value somewhere first. That
>>> case should be covered by `update`/`try_update` anyway. What is less
>>> acceptable imo is having to type the location twice in the same `write`
>>> statement.
>>>
>>> I spent the day testing different strategies to support the
>>> two-arguments write with both explicit values and closures to create a
>>> value from scratch. That included adding a trait to produce the value
>>> and making `write` generic against it: if both immediate values and
>>> closures implement the trait, that should work I thought. Except that in
>>> the call site the compiler is unable to infer the closure's argument and
>>> requires us to explicitly specify it - sending us back to square 1.
>>> again.
>>>
>>> Another strategy is to make `write` accept only closures, and implement
>>> `FnOnce` on immediate values... but that requires the `fn_traits`
>>> unstable feature.
>>>
>>> So that really leaves us with two options:
>>>
>>> - The current one-argument design based on `IoWrite`, which carries a
>>>   location and its value,
>>> - Or a pair of `write`/`write_with` methods for immediate values and
>>>   closures, respectively.
>>>
>>> I'm ok with either, but the first one looks more composable to me. I can
>>> send a version implementing the second one if people want to see what it
>>> would look like.
>>
>> Mmm looking closer at the two-methods alternative, it does look more
>> familiar in terms of Rust patterns and less hacky in the end (i.e. no
>> need for `IoWrite`). The drawbacks are also manageable. I'm torn.
>
> So, to get a better idea of these two options I have converted this
> patchset to use the 2-arguments `write_with` method. Here is the
> difference between the two - it is particularly interesting to see how
> nova-core changes:
>
> https://github.com/Gnurou/linux/compare/register_1arg..Gnurou:linux:register_2args
>
> The two-arguments version often results in *shorter* write statements
> for multi-line statements. One-liners are interestingly the same length.
> I haven't found any instance where I had to write the register location
> an extra time.

Looks pretty good. Thanks for trying out.

>
> Note that the `Map` trick to allow closures to return a `Result` is not
> implemented yet, so there are a couple of `unwrap`s to allow the code to
> build.

Alternatively we can in the initial version just add a `write_try_with` method
and add the `Map` trick later.

>
> One detail: when using `write_with`, it is more natural to specify the
> location first, and closure second, since the closure's argument is
> derived from the location. However, all the `write*` methods of `Io` use
> the `(value, location)` order. This introduces some dissonance in the
> API, unless we convert everyone to the `(location, value)` order.

Personally I was actually quite surprised that we use `(value, location)` in our
I/O trait, when implementing I/O projections. I would actually be in favour if
the write API always use `(location, value)`.

I think the current order is just due to the fact that `writel` and friends take
value first and address second. Which itself I think probably is just a
historical choice and is not consistent with APIs like `WRITE_ONCE` and
`atomic_set`.

Best,
Gary