Re: [PATCH v14 08/12] iio: frequency: adf41513: driver implementation

[Rodrigo Alencar]

On 26/05/24 11:12AM, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The driver fails to restore the hardware register state during pm_resume if the device was placed in a software power-down state, leading to power leaks and cache corruption.
> --
> 
> commit 829389d24bcc81427dbc259466ce431d38418a05
> Author: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> iio: frequency: adf41513: driver implementation
> 
> This commit introduces a new driver for the Analog Devices ADF41513 PLL
> Frequency Synthesizer. It supports Integer-N and fractional-N synthesis
> modes, high-resolution frequency calculations, IIO debugfs interface,
> and power management.
> 
> > diff --git a/drivers/iio/frequency/adf41513.c b/drivers/iio/frequency/adf41513.c
> > new file mode 100644
> > index 0000000000000..6f952ccc675e0
> > --- /dev/null
> > +++ b/drivers/iio/frequency/adf41513.c
> [ ... ]
> > +static int adf41513_pm_resume(struct device *dev)
> > +{
> > +	struct adf41513_state *st = dev_get_drvdata(dev);
> > +
> > +	guard(mutex)(&st->lock);
> > +	if (st->powerdown)
> > +		return 0; /* nothing to do */
> 
> [Severity: High]
> Does returning early here when the device is in software power-down fail to
> restore the hardware register state?
> 
> If the system loses power to the device during suspend, the hardware
> registers will reset to their Power-On Reset defaults.
> 
> By returning immediately without writing any state to the hardware, won't
> the hardware wake up in an unconfigured state rather than the requested
> power-down state?
> 
> Additionally, the software cache regs_hw is not invalidated and still
> holds the pre-suspend values. If a parameter is subsequently modified,
> the sync function uses ADF41513_SYNC_DIFF and skips writing unmodified
> registers since they falsely appear up-to-date in regs_hw.
> 
> Could this leave the hardware permanently in a corrupted mixed state of
> reset defaults and newly written values?

This is a similar concern raised before. Any resume path will program
all the registers, which is essentially a power-up sequence. Partial
writes could happen, but they would not be important.

> > +
> > +	return adf41513_resume(st);
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260524-adf41513-iio-driver-v14-0-06824d9c15f4@xxxxxxxxxx?part=8

-- 
Kind regards,

Rodrigo Alencar