[PATCH] octeon: false positive timeout

David Daney ddaney at caviumnetworks.com
Fri Jul 17 16:45:36 UTC 2009


roel kluin wrote:
> 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
> 

The burden of proof was never on me, however your new explanation 
actually makes sense to me now, so...

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

... if you put something like that in the change log, we could consider 
the patch.

I have no objection to the patch with a good change log, but we should 
be clear that it is only going to make a difference in the last 
iteration of a 1000 loop timeout, so other equivalent patches would be 
to change the condition of the if statement you have above or increase 
the iteration count to 1001, there is no real world problem that we are 
solving, the patch is purely for aesthetic reasons.

David Daney



More information about the devel mailing list