Add a bare minimum regulator abstraction to be used by Rust drivers.
This abstraction adds a small subset of the regulator API, which is
thought to be sufficient for the drivers we have now.

Regulators provide the power needed by many hardware blocks and thus are
likely to be needed by a lot of drivers.

It was tested on rk3588, where it was used to power up the "mali"
regulator in order to power up the GPU.

Note that each instance of [`Regulator`] obtained from
`Regulator::get()` can only be enabled once. This ensures that the calls
to enable and disable are perfectly balanced before `regulator_put()` is
called, as mandated by the C API.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/regulator.rs        | 120 ++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)
 create mode 100644 rust/kernel/regulator.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14..92504f19655e 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -28,6 +28,7 @@
 #include <linux/poll.h>
 #include <linux/property.h>
 #include <linux/refcount.h>
+#include <linux/regulator/consumer.h>
 #include <linux/sched.h>
 #include <linux/security.h>
 #include <linux/slab.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..0224f4c248c0 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -68,6 +68,8 @@
 pub mod prelude;
 pub mod print;
 pub mod rbtree;
+#[cfg(CONFIG_REGULATOR)]
+pub mod regulator;
 pub mod revocable;
 pub mod security;
 pub mod seq_file;
diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
new file mode 100644
index 000000000000..df6eb325d11a
--- /dev/null
+++ b/rust/kernel/regulator.rs
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Regulator abstractions.
+//!
+//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
+
+use crate::{
+    bindings,
+    device::Device,
+    error::{from_err_ptr, to_result, Result},
+    prelude::*,
+};
+
+use core::ptr::NonNull;
+
+/// A `struct regulator` abstraction.
+///
+/// Note that each instance of [`Regulator`] obtained from `Regulator::get()`
+/// can only be enabled once. This ensures that the calls to enable and disable
+/// are perfectly balanced before `regulator_put()` is called, as mandated by
+/// the C API.
+///
+/// # Invariants
+///
+/// - [`Regulator`] is a non-null wrapper over a pointer to a `struct regulator`
+///   obtained from `regulator_get()`.
+/// - Each instance of [`Regulator`] obtained from `Regulator::get()` can only
+///   be enabled once.
+pub struct Regulator {
+    inner: NonNull<bindings::regulator>,
+    enabled: bool,
+}
+
+impl Regulator {
+    /// Obtains a [`Regulator`] instance from the system.
+    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
+        // SAFETY: It is safe to call `regulator_get()`, on a device pointer
+        // earlier received from the C code.
+        let inner = from_err_ptr(unsafe { bindings::regulator_get(dev.as_raw(), name.as_ptr()) })?;
+
+        // SAFETY: We can safely trust `inner` to be a pointer to a valid
+        // regulator if `ERR_PTR` was not returned.
+        let inner = unsafe { NonNull::new_unchecked(inner) };
+
+        Ok(Self {
+            inner,
+            enabled: false,
+        })
+    }
+
+    /// Enable the regulator.
+    pub fn enable(&mut self) -> Result {
+        if self.enabled {
+            return Ok(());
+        }
+
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        let res = to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) });
+        if res.is_ok() {
+            self.enabled = true;
+        }
+
+        res
+    }
+
+    /// Disable the regulator.
+    pub fn disable(&mut self) -> Result {
+        if !self.enabled {
+            return Ok(());
+        }
+
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        let res = to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) });
+        if res.is_ok() {
+            self.enabled = false;
+        }
+
+        res
+    }
+
+    /// Set the voltage for the regulator.
+    pub fn set_voltage(&self, min_uv: Microvolt, max_uv: Microvolt) -> Result {
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        to_result(unsafe {
+            bindings::regulator_set_voltage(self.inner.as_ptr(), min_uv.0, max_uv.0)
+        })
+    }
+
+    /// Get the current voltage of the regulator.
+    pub fn get_voltage(&self) -> Result<Microvolt> {
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        let voltage = unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) };
+        if voltage < 0 {
+            Err(Error::from_errno(voltage))
+        } else {
+            Ok(Microvolt(voltage))
+        }
+    }
+}
+
+impl Drop for Regulator {
+    fn drop(&mut self) {
+        if self.enabled {
+            // It is a requirement from the C API that the calls to enable and
+            // disabled are balanced before calling `regulator_put()`.
+            self.disable();
+        }
+
+        // SAFETY: By the type invariants, we know that `self` owns a reference,
+        // so it is safe to relinquish it now.
+        unsafe { bindings::regulator_put(self.inner.as_ptr()) };
+    }
+}
+
+/// A voltage in microvolts.
+///
+/// The explicit type is used to avoid confusion with other multiples of the
+/// volt, which can be desastrous.
+#[repr(transparent)]
+pub struct Microvolt(pub i32);
-- 
2.48.1

