Message-Id: <DA07BSJPBP55.332DLW59WGDFA@nvidia.com>
Date: Mon, 19 May 2025 23:20:06 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Danilo
 Krummrich" <dakr@...nel.org>, "Boris Brezillon"
 <boris.brezillon@...labora.com>, "Sebastian Reichel"
 <sebastian.reichel@...labora.com>, "Liam Girdwood" <lgirdwood@...il.com>,
 "Mark Brown" <broonie@...nel.org>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3] rust: regulator: add a bare minimum regulator
 abstraction

Hi Daniel,

On Mon May 19, 2025 at 7:52 PM JST, Daniel Almeida wrote:
> Hi Alex,
>
> I still don’t understand your use case 100% :/

I should clarify that I don't have a use-case for this (yet), nor do I
foresee that Nova will need to use regulators ; it's just a drive-by
review as I am a bit familiar with the regulator consumer API thanks to
past work.

>
>> 
>> I just mean the cases where users will want to enable and disable the
>> regulator more frequently than just enabling it at probe time.
>
> This is already possible through kernel::types::Either. 
>
> i.e.: the current design - or the proposed typestate one - can already switch
> back and forth between Regulator and EnabledRegulator. Using Either makes it
> just work, because you can change the variant at runtime without hassle. This
> lets you consume self in an ergonomic way.
>
> By the way, the reason I'm pushing back slightly here is because you seem
> (IIUC) to be trying to reintroduce the pattern we had to move away from in v1.
>
> i.e.: we explicitly had to move away from trying to match enables and disables
> in Rust, because it was hard to get this right.
>
> The current design is a simplification that apparently works, because at best
> you have +1 on the count and that is encoded in the type itself, so there is
> nothing to actually "track" or "balance" within a given instance. Multiple
> calls to _get() or _enable() on the same instance are simply forbidden.
>
> Can you add some pseudocode that shows how this doesn't work (or is otherwise
> unergonomic) in Nova? I think it will make your point clearer.

So let's say you have this in your device struct:

  regulator: Either<Regulator, EnabledRegulator>,

And you want to set the voltage on it. Now you need to do:

  match &self.regulator {
    Either::Left(regulator) => regulator.set_voltage(...)?,
    Either::Right(regulator) => regulator.set_voltage(...)?,
  }

And you need to do that for every single method that is available on
both types. It's unergonomic and cumbersome.

Conversely, if you have:

  regulator: Regulator<Switch>,

then it's simply a matter of doing

  self.regulator.set_voltage(...)?;

It's more code. More code means more bugs. :) And it's going to be quite
a common pattern, so I think we should make it convenient.

Also it looks like `Either` is going away, which means each user will
now have to implement their own enum type.

>
>> 
>>> 
>>>> 
>>>> It has been proposed earlier to use a typestate, and this would indeed
>>>> provide several benefits, the first one being the ability to have shared
>>>> impl blocks (and shared documentation) between the enabled and disabled
>>>> states for methods like set/get_voltage().
>>>> 
>>>> But the key benefit I see is that it could also address the
>>>> aforementioned dynamic management problem through the introduction of a
>>>> third state.
>>>> 
>>>> Alongside the `Enabled` and `Disabled` states, there would be a third
>>>> state (`Dynamic`?) in which the regulator could either be enabled or
>>>> disabled. This `Dynamic` state is the only one providing `enable` and
>>>> `disable` methods (as well as `is_enabled`) to change its operational
>>>> state without affecting its type.
>>> 
>>> Dynamic is just "Regulator" in the current version of this patch. There is no
>>> "Disabled" because there is no guarantee that someone else won't enable the
>>> regulator, trivially breaking this invariant at any moment.
>> 
>> There is a core difference, which is that in your version of
>> `Regulator`, `enable` takes ownership of `self` and returns a different
>> type, whereas `Dynamic` would take `&mut self` and change its internal
>> state, like the C API does.
>
> I see now, but consuming self is something we're trying after considering the
> &mut self approach, which did not work very well in v1.

Just took a look at the comments on v1 since I arrived late in the
discussion.

I think both approaches have scenarios where they apply. It's a bit like
Rust's borrow checker: whenever possible, you want the rules to be
enforced at compile-time, but sometimes the access patterns become too
complex so you resort to RefCell to enforce these rules dynamically.

So please don't get me wrong, I think having a dedicated type for an
enabled regulator is great design, and its use should be preferred
whenever possible ; most drivers just want to get and enable a regulator
at probe time, and `Regulator::<Enabled>::get()` would be perfect for
that.

But if we only provide that then we also put restrictions on what the C
API allows, and users with more complex use-cases will pay the price by
rewriting code that already exists (a bit more on that later).

The following comment was made on v1:

