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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z7cbkfvJqkWaSwKR@finisterre.sirena.org.uk>
Date: Thu, 20 Feb 2025 12:09:53 +0000
From: Mark Brown <broonie@...nel.org>
To: Daniel Almeida <daniel.almeida@...labora.com>
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 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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)