[PATCH] octeon: false positive timeout

roel kluin roel.kluin at gmail.com
Fri Jul 17 10:20:09 UTC 2009


On Thu, Jul 16, 2009 at 5:36 PM, David Daney<ddaney at caviumnetworks.com> wrote:
> Roel Kluin wrote:
>>
>> Otherwise a `timeout' value of 0 would not have been a timeout, but
>> rather that `smi_rd.s.pending' caused the loop to end in the last
>> iteration.
>>
>> Signed-off-by: Roel Kluin <roel.kluin at gmail.com>
>
> NAK.
>
> I can't really parse the meaning out of your explanation, but this does not
> need to change.

Please reconsider, I think my patch is correct or prove me otherwise

> * timeout will always be a positive non-zero number because of the preceding
> if statement.  Any arguments about what would happen with a zero timeout are
> moot.

I think you misunderstand my changelog. It refers to when timeout reaches the
second(!):

        if (timeout <= 0) {
                cvmx_dprintf("cvmx_mdio_45_read: bus_id %d phy_id %2d "
                             "device %2d register %2d   TIME OUT(data)\n",
                     bus_id, phy_id, device, location);
                return -1;
        }

(just visible in my patch, below the change)

If we reach that with a `timeout' value of 0, this does not mean that
the timeout
caused the loop to end, but rather the `smi_rd.s.pending', in the last
iteration.
If timeout causes the loop to end, then `timeout' is -1, not 0.

> * You never say if you think this is causing or could potentially cause
> undesirable behaviour.

The undesirable behaviour would be that if timeout is 0 we currently
have a false positive cvmx_dprintf() and error return.

If still not convinced observe the output of the snippet below.

Roel

#include <stdio.h>
#include <stdlib.h>

int main()
{
        int i, r, timeout;
        for (i=0; i<1000; i++) {
                timeout=3;
                do {
                        r = rand();
                } while (r < RAND_MAX/2 && timeout--);
                if (timeout <= 0)
                        printf ("%d: %d, %d\n", i, r, timeout);
        }

        return 0;
}



More information about the devel mailing list