Re: [PATCH v3 5/5] drm/verisilicon: add DCUltraLite chip identity to HWDB

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A wildcard entry in vs_chip_identities shadows subsequent specific entries, making them unreachable.
- [High] The IRQ handler is registered before DRM structures are initialized, which can cause a NULL pointer dereference if an interrupt fires early or if initialization fails.
--

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

drm/verisilicon: add DCUltraLite chip identity to HWDB

This commit registers the Nuvoton MA35D1 DCUltraLite chip identity in
the hardware database. It places the entry last in the list to act as a
gate for hardware recognition.

> diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c b/drivers/gpu/drm/verisilicon/vs_hwdb.c
> index 91524d16f7785..7d630a667a3f0 100644
> --- a/drivers/gpu/drm/verisilicon/vs_hwdb.c
> +++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c
> @@ -129,6 +129,16 @@ static struct vs_chip_identity vs_chip_identities[] = {

[Severity: High]
This is a pre-existing issue, but does the wildcard entry at the start of
vs_chip_identities prevent specific entries from being matched?

Looking at vs_chip_identities, the first entry uses a wildcard for the
customer ID:

static struct vs_chip_identity vs_chip_identities[] = {
	{
		.model = 0x8200,
		.revision = 0x5720,
		.customer_id = ~0U,
...

In vs_fill_chip_identity(), the search loop returns on the first match:

	for (i = 0; i < ARRAY_SIZE(vs_chip_identities); i++) {
		if (vs_chip_identities[i].model == model &&
		    vs_chip_identities[i].revision == revision &&
		    (vs_chip_identities[i].customer_id == customer_id ||
		     vs_chip_identities[i].customer_id == ~0U)) {
...

Because the ~0U entry comes first, wouldn't it shadow the subsequent specific 
entries for the same model and revision (like customer_id 0x310 and 0x311),
preventing their specific formats from ever being applied?

>  		.max_cursor_size = 64,
>  		.formats = &vs_formats_no_yuv444,
>  	},
> +	{
> +		.model = 0x0,		/* DCUltraLite */
> +		.revision = 0x5560,
> +		.customer_id = 0x305,
> +
> +		.generation = VSDC_GEN_DC8000,
> +		.display_count = 1,
> +		.max_cursor_size = 32,
> +		.formats = &vs_formats_no_yuv444,
> +	},
>  };
>  
>  int vs_fill_chip_identity(struct regmap *regs,

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for an early
interrupt to cause a NULL pointer dereference during probe?

In vs_dc_probe() from vs_dc.c, the IRQ handler is registered before DRM
structures are initialized:

	ret = devm_request_irq(dev, irq, vs_dc_irq_handler, 0,
			       dev_name(dev), dc);
...
	ret = vs_drm_initialize(dc, pdev);

If an interrupt fires during this window, the handler seems to unconditionally
dereference dc->drm_dev which is allocated later in vs_drm_initialize():

vs_drm_handle_irq() in vs_drm.c:
...
			if (dc->drm_dev->crtcs[i])
				drm_crtc_handle_vblank(&dc->drm_dev->crtcs[i]->base);

Could this lead to a panic if the hardware has a pending interrupt before DRM
is ready?

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