Message-ID: <aMIqGBoNaJ7rUrYQ@yury>
Date: Wed, 10 Sep 2025 21:47:04 -0400
From: Yury Norov <yury.norov@...il.com>
To: Joel Fernandes <joelagnelf@...dia.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	dakr@...nel.org, acourbot@...dia.com,
	Alistair Popple <apopple@...dia.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	bjorn3_gh@...tonmail.com, Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	John Hubbard <jhubbard@...dia.com>, Timur Tabi <ttabi@...dia.com>,
	joel@...lfernandes.org, Elle Rhumsaa <elle@...thered-steel.dev>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	nouveau@...ts.freedesktop.org
Subject: Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield

On Wed, Sep 10, 2025 at 07:08:43PM -0400, Joel Fernandes wrote:
> > You've got only one negative test that covers the .from() method.
> > Can you add more?
> 
> Sure, but note that we can only add negative tests if there is a chance of
> failure, which at runtime can mainly happen with the fallible usage (?=>
> pattern). Also just to note, we already at ~300 lines of test code now :)
> 
> > 
> > What if I create a bitfield from a runtime value that exceeds
> > the capacity?
> > 
> >     bitfield! {
> >         struct bf: u8 {
> >             0:0       ready       as bool;
> >             1:1       error       as bool;
> >             3:2       state       as u32;
> Here you mean 'as u8', otherwise it wont compile.

No, I meant u32. Can you explain this limitation in docs please? From
a user perspective, the 'state' is a 2-bit variable, so any type wider
than that should be OK to hold the data. If it's just an implementation
limitation, maybe it's worth to relax it?
 
> >        }
> >     }
> > 
> >     let raw_value: u8 = 0xff;
> >     let bf = bf::from(raw_value);
> > 
> > I guess you'd return None or similar.
> 
> No, we would ignore the extra bits sent. There is a .raw() method and 'bf' is
> 8-bits, bf.raw() will return 0xff. So it is perfectly valid to do so.

So I'm lost. Do you ignore or keep untouched?

Imagine a code relying on the behavior you've just described. So, I
create a 5-bit bitfield residing in a u8 storage, and my user one
day starts using that 3-bit tail for his own purposes.

Is that OK? Can you guarantee that any possible combination of methods
that you've implemented or will implement in future will keep the tail
untouched?

In bitmaps, even for a single-bit bitmap the API reserves the whole word,
thus we have a similar problem. And we state clearly that any bit beyond
the requested area is 'don't care'. It's OK for C. Is it OK for rust?

(And even that, we have a couple of functions that take care of tails
for some good reasons.)

So the question is: do you
 - provide the same minimal guarantees as C does (undefined behavior); or
 - preserve tails untouched, so user can play with them; or
 - clean the tails for user; or
 - reject such requests?

Or something else? Whichever option you choose, please describe
it explicitly.

> I don't
> think we should return None here, this is also valid in C.

This doesn't sound like an argument in the rust world, isn't? :) I've
been told many times that the main purpose of rust is the bullet-proof
way of coding. Particularly: "rust is free of undefined behavior gray
zone".  

> > Can you add such a test?
> 
> Sure, I added such a test.
> 
> > The same question for the setters. What would happen for this:
> > 
> >     let bf = bf::default()
> >              .set_state(0xf)
> >              .set_ready(true);
> > 
> > I think that after the first out-of-boundary in set_state(), you
> > should abort the call chain, make sure you're not touching memory
> > in set_ready() and returning some type of error.
> 
> Here, on out of boundary, we just ignore the extra bits passed to set_state. I
> think it would be odd if we errored out honestly. We are using 'as u8' in the
> struct so we would accept any u8 as input, but then if we complained that extra
> bits were sent, that would be odd.

That really depends on your purpose. If your end goal is the safest API
in the world, and you're ready to sacrifice some performance (which is
exactly opposite to the C case), then you'd return to your user with a
simple question: are you sure you can fit this 8-bit number into a 3-bit
storage?   

> In C also this is valid. If you passed a
> higher value than what the bitfield can hold, the compiler will still just use
> the bits that it needs and ignore the rest.

In C we've got FIELD_{PREP,GET,MODIFY}, implementing the checks.
So those who want to stay on safe side have a choice.

> Now, I am not opposed to error'ing out on that, but that's not what we currently
> do and it is also not easy to do. The setters in the patch return Self, not
> Result<Self>, so they are infallible, which is what allows them to be chained as
> well (builder pattern).

That 'chainability' looks optional to me, not saying weird, anyways. 

> I added another test here as well, to ensure the behavior is as I describe.
> 
> > 
> > And for this:
> > 
> >     let ret: u32 = -EINVAL;
> >     bf = bf::default();
> >     bf = bf.set_state(ret);
> > 
> > For compile-time initializes, it should be a compile-time error, right?
> 
> Yes, since the struct in this example is u8, this wont compile. Yes, I will add
> a comment.

So, the following would work?

     bitfield! {
         struct bf: u32 {
             0:0       ready       as bool;
             1:1       error       as bool;
             3:2       state       as u32;
             ...
         }
     }

     let state: u32 = some_C_wrapper(); // returns 0..3 or -EINVAL
     bf = bf::default();
     bf = bf.set_state(state);

That doesn't look right...

> > Can you drop a comment on that?
> 
> Yes, I will do so.
> 
> > 
> > I tried to apply your series on top of master, but it failed. So
> > my apologies for not finding the answers to some questions above
> > by myself.
> 
> Oh ok, I applied it on top of drm-rust-next. I will rebase on -next for the next
> revision, thanks.
> 
> > For the next version, can you make sure your series is applicable
> > on top of master or -next?
> Sure, thanks.
>  - Joel

Powered by blists - more mailing lists