Re: [PATCH 06/10] media: rcar-fcp: Check device revision at probe time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] pm: async pm_runtime_put cancelled by pm_runtime_disable
- [Low] macro RCAR_FCP_REG_PICINFO1_STRIDE_DIV16 uses unbound identifier 'n'
--

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

media: rcar-fcp: Check device revision at probe time

This commit verifies that the device revision register reports a valid value
during initialization. It adds type information and rejects devices with an
unsupported version.

> diff --git a/drivers/media/platform/renesas/rcar-fcp.c b/drivers/media/platform/renesas/rcar-fcp.c
> index dfb0ca93e854d..d99b3ff976e80 100644
> --- a/drivers/media/platform/renesas/rcar-fcp.c
> +++ b/drivers/media/platform/renesas/rcar-fcp.c
> @@ -14,26 +14,75 @@
[ ... ]
> +#define RCAR_FCP_REG_TL_CTRL			0x0070
> +#define RCAR_FCP_REG_TL_CTRL_TLEN		BIT(31)
> +#define RCAR_FCP_REG_TL_CTRL_VPOS_C(n)		((n) << 16)
> +#define RCAR_FCP_REG_TL_CTRL_VPOS_Y(n)		((n) << 0)
> +
> +#define RCAR_FCP_REG_PICINFO1			0x00c4
> +#define RCAR_FCP_REG_PICINFO1_STRIDE_DIV16	((n) << 0)

[Severity: Low]
Is this macro missing its parameter list? It references the unbound identifier
n in its expansion but doesn't define it as an argument, unlike the
RCAR_FCP_REG_TL_CTRL_VPOS_C(n) macro above. 

Any code attempting to use this macro might fail to compile or silently 
capture a local variable named n.

[ ... ]
> @@ -169,6 +269,15 @@ static int rcar_fcp_probe(struct platform_device *pdev)
[ ... ]
>  	return 0;
> +
> +error_pm_put:
> +	pm_runtime_put(&pdev->dev);
> +error_pm_disable:
> +	pm_runtime_disable(&pdev->dev);
> +	return ret;
>  }

[Severity: Medium]
Can this error path leave the hardware powered on indefinitely?

When rcar_fcp_setup() returns an error, the error_pm_put path calls the
asynchronous pm_runtime_put(), which queues idle/suspend work. However, the
immediate call to pm_runtime_disable() will cancel that pending work.

Because the suspension is cancelled, the device hardware remains active with
its usage count decremented, causing a power resource leak. Should this use
pm_runtime_put_sync() instead to ensure the device is properly suspended
before runtime PM is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917-epaul-v7-3-rc1-x5h-vsp-fcp-v1-0-4c72d4e086dd@ideasonboard.com?part=6