> It's possible there's a need to split simple and complex consumer APIs
> in Rust?

And it sounds sensible to me.
>
>> 
>>> 
>>> The only thing we can guarantee is "Enabled", through our own call to
>>> "regulator_enable()".
>>> 
>>> In fact, for the typestate solution, I was thinking about "UnknownState" and
>>> "Enabled", or any nomenclature along these lines.
>>> 
>>>> 
>>>> All three states then implement `set_voltage` and `get_voltage` through
>>>> a common impl block, that could be extended with other methods from the
>>>> C API that are independent of the state, as needed.
>>>> 
>>>> To handle typestate transitions:
>>>> 
>>>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>>> method to transition the regulator to the `Enabled` state.
>>> 
>>> Why not “enable()” as we currently have?
>> 
>> `enable()` to me sounds like a method that mutates `self` in-place,
>> whereas your version consumes it and turns it into a different type.
>> Such methods are typically named `into_*`, or `try_into_*` when they can
>> fail.
>> 
>> Actually, this could even be a `TryInto` implementation, but in this
>> particular case having methods with the target state in their names may
>> result in clearer code and allow the reader to model the transition
>> graph more easily.
>> 
>>> 
>>> If we go with the "Dynamic" nomenclature, and we agree that there's no
>>> "Disabled",  then we can implement "pub fn enable(self) -> Regulator<Enabled>",
>>> for "Dynamic", which is what we currently have, but with other names.
>> 
>> Not if we want to provide the behavior of the C consumer API, which
>> requires multiple calls to `regulator_enable()` to be matched by an equal
>> number of calls to `regulator_disable()`, which could be useful to some
>> drivers (lest they reimplement their own counter).
>
> This is explicitly not supported, because (given the current code) why should it be?
>
> If you want a given regulator to be enabled, just make sure you have
> Regulator<Enabled> in your kernel::types::Either container.
>
> You don't need a counter either: Regulator<Enabled> has a count of one, and
> when that goes out of scope, it's decremented.

That works very well for simpler scenarios, but won't cover everything
optimally.

A common pattern is a driver (sensors come to mind, and maybe codecs)
that acquire a regulator at probe time, and enable it every time
user-space opens the device (and, conversely, disable it as the user
session closes). The driver wants the regulator to be enabled if there
is at least one user-space session opened, and only disable it when all
user-space sessions are closed. With the proposed scheme, a Rust driver
would have to keep track of the opened sessions count and disable the
regulator itself when it reaches zero - effectively duplicating what the
C API would happily do for it, with the potential of bugs being
introduced in these extra lines of code.

There was also a discussion stating that it is safer (for the hardware)
to leave a regulator on than it is to switch it off when, due to a bug,
it is released while its enable count is not zero, and I think that's a
behavior we should allow if the user expresses a need for it. Safety is
not only memory safety :) (and in this case, it is not affected anyway).

>> - `Regulator<Disabled>` guarantees that the regulator is not enabled on
>>  the consumer side. It could be useful to have for drivers that use
>>  distinct types to store their state depending on their power status:
>>  the powered-on type would store a `Regulator<Enabled>`, the
>>  powered-off type a `Regulator<Disabled>`. That way you cannot even
>>  write code transitioning between the states if you omit the regulator
>>  - which I think is really neat.
>> 
>
> I thought we had agreed that there is no “Disabled”, even in C?

The relevant function is called `regulator_disable()`, and it may or may
not actually disable the regulator, but the consumer behaves as if it
were. I think we should keep the same nomenclature for consistency.

>> These two should cover a large percentage of consumer needs, but for
>> those that need more fine-grained control we should also have one or two
>> policies that follow the C API a bit closer, e.g.:
>> 
>> - `Regulator<Controlled>` (or just `Regulator`): user is granted an API
>>  very close to the C one, and is responsible for balancing calls to
>>  `enable()` and `disable()`. Regulator remains in the state it was in
>>  when dropped.
>> 
>> - `Regulator<Switch>`: calls to `enable()` and `disable()` do not need
>>  to be balanced, and the regulator always transitions to one state if
>>  it was in the other. Regulator gets automatically disabled on drop.
>>  This provides a simpler, safer alternative to `Controlled`.
>> 
>> Note that I am not advocating for including these two policies
>> specifically, these are just examples. My main point is that we should
>> also provide a way to change the regulator's enabled state without
>> requiring a change of type ; which policy(es) to adopt will depend on
>> which restrictions we conclude are adequate to place on the C API, if
>> any.
>
> Can you expand a bit on the issues of changing types? Again, some pseudocode
> will probably help a lot :)

See my example using `Either` above. :) I hope to be forgiven for the
user-space sessions example as I believe it is easy to model.

Powered by blists - more mailing lists