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] [day] [month] [year] [list]

Message-ID: <Z7c40fpQVelj20gE@finisterre.sirena.org.uk>
Date: Thu, 20 Feb 2025 14:14:41 +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 Thu, Feb 20, 2025 at 10:48:31AM -0300, Daniel Almeida wrote:
> > On 20 Feb 2025, at 09:09, Mark Brown <broonie@...nel.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?

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