Re: [PATCH 1/2] drm/ofdrm: Cast PCI IDs to u32 for comparing

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3799 bytes --]

Hi

Am 28.10.22 um 08:33 schrieb Alexander Stein:
> Hi Thomas,
> 
> Am Donnerstag, 27. Oktober 2022, 16:04:34 CEST schrieb Thomas Zimmermann:
>> * PGP Signed: 10/27/2022 at 04:04:34 PM
>>
>> Hi
>>
>> Am 27.10.22 um 15:07 schrieb Alexander Stein:
>>> Hello Thomas,
>>>
>>> Am Donnerstag, 27. Oktober 2022, 13:57:06 CEST schrieb Thomas Zimmermann:
>>>> Properties of 32-bit integers are returned from the OF device tree
>>>> as type __be32. Cast PCI vendor and device IDs from __be32 to u32
>>>> before comparing them to constants. Fixes sparse warnings shown below.
>>>>
>>>>     drivers/gpu/drm/tiny/ofdrm.c:237:17: warning: restricted __be32
>>>>     degrades
>>>>
>>>> to integer drivers/gpu/drm/tiny/ofdrm.c:238:18: warning: restricted
>>>> __be32
>>>> degrades to integer drivers/gpu/drm/tiny/ofdrm.c:238:54: warning:
>>>> restricted __be32 degrades to integer
>>>>
>>>> See [1] for the bug report.
>>>>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Link:
>>>> https://lore.kernel.org/dri-devel/202210192208.D888I6X7-lkp@intel.com/ #
>>>> [1] ---
>>>>
>>>>    drivers/gpu/drm/tiny/ofdrm.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
>>>> index 0e1cc2369afcc..0da8b248ccc6e 100644
>>>> --- a/drivers/gpu/drm/tiny/ofdrm.c
>>>> +++ b/drivers/gpu/drm/tiny/ofdrm.c
>>>> @@ -231,8 +231,11 @@ static u64 display_get_address_of(struct drm_device
>>>> *dev, struct device_node *of return address;
>>>>
>>>>    }
>>>>
>>>> -static bool is_avivo(__be32 vendor, __be32 device)
>>>> +static bool is_avivo(__be32 vendor_id, __be32 device_id)
>>>>
>>>>    {
>>>>
>>>> +	u32 vendor = (__force u32)vendor_id;
>>>> +	u32 device = (__force u32)device_id;
>>>
>>> I don't have much context, but just from reading this, shouldn't this be
>>> be32_to_cpu() instead?
>>
>> I should have explained that in the commit message. The values are
>> supposed to be in big endian. We compare to PCI ids. The code originally
>> was taken from [1], which does the right thing. The next version will
>> add this info to the commit message.
>>
>> Best regards
>> Thomas
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.0.5/source/drivers/video/fbdev/offb.c#L3
>> 57
> 
> Thanks for the link. The commit message for that original change [2] indicates
> this was a fix on a Powerstation which happens to be a PowerPC aka big-endian.
> This this happens to work as intended, but not on little-endian machines.
> vendor_id is __be32 (big-endian) and you compare this to PCI_VENDOR_ID_ATI
> (0x1002) which is native-endian. I guess you see the problem.
> See also [3] where a property is converted properly.
> 
> Thinking about it, instead of calling 'is_avivo(*vendor_p, *device_p)'
> I'd prefer something like 'is_avivo(be32_to_cpup(vendor_p),
> be32_to_cpup(device_p))', so there is no need to pass __be32 around.

Makes sense. Thanks for reviewing.

Best regards
Thomas

> 
> Best regards,
> Alexander
> 
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> commit/?id=57a20d8fb0d2a05abe40abd6bb29e3f923721f1b
> [3] https://elixir.bootlin.com/linux/v6.0.5/source/drivers/bus/fsl-mc/fsl-mc-bus.c#L1027
> 
>>> Best regards,
>>> Alexander
>>>
>>>> +
>>>>
>>>>    	/* This will match most R5xx */
>>>>    	return (vendor == PCI_VENDOR_ID_ATI) &&
>>>>    	
>>>>    	       ((device >= PCI_VENDOR_ID_ATI_R520 && device < 0x7800) ||
> 
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]