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

[Robin Murphy]

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.