Re: [EXT] RE: [PATCH] media: amphion: fix some error related with undefined reference to __divdi3

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

 



On 2022-03-10 08:36, Ming Qian wrote:
-----Original Message-----
From: David Laight [mailto:David.Laight@xxxxxxxxxx]
Sent: Wednesday, March 9, 2022 9:27 PM
To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx;
shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx
Cc: hverkuil-cisco@xxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
dl-linux-imx <linux-imx@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>;
linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
Subject: [EXT] RE: [PATCH] media: amphion: fix some error related with
undefined reference to __divdi3

Caution: EXT Email

From: Ming Qian
Sent: 09 March 2022 05:02
...
3. use 'val >> 1' instead of ' val / 2'

The compiler should do that anyway.

Especially for unsigned values.
And it has the wrong (different) rounding for negative values.

         David


Hi David,
     Yes, you are right, if the value is negative, the behavior is wrong.
     But here, the value type is u32, so I think it's OK.

Well, it depends on the semantic intent, really. If you're packing a bitfield which encodes bits 31:1 of some value then a shift is the most appropriate operation. However if you're literally calculating half of a value for, say, a 50% threshold level, or the number of 16-bit words represented by a byte length, then semantically it's a division, so it should use the divide operator rather than obfuscating it behind a shift. Constant division is something that even the most basic optimising compiler should handle with ease.

One more thing that's not the fault of this patch, but stood out in the context:

@@ -1566,7 +1568,7 @@ static bool vpu_malone_check_ready(struct vpu_shared_addr *shared, u32 instance)
 	u32 wptr = desc->wptr;
 	u32 used = (wptr + size - rptr) % size;

-	if (!size || used < size / 2)
+	if (!size || used < (size >> 1))
 		return true;

 	return false;

That's not safe: if "size" is 0 then the undefined behaviour has already happened before the "!size" check is reached. If "size" really can be 0, then it needs to be checked *before* it is used as a divisor to calculate "used".

Robin.



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux