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.