On Wed, Feb 19, 2025 at 01:25:16PM -0300, Daniel Almeida wrote:

> Add a bare minimum regulator abstraction to be used by Rust drivers.
> This abstraction adds a small subset of the regulator API, which is
> thought to be sufficient for the drivers we have now.

...

> Note that each instance of [`Regulator`] obtained from
> `Regulator::get()` can only be enabled once. This ensures that the calls
> to enable and disable are perfectly balanced before `regulator_put()` is
> called, as mandated by the C API.

This sounds very wrong, a major use case for consumers is to enable and
disable regulators at runtime.  I would expect that the Rust code might
want to have it's own reference counting (and probably should), but not
being able to disable a regulator after enabling it seems to be a great
big red flag.

> +    /// Enable the regulator.
> +    pub fn enable(&mut self) -> Result {
> +        if self.enabled {
> +            return Ok(());
> +        }

This isn't what the changelog claimed, it's silently ignoring duplicate
attempts to enable and...

> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        let res = to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) });
> +        if res.is_ok() {
> +            self.enabled = false;
> +        }

...we actually support disabling and reenabling multiple times, it's
just that there's a maximum of one enable at once from the consumer and
we silently ignore duplicated enables and disables.  That's better but
it's asking for trouble.

If you're going to support both enable and disable (and all things
considered you probably should TBH) it would be better to just reference
count the enables like the C code does and then complain about it if
someone tries to free the regulator object without underwinding their
enables, or just possibly just clean them up.  Silently ignoring
duplicate enables or overflowing disables doesn't seem like it plays
well with the safety goals that Rust has, it's asking for bugs - a big
part of the reason the C API does things the way it does is that it will
notice if the enables and disables aren't joined up.  You can easily
wind up with one part of the code disabling the regulator underneath
another part that's still trying to use the hardware and things tend not
to go well when that happens.

Perhaps an enable should be an object that's allocated and carried about
by whatever's holding the reference, I don't know if that plays nicely
with how Rust likes to ensure safety with this stuff?

Hi Mark,

> On 19 Feb 2025, at 20:05, Mark Brown <broonie@kernel.org> wrote:
> 
> On Wed, Feb 19, 2025 at 01:25:16PM -0300, Daniel Almeida wrote:
> 
>> Add a bare minimum regulator abstraction to be used by Rust drivers.
>> This abstraction adds a small subset of the regulator API, which is
>> thought to be sufficient for the drivers we have now.
> 
> ...
> 
>> Note that each instance of [`Regulator`] obtained from
>> `Regulator::get()` can only be enabled once. This ensures that the calls
>> to enable and disable are perfectly balanced before `regulator_put()` is
>> called, as mandated by the C API.
> 
> This sounds very wrong, a major use case for consumers is to enable and
> disable regulators at runtime.  I would expect that the Rust code might
> want to have it's own reference counting (and probably should), but not
> being able to disable a regulator after enabling it seems to be a great
> big red flag.
> 
>> +    /// Enable the regulator.
>> +    pub fn enable(&mut self) -> Result {
>> +        if self.enabled {
>> +            return Ok(());
>> +        }
> 
> This isn't what the changelog claimed, it's silently ignoring duplicate
> attempts to enable and...
> 
>> +        // SAFETY: Safe as per the type invariants of `Regulator`.
>> +        let res = to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) });
>> +        if res.is_ok() {
>> +            self.enabled = false;
>> +        }
> 
> ...we actually support disabling and reenabling multiple times, it's
> just that there's a maximum of one enable at once from the consumer and
> we silently ignore duplicated enables and disables.  That's better but
> it's asking for trouble.

