Em Ter, 2016-09-13 às 20:40 +0100, Chris Wilson escreveu:
> I was looking at some wait_for() timeouts on a slow system, with lots
> of
> debug enabled (KASAN, lockdep, mmio_debug). Thinking that we were
> mishandling the timeout, I tried to ensure that we loop at least once
> after first testing COND. However, the double test of COND either
> side
> of the timeout check makes that unlikely. But we can do an equivalent
> loop, that keeps the COND check after testing for timeout (required
> so
> that we are not preempted between testing COND and then testing for a
> timeout) without expanding COND twice.
>
> The advantage of only expanding COND once is a dramatic reduction in
> code size:
>
> text data bss dec hex
> 1308733 5184 1152 1315069 1410fd
> before
> 1305341 5184 1152 1311677 1403bd
> after
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index cb99a2540863..597899d71df9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -52,13 +52,16 @@
> */
> #define _wait_for(COND, US, W) ({ \
> unsigned long timeout__ = jiffies + usecs_to_jiffies(US) +
> 1; \
> - int ret__ = 0;
> \
> - while (!(COND)) {
> \
> - if (time_after(jiffies, timeout__)) {
> \
> - if (!(COND))
> \
> - ret__ = -ETIMEDOUT;
> \
> + int ret__;
ret__ starts "uninitialized".
> \
> + for (;;) {
> \
> + if (time_after(jiffies, timeout__))
> \
> + ret__ = -ETIMEDOUT;
If we didn't hit the timeout, it's still "uninitialized".
> \
> + if (COND) {
> \
> + ret__ = 0;
If the condition was not met, it's still "uninitialized".
> \
> break;
> \
> }
> \
> + if (ret__)
But we read its "uninitialized" value here.
But why isn't the compiler complaining about this? Am I failing to see
something here?
If my analysis is correct, all you need to do is to keep ret__ being
initialized to zero. At least for clarity of the future code readers in
case it's expected to be auto-initialized to zero due to some weird
rule about compound statements or something.
With the ret__ initialization to zero:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> \
> + break;
> \
> if ((W) && drm_can_sleep()) {
> \
> usleep_range((W), (W)*2);
> \
> } else {
> \
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
