Message-ID: <nlngenb6udempavyevw62qvdzuo7jr4m5mt4fwvznza347vicl@ynn4c5lojoub>
Date: Thu, 27 Feb 2025 11:25:55 +1100
From: Alistair Popple <apopple@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>, 
	gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com, ojeda@...nel.org, 
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, 
	benno.lossin@...ton.me, tmgross@...ch.edu, a.hindborg@...sung.com, aliceryhl@...gle.com, 
	airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net, 
	pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com, robh@...nel.org, 
	daniel.almeida@...labora.com, saravanak@...gle.com, dirk.behme@...bosch.com, j@...nau.net, 
	fabien.parent@...aro.org, chrisi.schrefl@...il.com, paulmck@...nel.org, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org, 
	devicetree@...r.kernel.org, rcu@...r.kernel.org
Subject: Re: [PATCH v7 07/16] rust: add `io::{Io, IoRaw}` base types

On Tue, Feb 25, 2025 at 12:04:35PM +0100, Danilo Krummrich wrote:
> On Tue, Feb 25, 2025 at 04:50:05PM +1100, Alistair Popple wrote:
> > Kind of, but given the current state of build_assert's and the impossiblity of
> > debugging them should we avoid adding them until they can be fixed?
> 
> If you build the module as built-in the linker gives you some more hints, but
> I agree it's still not great.

Yeah, that is how I eventually figured it out as a result of trying to resolve
the "undefined symbol" build error.

> I think build_assert() is not widely used yet and, until the situation improves,
> we could also keep a list of common pitfalls if that helps?

I've asked a few times, but are there any plans/ideas on how to improve the
situation? I'm kind of suprised we're building things on top of a fairly broken
feature without an idea of how we might make that feature work. I'd love to
help, but being new to R4L no immediately useful ideas come to mind.

At the very least if we could produce something more informative in the output
than the objtool "falls through to next function" warning and the undefined
reference that would help. I'm not sure if there is something we could do in the
build system to detect that and print a build-time hint along the lines of "hey,
you probably hit a build assert".

> > Unless the code absolutely cannot compile without them I think it would be
> > better to turn them into runtime errors that can at least hint at what might
> > have gone wrong. For example I think a run-time check would have been much more
> > appropriate and easy to debug here, rather than having to bisect my changes.
> 
> No, especially for I/O the whole purpose of the non-try APIs is to ensure that
> boundary checks happen at compile time.

To be honest I don't really understand the utility here because the compile-time
check can't be a definitive check. You're always going to have to fallback to
a run-time check because at least for PCI (and likely others) you can't know
for at compile time if the IO region is big enough or matches the compile-time
constraint.

So this seems more like a quiz for developers to check if they really do want
to access the given offset. It's not really doing any useful compile-time bounds
check that is preventing something bad from happening, becasue that has to
happen at run-time. Especially as the whole BAR is mapped anyway.

Hence why I think an obvious run-time error instead of an obtuse and difficult
to figure out build error would be better. But maybe I'm missing some usecase
here that makes this more useful.

> > I was hoping I could suggest CONFIG_RUST_BUILD_ASSERT_ALLOW be made default yes,
> > but testing with that also didn't yeild great results - it creates a backtrace
> > but that doesn't seem to point anywhere terribly close to where the bad access
> > was, I'm guessing maybe due to inlining and other optimisations - or is
> > decode_stacktrace.sh not the right tool for this job?
> 
> I was about to suggest CONFIG_RUST_BUILD_ASSERT_ALLOW=y to you, since this will
> make the kernel panic when hitting a build_assert().
> 
> I gave this a quick try with [1] in qemu and it lead to the following hint,
> right before the oops:
> 
> [    0.957932] rust_kernel: panicked at /home/danilo/projects/linux/nova/nova-next/rust/kernel/io.rs:216:9:
> 
> Seeing this immediately tells me that I'm trying to do out of bound I/O accesses
> in my driver, which indeed doesn't tell me the exact line (in case things are
> inlined too much to gather it from the backtrace of the oops), but it should be
> good enough, no?

*smacks forehead*

Yes. So to answer this question:

> or is decode_stacktrace.sh not the right tool for this job?

No, it isn't. Just reading the kernel logs properly would have been a better
option! I guess coming from C I'm just too used to jumping straight to the stack
trace in the case of BUG_ON(), etc. Thanks for point that out.

> [1]
> 
> diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> index 1fb6e44f3395..2ff3af11d711 100644
> --- a/samples/rust/rust_driver_pci.rs
> +++ b/samples/rust/rust_driver_pci.rs
> @@ -13,7 +13,7 @@ impl Regs {
>      const OFFSET: usize = 0x4;
>      const DATA: usize = 0x8;
>      const COUNT: usize = 0xC;
> -    const END: usize = 0x10;
> +    const END: usize = 0x2;
>  }
> 
>  type Bar0 = pci::Bar<{ Regs::END }>;

Powered by blists - more mailing lists