Re: [PATCH v5 3/7] drm/verisilicon: introduce per-variant hardware ops table

[sashiko-bot@kernel.org]

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

New issues:
- [High] CRTC disable hardware operation is called after disabling the pixel clock.
- [High] The register clearing sequence in `vs_dc8200_panel_disable_ex` is inverted compared to the original code, clearing the configuration bit before stopping the panel.
- [Medium] Unknown hardware interrupts are silently filtered out by the irq_ack abstraction, rendering the unknown interrupt diagnostic warning unreachable.

Pre-existing issues:
- [High] Primary plane disable function incorrectly sets the enable bit rather than clearing it.
- [High] The driver requests its IRQ line before it fully initializes its internal state, creating a race condition that can lead to a NULL pointer dereference.
- [High] `vs_dc_irq_handler` unconditionally returns `IRQ_HANDLED` regardless of whether any interrupt bits were set.
--

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

drm/verisilicon: introduce per-variant hardware ops table

The DC8200 and DCUltraLite share a broadly similar register layout but differ
in how the bridge, CRTC, primary plane and IRQ paths are driven. This commit
introduces a vs_dc_funcs vtable so each variant can supply its own
implementation without scattering conditionals across multiple files.

> diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c
> index 0b8a35d09cd2e..1c4aac7086694 100644
> --- a/drivers/gpu/drm/verisilicon/vs_crtc.c
> +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c
[ ... ]
> @@ -30,6 +53,9 @@ static void vs_crtc_atomic_disable(struct drm_crtc *crtc,
>  	drm_crtc_vblank_off(crtc);
>  
>  	clk_disable_unprepare(dc->pix_clk[output]);
> +
> +	if (dc->funcs->crtc_disable_ex)
> +		dc->funcs->crtc_disable_ex(dc, output);
>  }

[Severity: High]
Is it safe to access hardware registers via crtc_disable_ex() after the pixel
clock has been gated? The teardown order should typically reverse the
initialization order, meaning the hook must be invoked before gating the clock.

[ ... ]
> diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c
> index dad9967bc10b8..9729b693d360e 100644
> --- a/drivers/gpu/drm/verisilicon/vs_dc.c
> +++ b/drivers/gpu/drm/verisilicon/vs_dc.c
> @@ -33,7 +31,7 @@ static irqreturn_t vs_dc_irq_handler(int irq, void *private)
>  	struct vs_dc *dc = private;
>  	u32 irqs;
>  
> -	regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &irqs);
> +	irqs = dc->funcs->irq_ack(dc);
>  
>  	vs_drm_handle_irq(dc, irqs);
>  
>  	return IRQ_HANDLED;

[Severity: High]
This isn't a bug introduced by this patch, but does unconditionally returning
IRQ_HANDLED prevent the kernel from detecting and disabling a stuck shared
interrupt line? If the handler processes no interrupts, shouldn't it return
IRQ_NONE?

[Severity: High]
This is another pre-existing issue, but in vs_dc_probe(), the IRQ is
requested before vs_drm_initialize() is called. Can the IRQ handler fire
before dc->drm_dev is allocated, resulting in a NULL pointer dereference
when vs_drm_handle_irq() executes?

[ ... ]
> diff --git a/drivers/gpu/drm/verisilicon/vs_dc8200.c b/drivers/gpu/drm/verisilicon/vs_dc8200.c
> new file mode 100644
> index 0000000000000..17378f4ef96d1
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_dc8200.c
[ ... ]
> +static void vs_dc8200_panel_disable_ex(struct vs_dc *dc, unsigned int output)
> +{
> +	regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
> +			  VSDC_DISP_PANEL_CONFIG_RUNNING);
> +	regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
> +			  VSDC_DISP_PANEL_START_MULTI_DISP_SYNC |
> +			  VSDC_DISP_PANEL_START_RUNNING(output));

[Severity: High]
Does reversing the clear order of the configuration and start registers risk
hardware underflows or state desynchronization? In the original
vs_bridge_atomic_disable() implementation, the panel was shut down by first
clearing VSDC_DISP_PANEL_START_RUNNING and then clearing
VSDC_DISP_PANEL_CONFIG_RUNNING.

[ ... ]
> +static void vs_dc8200_primary_plane_disable_ex(struct vs_dc *dc, unsigned int output)
> +{
> +	regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output),
> +			VSDC_FB_CONFIG_EX_FB_EN);
> +
> +	vs_dc8200_plane_commit(dc, output);
> +}

[Severity: High]
This is a pre-existing issue carried over from the original code, but does
the primary plane disable sequence leave the plane enabled by using
regmap_set_bits() instead of clearing the VSDC_FB_CONFIG_EX_FB_EN bit?

[ ... ]
> +static u32 vs_dc8200_irq_ack(struct vs_dc *dc)
> +{
> +	u32 hw_irqs, unified = 0;
> +	unsigned int i;
> +
> +	regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &hw_irqs);
> +
> +	for (i = 0; i < VSDC_MAX_OUTPUTS; i++) {
> +		if (hw_irqs & VSDC_TOP_IRQ_VSYNC(i))
> +			unified |= VSDC_IRQ_VSYNC(i);
> +	}
> +
> +	return unified;
> +}

[Severity: Medium]
Does this filter out unknown hardware interrupts, making the diagnostic
warning in vs_drm_handle_irq() unreachable? By only passing through known
VSDC_IRQ_VSYNC bits, any unmapped hardware interrupt bits are silently
discarded.

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