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

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

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

The is_avivo() function has a logic error where it compares a constant
to another constant instead of checking the device parameter:

  (PCI_VENDOR_ID_ATI_R600 >= 0x9400)

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/sysfb/ofdrm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
index 819aed466727..1d95df522ad5 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) ||
-		(PCI_VENDOR_ID_ATI_R600 >= 0x9400));
+		(device >= PCI_VENDOR_ID_ATI_R600));
 }
 
 static enum ofdrm_model display_get_model_of(struct drm_device *dev, struct device_node *of_node)
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

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

[sashiko-bot]

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 <oushixiong@kylinos.cn>

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/20260825104152.670588-1-oushixiong1025@163.com?part=1

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

[Thomas Zimmermann]

Hi

Am 25.08.26 um 12:41 schrieb oushixiong1025@163.com:
> From: Shixiong Ou <oushixiong@kylinos.cn>
>
> The is_avivo() function has a logic error where it compares a constant
> to another constant instead of checking the device parameter:
>
>    (PCI_VENDOR_ID_ATI_R600 >= 0x9400)
>
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>

I see this is a recent of an earlier patch I missed. I've r-b'ed that 
one and will merged it soon. Thanks for the fix.

Best regards
Thomas

> ---
>   drivers/gpu/drm/sysfb/ofdrm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
> index 819aed466727..1d95df522ad5 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) ||
> -		(PCI_VENDOR_ID_ATI_R600 >= 0x9400));
> +		(device >= PCI_VENDOR_ID_ATI_R600));
>   }
>   
>   static enum ofdrm_model display_get_model_of(struct drm_device *dev, struct device_node *of_node)

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

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

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

The is_avivo() function has a logic error where it compares a constant
to another constant instead of checking the device parameter:

  (PCI_VENDOR_ID_ATI_R600 >= 0x9400)

However, according to the code comment and the OFDRM_MODEL_AVIVO
definition ("ATI R5xx"), this function is intended to match only R5xx
cards (device IDs 0x7100-0x7800), not R6xx or later. The second
condition checking for R600 is erroneous and should be removed.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/sysfb/ofdrm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
index a6dc34b9ec0f..ea1db1cfe3ed 100644
--- a/drivers/gpu/drm/sysfb/ofdrm.c
+++ b/drivers/gpu/drm/sysfb/ofdrm.c
@@ -238,8 +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) ||
-		(PCI_VENDOR_ID_ATI_R600 >= 0x9400));
+	       (device >= PCI_VENDOR_ID_ATI_R520 && device < 0x7800);
 }
 
 static enum ofdrm_model display_get_model_of(struct drm_device *dev, struct device_node *of_node)
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

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

[Thomas Zimmermann]

Hi

Am 24.07.26 um 11:41 schrieb oushixiong1025@163.com:
> From: Shixiong Ou <oushixiong@kylinos.cn>
>
> The is_avivo() function has a logic error where it compares a constant
> to another constant instead of checking the device parameter:
>
>    (PCI_VENDOR_ID_ATI_R600 >= 0x9400)
>
> However, according to the code comment and the OFDRM_MODEL_AVIVO
> definition ("ATI R5xx"), this function is intended to match only R5xx
> cards (device IDs 0x7100-0x7800), not R6xx or later. The second
> condition checking for R600 is erroneous and should be removed.
>
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
> ---
>   drivers/gpu/drm/sysfb/ofdrm.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
> index a6dc34b9ec0f..ea1db1cfe3ed 100644
> --- a/drivers/gpu/drm/sysfb/ofdrm.c
> +++ b/drivers/gpu/drm/sysfb/ofdrm.c
> @@ -238,8 +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) ||
> -		(PCI_VENDOR_ID_ATI_R600 >= 0x9400));

I tried to match exactly this: 
https://elixir.bootlin.com/linux/v7.1.4/source/drivers/video/fbdev/offb.c#L365

IDK if that works in practice, but I don't dare changing it without a 
concrete bug report.

Best regards
Thomas

> +	       (device >= PCI_VENDOR_ID_ATI_R520 && device < 0x7800);
>   }
>   
>   static enum ofdrm_model display_get_model_of(struct drm_device *dev, struct device_node *of_node)

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

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

[Shixiong Ou]

