Re: [PATCH v13 2/2] platform: Add initial synology microp driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The dynamically allocated name for the conditionally registered alert LED is dropped prematurely, leaving a dangling pointer in the LED subsystem.
- [Medium] Concurrent LED operations can interleave serial commands, corrupting the communication protocol with the microcontroller.
- [Medium] Setting an LED's brightness to a non-zero value fails to override and disable hardware blinking.
- [Low] The config SYNOLOGY_MICROP depends on a non-existent symbol RUST_SERIAL_DEV_BUS_ABSTRACTIONS.
--

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

platform: Add initial synology microp driver

Add an initial synology microp driver, written in Rust. The driver targets a
microcontroller found in Synology NAS devices to support controlling of the
power led, status led, alert led and usb led.

> diff --git a/drivers/platform/synology_microp/Kconfig b/drivers/platform/synology_microp/Kconfig
> new file mode 100644
> --- /dev/null
> +++ b/drivers/platform/synology_microp/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config SYNOLOGY_MICROP
> +	tristate "Synology Microp driver"
> +	depends on LEDS_CLASS && LEDS_CLASS_MULTICOLOR
> +	depends on RUST_SERIAL_DEV_BUS_ABSTRACTIONS

[Severity: Low]
Does RUST_SERIAL_DEV_BUS_ABSTRACTIONS exist in the kernel tree?

Kconfig guidelines state that all dependencies must correspond to a valid
config definition.

Could this missing symbol cause silent build failures by preventing the
driver from ever being enabled?

[ ... ]
> diff --git a/drivers/platform/synology_microp/command.rs b/drivers/platform/synology_microp/command.rs
> new file mode 100644
> --- /dev/null
> +++ b/drivers/platform/synology_microp/command.rs
[ ... ]
> +impl Command {
> +    pub(crate) fn write(self, dev: &serdev::Device<Bound>) -> Result {
> +        dev.write_all(
> +            match self {
[ ... ]
> +                Self::EsataLed(led::State::Off) => &[0x4C, 0x45, 0x33],
> +            },
> +            serdev::Timeout::Max,
> +        )?;
> +        dev.wait_until_sent(serdev::Timeout::Max);
> +        Ok(())
> +    }
> +}

[Severity: Medium]
Since multiple LEDs share the same serdev bus, can their operations (like
brightness_set and blink_set) be called concurrently from different sysfs
contexts?

Without a driver-wide lock protecting the dev.write_all() and
dev.wait_until_sent() sequence here, could concurrent calls interleave
bytes in the UART transmit buffer and corrupt the communication protocol
with the microcontroller?

[ ... ]
> diff --git a/drivers/platform/synology_microp/led.rs b/drivers/platform/synology_microp/led.rs
> new file mode 100644
> --- /dev/null
> +++ b/drivers/platform/synology_microp/led.rs
[ ... ]
> +impl Data {
> +    pub(super) fn register<'a>(
> +        dev: &'a serdev::Device<Bound>,
> +        model: &'a Model,
> +    ) -> impl PinInit<Self, Error> + 'a {
> +        pin_init_scope(move || {
> +            if let Some(color) = model.led_alert {
> +                let name =
> +                    CString::try_from_fmt(fmt!("synology:{}:alarm", color.as_c_str().to_str()?))?;
> +                devres::register(
> +                    dev.as_ref(),
> +                    led::DeviceBuilder::new().color(color).name(&name).build(

[Severity: Critical]
Since the name variable is a local CString scoped to the if let block, won't
it be dropped at the end of the block?

The LED core doesn't copy the string, so could passing the reference &name
leave a dangling pointer that triggers a use-after-free when sysfs,
tracepoints, or triggers access it later?

> +                        dev,
> +                        try_pin_init!(LedHandler {
> +                            blink <- new_mutex!(false),
> +                            command: Command::AlertLed,
> +                        }),
> +                    ),
> +                    GFP_KERNEL,
> +                )?;
> +            }
[ ... ]
> +    fn brightness_set(
> +        &self,
> +        dev: &Self::Bus,
> +        _classdev: &led::Device<Self>,
> +        brightness: u32,
> +    ) -> Result<()> {
> +        let mut blink = self.blink.lock();
> +        (self.command)(if brightness == 0 {
> +            *blink = false;
> +            State::Off
> +        } else if *blink {
> +            State::Blink
> +        } else {
> +            State::On
> +        })

[Severity: Medium]
The LED subsystem API semantics normally dictate that writing a non-zero
value to the brightness sysfs file must stop any active hardware blinking
and turn the LED solid on.

By preserving State::Blink here when brightness is greater than 0, does this
force users to explicitly write 0 to brightness to stop the blinking before
they can turn it solid on?

> +        .write(dev)?;
> +
> +        Ok(())
> +    }
[ ... ]
> +    fn brightness_set(
> +        &self,
> +        dev: &Self::Bus,
> +        classdev: &led::MultiColorDevice<Self>,
> +        brightness: u32,
> +    ) -> Result<()> {
[ ... ]
> +        Command::StatusLed(
> +            color,
> +            if subled_brightness == 0 {
> +                State::Off
> +            } else if *blink {
> +                State::Blink
> +            } else {
> +                State::On
> +            },
> +        )

[Severity: Medium]
Similarly to the standard LedHandler, does preserving State::Blink here
when setting a non-zero brightness break the LED subsystem expectation
that brightness > 0 overrides and stops blinking?

> +        .write(dev)
> +    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530-synology_microp_initial-v13-0-aa2bb9c56efc@posteo.de?part=2