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