Maybe there was a language issue on my end here.

When I say once, I mean exactly what the source code shows, i.e.: it can be
enabled once, and further calls will be ignored. If it is disabled, it can then be
enabled once, with all further calls ignored, etc.

What I wanted to say is that this abstraction does not support exactly what you
suggested, i.e.: a refcount of how many times it was enabled.

I agree that this is not what the word “once” means. Sorry for the confusion.

> 
> If you're going to support both enable and disable (and all things
> considered you probably should TBH) it would be better to just reference
> count the enables like the C code does and then complain about it if
> someone tries to free the regulator object without underwinding their
> enables, or just possibly just clean them up.  Silently ignoring
> duplicate enables or overflowing disables doesn't seem like it plays
> well with the safety goals that Rust has, it's asking for bugs - a big
> part of the reason the C API does things the way it does is that it will
> notice if the enables and disables aren't joined up.  You can easily
> wind up with one part of the code disabling the regulator underneath
> another part that's still trying to use the hardware and things tend not
> to go well when that happens.

I thought about that, something like:

```
  fn drop(&mut self) {

   // someone probably forgot to call disable?
    while self.enabled_count > 0 {

            // Try to disable, this will decrement self.enabled_count
            if let Err(e) = self.disable() {

              // But don’t loop forever if it fails
              break;
            }
        }
  }
```

I asked for a few opinions privately and was told that “if the C API prefers not to do that
why should you?”

> 
> Perhaps an enable should be an object that's allocated and carried about
> by whatever's holding the reference, I don't know if that plays nicely
> with how Rust likes to ensure safety with this stuff?

As I said, this doesn’t work very well, unless someone corrects my reasoning on a
previous email. Having a single type “Regulator” is probably much saner than “Regulator”
and “EnabledRegulator”.

Your “refcount” idea works just fine, though. I’ll send a v2.

— Daniel

On Wed, Feb 19, 2025 at 08:35:47PM -0300, Daniel Almeida wrote:
> > On 19 Feb 2025, at 20:05, Mark Brown <broonie@kernel.org> wrote:

> > If you're going to support both enable and disable (and all things
> > considered you probably should TBH) it would be better to just reference
> > count the enables like the C code does and then complain about it if
> > someone tries to free the regulator object without underwinding their
> > enables, or just possibly just clean them up.  Silently ignoring
> > duplicate enables or overflowing disables doesn't seem like it plays
> > well with the safety goals that Rust has, it's asking for bugs - a big
> > part of the reason the C API does things the way it does is that it will
> > notice if the enables and disables aren't joined up.  You can easily
> > wind up with one part of the code disabling the regulator underneath
> > another part that's still trying to use the hardware and things tend not
> > to go well when that happens.

> I thought about that, something like:

