Message-ID: <20260204091104.0a9c4a13@fedora>
Date: Wed, 4 Feb 2026 09:11:04 +0100
From: Boris Brezillon <boris.brezillon@...labora.com>
To: "Gary Guo" <gary@...yguo.net>
Cc: "Daniel Almeida" <daniel.almeida@...labora.com>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Maxime Ripard" <mripard@...nel.org>, "Rafael J.
 Wysocki" <rafael@...nel.org>, "Viresh Kumar" <viresh.kumar@...aro.org>,
 "Danilo Krummrich" <dakr@...nel.org>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Thomas Zimmermann"
 <tzimmermann@...e.de>, "David Airlie" <airlied@...il.com>, "Simona Vetter"
 <simona@...ll.ch>, "Drew Fustini" <fustini@...nel.org>, "Guo Ren"
 <guoren@...nel.org>, "Fu Wei" <wefu@...hat.com>, Uwe Kleine-König <ukleinek@...nel.org>, "Michael Turquette"
 <mturquette@...libre.com>, "Stephen Boyd" <sboyd@...nel.org>, "Miguel
 Ojeda" <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor
 Gross" <tmgross@...ch.edu>, <linux-pm@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 <linux-riscv@...ts.infradead.org>, <linux-pwm@...r.kernel.org>,
 <linux-clk@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

Hi Gary, Daniel,

On Tue, 03 Feb 2026 20:36:30 +0000
"Gary Guo" <gary@...yguo.net> wrote:

> On Tue Feb 3, 2026 at 7:26 PM GMT, Daniel Almeida wrote:
> >  
> >> 
> >> I think it's fine to have all of these:
> >> * `Clone` impl
> >> * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
> >> * `with_enabled` that gives `&Clk<Enabled>`
> >> 
> >> This way, if you only want to enable in short time, you can do `with_enabled`.
> >> If the closure callback wants to keep clock enabled for longer, it can just do
> >> `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
> >> 
> >> If the user just have a reference and want to enable the callback they can do
> >> `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
> >> 
> >> Best,
> >> Gary  
> >
> >
> > I’m ok with what you proposed above. The only problem is that implementing
> > clone() is done through an Arc<*mut bindings::clk>  in Boris’ current
> > design, so this requires an extra allocation.  
> 
> Hmm, that's a very good point. `struct clk` is already a reference into
> clk_core, so having to put another level of indirection over is not ideal.
> However, if we're going to keep C code unchanged and do a zero-cost abstraction
> on the Rust side, then we won't be able to have have multiple prepare/enable to
> the same `struct clk` with the current design.
> 
> It feels like we can to do a trade-off and choose from:
> 1. Not be able to have multiple prepare/enable calls on the same `clk` (this can
>    limit users that need dynamically enable/disable clocks, with the very limited
>    exception that closure-callback is fine).
> 2. Do an extra allocation
> 3. Put lifetime on types that represent a prepared/enabled `Clk`
> 4. Change C to make `struct clk` refcounted.

It probably comes to no surprise that I'd be more in favor of option 2
or 4. Maybe option 2 first, so we can get the user-facing API merged
without too much churn, and then we can see if the clk maintainers are
happy adding a refcnt to struct clk to optimize things.

If we really feel that the indirection/memory overhead is going to
hurt us, we can also start with option 1, and extend it to 2 and/or 4
(needed to add a Clone support) when it becomes evident we can't do
without it. But as I was saying in my previous reply to Daniel, I
expect the extra indirection/memory overhead to be negligible since:

1. clks are usually not {prepared,enabled}/{disabled,unprepared} in a
   hot path
2. in the rare occasions where they might be ({dev,cpu}freq ?), this
   clk state change is usually one operation in an ocean of other
   slower operations (regulator reconfiguration, for instance, which
   usually goes over a slow I2C bus, or a
   relatively-faster-but-still-slow SPI one, at least when we compare
   it to an IoMem access for in-SoCs clks). So overall, the clk state
   change might account for a very small portion of the CPU cycles
   spent in this bigger operation
3. if I focus solely on the clk aspect, and look at the existing
   indirections in the clk framework (clk -> clk_core -> clk_{hw,ops} ->
   clk_methods), I'd expect the Arc indirection to be just noise in
   this pre-existing overhead
4. in term of memory, we're talking about 16 more bytes allocated per
   Clk on a 64-bit architecture (refcount is an int, but the alignment
   for the clk pointer forces 4 bytes of padding on most
   architectures). On a 64 bit arch, struct clk is 72 bytes if my math
   is correct, so that's a 22% overhead, compared to 11% overhead if
   the refcount was in struct clk (or in a struct
   refcounted_clk variant if we don't want C users to pay the price).
   Not great, but not terrible either

So yeah my gut feeling is that we might be overthinking this extra
allocation/indirection issue. This being said, one thing I'd really like
to avoid is us being dragged into infinite discussions about a perfect
implementation causing the merging of these changes to be delayed and
other contributions being blocked on this (perfect is the enemy of
good). I mean, option #1 is already an improvement compared to the raw
functions we have at the moment, so if that's the middle-ground we
agree on, I'm happy to give it my R-b.

Regards,

Boris

Powered by blists - more mailing lists