On 2026/7/24 19:34, Thomas Zimmermann wrote:
> Hi
>
> Am 24.07.26 um 11:41 schrieb oushixiong1025@163.com:
>> From: Shixiong Ou <oushixiong@kylinos.cn>
>>
>> The is_avivo() function has a logic error where it compares a constant
>> to another constant instead of checking the device parameter:
>>
>>    (PCI_VENDOR_ID_ATI_R600 >= 0x9400)
>>
>> However, according to the code comment and the OFDRM_MODEL_AVIVO
>> definition ("ATI R5xx"), this function is intended to match only R5xx
>> cards (device IDs 0x7100-0x7800), not R6xx or later. The second
>> condition checking for R600 is erroneous and should be removed.
>>
>> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
>> ---
>>   drivers/gpu/drm/sysfb/ofdrm.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sysfb/ofdrm.c 
>> b/drivers/gpu/drm/sysfb/ofdrm.c
>> index a6dc34b9ec0f..ea1db1cfe3ed 100644
>> --- a/drivers/gpu/drm/sysfb/ofdrm.c
>> +++ b/drivers/gpu/drm/sysfb/ofdrm.c
>> @@ -238,8 +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) ||
>> -        (PCI_VENDOR_ID_ATI_R600 >= 0x9400));
>
> I tried to match exactly this: 
> https://elixir.bootlin.com/linux/v7.1.4/source/drivers/video/fbdev/offb.c#L365
>
> IDK if that works in practice, but I don't dare changing it without a 
> concrete bug report.
>
> Best regards
> Thomas
>
Hi,

PCI_VENDOR_ID_ATI_R600 is defined as 0x9400 . Therefore, this expression 
always evaluates to true (0x9400 >= 0x9400).
It does not depend on the device parameter passed to is_avivo().
So is this a coding mistake?

Best regards
Shixiong Ou.

>> +           (device >= PCI_VENDOR_ID_ATI_R520 && device < 0x7800);
>>   }
>>     static enum ofdrm_model display_get_model_of(struct drm_device 
>> *dev, struct device_node *of_node)
>

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

[Thomas Zimmermann]

Hi

Am 24.07.26 um 15:21 schrieb Shixiong Ou:
>
> On 2026/7/24 19:34, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 24.07.26 um 11:41 schrieb oushixiong1025@163.com:
>>> From: Shixiong Ou <oushixiong@kylinos.cn>
>>>
>>> The is_avivo() function has a logic error where it compares a constant
>>> to another constant instead of checking the device parameter:
>>>
>>>    (PCI_VENDOR_ID_ATI_R600 >= 0x9400)
>>>
>>> However, according to the code comment and the OFDRM_MODEL_AVIVO
>>> definition ("ATI R5xx"), this function is intended to match only R5xx
>>> cards (device IDs 0x7100-0x7800), not R6xx or later. The second
>>> condition checking for R600 is erroneous and should be removed.
>>>
>>> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
>>> ---
>>>   drivers/gpu/drm/sysfb/ofdrm.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/sysfb/ofdrm.c 
>>> b/drivers/gpu/drm/sysfb/ofdrm.c
>>> index a6dc34b9ec0f..ea1db1cfe3ed 100644
>>> --- a/drivers/gpu/drm/sysfb/ofdrm.c
>>> +++ b/drivers/gpu/drm/sysfb/ofdrm.c
>>> @@ -238,8 +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) ||
>>> -        (PCI_VENDOR_ID_ATI_R600 >= 0x9400));
>>
>> I tried to match exactly this: 
>> https://elixir.bootlin.com/linux/v7.1.4/source/drivers/video/fbdev/offb.c#L365
>>
>> IDK if that works in practice, but I don't dare changing it without a 
>> concrete bug report.
>>
>> Best regards
>> Thomas
>>
> Hi,
>
> PCI_VENDOR_ID_ATI_R600 is defined as 0x9400 . Therefore, this 
> expression always evaluates to true (0x9400 >= 0x9400).
> It does not depend on the device parameter passed to is_avivo().
> So is this a coding mistake?

Oh, now I see it. That's indeed a bug. It should test for (device >= 
PCI_VENDOR_ID_ATI_R600) instead.  Thanks for addressing this.

Best regards
Thomas


>
> Best regards
> Shixiong Ou.
>
>>> +           (device >= PCI_VENDOR_ID_ATI_R520 && device < 0x7800);
>>>   }
>>>     static enum ofdrm_model display_get_model_of(struct drm_device 
>>> *dev, struct device_node *of_node)
>>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)