Re: [PATCH v2 10/11] media: rcar-fcp: Handle resets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: rcar-fcp: hardware access while in reset due to PM ordering
- [Medium] media: rcar-fcp: missing reset assertion on driver unbind due to async PM
--

commit d0433a4b8db079b09c43d3564a42e501b20457b1
Author: Paul Elder <paul.elder+renesas@ideasonboard.com>

media: rcar-fcp: Handle resets

On some versions of the FCP, such as the one on the X5H, reset control
is required to operate the FCP. Add support for handling resets
optionally, to continue supporting versions that do not require it.

> diff --git a/drivers/media/platform/renesas/rcar-fcp.c b/drivers/media/platform/renesas/rcar-fcp.c
> index dfb0ca93e854d..76d4c250b3081 100644
> --- a/drivers/media/platform/renesas/rcar-fcp.c
> +++ b/drivers/media/platform/renesas/rcar-fcp.c

[ ... ]

> @@ -190,6 +197,24 @@ static void rcar_fcp_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  }

[Severity: Medium]
Will this leave the device powered on and out of reset if the driver is
unbound?

A client driver (like VSP1) calls rcar_fcp_disable(), which performs an
asynchronous pm_runtime_put(). If the rcar_fcp driver is removed via sysfs
immediately afterward, rcar_fcp_remove() calls pm_runtime_disable() which
cancels any pending asynchronous idle/suspend work.

Because rcar_fcp_remove() relies solely on pm_runtime_disable() without a
synchronous suspend (e.g., via pm_runtime_put_sync() or manually asserting
the reset), the device is trapped in RPM_ACTIVE and the reset control will be
released without being asserted.

> +static int fcp_pm_runtime_suspend(struct device *dev)
> +{
> +	struct rcar_fcp_device *fcp = dev_get_drvdata(dev);
> +
> +	return reset_control_assert(fcp->rstc);
> +}
> +
> +static int fcp_pm_runtime_resume(struct device *dev)
> +{
> +	struct rcar_fcp_device *fcp = dev_get_drvdata(dev);
> +
> +	return reset_control_deassert(fcp->rstc);
> +}

[Severity: High]
Does tying the reset control to runtime PM here cause the hardware to be
accessed while still in reset?

The vsp1 driver's PM resume callback (vsp1_pm_runtime_resume) calls
vsp1_device_init(), which in turn calls vsp1_reset_wpf(). This function
invokes rcar_fcp_soft_reset(), performing register I/O on the FCP:

drivers/media/platform/renesas/rcar-fcp.c:rcar_fcp_soft_reset() {
    ...
	rcar_fcp_write(fcp, RCAR_FCP_REG_RST, RCAR_FCP_REG_RST_SOFTRST);
	ret = readl_poll_timeout(fcp->base + RCAR_FCP_REG_STA, ...);
    ...
}

However, vsp1_pm_runtime_resume() only enables the FCP (via
rcar_fcp_enable()) after calling vsp1_device_init().

Since the FCP is not yet PM-resumed when rcar_fcp_soft_reset() executes, its
reset control is still asserted. Writing to hardware registers while the
device is held in reset causes a bus hang or imprecise external abort.

> +
> +static const struct dev_pm_ops fcp_pm_ops = {
> +	RUNTIME_PM_OPS(fcp_pm_runtime_suspend, fcp_pm_runtime_resume, NULL)
> +};
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919-epaul-v7-3-rc1-x5h-vsp-fcp-v2-0-dbe9236bfd22@ideasonboard.com?part=10