Re: [PATCH] drm/sysfb: ofdrm: Fix is_avivo() constant comparison bug

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

New issues:
- [Medium] The patch excludes AVIVO-capable RS600, RS690, and RS740 IGPs from being matched, causing a loss of color map support.

Pre-existing issues:
- [High] The device ID heuristic incorrectly identifies modern non-AVIVO GPUs as AVIVO, which can lead to invalid register writes.
--

--- Patch [1]: [PATCH] drm/sysfb: ofdrm: Fix is_avivo() constant comparison bug ---
commit aeacd4e00d7ba70a30fc346d0e3ce8c00c4f6b2b
Author: Shixiong Ou <[email protected]>

drm/sysfb: ofdrm: Fix is_avivo() constant comparison bug

This patch replaces a constant comparison (PCI_VENDOR_ID_ATI_R600 >= 0x9400)
with a check against the device ID parameter to fix a logic error.

> diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
> index 819aed466727e..1d95df522ad58 100644
> --- a/drivers/gpu/drm/sysfb/ofdrm.c
> +++ b/drivers/gpu/drm/sysfb/ofdrm.c
> @@ -238,7 +238,7 @@ static bool is_avivo(u32 vendor, u32 device)
>  	/* This will match most R5xx */
>  	return (vendor == PCI_VENDOR_ID_ATI) &&
>  	       ((device >= PCI_VENDOR_ID_ATI_R520 && device < 0x7800) ||

[Severity: High]
This isn't a bug introduced by this patch, but does the overly broad upper
bound in is_avivo() incorrectly identify modern AMD GPUs as AVIVO hardware?

Looking at modern GPU device IDs (like Fiji with ID 0x7300, or Navi 10 with
ID 0x731F), they fall directly into this legacy range check (device < 0x7800).

If present on an Open Firmware system, would the driver mistakenly match them
as OFDRM_MODEL_AVIVO, map the PCIe BAR, and attempt color map updates by doing
MMIO writes to legacy AVIVO offsets (0x6480, 0x6488, 0x6494)? Could this
overwrite unrelated or critical registers on modern DCN/DCE architectures,
causing memory corruption or hardware lockups?

> -		(PCI_VENDOR_ID_ATI_R600 >= 0x9400));
> +		(device >= PCI_VENDOR_ID_ATI_R600));

[Severity: Medium]
Does fixing this typo accidentally break support for legacy AVIVO IGPs in the
0x79xx range?

The original flawed constant check unintentionally allowed devices like RS600,
RS690, and RS740 IGPs (e.g., device ID 0x791e) to be matched as AVIVO when
is_avivo() is called during boot.

By bounding the checks to device < 0x7800 and device >= 0x9400 (assuming
PCI_VENDOR_ID_ATI_R600 is 0x9400), does this exclude the 0x79xx range,
causing these IGPs to be incorrectly classified as OFDRM_MODEL_UNKNOWN
rather than OFDRM_MODEL_AVIVO, and subsequently causing the driver to skip
initializing cmap functions?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1