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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Hardware blinking is not disabled when setting a solid brightness.
- [Medium] StatusLedHandler::blink_set does not update delay_on and delay_off when subled_intensity == 0, causing incorrect sysfs blink rate reporting.
--

--- Patch [2]: [PATCH v17 2/2] platform: Add initial synology microp driver ---
commit 4d27f9676f69d36945bb122b6d9df48adadc7625
Author: Markus Probst <[email protected]>

platform: Add initial synology microp driver

This commit adds an initial Synology microp driver written in Rust to
control the power, status, alert, and USB LEDs on Synology NAS devices.
The driver targets a microcontroller found on these devices.

> diff --git a/drivers/platform/synology_microp/led.rs b/drivers/platform/synology_microp/led.rs
> new file mode 100644
> index 0000000000000..ce57adfd307f2
> --- /dev/null
> +++ b/drivers/platform/synology_microp/led.rs

[ ... ]

> +    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

[Severity: Medium]
Does this prevent transitioning the LED from a blinking state to a solid ON
state?

If userspace sets a hardware blink, the blink variable becomes true. Later, if
userspace writes a non-zero brightness to turn the LED solid ON, this code
path checks the blink variable and issues State::Blink instead of State::On,
leaving the hardware in a blinking state.

> +        } else {
> +            State::On
> +        })
> +        .write(dev)?;
> +
> +        Ok(())
> +    }

[ ... ]

> +    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

[Severity: Medium]
Similarly here, does this prevent the multi-color status LED from being set to
a solid ON state once it is blinking?

> +            } else {
> +                State::On
> +            },
> +        )
> +        .write(dev)
> +    }

[ ... ]

> +    fn blink_set<'bound>(
> +        &self,
> +        dev: &'bound Self::Bus,
> +        classdev: &led::MultiColorDevice<'bound, Self>,
> +        delay_on: &mut usize,
> +        delay_off: &mut usize,
> +    ) -> Result<()> {
> +        let mut blink = self.blink.lock();
> +        *blink = true;
> +
> +        let amber_intensity = classdev.subleds()[1].intensity();
> +
> +        let (color, subled_intensity) = if amber_intensity == 0 {
> +            (StatusLedColor::Green, classdev.subleds()[0].intensity())
> +        } else {
> +            (StatusLedColor::Amber, amber_intensity)
> +        };
> +        Command::StatusLed(
> +            color,
> +            if *delay_on == 0 && *delay_off != 0 {
> +                *blink = false;
> +                State::Off
> +            } else if subled_intensity == 0 {
> +                State::Off

[Severity: Medium]
Are delay_on and delay_off left un-updated when subled_intensity is 0?

If userspace configures a blink rate while the multi-color LED intensity is 0,
this returns State::Off to the hardware but successfully returns to the LED
core without updating the delay values to the fixed hardware blink rate.
This can cause sysfs to report the user-provided values rather than the actual
hardware rate.

> +            } else if *delay_on != 0 && *delay_off == 0 {
> +                *blink = false;
> +                State::On
> +            } else {
> +                *delay_on = BLINK_DELAY.as_millis() as usize;
> +                *delay_off = BLINK_DELAY.as_millis() as usize;
> +
> +                State::Blink
> +            },
> +        )
> +        .write(dev)
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2