Re: [PATCH] pmdomain: renesas: rmobile-sysc: propagate power-off timeout

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 3774 bytes --]

Hi,

Sashiko found issues with your patch. I curated the list and left only those
which I see as valid. Please have a look at these issues.

Thank you and happy hacking,

   Wolfram

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Propagating the power-down timeout without canceling the hardware request leads to state machine desynchronization and kernel panic.
> 
> Pre-existing issues:
> - [High] Inverted polling condition in `__rmobile_pd_power_up()` causes premature return before hardware is powered on.
> --
> 
> commit 8269f5684034da68db8ce9747b740d278e00d7c9
> Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
> 
> pmdomain: renesas: rmobile-sysc: propagate power-off timeout
> 
> This patch updates rmobile_pd_power_down() to propagate the return value
> of readl_poll_timeout_atomic(). This aims to prevent genpd from
> considering the domain powered off if the controller did not acknowledge
> the request.
> 
> > pmdomain: renesas: rmobile-sysc: propagate power-off timeout
> >
> > rmobile_pd_power_down() writes SPDCR and polls for the power-down
> > request bit to clear, but drops the poll result. It can therefore return
> > success to genpd even if the controller did not acknowledge the
> > power-off request.
> >
> > Return the poll timeout just like the matching power-on path already
> > does for SWUCR.
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but is the matching power-on path
> for SWUCR functioning correctly?
> 
> Looking at __rmobile_pd_power_up(), it uses readl_poll_timeout_atomic() with
> the condition (val & mask) after writing the bit to SWUCR to initiate power-on.
> Since the bit was just set to 1, this condition evaluates to true immediately.
> 
> Doesn't this cause the macro to return success instantly, completely failing
> to wait for the hardware to clear the bit to 0 to indicate the power-on
> sequence has completed?
> 
> Could this lead to a synchronous external abort when the device driver accesses
> MMIO registers before the power domain is physically active?
> 
> > diff --git a/drivers/pmdomain/renesas/rmobile-sysc.c b/drivers/pmdomain/renesas/rmobile-sysc.c
> > index 93103ff33d6e7..9abf60ea0c399 100644
> > --- a/drivers/pmdomain/renesas/rmobile-sysc.c
> > +++ b/drivers/pmdomain/renesas/rmobile-sysc.c
> 
> [ ... ]
> 
> > @@ -59,8 +60,11 @@ static int rmobile_pd_power_down(struct generic_pm_domain *genpd)
> >  	if (readl(rmobile_pd->base + PSTR) & mask) {
> >  		writel(mask, rmobile_pd->base + SPDCR);
> >  
> > -		readl_poll_timeout_atomic(rmobile_pd->base + SPDCR, val,
> > -					  !(val & mask), 0, PSTR_RETRIES);
> > +		ret = readl_poll_timeout_atomic(rmobile_pd->base + SPDCR, val,
> > +						!(val & mask), 0,
> > +						PSTR_RETRIES);
> > +		if (ret)
> > +			return ret;
> 
> [Severity: High]
> If this power-down request times out and we return -ETIMEDOUT to the genpd
> core, is the hardware request to SPDCR canceled?
> 
> When the genpd core receives this error, it marks the domain as ON. However,
> the hardware power-down request remains pending in the SPDCR register.
> If hardware conditions are met later, the controller could asynchronously power
> down the domain.
> 
> If the genpd core thinks the domain is ON, wouldn't subsequent device resumes
> skip the .power_on callback? This seems like it could lead to a bus fault when
> drivers attempt to access registers in a physically unpowered domain.
> 
> Should we explicitly cancel the pending power-down request before returning
> the error?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260623135439.44970-1-pengpeng@iscas.ac.cn?part=1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]