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: <Z7aHQBYXZ5jlGRte@finisterre.sirena.org.uk>
Date: Thu, 20 Feb 2025 01:37:04 +0000
From: Mark Brown <broonie@...nel.org>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: 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, aliceryhl@...gle.com, 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 08:35:47PM -0300, Daniel Almeida wrote:
> > On 19 Feb 2025, at 20:05, Mark Brown <broonie@...nel.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.

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