Message-Id: <53A50677-CB35-441F-8E58-0FB1EA577C4E@collabora.com>
Date: Thu, 20 Feb 2025 10:48:31 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Mark Brown <broonie@...nel.org>
Cc: Alice Ryhl <aliceryhl@...gle.com>,
 lgirdwood@...il.com,
 sebastian.reichel@...labora.com,
 sjoerd.simons@...labora.co.uk,
 ojeda@...nel.org,
 alex.gaynor@...il.com,
 boqun.feng@...il.com,
 gary@...yguo.net,
 bjorn3_gh@...tonmail.com,
 a.hindborg@...nel.org,
 benno.lossin@...ton.me,
 tmgross@...ch.edu,
 dakr@...nel.org,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: regulator: add a bare minimum regulator abstraction



> On 20 Feb 2025, at 09:09, Mark Brown <broonie@...nel.org> wrote:
> 
> On Wed, Feb 19, 2025 at 02:10:24PM -0300, Daniel Almeida wrote:
> 
>> This means that now, `EnabledRegulator` has to depend on `Regulator` somehow to ensure
>> a proper drop order. Otherwise you might have an enabled regulator for which you don’t own
>> the refcount. Furthermore, if Regulator drops while EnabledRegulator is alive, you get a splat.
> 
> Having an enabled regulator object depend on a regulator object seems
> like a goal rather than a problem, surely it's common to need such
> relationships and there's an idiomatic way to do it?  It seems to be how
> Rust does mutexes…

Not when you need to store both of them at the same time.

For Mutex<T>, for example, you don’t store both Mutex<T> and MutexGuard<'_,T>
at the same time. You store a Mutex<T>, and create MutexGuard<'_, T> on the
stack by locking the Mutex when you need to access T. When MutexGuard goes away,
the Mutex is automatically unlocked.

The equivalent behavior for us here would be to enable a regulator on the
stack, perform some work, and then have this regulator be automatically
disabled. There would be no way to keep it enabled for a longer period.

> 
>> In a driver, you now have to store both Regulator - for the refcount - and EnabledRegulator
>> - as a way to tell the system you need that regulator to be active.
> 
> That's true, unless you can make a type of enable that just fully takes
> ownership of the regulator (which TBH people want, people really want a
> devm_regulator_get_enable() C API which just gets and holds an enabled
> regulator for the simple case where you don't actually ever manage the
> power).  It's possible there's a need to split simple and complex
> consumer APIs in Rust?
> 
>> If EnabledRegulator is a guard type, this doesn’t work, as it creates a self-reference - on top
>> of being extremely clunky.
>> 
>> You can then have EnabledRegulator consume Regulator, but this assumes that the regulator
>> will be on all the time, which is not true. A simple pattern of
> 
> I don't understand the self reference thing?

It’s a Rust thing.

Let’s say you have this:

```
let regulator: Regulator = Regulator::get(/*…*/)?;
let regulator_enable: EnabledRegulator = regulator.enable()?;
```

You can encode that `EnabledRegulator` depends on Regulator, i.e.: that it must
not outlive it:

```
struct EnabledRegulator<‘a>(&’a Regulator);

<snip> 

impl Regulator {
  pub fn enable(&self) -> Result<EnabledRegulator<‘_>> {
    /* do the work to enable, error check, etc, then.. */
    
    EnabledRegulator(&self) // EnabledRegulator keeps a reference to Regulator
  }
}
```

Now the compiler will enforce that EnabledRegulator doesn’t outlive
Regulator:

```
let regulator = Regulator::get(/*…*/)?;
let enabled_regulator = regulator.enable()?;

 // error: regulator is needed by enabled_regulator, so you can’t do this.
core::mem::drop(regulator);
```

This is fine on the stack, but if you want to store both of them in a struct,
that is a problem.

```
struct Foo {
  // A refcount obtained by regulator_get()
  regulator: Regulator,
   
  // Indicates that the regulator is enabled.
  enabled: EnabledRegulator<‘a>  // error: what is ‘a ?
} 
```

Now `enabled` contains an internal reference to `regulator`, which is a sibling
member.

This is where my “self-reference” comment comes from, i.e.: Foo has a
member that has a reference to another member of Foo itself.

The problem here is that Rust has move semantics by default. If you allow a
struct to keep a reference to parts of itself, this breaks when the type is
moved. In other words, if Foo is ever moved, `enabled` would contain a
reference to the location it was moved from, which would be unsound.

There is no way to fix this, because, unlike C++, Rust does not have move
constructors, move assignments and etc. Here, moving means a bitwise copy from
address A to B, where A is then invalidated. This means that self-references
are simply disallowed. (Well, there is Pin<T>, but let's not get into that)

Note that, in this particular example, we never access this reference, it's
there just to tell the compiler that we need a Regulator to be alive in order
for a EnabledRegulator to exist. It does not matter though, it still won't
compile.

Even if we use the PhantomData type, which doesn't store a reference to the
actual memory location within it, just the lifetime itself, it will still not
work for the same reason. At no point can you create a self-referential
type even when there is no actual memory location involved.


I am not saying that we cannot encode this relationship through some other
means. I am merely saying that doing it *this way* will not work.

> 
>> ```
>> regulator_enable()
>> do_fancy_stuff()
>> regulator_disable()
>> ```
> 
>> Becomes a pain when one type consumes the other:
>> 
>> ```
>> self.my_regulator.enable() // error, moves out of `&self`
>> ```
> 
> Your second block of code doesn't look obviously painful there?

It is painful in the sense that it won’t even compile.

This example assumes a different API where EnabledRegulator consumes Regulator
to exist, instead of keeping an internal reference:

```
impl Regulator {
  // Note that we take `self`, not `&self`, so this moves `Regulator` into
  // `enable()`, therefore 'killing' it.

  fn enable(self) -> Result<EnabledRegulator> {

    /* enable, do error checking, etc, and then */

    EnabledRegulator(self) // We transformed Regulator into EnabledRegulator.
  }
}

In our struct Foo example, this would move the Regulator out of the field, i.e.:

```
let foo = Foo { regulator: Regulator::get(/*...*/)?; };

let regulator_enabled = foo.regulator.enable()?;
```

This is an error, because if we move out of foo.regulator to create
EnabledRegulator, then what would be left in there?

We can get around this using Option<T>:

```
let foo = Foo { regulator: Some(Regulator::get(/*...*/)?); };

let regulator_enabled = foo.regulator.take()?.enable()?;
```

But this is extremely unergonomic because Option<T> is meant for cases where we
may or may not have something, but this is not the case here. We're simply
abusing its API to make our own Regulator API work.

> 
>> I am sure we can find ways around that, but a simple `bool` here seems to fix this problem.
> 
>> Now you only have to store `Regulator`. If you need another part of your code to also keep
>> the regulator enabled, you store a `Regulator` there and enable that as well. All calls to
>> enable and disable will be automatically balanced for all instances of `Regulator` by
>> virtue of the `enabled` bool as well.
> 
> What you're describing here with creating one Regulator object per
> enable sounds more like you want it to be an error to do multiple
> enables on a single regulator.  Instead of reference counting or
> silently ignoring duplicate enables it should error out on a duplicate
> enable.

The problem here isn't necessarily forbiding multiple enables, but figuring out
a way to make sure they are balanced in Rust. Let me think this through some
more, because I do not have a solution in hand right now.

— Daniel

Powered by blists - more mailing lists