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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The DCU Lite driver does not start the panel output because it fails to set the PANEL_CONFIG.RUNNING bit in the bridge enable path.
- [High] The DCU Lite driver does not implement a plane_disable_ex callback, breaking independent primary plane disable.
- [High] The pixel clock is disabled before the CRTC hardware is disabled, causing potential register access hangs.
- [High] The DCUltraLite IRQ handler fails to clear the interrupt status, leading to an interrupt storm.
- [Medium] The `irqs` variable in `vs_dcu_lite_irq_handler` may be used uninitialized if `regmap_read` fails.

Pre-existing issues:
- [High] The DC8200 plane disable function incorrectly sets the enable bit instead of clearing it.
- [High] The IRQ handler is registered before DRM initialization is complete and remains active after teardown, risking a NULL pointer dereference.
--

commit c0e566390dbc25177931f48aa8bb45a4670fc241
Author: Joey Lu <a0987203069@gmail.com>
Subject: drm/verisilicon: add Nuvoton MA35D1 DCU Lite display controller support

This commit adds support for the Nuvoton MA35D1 DCUltraLite display
controller. It introduces vs_dcu_lite.c with a new vs_dc_funcs vtable and
adjusts the probe function to handle optional clocks.

> diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c
> --- a/drivers/gpu/drm/verisilicon/vs_dc.c
> +++ b/drivers/gpu/drm/verisilicon/vs_dc.c
[ ... ]
> @@ -134,7 +134,10 @@ static int vs_dc_probe(struct platform_device *pdev)
>  	dev_info(dev, "Found DC%x rev %x customer %x\n", dc->identity.model,
>  		 dc->identity.revision, dc->identity.customer_id);
>  
> -	dc->funcs = &vs_dc8200_funcs;
> +	if (dc->identity.generation == VSDC_GEN_DC8200)
> +		dc->funcs = &vs_dc8200_funcs;
> +	else
> +		dc->funcs = &vs_dcu_lite_funcs;
>  
>  	if (port_count > dc->identity.display_count) {
>  		dev_err(dev, "too many downstream ports than HW capability\n");

[Severity: High]
This is a pre-existing issue, but I noticed a potential race during probe
and initialization later in vs_dc_probe().

vs_dc.c:vs_dc_probe() {
    ...
    ret = devm_request_irq(dev, irq, vs_dc_irq_handler, ...);
    ...
    ret = vs_drm_initialize(dc, pdev);
    ...
}

The IRQ handler is registered before vs_drm_initialize() completes. If an
interrupt fires early, vs_drm_handle_irq() will be invoked:

vs_drm.c:vs_drm_handle_irq() {
    ...
    if (dc->drm_dev->crtcs[i])
        drm_crtc_handle_vblank(&dc->drm_dev->crtcs[i]->base);
    ...
}

Could this cause a NULL pointer dereference since dc->drm_dev is not fully
set up until vs_drm_initialize() finishes?

> diff --git a/drivers/gpu/drm/verisilicon/vs_dcu_lite.c b/drivers/gpu/drm/verisilicon/vs_dcu_lite.c
> new file mode 100644
> --- /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);
> +}

[Severity: High]
The commit message notes that panel output starts when PANEL_CONFIG.RUNNING
is set. However, vs_dcu_lite_bridge_enable() only sets VSDC_FB_CONFIG_RESET
and does not appear to write to VSDC_DISP_PANEL_CONFIG.

Will the display output pipeline start successfully without setting the
RUNNING bit?

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

[Severity: Medium]
If regmap_read() fails, the irqs variable remains uninitialized. Could this
result in returning a garbage value that gets processed as a bitmask of
triggered interrupts?

[Severity: High]
Also, vs_dcu_lite_irq_handler() reads the status register but does not seem
to perform a write operation to acknowledge or clear the pending interrupts.
Will this cause the IRQ line to remain asserted, potentially leading to an
interrupt storm?

> +const struct vs_dc_funcs vs_dcu_lite_funcs = {
> +	.bridge_enable		= vs_dcu_lite_bridge_enable,
> +	.bridge_disable		= vs_dcu_lite_bridge_disable,
> +	.crtc_begin		= vs_dcu_lite_crtc_begin,
> +	.crtc_flush		= vs_dcu_lite_crtc_flush,
> +	.crtc_enable		= vs_dcu_lite_crtc_enable,
> +	.crtc_disable		= vs_dcu_lite_crtc_disable,
> +	.enable_vblank		= vs_dcu_lite_enable_vblank,
> +	.disable_vblank		= vs_dcu_lite_disable_vblank,
> +	.irq_handler		= vs_dcu_lite_irq_handler,
> +};

[Severity: High]
Looking at vs_primary_plane_atomic_disable() in vs_primary_plane.c, it
relies on plane_disable_ex() to disable the plane. Since vs_dcu_lite_funcs
lacks a plane_disable_ex callback, will the hardware keep scanning out the
old framebuffer if user-space disables the primary plane while keeping the
CRTC active?

[Severity: High]
This isn't a bug introduced by this patch, but since we are discussing plane
disable operations, I noticed an issue with the DC8200 equivalent.

vs_dc8200.c:vs_dc8200_plane_disable_ex() {
    regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
                    VSDC_FB_CONFIG_EX_FB_EN);
}

This function calls regmap_set_bits() with VSDC_FB_CONFIG_EX_FB_EN. Does
this incorrectly set the enable bit to 1 instead of clearing it, preventing
the primary plane from being properly disabled?

[Severity: High]
This is a pre-existing issue, but looking at how crtc_disable is used in
vs_crtc.c:vs_crtc_atomic_disable():

vs_crtc.c:vs_crtc_atomic_disable() {
    clk_disable_unprepare(dc->pix_clk[output]);

    if (dc->funcs->crtc_disable)
        dc->funcs->crtc_disable(dc, output);
    ...
}

The pixel clock is disabled before the crtc_disable hardware callback
accesses the registers. Could this cause bus faults or leave the hardware in
an invalid state since the pipeline's primary pixel clock is already off?

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