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
