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

[sashiko-bot@kernel.org]

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 dangling pointers in the global V4L2 async lists when `isp_available` is true.
- [High] The shared IRQ handler accesses hardware registers without checking if the device is active, which can cause bus errors when the device is runtime suspended.
- [High] The runtime suspend callback does not call `synchronize_irq()` before disabling clocks, risking mid-flight IRQ handler execution on powered-down hardware.
--

--- Patch [3]: [PATCH v5 3/3] media: sun6i-csi: bridge: Use V4L2 subdev active state ---
commit 6cd90e8e72a0cec4746b7a5b2e01f5104228f654
Author: Arash Golgol <arash.golgol@gmail.com>

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)
>  
>  	v4l2_device_unregister_subdev(subdev);
>  
> +	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 might
be worth looking into while working in this subsystem.

In sun6i_csi_interrupt():

	struct sun6i_csi_device *csi_dev = private;
	bool capture_streaming = csi_dev->capture.state.streaming;
	struct regmap *regmap = csi_dev->regmap;
	u32 status = 0, enable = 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 hardware
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 = 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 hardware?

Is it necessary to call synchronize_irq() before turning off the clocks here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804072630.28745-1-arash.golgol@gmail.com?part=3