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: <Z7rVC-V-4QxGwMAy@Mac.home>
Date: Sat, 22 Feb 2025 23:58:03 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Alice Ryhl <aliceryhl@...gle.com>, lgirdwood@...il.com,
	broonie@...nel.org, sebastian.reichel@...labora.com,
	sjoerd.simons@...labora.co.uk, ojeda@...nel.org,
	alex.gaynor@...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:
> 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`
> ``` 

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
>