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

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

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.

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.

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.