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

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

New issues:
- [High] Variant-specific CRTC disable hook (`crtc_disable_ex`) is called after disabling the pixel clock, leading to hardware faults.
- [High] The panel disable sequence in `vs_dc8200_panel_disable_ex` reverses the order of clearing CONFIG and START registers, violating the original sequence and hardware state machine.

Pre-existing issues:
- [High] Primary plane disable function incorrectly sets the plane enable bit instead of clearing it.
- [High] IRQ handler is registered before DRM device initialization is complete, causing a potential NULL pointer dereference on early interrupts.
- [High] Sleeping clock functions (`clk_prepare_enable`, `clk_disable_unprepare`) are called from CRTC atomic callbacks, violating DRM atomic context requirements.
--

commit c35af4d919e849e303afa29d897c4d1581720138
Author: Joey Lu [off-list ref]

drm/verisilicon: introduce per-variant hardware ops table

This commit introduces a hardware ops table for the Verisilicon DC driver to
handle variant-specific implementations. It extracts DC8200-specific ops into a
new file and updates the shared code to dispatch hardware sequences through the
new function pointers. The patch aims to avoid scattering variant checks across
the core driver.

[Severity: High]
This is a pre-existing issue, but calling clk_disable_unprepare() here
sleeps in an atomic context, which is forbidden during non-blocking commits
and can lead to system instability.

+
+	if (dc->funcs->crtc_disable_ex)
+		dc->funcs->crtc_disable_ex(dc, output);

[Severity: High]
Will this sequence cause hardware faults? The pixel clock is disabled
via clk_disable_unprepare() right before calling the variant-specific
crtc_disable_ex() callback.

If follow-up implementations of crtc_disable_ex() attempt to access hardware
registers within the now-disabled pixel clock domain, it could trigger an
asynchronous external abort or bus error when userspace disables the display.
Should the callback be invoked before disabling the clock?

[Severity: High]
This is a pre-existing issue, but does calling clk_prepare_enable()
from vs_crtc_atomic_enable() violate the DRM atomic context requirements?

The clock framework functions clk_prepare_enable() and clk_disable_unprepare()
can sleep. Since CRTC atomic enable, disable, and update callbacks run in an
atomic context during non-blocking commits, this can cause scheduling while
atomic warnings and system instability during userspace modesets.

+	if (dc->funcs->crtc_enable_ex)
+		dc->funcs->crtc_enable_ex(dc, output);
+
 	drm_crtc_vblank_on(crtc);
 }

[ ... ]

[ ... ]

+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 this reverse the original tear-down sequence?

In the original vs_bridge_atomic_disable() code, VSDC_DISP_PANEL_START_RUNNING
was cleared before clearing VSDC_DISP_PANEL_CONFIG_RUNNING.

Here in vs_dc8200_panel_disable_ex(), VSDC_DISP_PANEL_CONFIG_RUNNING is
cleared first. Modifying or clearing a pipeline's configuration while it is
actively sending pixels could put the hardware into an invalid state and
cause synchronization errors.

+
+	regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(output),
+			VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
+}

[ ... ]

+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);

[Severity: High]
This is a pre-existing issue, but does this function actually enable the plane
instead of disabling it?

The code uses regmap_set_bits() to write a 1 to VSDC_FB_CONFIG_EX_FB_EN, which
sets the enable bit. This could leave the primary plane active on the screen
when userspace expects it to be hidden via vs_dc8200_primary_plane_disable_ex().

+
+	vs_dc8200_plane_commit(dc, output);
+}

[ ... ]

+static u32 vs_dc8200_irq_ack(struct vs_dc *dc)
+{
+	u32 hw_irqs, unified = 0, known = 0;
+	unsigned int i;
+
+	regmap_read(dc->regs, VSDC_TOP_IRQ_ACK, &hw_irqs);
+
+	for (i = 0; i < VSDC_MAX_OUTPUTS; i++) {
+		known |= VSDC_TOP_IRQ_VSYNC(i);
+		if (hw_irqs & VSDC_TOP_IRQ_VSYNC(i))
+			unified |= VSDC_IRQ_VSYNC(i);
+	}
+
+	drm_WARN_ONCE(&dc->drm_dev->base, hw_irqs & ~known,
+		      "Unknown hardware IRQ bits: %#x\n", hw_irqs & ~known);

[Severity: High]
This is a pre-existing issue, but could this drm_WARN_ONCE() trigger a NULL
pointer dereference during boot?

In vs_dc_probe(), the IRQ handler is requested via devm_request_irq() before
vs_drm_initialize() is called to allocate the DRM device. If a residual boot
interrupt fires during this window, vs_dc_irq_handler() will run and call
vs_dc8200_irq_ack() while dc->drm_dev is still NULL, causing the warning macro
to dereference NULL.

+
+	return unified;
+}

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