Message-ID: <20250919215634.7a1c184e.gary@garyguo.net>
Date: Fri, 19 Sep 2025 21:56:34 +0100
From: Gary Guo <gary@...yguo.net>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: "Joel Fernandes" <joelagnelf@...dia.com>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "David Airlie"
 <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
 <rafael@...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>,
 "Bjorn Helgaas" <bhelgaas@...gle.com>, Krzysztof Wilczyński <kwilczynski@...nel.org>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 <linux-pci@...r.kernel.org>
Subject: Re: [PATCH] rust: io: use const generics for read/write offsets

On Fri, 19 Sep 2025 01:26:28 +0200
"Danilo Krummrich" <dakr@...nel.org> wrote:

> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
> > On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:  
> >> Using build_assert! to assert that offsets are in bounds is really
> >> fragile and likely to result in spurious and hard-to-debug build
> >> failures. Therefore, build_assert! should be avoided for this case.
> >> Thus, update the code to perform the check in const evaluation instead.  
> >
> > I really don't think this patch is a good idea (and nobody I spoke to thinks
> > so). Not only does it mess up the user's caller syntax completely, it is also  
> 
> I appreacite you raising the concern, but I rather have other people speak up
> themselves.

Joel asked me for opinion on this syntax during Kangrejos and I said
that I hated it.

> 
> > super confusing to pass both a generic and a function argument separately.  
> 
> Why? We assert that the offset has to be const, whereas the value does not
> have this requirement, so this makes perfect sense to me.

In some peripherals there are also cases where there are a window of
registers and you want to use a computed value to access them.
If I have a window of registers where the first address is `FOO` and
the last is `FOO_END`, then I might reasonable want to do access
registers in a loop.

> 
> (I agree that it can look unfamiliar at the first glance though.)
> 
> > Sorry if I knew this would be the syntax, I would have objected even when we
> > spoke :)
> >
> > I think the best fix (from any I've seen so far), is to move the bindings
> > calls of offending code into a closure and call the closure directly, as I
> > posted in the other thread. I also passed the closure idea by Gary and he
> > confirmed the compiler should behave correctly (I will check the code gen
> > with/without later). Gary also provided a brilliant suggestion that we can
> > call the closure directly instead of assigning it to a variable first. That
> > fix is also smaller, and does not screw up the users. APIs should fix issues
> > within them instead of relying on user to work around them.  
> 
> This is not a workaround, this is an idiomatic solution (which I probably should
> have been doing already when I introduced the I/O code).

I don't think this can be said to be idiomatic. Syntax churn is a real
issue, and quite a big one.

Turbofish is cumbersome to write with just magic numbers, and the
fact `{}` is needed to pass in constant expressions made this much
worse. If the drivers try hard to avoid magic numbers, you would
effective require  all code to be `::<{ ... }>()` and this is ugly.

In the design of generic atomics this is taken into consideration and
is why the `load`/`store` functions don't take `Ordering` type
parameter itself, but additionally takes a value of it and just throw it
away. This is so that you can still write `.load(Relaxed)` rather
than needing to write `.load::<Relaxed>()`.

If we have argument position const generics today, I'd say let's make
the switch. However with the tools that we have I don't think it's a
clear cut. I would even personally prefer a BUG than to having to do
turbofish every single time.

Best,
Gary

> 
> We do exactly the same thing for DmaMask::new() [1] and we agreed on doing the
> same thing for Alignment as well [2].
> 
> [1] https://rust.docs.kernel.org/kernel/dma/struct.DmaMask.html#method.new
> [2] https://lore.kernel.org/rust-for-linux/20250908-num-v5-1-c0f2f681ea96@nvidia.com/

Powered by blists - more mailing lists