> ```
>   fn drop(&mut self) {
> 

> I asked for a few opinions privately and was told that “if the C API prefers not to do that
> why should you?”

Well, the C API also doesn't ignore either enable or disable attempts...
the theory is that if the consumer messed up it's safer to not power the
hardware off suddenly when something might not have been cleaned up.
The general approach the API takes is to only take actions it's been
explicitly asked to do, that way we're not hard coding anything that
causes trouble for consumers and since we need constraints to enable any
action that gets taken we're less likely to have default behaviour
causing hardware damage somehow.  If we think we've lost track of the
reference counting we just scream about it but don't try to take
corrective action.

> > Perhaps an enable should be an object that's allocated and carried about
> > by whatever's holding the reference, I don't know if that plays nicely
> > with how Rust likes to ensure safety with this stuff?

> As I said, this doesn’t work very well, unless someone corrects my reasoning on a

I don't think I saw the previous mail?

> previous email. Having a single type “Regulator” is probably much saner than “Regulator”
> and “EnabledRegulator”.

OK.  It's a bit of a shame that there's not an idiomatic way to help
with the reference counting here.

Hi Mark,

> 
>> I asked for a few opinions privately and was told that “if the C API prefers not to do that
>> why should you?”
> 
> Well, the C API also doesn't ignore either enable or disable attempts...
> the theory is that if the consumer messed up it's safer to not power the
> hardware off suddenly when something might not have been cleaned up.
> The general approach the API takes is to only take actions it's been
> explicitly asked to do, that way we're not hard coding anything that
> causes trouble for consumers and since we need constraints to enable any
> action that gets taken we're less likely to have default behaviour
> causing hardware damage somehow.  If we think we've lost track of the
> reference counting we just scream about it but don't try to take
> corrective action.

So, are you OK with this approach? i.e.:

> ```
>  fn drop(&mut self) {
> 
>    while self.enabled_count > 0 {
> 
>            if let Err(e) = self.disable() {
>              break;
>            }
>        }
>  }
> ```

Where `enable()` increments self.enable_count and `disable()` decrements it.

>>> Perhaps an enable should be an object that's allocated and carried about
>>> by whatever's holding the reference, I don't know if that plays nicely
>>> with how Rust likes to ensure safety with this stuff?
> 
>> As I said, this doesn’t work very well, unless someone corrects my reasoning on a
> 
> I don't think I saw the previous mail?

You didn’t get this?

https://lore.kernel.org/rust-for-linux/Z7aHQBYXZ5jlGRte@finisterre.sirena.org.uk/T/#m9348ad4fdc056d7f6d0bfec6529d4c80afdcd335


— Daniel

On Thu, Feb 20, 2025 at 06:42:25AM -0300, Daniel Almeida wrote:

> >> I asked for a few opinions privately and was told that “if the C API prefers not to do that
> >> why should you?”

> > Well, the C API also doesn't ignore either enable or disable attempts...
> > the theory is that if the consumer messed up it's safer to not power the
> > hardware off suddenly when something might not have been cleaned up.
> > The general approach the API takes is to only take actions it's been
> > explicitly asked to do, that way we're not hard coding anything that
> > causes trouble for consumers and since we need constraints to enable any
> > action that gets taken we're less likely to have default behaviour
> > causing hardware damage somehow.  If we think we've lost track of the
> > reference counting we just scream about it but don't try to take
> > corrective action.

> So, are you OK with this approach? i.e.:

> > ```
> >  fn drop(&mut self) {
> > 
> >    while self.enabled_count > 0 {
> > 
> >            if let Err(e) = self.disable() {
> >              break;
> >            }
> >        }
> >  }
> > ```

> Where `enable()` increments self.enable_count and `disable()` decrements it.

It's not ideal since it'll just silently clean up any enables that were
done, ideally the Rust bindings would prevent leaks of enables like they
are intended to prevent memory leaks and this wouldn't come up at all.
This seems less safe than the C API since we don't even detect any
possible isues, we'll just silently hide leaks.

> >>> Perhaps an enable should be an object that's allocated and carried about
> >>> by whatever's holding the reference, I don't know if that plays nicely
> >>> with how Rust likes to ensure safety with this stuff?

> >> As I said, this doesn’t work very well, unless someone corrects my reasoning on a

> > I don't think I saw the previous mail?

> You didn’t get this?

> https://lore.kernel.org/rust-for-linux/Z7aHQBYXZ5jlGRte@finisterre.sirena.org.uk/T/#m9348ad4fdc056d7f6d0bfec6529d4c80afdcd335

Don't think so (but the copy I'm getting from the archive has mangled
formatting with weird line lengths, it's possible that it got hit by
spam filtering or I discarded it manually as junk due to the
formatting).  I'm not sure I understand the concerns raised there, but
it's possible Rust is less helpful for this stuff than I had imagined,
or that the whole thing with layering Rust on top of the C code is
getting in the way of whatever idioms fully native Rust code would use
to avoid issues.  I'll reply there...

On Wed, Feb 19, 2025 at 4:26 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Add a bare minimum regulator abstraction to be used by Rust drivers.
> This abstraction adds a small subset of the regulator API, which is
> thought to be sufficient for the drivers we have now.
>
> Regulators provide the power needed by many hardware blocks and thus are
> likely to be needed by a lot of drivers.
>
> It was tested on rk3588, where it was used to power up the "mali"
> regulator in order to power up the GPU.
>
> Note that each instance of [`Regulator`] obtained from
> `Regulator::get()` can only be enabled once. This ensures that the calls
> to enable and disable are perfectly balanced before `regulator_put()` is
> called, as mandated by the C API.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/kernel/lib.rs              |   2 +
>  rust/kernel/regulator.rs        | 120 ++++++++++++++++++++++++++++++++
>  3 files changed, 123 insertions(+)
>  create mode 100644 rust/kernel/regulator.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 55354e4dec14..92504f19655e 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -28,6 +28,7 @@
>  #include <linux/poll.h>
>  #include <linux/property.h>
>  #include <linux/refcount.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/sched.h>
>  #include <linux/security.h>
>  #include <linux/slab.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911..0224f4c248c0 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -68,6 +68,8 @@
>  pub mod prelude;
>  pub mod print;
>  pub mod rbtree;
> +#[cfg(CONFIG_REGULATOR)]
> +pub mod regulator;
>  pub mod revocable;
>  pub mod security;
>  pub mod seq_file;
> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> new file mode 100644
> index 000000000000..df6eb325d11a
> --- /dev/null
> +++ b/rust/kernel/regulator.rs
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Regulator abstractions.
> +//!
> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
> +
> +use crate::{
> +    bindings,
> +    device::Device,
> +    error::{from_err_ptr, to_result, Result},
> +    prelude::*,
> +};
> +
> +use core::ptr::NonNull;
> +
> +/// A `struct regulator` abstraction.
> +///
> +/// Note that each instance of [`Regulator`] obtained from `Regulator::get()`
> +/// can only be enabled once. This ensures that the calls to enable and disable
> +/// are perfectly balanced before `regulator_put()` is called, as mandated by
> +/// the C API.
> +///
> +/// # Invariants
> +///
> +/// - [`Regulator`] is a non-null wrapper over a pointer to a `struct regulator`
> +///   obtained from `regulator_get()`.
> +/// - Each instance of [`Regulator`] obtained from `Regulator::get()` can only
> +///   be enabled once.
> +pub struct Regulator {
> +    inner: NonNull<bindings::regulator>,
> +    enabled: bool,
> +}

I wonder if enabled vs disabled should be two different types?

Alice

Hi Alice,

> On 19 Feb 2025, at 13:28, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> I wonder if enabled vs disabled should be two different types?
> 
> Alice

I thought about having two types too, but I think it complicates the design.


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

Let’s first agree that `Regulator::drop` is the right place to have `regulator_put`, since
`Regulator::get()` acquired the reference in the first place.

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.

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.

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

```
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`
``` 

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.

