Re: [PATCH 4/4] leds: add driver for synology atmega1608 controlled LEDs

[Markus Probst <markus.probst@posteo.de>]

On Thu, 2025-10-09 at 14:20 +0200, Danilo Krummrich wrote:
> (Not a full review (let's work out the dependencies first), but
> there's one
> thing that stood out to me.)
> 
> On Wed Oct 8, 2025 at 8:10 PM CEST, Markus Probst wrote:
> > +struct Atmega1608Led {
> > +    addr: Atmega1608LedAddress,
> > +    id: Atmega1608LedId,
> > +
> > +    client: ARef<I2cClient>,
> > +
> > +    mode_lock: Arc<Mutex<()>>,
> 
> Mutex<()> raises an eyebrow, since a mutex that doesn't protect
> anything is
> pointless. So, I assume it is protecting some data, but in an unsound
> way.
> 
> > +impl Atmega1608Led {
> > +    fn update_mode(&self, mode: Atmega1608LedMode) ->
> > Result<Atmega1608LedMode> {
> > +        let _guard = self.mode_lock.lock();
> 
> What exactly does the mutex protect in the code below?
Otherwise there would be a race condition. Each register has 8 bits,
each led has 2 bits. If the led mode is updated at the same time with
another one in the same register, it could lead to the first action
being overwritten by the second.
Meaning if two actions run at the same time:
- led0 reads from the register
- led1 reads from the register
- led0 writes to the register
- led1 writes to the register (the changes for led0 have been
overwritten here, as it did read the register before led0 has written
to it)

> 
> > +
> > +        let mut current = self
> > +            .client
> > +            .read_byte_data(self.addr as u8)
> > +            .inspect_err(|err| {
> > +                dev_err!(
> > +                    self.client.as_ref(),
> > +                    "failed to read {:#2x}: {err:?}\n",
> > +                    self.addr as u8
> > +                );
> > +            })?;
> > +
> > +        current =
> > +            (current & !self.id.mask()) | (((mode as u8) <<
> > self.id.shift()) & self.id.mask());
> > +
> > +        self.client
> > +            .write_byte_data(self.addr as u8, current)
> > +            .inspect_err(|err| {
> > +                dev_err!(
> > +                    self.client.as_ref(),
> > +                    "failed to write {:#2x}: {err:?}",
> > +                    self.addr as u8
> > +                );
> > +            })?;
> > +
> > +        Ok(mode)
> > +    }
> > +}

Thanks
- Markus Probst