Re: [PATCH v4 3/8] media: rockchip: rkcif: add support for rk3588 vicap mipi capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unconditional overwrite of shared RKCIF_MIPI_CTRL register clears other bits and risks RK3568 regression.
- [High] Sleeping in atomic context due to mutex acquisition in the hardirq handler.
- [High] Incorrect read-modify-write on a Write-1-to-Clear (W1C) interrupt status register leads to lost interrupts.
- [High] Unhandled hardware power state causes an asynchronous external abort (SError) in the shared IRQ handler.
--

commit 8bafc9e50c3973309c417c259fe6059984848b43
Author: Michael Riesch <michael.riesch@collabora.com>

media: rockchip: rkcif: add support for rk3588 vicap mipi capture

The RK3588 Video Capture (VICAP) unit features a Digital Video Port (DVP)
and six MIPI CSI-2 capture interfaces. Add initial support for this variant
to the rkcif driver and enable the MIPI CSI-2 capture interfaces.

> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-capture-mipi.c b/drivers/media/platform/rockchip/rkcif/rkcif-capture-mipi.c
> index 9e67160a16e46..ad083dc9f5add 100644
> --- a/drivers/media/platform/rockchip/rkcif/rkcif-capture-mipi.c
> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-capture-mipi.c

[ ... ]

> @@ -481,6 +489,132 @@ const struct rkcif_mipi_match_data rkcif_rk3568_vicap_mipi_match_data = {
>  	},
>  };
>  

[ ... ]

> +const struct rkcif_mipi_match_data rkcif_rk3588_vicap_mipi_match_data = {
> +	.mipi_num = 6,

This patch activates the MIPI capture code path for 6 new RK3588 interfaces,
but does this expose an existing locking issue in the interrupt handler?

The interrupt handler rkcif_mipi_isr() runs in hardirq context and iterates
over active streams. If stream->stopping is true, it calls
rkcif_mipi_stop_streaming().

Inside that function, it calls v4l2_subdev_lock_and_get_active_state(),
which internally acquires a mutex.

Will acquiring a mutex within a hardirq context cause a sleeping in atomic
context panic when stopping a stream?

> +	.mipi_ctrl0 = rkcif_rk3588_mipi_ctrl0,
> +	.regs = {
> +		[RKCIF_MIPI_CTRL] = 0x20,
> +		[RKCIF_MIPI_INTEN] = 0x74,
> +		[RKCIF_MIPI_INTSTAT] = 0x78,

Since this patch adds support for the RK3588 MIPI INTSTAT registers, is it
safe to read them from the shared interrupt handler if the device is
suspended?

The driver requests its main interrupt line using IRQF_SHARED. If a shared
interrupt fires while the RKCIF device is runtime-suspended and its power
domains are disabled, the kernel will still invoke rkcif_mipi_isr().

The ISR then loops and reads the RKCIF_MIPI_INTSTAT registers across all
MIPI blocks.

Could reading from a power-gated or unclocked hardware block cause an
asynchronous external abort (SError) and a kernel panic? Should the ISR
verify the device's power state before accessing hardware registers?

Also related to these INTSTAT registers, does the current code handle
Write-1-to-Clear (W1C) bits correctly?

In rkcif_mipi_start_streaming(), the code attempts to clear stream-specific
interrupts using a read-modify-write sequence:

    int_temp = rkcif_mipi_read(..., RKCIF_MIPI_INTSTAT);
    int_temp &= ~int_mask;
    rkcif_mipi_write(..., RKCIF_MIPI_INTSTAT, int_temp);

By reading the register, clearing its own mask, and writing back the
remaining 1s, does this inadvertently write 1s to the pending interrupt bits
of other active streams?

Could this clear other streams' pending interrupts before the ISR can handle
them, while simultaneously failing to clear its own interrupt bits since it
writes 0 to them?

[ ... ]

> @@ -631,6 +765,13 @@ static int rkcif_mipi_start_streaming(struct rkcif_stream *stream)
>  	rkcif_mipi_stream_write(stream, RKCIF_MIPI_CTRL1, ctrl1);
>  	rkcif_mipi_stream_write(stream, RKCIF_MIPI_CTRL0, ctrl0);
>  
> +	/*
> +	 * TODO: This bit has a different meaning on the RK3568, but it is
> +	 * set there by default anyway. While correct, this is not exactly
> +	 * nice and shall be reworked during the next refactoring.
> +	 */
> +	rkcif_mipi_write(interface, RKCIF_MIPI_CTRL, RK3588_MIPI_CTRL_CAP_EN);

Could this unconditional write clear other configuration bits and cause a
regression on the RK3568?

Because rkcif_mipi_write() translates directly to writel(), this operation
blindly overwrites the entire 32-bit register with BIT(0). Since this code
path is shared with the RK3568 variant, does this destroy any default,
bootloader-initialized, or PHY-related configuration bits?

Additionally, RKCIF_MIPI_CTRL is an interface-level register shared across
up to 4 concurrent streams.

Would updating a single bit require a read-modify-write pattern to preserve
existing configurations from concurrently running streams?

Is this interface-level capture enable bit ever cleared in
rkcif_mipi_stop_streaming(), or does it leave the hardware perpetually
enabled?

> +
>  	ret = 0;
>  
>  out:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-rk3588-vicap-v4-0-6a6cd6f7c90b@collabora.com?part=3