Re: [PATCH v1 3/3] rust: sync: atomic: Add i8/i16 xchg and cmpxchg support

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Dec 29, 2025 at 10:04:39PM +0900, FUJITA Tomonori wrote:
> On Mon, 29 Dec 2025 20:30:58 +0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Mon, Dec 29, 2025 at 08:27:01PM +0800, Boqun Feng wrote:
> >> On Sun, Dec 28, 2025 at 09:05:46PM +0900, FUJITA Tomonori wrote:
> >> > Add atomic xchg and cmpxchg operation support for i8 and i16 types
> >> > with tests.
> >> > 
> >> 
> >> I think we also needs the following, otherwise architectures may
> >> accidentally enable Rust but don't have the correct atomic
> >> implementation for i8 and i16.
> >> 
> >> diff --git a/rust/kernel/sync/atomic/predefine.rs b/rust/kernel/sync/atomic/predefine.rs
> >> index 248d26555ccf..a4e5bbd45eb2 100644
> >> --- a/rust/kernel/sync/atomic/predefine.rs
> >> +++ b/rust/kernel/sync/atomic/predefine.rs
> >> @@ -5,14 +5,22 @@
> >>  use crate::static_assert;
> >>  use core::mem::{align_of, size_of};
> >> 
> >> +// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
> >> +// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
> >> +//
> >>  // SAFETY: `i8` has the same size and alignment with itself, and is round-trip transmutable to
> >>  // itself.
> >> +#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
> >>  unsafe impl super::AtomicType for i8 {
> >>      type Repr = i8;
> >>  }
> >> 
> >> +// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
> >> +// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
> >> +//
> >>  // SAFETY: `i16` has the same size and alignment with itself, and is round-trip transmutable to
> >>  // itself.
> >> +#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
> >>  unsafe impl super::AtomicType for i16 {
> >>      type Repr = i16;
> >>  }
> >> 
> >> I can fold it into your patch if that works.
> >> 
> > 
> > OK, the right place should be at AtomicImpl instead of AtomicType:
> > 
> > diff --git a/rust/kernel/sync/atomic/internal.rs b/rust/kernel/sync/atomic/internal.rs
> > index ac689ce8ee8c..f4760e3a916e 100644
> > --- a/rust/kernel/sync/atomic/internal.rs
> > +++ b/rust/kernel/sync/atomic/internal.rs
> > @@ -37,10 +37,16 @@ pub trait AtomicImpl: Sized + Send + Copy + private::Sealed {
> >      type Delta;
> >  }
> > 
> > +// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
> > +// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
> > +#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
> >  impl AtomicImpl for i8 {
> >      type Delta = Self;
> >  }
> > 
> > +// The current helpers of load/store uses `{WRITE,READ}_ONCE()` hence the atomicity is only
> > +// guaranteed against read-modify-write operations if the architecture supports native atomic RmW.
> > +#[cfg(CONFIG_ARCH_SUPPORTS_ATOMIC_RMW)]
> >  impl AtomicImpl for i16 {
> >      type Delta = Self;
> >  }
> 
> With the above change, won't it cause a compile error on architectures
> where CONFIG_ARCH_SUPPORTS_ATOMIC_RMW is disabled?
> 
> If that is intended, I'm fine with it.
> 

Right, the intention is to cause build errors because then we need to
add lock-based atomic_{i8,i16}_read() and atomic_{i8,i16}_set() when
those archs begin to support Rust.

Regards,
Boqun