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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] rust: leds: unsound shared reference to Opaque in
  MultiColorDevice::subleds()
- [Low] rust: leds: broken intra-doc links in build_multicolor() and
  subleds()
- [Low] rust: leds: missing #[inline] on MultiColorDevice::deref() and
  clone()
--

commit c1c84e805b37bfbeeb46b779f3b5faebdc6abfeb
Author: Markus Probst <markus.probst@posteo.de>

rust: leds: Add multicolor classdev abstractions

This commit adds Rust abstractions for multicolor LED class devices,
including MultiColor, MultiColorSubLed, MultiColorDevice, and
DeviceBuilder::build_multicolor().

> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> index 0000000000000..58dd1a12bc3c0
> --- /dev/null
> +++ b/rust/kernel/led/multicolor.rs
[ ... ]
> +impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
> +    /// Registers a new [`MulticolorDevice`].
> +    pub fn build_multicolor<'bound: 'init, T: LedOps<Mode = MultiColor> + 'bound>(
[ ... ]
> +    /// Returns the subleds passed to [`Device::new_multicolor`].
> +    #[inline]
> +    pub fn subleds(&self) -> &[MultiColorSubLed] {

[Severity: Low]
Will these two intra-doc links break rustdoc builds when
CONFIG_LEDS_CLASS_MULTICOLOR is enabled?

The struct is named MultiColorDevice (with a capital C) rather than
MulticolorDevice, and Device::new_multicolor does not exist; subleds are
passed to DeviceBuilder::build_multicolor.

> +        // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
> +        // valid `led_classdev_mc`.
> +        let raw = unsafe { &*self.classdev.get() };
> +        // SAFETY: `raw.subled_info` is a valid pointer to `mc_subled[num_colors]`.
> +        // CAST: The safeguards in the const block ensure that `MultiColorSubLed` has an identical
> +        // layout to `mc_subled`.
> +        unsafe {
> +            core::slice::from_raw_parts(
> +                raw.subled_info.cast::<MultiColorSubLed>(),
> +                // CAST: It is guaranteed that `num_colors` fits into an `usize`.
> +                raw.num_colors as usize,
> +            )
> +        }
> +    }
> +}

[Severity: Medium]
Is it sound to create a shared reference &bindings::led_classdev_mc from
self.classdev.get() here?

MultiColorDevice wraps bindings::led_classdev_mc in Opaque because the
embedded struct led_classdev contains fields (such as work_flags,
blink_timer, set_brightness_work, and led_access) that the C LED core
mutates without UnsafeCell wrappers. Materializing &*self.classdev.get()
asserts that no bytes within bindings::led_classdev_mc are concurrently
mutated while the reference is live, which can race with C updates when a
driver calls subleds() on a shared &MultiColorDevice (for example from
brightness_set_callback() running in workqueue or timer context).

PinnedDrop::drop() also does let led_cdev = unsafe { &*raw }. Both sites
can read (*raw).subled_info and (*raw).num_colors directly through the raw
pointer instead of creating a shared struct reference.

> +
> +impl<'bound, T: LedOps + 'bound> Deref for MultiColorDevice<'bound, T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.ops
> +    }
> +}

[Severity: Low]
This isn't a bug, but small functions in kernel crate abstractions should
be marked #[inline]; both MultiColorDevice::deref() and
MultiColorSubLed::clone() are missing the attribute.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-rust_leds-v28-0-b8cf2f74fa57@posteo.de?part=3