— Daniel

On Wed, Feb 19, 2025 at 02:10:24PM -0300, Daniel Almeida wrote:
> Hi Alice,

> > On 19 Feb 2025, at 13:28, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > I wonder if enabled vs disabled should be two different types?
> >
> > Alice

> I thought about having two types too, but I think it complicates the  
> design.


> ```
> let foo: Regulator = Regulator::get(/*…*/)?;
> let foo_enabled: EnabledRegulator = foo.enable()?:
> ```

> Let’s first agree that `Regulator::drop` is the right place to have  
> `regulator_put`, since
> `Regulator::get()` acquired the reference in the first place.

> 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.

> 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.

> If EnabledRegulator is a guard type, this doesn’t work, as it creates a  
> self-reference - on top
> of being extremely clunky.

You definitely do not want a guard type.

> 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

> ```
> 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`
> ```

> I am sure we can find ways around that, but a simple `bool` here seems to  
> fix this problem.

What I meant to suggest is two types:

	pub struct DisabledRegulator {
	    inner: NonNull<bindings::regulator>,
	}
	pub struct EnabledRegulator {
	    inner: DisabledRegulator,
	}

	impl Drop for EnabledRegulator {
	    fn drop(&mut self) {
	        unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
		// Destructor of DisabledRegulator runs now since it's a
		// field of EnabledRegulator.
	    }
	}
	impl Drop for DisabledRegulator {
	    fn drop(&mut self) {
	        unsafe { bindings::regulator_put(self.inner.as_ptr()) };
	    }
	}

This gives the right behavior. For devices where the regulator is always
going to be enabled, you just store an EnabledRegulator. If you need to
dynamically switch between them, you store an enum like Boqun suggested.

Alice

On Wed, Feb 19, 2025 at 02:10:24PM -0300, Daniel Almeida wrote:
> Hi Alice,
> 
> > On 19 Feb 2025, at 13:28, Alice Ryhl <aliceryhl@google.com> wrote:
> > 
> > I wonder if enabled vs disabled should be two different types?
> > 
> > Alice
> 
> I thought about having two types too, but I think it complicates the design.
> 
> 
> ```
> let foo: Regulator = Regulator::get(/*...*/)?;
> let foo_enabled: EnabledRegulator = foo.enable()?:
> ```
> 
> Let´s first agree that `Regulator::drop` is the right place to have `regulator_put`, since
> `Regulator::get()` acquired the reference in the first place.
> 
> 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.
> 
> 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.
> 
> 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
> 
> ```
> 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`
> ``` 

