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

[Ming Qian]

> >>
> >> 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.
>
Hi Robin,

    Thanks for the detailed explanation, and I agree with you, I will use " / 2" in the v2 patch as it's indeed calculating half of a value.
 

> 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.

Yes, it's problem, and Dan has also pointed it, I 'll fix it in another patch.

Ming