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 <[email protected]> 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/[email protected]?part=3
