Re: [PATCH v5 3/3] media: sun6i-csi: bridge: Use V4L2 subdev active state

Thank you for your contribution! Sashiko AI review found 3 potential issue(=
s) to consider:

Pre-existing issues:
- [High] Incomplete unregistration of the bridge V4L2 subdevice leaves dang=
ling pointers in the global V4L2 async lists when `isp_available` is true.
- [High] The shared IRQ handler accesses hardware registers without checkin=
g if the device is active, which can cause bus errors when the device is ru=
ntime suspended.
- [High] The runtime suspend callback does not call `synchronize_irq()` bef=
ore disabling clocks, risking mid-flight IRQ handler execution on powered-d=
own hardware.
--

--- Patch [3]: [PATCH v5 3/3] media: sun6i-csi: bridge: Use V4L2 subdev act=
ive state ---
commit 6cd90e8e72a0cec4746b7a5b2e01f5104228f654
Author: Arash Golgol <[email protected]>

media: sun6i-csi: bridge: Use V4L2 subdev active state

Use the V4L2 subdev active state API to store the active format.
This simplifies the driver not only by dropping the bridge mbus_format
field, but it also allows dropping the bridge lock, replaced with
the state lock.

> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c b/=
drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> index d006d9dd01708..43a85bcc2ba28 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
[ ... ]
> @@ -868,5 +847,7 @@ void sun6i_csi_bridge_cleanup(struct sun6i_csi_device=
 *csi_dev)
> =20
>  	v4l2_device_unregister_subdev(subdev);
> =20
> +	v4l2_subdev_cleanup(subdev);
> +
>  	media_entity_cleanup(&subdev->entity);
>  }

[Severity: High]
This isn't a regression introduced by this patch, but since isp_available
allows the bridge subdevice to be registered asynchronously via
v4l2_async_register_subdev() in sun6i_csi_bridge_setup(), should
sun6i_csi_bridge_cleanup() conditionally call v4l2_async_unregister_subdev()
instead?

Using v4l2_device_unregister_subdev() unconditionally skips removing the
subdevice from the global async list when isp_available is true. Might this
leave a dangling pointer that leads to list corruption upon module unload?

I also noticed a couple of pre-existing regressions in sun6i_csi.c that mig=
ht
be worth looking into while working in this subsystem.

In sun6i_csi_interrupt():

	struct sun6i_csi_device *csi_dev =3D private;
	bool capture_streaming =3D csi_dev->capture.state.streaming;
	struct regmap *regmap =3D csi_dev->regmap;
	u32 status =3D 0, enable =3D 0;

	regmap_read(regmap, SUN6I_CSI_CH_INT_STA_REG, &status);

[Severity: High]
This is a pre-existing regression, but the shared IRQ handler accesses hard=
ware
registers without checking if the device is active.

If another device sharing this IRQ line fires an interrupt while the sun6i-=
csi
device is runtime suspended, can this MMIO read cause a bus error?

Should pm_runtime_get_if_active() be used to verify the device is powered
before reading SUN6I_CSI_CH_INT_STA_REG?

In sun6i_csi_suspend():

	struct sun6i_csi_device *csi_dev =3D dev_get_drvdata(dev);

	reset_control_assert(csi_dev->reset);
	clk_disable_unprepare(csi_dev->clock_ram);
	clk_disable_unprepare(csi_dev->clock_mod);

[Severity: High]
This is a pre-existing regression, but since the device uses a shared IRQ, =
the
suspend routine disables clocks before ensuring no IRQ handler is currently
running.

If the interrupt handler is mid-flight on another CPU when the clocks are
disabled, could it attempt to access MMIO registers on powered-down hardwar=
e?

Is it necessary to call synchronize_irq() before turning off the clocks her=
e?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804072630.2874=
[email protected]?part=3D3