Re: [PATCH v2 2/4] media: ipu6: Always run cleanup in ipu7 power on/off on timeout

[Antti Laakso <antti.laakso@linux.intel.com>]

Hi Manik,
On Tue, Sep 29, 2026 at 02:01:28PM +0530, Manik Bajpai wrote:
> __ipu7_power_on() and __ipu7_power_off() both return early when the
> pwr_status readl_poll_timeout() call times out, skipping cleanup that
> was previously done only on the success path:
> 
>  - __ipu7_power_on() left the clock-ownership override bit set in
>    SLEEP_LEVEL_CFG instead of clearing it, which could break
>    subsequent power transitions.
> 
>  - __ipu7_power_off() skipped D2D power-down and NDE-disable, leaving
>    those subsystems powered despite reporting a failure.
> 
>  - Additionally, the return value of ipu7_isys_d2d_power() in
>    __ipu7_power_off() was discarded, silently masking D2D power-down
>    failures.
> 
> Always run the clock-override clear / D2D-NDE teardown regardless of
> whether the status poll timed out, and propagate a D2D power-down
> failure through the return value when the poll itself succeeded.
> 
> Assisted-by: Claude:claude-sonnet-5
> Signed-off-by: Manik Bajpai <manik.bajpai@intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-buttress.c | 24 ++++++++++++--------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> index 105de1744dff..06a773b5b66b 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> @@ -530,16 +530,15 @@ static int __ipu7_power_on(struct device *dev,
>  	ret = readl_poll_timeout(isp->base + isp->buttress.regs->pwr_status,
>  				 val, (val & ctrl->pwr_sts_mask) == pwr_sts,
>  				 100, BUTTRESS_POWER_TIMEOUT_US);
> -	if (ret) {
> +	if (ret)
>  		dev_err(&isp->pdev->dev,
>  			"Change power status timeout with 0x%x\n", val);
> -		return ret;
> -	}
>  
> +	/* Always drop the clock override, even if the poll above timed out. */
>  	slp = readl(isp->base + IPU7_BUTTRESS_REG_SLEEP_LEVEL_CFG);
>  	writel(slp & ~ovrd_clk, isp->base + IPU7_BUTTRESS_REG_SLEEP_LEVEL_CFG);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int __ipu7_power_off(struct device *dev,
> @@ -555,18 +554,25 @@ static int __ipu7_power_off(struct device *dev,
>  	ret = readl_poll_timeout(isp->base + isp->buttress.regs->pwr_status,
>  				 val, (val & ctrl->pwr_sts_mask) == pwr_sts,
>  				 100, BUTTRESS_POWER_TIMEOUT_US);
> -	if (ret) {
> +	if (ret)
>  		dev_err(&isp->pdev->dev,
>  			"Change power status timeout with 0x%x\n", val);
> -		return ret;
> -	}
>  

I wonder if powering down D2D/NDE is right thing to do. In a caller,
bus_pm_runtime_suspend(), we try to recover from error and call
isys_runtime_pm_resume(). But after this change the D2D and NDE
would be powered off and recovery would not make sense.

> +	/* Always release D2D/NDE, even if the power status poll timed out. */
>  	if (ctrl->subsys_id == IPU_ISYS) {
> -		ipu7_isys_d2d_power(isp, false);
> +		int d2d_ret;
> +
> +		d2d_ret = ipu7_isys_d2d_power(isp, false);
> +		if (d2d_ret) {
> +			dev_err(&isp->pdev->dev,
> +				"D2D power down failed (%d)\n", d2d_ret);
> +			if (!ret)
> +				ret = d2d_ret;
> +		}
>  		ipu7_nde_control(isp, false);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int __ipu6_power(struct device *dev,
> -- 
> 2.53.0
>