Re: [PATCH v5 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 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing NULL pointer check for `match_data->mipi` in `rkcif_mipi_isr()` leads to a kernel panic on devices without MIPI support.
- [High] Incorrect read-modify-write logic when clearing pending interrupts in the W1C `RKCIF_MIPI_INTSTAT` register.
- [High] The hardirq handler calls a function that acquires a mutex, leading to a sleep in atomic context BUG and potential deadlocks.
--

commit dbb001c2e64feff7a8b087afc4307525237930d8
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..bc9518f8db507 100644
> --- a/drivers/media/platform/rockchip/rkcif/rkcif-capture-mipi.c
> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-capture-mipi.c
[ ... ]
> @@ -631,6 +762,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);
> +

[Severity: High]
This isn't a bug introduced by this patch, but I noticed an issue in the
surrounding code for rkcif_mipi_start_streaming() and
rkcif_mipi_stop_streaming().

The RKCIF_MIPI_INTSTAT register is Write-1-to-Clear (W1C), which is
demonstrated by the ISR reading and writing back the exact same value to clear
interrupts. However, in rkcif_mipi_start_streaming():

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

Does this sequence incorrectly write 0 to the intended stream's interrupt bit,
failing to clear it, while writing 1 to any currently pending interrupts of
other active streams?

Could this erroneously clear interrupts for concurrent streams before the ISR
can handle them, potentially leading to timeouts or dropped frames?

[Severity: High]
This is a pre-existing issue, but I noticed a potential sleep in atomic
context BUG related to the stream stopping logic.

When stopping a stream, rkcif_stream_stop_streaming() (called from process
context) sets stream->stopping = true and waits.

The hardware interrupt handler detects this flag and directly calls
rkcif_mipi_stop_streaming():

rkcif_mipi_isr()
    if (stream->stopping) {
        rkcif_mipi_stop_streaming(stream);
        ...

rkcif_mipi_stop_streaming()
    state = v4l2_subdev_lock_and_get_active_state(&interface->sd);

Since v4l2_subdev_lock_and_get_active_state() acquires a mutex, and it is
being called from the hardirq handler rkcif_mipi_isr(), will this trigger a
sleep in atomic context BUG and potentially deadlock the CPU?

>  	ret = 0;
>  
>  out:
> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-dev.c b/drivers/media/platform/rockchip/rkcif/rkcif-dev.c
> index b4cf1146f1311..be3a174b9aab0 100644
> --- a/drivers/media/platform/rockchip/rkcif/rkcif-dev.c
> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-dev.c
> @@ -53,6 +53,20 @@ static const struct rkcif_match_data rk3568_vicap_match_data = {
>  	.mipi = &rkcif_rk3568_vicap_mipi_match_data,
>  };
>  
> +static const char *const rk3588_vicap_clks[] = {
> +	"aclk",
> +	"hclk",
> +	"dclk",
> +	"iclk",
> +	"iclk1",
> +};
> +
> +static const struct rkcif_match_data rk3588_vicap_match_data = {
> +	.clks = rk3588_vicap_clks,
> +	.clks_num = ARRAY_SIZE(rk3588_vicap_clks),
> +	.mipi = &rkcif_rk3588_vicap_mipi_match_data,
> +};
> +

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference related to the mipi match data.

In the global interrupt handler rkcif_isr(), it unconditionally calls
rkcif_mipi_isr():

rkcif_isr()
    ...
    if (rkcif_mipi_isr(irq, ctx) == IRQ_HANDLED)
        ret = IRQ_HANDLED;

Inside rkcif_mipi_isr(), the code iterates up to mipi_num:

rkcif_mipi_isr()
    for (unsigned int i = 0; i < rkcif->match_data->mipi->mipi_num; i++) {
        ...

For devices that do not support MIPI (such as rockchip,px30-vip) where
match_data->mipi is not defined (NULL), won't any hardware interrupt
triggered by the DVP interface cause a guaranteed NULL pointer dereference
and kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-rk3588-vicap-v5-0-d1d1f5265c56@collabora.com?part=3