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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Link:
https://lore.kernel.org/dri-devel/202210192208.D888I6X7-lkp@xxxxxxxxx/ #
[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: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux