Re: [PATCH v26 3/4] rust: leds: Add multicolor classdev abstractions

[Boqun Feng <boqun@kernel.org>]

On Fri, Oct 09, 2026 at 07:59:02PM +0000, Markus Probst wrote:
[..]
> > > > > +/// The multicolor sub led info representation.
> > > > > +///
> > > > > +/// This structure represents the Rust abstraction for a C `struct mc_subled`.
> > > > > +#[repr(C)]
> > > > > +#[derive(Debug)]
> > > > > +#[non_exhaustive]
> > > > > +pub struct MultiColorSubLed {
> > > > > +    /// The color of the sub led
> > > > > +    pub color: Color,
> > > > > +    brightness: UnsafeCell<u32>,
> > > > > +    intensity: UnsafeCell<u32>,
> > > > 
> > > > These should be `Atomic<u32>`, or am I missing something here? Using
> > > > `Atomic<u32>` should resolve sashiko's comment on this patch.
> > > Snippet of the `Atomic::from_ptr` rustdoc:
> > > 
> > > "
> > > For the duration of 'a, other accesses to *ptr must not cause data
> > > races (defined by LKMM) against atomic operations on the returned
> > > reference. Note that if all other accesses are atomic, then this safety
> > > requirement is trivially fulfilled.
> > > "
> > > 
> > > This safety requirement is likely not met if I see this correctly,
> > > because the led subsystem does not use atomic accesses.
> > > 
> > 
> > Then the C side has a data race that needs some fix (or they use
> > READ_ONCE() or WRITE_ONCE() which are *atomic* to avoid the data race).
> They don't use READ_ONCE or WRITE_ONCE.
> 
> Writes to "intensity" can happen at anytime by `multi_intensity_store`.
> It does lock the `led_access` mutex on write. It is not locked on read
> and `grep "READ_ONCE" -r drivers/leds/` has no matches in drivers
> either.
> 

I wonder whether KCSAN will report an issue of this (w/o
CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC).

> Writes to "brightness" are on the C-side handled by the driver by
> calling `led_mc_calc_color_components`. This rust abstraction always
> calles it in `brightness_set_callback`. So on the C-side, this at least
> is less of an issue, as writes and reads are controlled by the C
> driver.
> 

Thank you for taking a look into this.

> > 
> > The general rule is: if C side has a data race, they should fix it, if C
> > side doesn't care ("the compiler should not data race on this code"),
> > then the Rust side treat it as atomic operations. This is the only way
> > to better code regarding data races.
> It probably should use WRITE_ONCE and READ_ONCE, but it also shouldn't
> create any issues if its not used. There is no load tearing on a 32-bit
> integer and memory ordering is not required. Not sure if its worth the

I think some people would disagree with you on "no load tearing"
(because data race = UB = anything can happen), but..

> trouble changing every existing multicolor led driver.
> 

I agree it's probably not worth doing this at the moment.

> > 
> > > Ofc, this function won't be used, but I think given that the same
> > > struct is also accessed by the C-side, it should also apply here.
> > > 
> > > Like Sashiko suggests, "core::ptr::read_volatile()" might be a better
> > > option to prevent certain compiler optimizations.
> > > 
> > 
> > No, please don't over-use read_volatile(). The reason that READ_ONCE()
> > and WRITE_ONCE() are safe to use for synchronization is because
> > semantics-wise they are atomic on certain types (if aligned), and the
> > them being volatile is just an implementation detail.
> Ok.
> 
> I will need to make .get_mut() const for this.
> 

Sounds good to me.

Regards,
Boqun

> > 
> > Regards,
> > Boqun
> 
> Thanks
> - Markus Probst
> 
> > 
> > > Thanks
> > > - Markus Probst
> > > 
> > > > 
> > > > Regards,
> > > > Boqun
> > > > 
> > > > > +    /// The maximum supported intensity value.
> > > > > +    ///
> > > > > +    /// If None the maximum intensity equals to [`LedOps::MAX_BRIGHTNESS`].
> > > > > +    pub max_intensity: Option<NonZero<u32>>,
> > > > > +    /// Arbitrary data for the driver to store.
> > > > > +    pub channel: u32,
> > > > > +}
> > > > > +
> > > > [...]
> >