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; }