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

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

Pre-existing issues:
- [High] The driver registers a shared IRQ but accesses hardware unconditionally in the handler without checking if the device is active. Furthermore, it powers down hardware in its runtime suspend callback without calling synchronize_irq().
- [High] Asymmetric unregistration in sun6i_csi_bridge_cleanup leaves the subdevice in the global async list, causing list corruption and use-after-free.
--

--- Patch [1]: [PATCH v5 1/2] media: sun6i-csi: bridge: Use V4L2 subdev active state ---
commit 83bd3dbd77fc8cb9460cfb43960a67e0c7ee1c83
Author: Arash Golgol <[email protected]>

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

This commit updates the driver to use the V4L2 subdev active state API
for storing the active format. It simplifies the driver by removing the
bridge mbus_format field and replacing the bridge lock with the state
lock. It also enforces identical formats on the sink and source pads.

> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> --- 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);

[Severity: High]
This is a pre-existing issue, but does this unconditional call to
v4l2_device_unregister_subdev() leave a dangling pointer in the global
async list if the device was registered asynchronously?

During setup in sun6i_csi_bridge_setup(), the subdevice is registered
using v4l2_async_register_subdev() when csi_dev->isp_available is true.
This adds the subdevice's async_list entry to the global subdev_list.
If v4l2_device_unregister_subdev() is called here during cleanup instead
of v4l2_async_unregister_subdev(), the entry remains in the global list,
which can lead to a use-after-free when the list is traversed later.

> +	v4l2_subdev_cleanup(subdev);
> +
>  	media_entity_cleanup(&subdev->entity);
>  }

I also noticed a couple of pre-existing issues in the related sun6i_csi.c file.

[Severity: High]
This is a pre-existing issue, but does the shared IRQ handler access hardware
while the device might be suspended?

When devices use shared IRQs, another device on the same IRQ line can trigger
an interrupt while this device is suspended.

In drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:sun6i_csi_interrupt():

    regmap_read(regmap, SUN6I_CSI_CH_INT_STA_REG, &status);
    regmap_read(regmap, SUN6I_CSI_CH_INT_EN_REG, &enable);

The handler performs unconditional MMIO reads without checking the PM state
via pm_runtime_get_if_active(). Reading powered-off hardware can cause a
bus fault or external abort.

Additionally, in sun6i_csi_suspend():

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

Is it safe to power down the hardware here without calling synchronize_irq()
first? Without waiting for any executing shared interrupt handlers to finish,
the IRQ handler could execute mid-flight during power down.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1