Re: [PATCH v2 4/4] drm/verisilicon: add Nuvoton MA35D1 DCU Lite display controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Pre-existing early IRQ request race condition leads to a NULL pointer dereference crash during probe.
- [High] The bridge_enable callback fails to set the PANEL_CONFIG.RUNNING bit, contradicting the commit message and preventing panel output.
- [High] Framebuffer enablement is incorrectly tied to the CRTC lifecycle rather than the Plane lifecycle, risking IOMMU faults or memory corruption.
- [High] The DCU Lite interrupt handler reads the status register but fails to acknowledge (clear) the interrupts, risking an interrupt storm.
- [High] Pre-existing heap buffer overflow in `vs_dc_probe()` due to unchecked `display_count` from hardware.
--

commit 234a098981976180e2d53f562af404422d7cd953
Author: Joey Lu <a0987203069@gmail.com>

drm/verisilicon: add Nuvoton MA35D1 DCU Lite display controller support

This commit adds support for the Verisilicon DCU Lite display controller
found on the Nuvoton MA35D1 SoC. It implements a new vs_dc_funcs vtable
with dedicated hardware operations to account for differences from the
existing DC8200 controller.

> 2. No PANEL_START register: panel output starts when
>    PANEL_CONFIG.RUNNING is set; no multi-display sync start register
>    is used.

The commit message explicitly documents that panel output starts when
PANEL_CONFIG.RUNNING is set.

[ ... ]

> diff --git a/drivers/gpu/drm/verisilicon/vs_dcu_lite.c b/drivers/gpu/drm/verisilicon/vs_dcu_lite.c
> new file mode 100644
> index 0000000000000..11ef57d5ebaa4
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_dcu_lite.c
> @@ -0,0 +1,78 @@

[ ... ]

> +static void vs_dcu_lite_bridge_enable(struct vs_dc *dc, unsigned int output)
> +{
> +	regmap_set_bits(dc->regs, VSDC_FB_CONFIG(output),
> +			VSDC_FB_CONFIG_RESET);
> +}

However, vs_dcu_lite_bridge_enable() sets VSDC_FB_CONFIG_RESET instead,
and never writes to PANEL_CONFIG.RUNNING.

Will this prevent panel output from starting? Also, could asserting a reset
bit during the enable path hold the framebuffer block in a continuous reset
state?

[ ... ]

> +static void vs_dcu_lite_crtc_enable(struct vs_dc *dc, unsigned int output)
> +{
> +	regmap_set_bits(dc->regs, VSDC_FB_CONFIG(output),
> +			VSDC_FB_CONFIG_ENABLE);
> +}
> +
> +static void vs_dcu_lite_crtc_disable(struct vs_dc *dc, unsigned int output)
> +{
> +	regmap_clear_bits(dc->regs, VSDC_FB_CONFIG(output),
> +			  VSDC_FB_CONFIG_ENABLE);
> +}

The patch implements crtc_enable and crtc_disable to toggle
VSDC_FB_CONFIG_ENABLE, but leaves plane disable callbacks unimplemented.

If userspace dynamically disables the primary plane while keeping the CRTC
active, will the hardware framebuffer remain enabled?

Could this cause the hardware to continuously scan out from a disabled DRM
framebuffer that the kernel might unpin or free, leading to IOMMU faults?

[ ... ]

> +static u32 vs_dcu_lite_irq_handler(struct vs_dc *dc)
> +{
> +	u32 irqs;
> +
> +	regmap_read(dc->regs, VSDC_DISP_IRQ_STA, &irqs);
> +	return irqs;
> +}

The interrupt handler reads VSDC_DISP_IRQ_STA but does not appear to write
back to clear the pending interrupt bits.

If this status register requires a Write-1-to-Clear (W1C) operation, could
failing to explicitly acknowledge the interrupt cause the IRQ line to remain
asserted, resulting in an interrupt storm?

While reviewing this patch, I noticed a pre-existing issue in vs_dc_probe().
The code reads dc->identity.display_count from hardware registers and
loops up to this value to populate the dc->pix_clk array:

drivers/gpu/drm/verisilicon/vs_dc.c:vs_dc_probe() {
    ...
    if (port_count > dc->identity.display_count) {
        dev_err(dev, "too many downstream ports than HW capability\n");
        ret = -EINVAL;
        goto err_rst_assert;
    }

    for (i = 0; i < dc->identity.display_count; i++) {
        snprintf(pixclk_name, sizeof(pixclk_name), "pix%u", i);
        dc->pix_clk[i] = devm_clk_get(dev, pixclk_name);
    ...
}

While the code validates port_count <= VSDC_MAX_OUTPUTS and
port_count <= dc->identity.display_count, it doesn't appear to ensure
dc->identity.display_count <= VSDC_MAX_OUTPUTS.

Could a malfunctioning hardware device reporting a large display_count
cause an out-of-bounds write of clock pointers on the kernel heap?

There also appears to be a pre-existing race condition in vs_dc_probe()
where the IRQ is requested before the DRM device is initialized:

drivers/gpu/drm/verisilicon/vs_dc.c:vs_dc_probe() {
    ...
    ret = devm_request_irq(dev, irq, vs_dc_irq_handler, 0,
                           dev_name(dev), dc);
    ...
    ret = vs_drm_initialize(dc, pdev);
    ...
}

If the bootloader leaves the display controller active, could the hardware
immediately fire an interrupt once unmasked?

If this occurs, the interrupt handler vs_dc_irq_handler() would call
vs_drm_handle_irq(), which dereferences dc->drm_dev->crtcs[i]. Because
dc->drm_dev is not allocated and assigned until vs_drm_initialize() runs
later in the probe sequence, would this cause a NULL pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519055114.1886525-1-a0987203069@gmail.com?part=4