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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The disable sequence in `vs_dc8200_bridge_disable` reverses the order of clearing `VSDC_DISP_PANEL_START` and `VSDC_DISP_PANEL_CONFIG`.
- [High] Incorrect teardown sequence in CRTC disable path where pixel clock is disabled before calling the CRTC hardware disable hook.

Pre-existing issues:
- [High] The plane disable function incorrectly sets the enable bit instead of clearing it.
- [High] Calling `clk_prepare_enable()` and `clk_disable_unprepare()` in the CRTC atomic enable/disable callbacks violates atomic context rules.
- [High] Potential NULL pointer dereference if IRQ fires before drm_dev is initialized.
--

commit 7843af23baf865801f80d2910ca556ff5e144ece
Author: Joey Lu <a0987203069@gmail.com>
Subject: 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.
Introduce 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..679d6541ba1b9 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]);

[Severity: High]
This is a pre-existing issue, but does calling clk_disable_unprepare() here
violate atomic context rules?

The DRM subsystem requires that CRTC atomic disable callbacks run in atomic
context where sleeping is forbidden, but clk_disable_unprepare() can sleep.

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

[Severity: High]
Could disabling the pixel clock before invoking the hardware-specific
crtc_disable callback cause bus hangs?

If the crtc_disable hook requires the pixel clock to be active to perform
register writes, turning it off first might result in dropped writes or
stalls. Should the clock be disabled after the hook is called, reversing
the setup sequence?

>  
>  static void vs_crtc_atomic_enable(struct drm_crtc *crtc,
> @@ -42,6 +68,9 @@ static void vs_crtc_atomic_enable(struct drm_crtc *crtc,
>  	drm_WARN_ON(&dc->drm_dev->base,
>  		    clk_prepare_enable(dc->pix_clk[output]));

[Severity: High]
This is a pre-existing issue, but does this clk_prepare_enable() call also
violate atomic context rules? 

Since atomic_enable runs in atomic context, calling sleeping functions can
cause kernel warnings or potential deadlocks during non-blocking commits.

>  
> +	if (dc->funcs->crtc_enable)
> +		dc->funcs->crtc_enable(dc, output);
> +
>  	drm_crtc_vblank_on(crtc);
>  }
>  
> diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c
> index dad9967bc10b8..c949570241890 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_handler(dc);
>  
>  	vs_drm_handle_irq(dc, irqs);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the IRQ to
fire before the DRM device is fully initialized?

If the bootloader leaves the display hardware active and generating VSYNC
interrupts, vs_dc_irq_handler() could execute and call vs_drm_handle_irq(),
which accesses dc->drm_dev. Since devm_request_irq() is called in
vs_dc_probe() before vs_drm_initialize() allocates dc->drm_dev, could
this lead to a NULL pointer dereference during early boot?

[ ... ]
> diff --git a/drivers/gpu/drm/verisilicon/vs_dc8200.c b/drivers/gpu/drm/verisilicon/vs_dc8200.c
> new file mode 100644
> index 0000000000000..db9e1b3cd9032
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_dc8200.c
[ ... ]
> +static void vs_dc8200_bridge_disable(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 this reversal of the clearing sequence cause bus stalls or visual tearing?

In the original code, VSDC_DISP_PANEL_START was cleared before
VSDC_DISP_PANEL_CONFIG. This refactored sequence disables the panel
configuration while the pipeline is potentially still active. Should
VSDC_DISP_PANEL_START be cleared first to safely stop the pipeline?

[ ... ]
> +static void vs_dc8200_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 this actually disable the plane?

By using regmap_set_bits() with VSDC_FB_CONFIG_EX_FB_EN, the frame buffer
enable bit is being set to 1 instead of being cleared. Should this use
regmap_clear_bits() instead to properly disable the hardware plane?

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