You can introduce an enum:

	pub enum WaitForAGoodName {
		Regulator(Regulator),
		Enabled(EnableRegulator),
	}

for this case, and `my_regulator` is this type (or you can use
`kernel::types::Either`). The eventual code generation will probably use
a byte flag somewhere, but it only needs the byte flag for such a case.
In other cases, for example, the driver knows the regulator is always
enabled, you save the byte flag and the complexity of impl Regulator.

Regards,
Boqun

> 
> 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.
> 
> - Daniel
> 

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... 

> 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?

> ```
> 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?

> 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.

> On 20 Feb 2025, at 09:09, Mark Brown <broonie@kernel.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

On Thu, Feb 20, 2025 at 10:48:31AM -0300, Daniel Almeida wrote:
> > On 20 Feb 2025, at 09:09, Mark Brown <broonie@kernel.org> wrote:

> > 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.

Surely this need also exists with other lock types in Rust?  Holding a
semaphore for a long lived series of operations or similar.  It seems
surprising that this isn't a standard pattern.

> >> ```
> >> 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:

As I mentioned that sounds like something users do want.

> 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.

Are we?  We may or may not have enabled the regulator.

> >> 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.

Right, what I'm saying is that when you talk above about each part of
the code getting another Regulator for the same underlying regulator and
doing it's enable there that sounds like a pattern where each Regulator
has a maximum enable count of one - if it's idiomatic to have different
Regulator objects for different uses then you can say that each use
can have only one enable and needs to resolve it's own concurrency
issues.  The C regulator API is perfectly happy with this (modulo
exclusive regulators which you've not covered here) so if it simplifies
things that should be fine.

Your formatting here is still odd BTW, line lengths are too long (though
less long than on the prior mail) and it looks like there's some '''
formatting rather than indents around code that looks like one of the
structured text formats?