Re: [PATCH v4 5/6] 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 specific customer ID entries, making them unreachable.
- [High] The function vs_fill_chip_identity() ignores the return values of regmap_read() calls, leading to potential use of uninitialized stack memory.
--

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

drm/verisilicon: add DCUltraLite chip identity to HWDB
    
Register the Nuvoton MA35D1 DCUltraLite chip identity in
vs_chip_identities[]:
  model       = 0x0   (DCUltraLite; Verisilicon uses 0 for this IP)
[ ... ]

> 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[] = {
>  		.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,
>  			  struct vs_chip_identity *ident)
>  {
>  	u32 model;
>  	u32 revision;
>  	u32 customer_id;
>  	int i;
>  
>  	regmap_read(regs, VSDC_TOP_CHIP_MODEL, &model);
>  	regmap_read(regs, VSDC_TOP_CHIP_REV, &revision);
>  	regmap_read(regs, VSDC_TOP_CHIP_CUSTOMER_ID, &customer_id);

[Severity: High]
This isn't a bug introduced by this patch, but does vs_fill_chip_identity()
ignore the return values of these regmap_read() calls?

The regmap_read() API returns an integer error code on failure and leaves
the output pointer's target unmodified. 

If any of these reads fail due to bus errors or unmapped I/O, will the
function proceed to use uninitialized stack memory in the loop to compare
against the vs_chip_identities array?

>  
>  	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)) {
>  			memcpy(ident, &vs_chip_identities[i], sizeof(*ident));
>  			ident->customer_id = customer_id;
>  			return 0;
>  		}

[Severity: High]
This is a pre-existing issue, but does the wildcard entry in
vs_chip_identities[] shadow specific customer ID entries?

Looking at vs_chip_identities[] in drivers/gpu/drm/verisilicon/vs_hwdb.c,
there is a wildcard entry at the beginning of the array:

	{
		.model = 0x8200,
		.revision = 0x5720,
		.customer_id = ~0U,
		...
	}

Since vs_fill_chip_identity() searches sequentially and returns on the first
match, will any hardware with model 0x8200 and revision 0x5720 instantly
match this wildcard entry?

If so, the specific configurations for customer IDs 0x310 and 0x311 defined
later in the array might be completely shadowed and never reached. This would
cause customer ID 0x310 to incorrectly use vs_formats_no_yuv444 instead of
its intended vs_formats_with_yuv444.

>  	}
>  
>  	return -EINVAL;
>  }

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