linux-kernel - Re: [PATCH] rust: regulator: add a bare minimum regulator abstraction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <E24A1EA3-DC87-4A33-AD93-1E3B307942E8@collabora.com>
Date: Wed, 19 Feb 2025 14:10:24 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: lgirdwood@...il.com,
 broonie@...nel.org,
 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

Hi Alice,

> On 19 Feb 2025, at 13:28, Alice Ryhl <aliceryhl@...gle.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