Re: [PATCH v5 4/7] drm/verisilicon: make axi and ahb clocks optional

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Possible NULL pointer dereference in the IRQ handler if an interrupt triggers before DRM initialization completes.
--

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

drm/verisilicon: make axi and ahb clocks optional

The Nuvoton MA35D1 SoC integrates a DCUltraLite display controller whose
AXI and AHB bus clocks share a single gate enable bit with the display
core clock, so the clock driver does not expose them separately. This
patch makes the axi and ahb clocks optional in the probe.

> diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c b/drivers/gpu/drm/verisilicon/vs_dc.c
> index 9729b693d360e..fd1f5fe67a68b 100644
> --- a/drivers/gpu/drm/verisilicon/vs_dc.c
> +++ b/drivers/gpu/drm/verisilicon/vs_dc.c
[ ... ]
> @@ -90,13 +90,13 @@ static int vs_dc_probe(struct platform_device *pdev)
>  		return PTR_ERR(dc->core_clk);
>  	}
>  
> -	dc->axi_clk = devm_clk_get_enabled(dev, "axi");
> +	dc->axi_clk = devm_clk_get_optional_enabled(dev, "axi");
>  	if (IS_ERR(dc->axi_clk)) {

[Severity: High]
This is a pre-existing issue, but is it possible for an interrupt to trigger
a NULL pointer dereference during probe if the bootloader left the display
active?

Looking further down in vs_dc_probe(), the IRQ handler is registered before
vs_drm_initialize() is called:

	ret = devm_request_irq(dev, irq, vs_dc_irq_handler, 0,
			       dev_name(dev), dc);
	if (ret) {
		dev_err(dev, "can't request irq\n");
		goto err_rst_assert;
	}

	dev_set_drvdata(dev, dc);

	ret = vs_drm_initialize(dc, pdev);

If a pending interrupt (such as VSYNC) fires immediately after
devm_request_irq() is called, vs_dc_irq_handler() will be triggered and will
eventually call vs_drm_handle_irq().

Looking at vs_drm_handle_irq() in vs_drm.c, it unconditionally dereferences
the drm_dev pointer:

	if (dc->drm_dev->crtcs[i])
		drm_crtc_handle_vblank(&dc->drm_dev->crtcs[i]->base);

Since vs_drm_initialize() hasn't allocated or assigned dc->drm_dev yet,
could this cause a kernel panic during boot on platforms where the bootloader
leaves interrupts enabled?

Would it be safer to request the IRQ after the DRM device is fully
initialized, or to mask interrupts in the hardware before registering the
handler?

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