Re: [PATCH v2 2/4] drm/verisilicon: add model ID constants and DCU Lite chip identity

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Specific chip identities in the `vs_chip_identities[]` array are permanently shadowed by an earlier wildcard entry, making them unreachable.
- [High] Missing error checks on `regmap_read()` cause uninitialized stack variables to be used in hardware matching.
--

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

drm/verisilicon: add model ID constants and DCU Lite chip identity

This commit introduces symbolic constants for model IDs to replace magic
numbers and adds the DCU Lite chip identity to vs_chip_identities[]. It also
adds register-level macros for forthcoming DCU Lite support.

> diff --git a/drivers/gpu/drm/verisilicon/vs_hwdb.c b/drivers/gpu/drm/verisilicon/vs_hwdb.c
> index 09336af0900ae..a25c4b16181d6 100644
> --- a/drivers/gpu/drm/verisilicon/vs_hwdb.c
> +++ b/drivers/gpu/drm/verisilicon/vs_hwdb.c
> @@ -90,7 +90,7 @@ static const struct vs_formats vs_formats_with_yuv444 = {
>  
>  static struct vs_chip_identity vs_chip_identities[] = {
>  	{
> -		.model = 0x8200,
> +		.model = VSDC_MODEL_DC8200,
>  		.revision = 0x5720,
>  		.customer_id = ~0U,

This isn't a bug introduced by this patch, but does this first entry with
.customer_id = ~0U permanently shadow the later entries for the same model
and revision?

The loop in vs_fill_chip_identity() evaluates these sequentially and returns
on the first match. Since ~0U acts as a wildcard, it appears the later
specific entries for revision 0x5720 (like customer 0x310 which enables
YUV444) will never be reached.

[ ... ]

> @@ -114,13 +114,21 @@ static struct vs_chip_identity vs_chip_identities[] = {
>  		.formats = &vs_formats_with_yuv444,
>  	},
>  	{
> -		.model = 0x8200,
> +		.model = VSDC_MODEL_DC8200,
>  		.revision = 0x5720,
>  		.customer_id = 0x311,
>  
>  		.display_count = 2,
>  		.formats = &vs_formats_no_yuv444,
>  	},
> +	{
> +		.model = VSDC_MODEL_DCU_LITE,
> +		.revision = 0x5560,
> +		.customer_id = 0x305,
> +
> +		.display_count = 1,
> +		.formats = &vs_formats_no_yuv444,
> +	},
>  };
>  
>  int vs_fill_chip_identity(struct regmap *regs,
>  			  struct vs_chip_identity *ident)

Also not introduced by this patch, but looking at the rest of
vs_fill_chip_identity() just below this array, do we need to check the return
values of the regmap_read() calls?

It looks like the model, revision, and customer_id variables are declared on
the stack without initialization:

    u32 model;
    u32 revision;
    u32 customer_id;

    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);

If regmap_read() fails (e.g., due to an unclocked device or hardware
timeout), it would leave these variables uninitialized.

Could this lead to undefined behavior when they are evaluated in the
vs_chip_identities loop condition, potentially matching a random hardware
profile?

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