Message-Id: <D9VXWBFF85HC.VQBCQLD2X9VE@kernel.org>
Date: Wed, 14 May 2025 16:06:03 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Mark Brown" <broonie@...nel.org>
Cc: "Daniel Almeida" <daniel.almeida@...labora.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, "Boris Brezillon"
 <boris.brezillon@...labora.com>, "Sebastian Reichel"
 <sebastian.reichel@...labora.com>, "Liam Girdwood" <lgirdwood@...il.com>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3] rust: regulator: add a bare minimum regulator
 abstraction

On Wed May 14, 2025 at 2:48 PM CEST, Mark Brown wrote:
> On Wed, May 14, 2025 at 02:23:04PM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 1:50 PM CEST, Mark Brown wrote:
>
>> > In the C API the disable operation just fails and it's treated as though
>> > you hadn't done anything from a refcounting point of view.
>
>> But if it succeeds it takes ownership? The function `regulator_disable`
>> is also used in the `Drop` impl of the `EnabledRegulator`, so it better
>> give up the refcount, otherwise we would leak it.
>
> I can't understand what you are saying at all, sorry.  What does "take
> ownership" mean, and what is the "it" here?  We are talking about the
> case where regulator_disable() fails here, that means it didn't do what
> it was asked to do.

My confusion got cleared by what Daniel wrote:

> I am operating under the assumption that regulator_enable() and
> regulator_disable() do not touch the reference count. Note that we do not
> acquire a new reference when we build EnabledRegulator in Regulator::enable(),
> we merely move our instance of Regulator into EnabledRegulator.

and

>>> +impl Drop for EnabledRegulator {
>>> +    fn drop(&mut self) {
>>> +        // SAFETY: By the type invariants, we know that `self` owns a reference,
>>> +        // so it is safe to relinquish it now.
>>> +        unsafe { bindings::regulator_disable(self.as_ptr()) };
>>
>> Same here, what happens to the refcount?
>
> It remains the same, we never acquired one when we enabled, so we are relying
> on inner.drop() to decrement it.

I'll try to explain what my initial question regardless, maybe that'll
clear up your confusion :)

My initial question was whether `regulator_disable` would drop the
refcount (in the case of success and/or in the case of failure). Since
if it failed in the quoted code:

> +    /// Disables the regulator.
> +    pub fn disable(self) -> Result<Regulator> {
> +        // Keep the count on `regulator_get()`.
> +        let regulator = ManuallyDrop::new(self);
> +
> +        // SAFETY: Safe as per the type invariants of `Self`.
> +        let res = to_result(unsafe { bindings::regulator_disable(regulator.as_ptr()) });
So this call would fail                           ^^^^^^^^^^^^^^^^^

> +
> +        res.map(|()| Regulator {
> +            inner: regulator.inner.inner,
> +        })
> +    }

Then the `.map` call below the `regulator_disable` call would not take
ownership of the `inner.inner` value of the `regulator` variable, since
the `res` variable would be the `Err` variant of the `Result` enum (the
closure supplied to the `map` function only operates on the `Ok`
variant). This would mean that the refcount is not decremented, but the
`Regulator` wouldn't be available to the caller any more, thus leaking
the refcount.

Now if the `regulator_disable` function took ownership of the refcount
(ie decrement it, or store the regulator somewhere else that would own
that refcount), then this would be fine.

See my reply to Daniel as for what I suggest to do about the refcount
leak.

---
Cheers,
Benno

Powered by blists - more mailing lists