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

Markus Probst <[email protected]> Fri, 24 Jul 2026 17:35:22 +0000
Newsgroups	org.kernel.vger.linux-leds,dev.linux.lists.sashiko-reviews,org.kernel.vger.linux-devicetree
Message-ID	<[email protected]>
On Fri, 2026-07-24 at 17:21 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] The dynamically formatted `name` CString for the alert LED is dropped immediately after initialization, leaving a dangling pointer in the LED subsystem.
> - [High] The serdev device is never opened, causing subsequent LED operations to hang the kernel worker thread indefinitely.
> - [High] The `blink_set` callback sleeps by acquiring a Mutex and waiting for synchronous serial I/O, but it is called from an RCU read-side critical section.
> - [High] Incorrect struct field declaration order causes `power_name` to be freed before the `power` LED device is unregistered.
> - [Medium] Hardware blinking is not disabled when setting a solid brightness level, preventing the LED from turning on solidly.
> - [Medium] The driver lacks a device-level lock to serialize commands sent to the microcontroller over the shared serdev bus, allowing concurrent LED updates to interleave.
> --
> 
> --- Patch [2]: [PATCH v18 2/2] platform: Add initial synology microp driver ---
> commit 7eb4c67aa911615ee8b6562c3f14a129d9b90280
> Author: Markus Probst <[email protected]>
> 
> platform: Add initial synology microp driver
>     
> Add a initial synology microp driver, written in Rust.
> The driver targets a microcontroller found in Synology NAS devices. It
> currently only supports controlling of the power led, status led, alert
> led and usb led. Other components such as fan control or handling
> on-device buttons will be added once the required rust abstractions are
> there.
> 
> > diff --git a/drivers/platform/synology_microp/command.rs b/drivers/platform/synology_microp/command.rs
> > new file mode 100644
> > index 0000000000000..490719490391c
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/command.rs
> > @@ -0,0 +1,54 @@
> [ ... ]
> > +impl Command {
> > +    pub(crate) fn write(self, dev: &serdev::Device<Bound>) -> Result {
> > +        dev.write_all(
> 
> [Severity: Medium]
> Since multiple LEDs can be updated concurrently, does this write_all() call
> need a device-level lock to prevent interleaving commands on the shared serdev
> bus?
> 
> > +            match self {
> [ ... ]
> > +            },
> > +            0,
> > +        )?;
> > +        dev.wait_until_sent(0);
> 
> [Severity: High]
> Could this code cause a BUG by sleeping in an atomic context?
> 
> When the LED core invokes blink_set() (for instance, via a timer trigger),
> it holds an rwlock and executes in an RCU read-side critical section. The
> wait_until_sent() call performs a sleeping wait, which could cause a panic
> or instability when called from that atomic context.
BLOCKING = true is set in every LedOps implementation. Therefore
sleeping is allowed.
> 
> Additionally, could this block indefinitely if dev.open() hasn't been called?
This function does not exist. The serdev rust abstraction opens it
automatically before probe.

Also if it isn't open, it would cause a null pointer dereference
instead of blocking indefinitely.
> 
> > +        Ok(())
> > +    }
> > +}
> > diff --git a/drivers/platform/synology_microp/led.rs b/drivers/platform/synology_microp/led.rs
> > new file mode 100644
> > index 0000000000000..01cad98616fdf
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/led.rs
> > @@ -0,0 +1,304 @@
> [ ... ]
> > +#[pin_data]
> > +pub(crate) struct Data<'bound> {
> > +    #[pin]
> > +    status: led::MultiColorDevice<'bound, StatusLedHandler>,
> > +    power_name: CString,
> > +    #[pin]
> > +    power: led::Device<'bound, LedHandler>,
> 
> [Severity: High]
> Does this struct field declaration order cause a use-after-free during
> device unbinding?
> 
> In Rust, struct fields are dropped in the order they are declared. Because
> power_name is declared before power, the CString buffer for the name is freed
> first. The kernel would be left with a dangling pointer in led_cdev->name while
> the power LED device is still registered and active, which could lead to memory
> corruption if read concurrently before unregistration completes.
`power_name` only needs to outlive the initialization. It is copied by
the led subsystem on register.
> 
> > +    alert: Option<Pin<KBox<led::Device<'bound, LedHandler>>>>,
> > +    usb_copy: Option<Pin<KBox<led::Device<'bound, LedHandler>>>>,
> > +    esata: Option<Pin<KBox<led::Device<'bound, LedHandler>>>>,
> > +}
> > +
> > +impl<'bound> Data<'bound> {
> > +    pub(super) fn register<'a>(
> > +        dev: &'bound serdev::Device<Bound>,
> > +        model: &'a Model,
> > +    ) -> impl PinInit<Self, Error> + 'a
> > +    where
> > +        'bound: 'a,
> > +    {
> > +        pin_init_scope(move || {
> > +            Ok(try_pin_init!(Self {
> [ ... ]
> > +                alert: {
> > +                    if let Some(color) = model.led_alert {
> > +                        let name = CString::try_from_fmt(fmt!(
> > +                            "synology:{}:alarm",
> > +                            color.as_c_str().to_str()?
> > +                        ))?;
> > +                        Some(KBox::pin_init(
> > +                            led::DeviceBuilder::new().color(color).name(&name).build(
> 
> [Severity: High]
> Is the dynamically formatted name CString dropped immediately after this
> initialization block?
Yes.
> 
> The local CString variable is passed as a reference to the builder but is
> dropped at the end of the `if let` block. The LED classdev will retain a
> dangling pointer to its freed buffer, leading to a use-after-free when the LED
> subsystem accesses the name.
No, it will not retain a pointer. It copies the string in the
initialization.

> 
> > +                                dev,
> > +                                try_pin_init!(LedHandler {
> > +                                    blink <- new_mutex!(false),
> > +                                    command: Command::AlertLed,
> > +                                }),
> > +                            ),
> > +                            GFP_KERNEL,
> > +                        )?)
> > +                    } else {
> > +                        None
> > +                    }
> > +                },
> [ ... ]
> > +    fn brightness_set<'bound>(
> > +        &self,
> > +        dev: &'bound Self::Bus,
> > +        _classdev: &led::Device<'bound, 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]
> If the LED is currently blinking, does this logic prevent it from turning on
> solidly?
> 
> When a solid brightness is requested and the blink state is true, the state
> remains State::Blink. The blink state needs to be reset so the hardware can
> stop blinking and remain solidly on.
Led Subsystem documentation explicitly says
- "passing LED_OFF stops blinking"
- "passing LED_OFF stops hardware blinking"
- "To turn off blinking, use the API function led_brightness_set() with
brightness value LED_OFF, which should stop any software timers that
may have been required for blinking."

It is intended to continue blinking if a non-0 brightness is set.

See https://docs.kernel.org/leds/leds-class.html

> 
> > +        .write(dev)?;
> > +
> > +        Ok(())
> > +    }
> > +
> > +    fn blink_set<'bound>(
> > +        &self,
> > +        dev: &'bound Self::Bus,
> > +        _classdev: &led::Device<'bound, Self>,
> > +        delay_on: &mut usize,
> > +        delay_off: &mut usize,
> > +    ) -> Result<()> {
> > +        let mut blink = self.blink.lock();
> 
> [Severity: High]
> Is it safe to acquire this sleeping Mutex here?
> 
> Since the blink_set() callback can be called from an RCU read-side critical
> section by the LED core, acquiring a Mutex here could lead to a kernel panic.
BLOCKING = true is set in every LedOps implementation. Therefore
sleeping is allowed.

> 
> > +
> > +        (self.command)(if *delay_on == 0 && *delay_off != 0 {
> [ ... ]
> > +    fn brightness_set<'bound>(
> > +        &self,
> > +        dev: &'bound Self::Bus,
> > +        classdev: &led::MultiColorDevice<'bound, Self>,
> > +        brightness: u32,
> > +    ) -> Result<()> {
> > +        let mut blink = self.blink.lock();
> > +        if brightness == 0 {
> > +            *blink = false;
> > +        }
> > +
> > +        let amber_brightness = classdev.subleds()[1].brightness();
> > +
> > +        let (color, subled_brightness) = if amber_brightness == 0 {
> > +            (StatusLedColor::Green, classdev.subleds()[0].brightness())
> > +        } else {
> > +            (StatusLedColor::Amber, amber_brightness)
> > +        };
> > +
> > +        Command::StatusLed(
> > +            color,
> > +            if subled_brightness == 0 {
> > +                State::Off
> > +            } else if *blink {
> > +                State::Blink
> > +            } else {
> > +                State::On
> > +            },
> 
> [Severity: Medium]
> Does the multi-color status LED suffer from the same logic flaw as
> LedHandler::brightness_set(), where the LED stays blinking instead of turning
> solid when a solid brightness level is requested?
As explained above, it is not a flaw.
> 
> > +        )
> > +        .write(dev)
> > +    }
> > +
> > +    fn blink_set<'bound>(
> > +        &self,
> [ ... ]
> > diff --git a/drivers/platform/synology_microp/synology_microp.rs b/drivers/platform/synology_microp/synology_microp.rs
> > new file mode 100644
> > index 0000000000000..f7168309e6116
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/synology_microp.rs
> > @@ -0,0 +1,91 @@
> [ ... ]
> > +    fn probe<'bound>(
> > +        dev: &'bound serdev::Device<device::Core<'_>>,
> > +        model: Option<&'bound Model>,
> > +    ) -> impl PinInit<Self::Data<'bound>, kernel::error::Error> + 'bound {
> > +        pin_init_scope(move || {
> > +            let model = model.ok_or(EINVAL)?;
> > +
> > +            dev.set_baudrate(9600).map_err(|_| EINVAL)?;
> > +            dev.set_flow_control(false);
> > +            dev.set_parity(serdev::Parity::None)?;
> > +
> > +            Ok(try_pin_init!(SynologyMicropDriver {
> 
> [Severity: High]
> Is a call to dev.open() missing during this setup?
This function does not exist. The serdev rust abstraction automatically
opens the device before probe.
> 
> The probe function configures the serdev device but doesn't seem to open it.
> When subsequent operations try to write to the bus, the closed tty port will
> accept 0 bytes and cause the driver's write logic to hang forever.
Writing to a closed serdev device would case a null pointer
dereference, which does not happen in testing.
> 
> > +                led <- led::Data::register(dev, model),
> > +            }))
> > +        })
> > +    }
> > +}

This review does not contain a single issue. All false positives.
signature.asc (application/pgp-signature, 870 B)