[PR 17] GRETH: Fixed autonego timeout overflow problem

[Daniel Hellstrom]

On 03/20/2012 02:41 PM, Joel Sherrill wrote:
> On 03/20/2012 08:29 AM, Daniel Hellstrom wrote:
>> Hello,
>>
>> Ralf and Thomas, you are correct. This patch was created a long time ago when 'struct rtems_clock_time_value' was used it was defined:
>>
>> typedef struct {
>>     /** This is the seconds portion of a time of day. */
>>     uint32_t    seconds;
>>     /** This is the microseconds portion of a time of day. */
>>     uint32_t    microseconds;
>> } rtems_clock_time_value;
>>
>> Joel removed the structure in c5fd2cb6746ab2af2f38d74b17db07d39e6fb97a and 57674ef62e38b6c5566ea4c47fcd616aadb3ee5b, and introduced 'struct timeval' instead which is defined with signed tv_usec
>> instead of unsigned microseconds. I have missed this during merging somewhere long time ago.
>>
>> Joel, can you ignore this patch and close the PR1465. I will update the spreadsheet.
>
> Daniel...
>
> I remembered coming across this code a while back while trying to
> get rid of calls to the deprecated rtems_clock_set(). This driver
> shouldn't be using wall time at all. It should be using interval time.
>
> Can it be changed to either use rtems_clock_get_ticks_since_boot()
> or rtems_clock_get_uptime()?
>
> It really seems wrong that a driver could implicitly set the time of
> day.
>
> If you use rtems_clock_get_uptime(), you can use the internal
> timespec math routines like _Timespec_Greater_than() to
> help.
>
> And yes.. those eventually need to have a public API wrapper
> around them.

Hehe :) I fully agree with you, the current code is nonsense. I will try to create a patch later on that fixes this complete miss-behavior. Hopefully it wasn't me that wrote this code ;)

Thanks again,
Daniel

>
>
>> Thanks for reviewing,
>> Daniel
>>
>>
>> On 03/20/2012 08:51 AM, Daniel Hellstrom wrote:
>>> Hello Ralf,
>>>
>>> The problem was first reported by Joris, please see BUG 1465.
>>>
>>> -    msecs = (tnow.tv_sec-tstart.tv_sec)*1000+(tnow.tv_usec-tstart.tv_usec)/1000;
>>> +   msecs = (int)(tnow.tv_sec-tstart.tv_sec)*1000+((int)tnow.tv_usec-(int)tstart.tv_usec)/1000;
>>>
>>> The seconds part is no problem. If (tstart.tv_usec>  tnow.tv_usec) will be negative, but since the variables are unsigned a underflow will result in a very long msecs timeout.
>>>
>>> Thanks,
>>> Daniel
>>>
>>>
>>> On 03/19/2012 11:26 PM, Ralf Corsepius wrote:
>>>> On 03/19/2012 08:59 PM, Joel Sherrill wrote:
>>>>> This looks ok to me.
>>>> I am having strong doubts on this patch.
>>>>
>>>> Daniel, could you elaborate the problem you are trying to solve?
>>>>
>>>> Ralf
>>>>
>>>>
>>>>> I would have thought we had some support routines to do this but alas.
>>>>>
>>>>> Please commit someone.
>>>>> ________________________________________
>>>>> From: rtems-devel-bounces at rtems.org [rtems-devel-bounces at rtems.org] On Behalf Of Daniel Hellstrom [daniel at gaisler.com]
>>>>> Sent: Monday, February 06, 2012 8:15 AM
>>>>> To: rtems-devel at rtems.org
>>>>> Subject: [PR 17] GRETH: Fixed autonego timeout overflow problem
>>>>>
>>>>> Signed-off-by: Daniel Hellstrom<daniel at gaisler.com>
>>>>> ---
>>>>>    c/src/libchip/network/greth.c |    2 +-
>>>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/c/src/libchip/network/greth.c b/c/src/libchip/network/greth.c
>>>>> index aff4d0f..b0e70b4 100644
>>>>> --- a/c/src/libchip/network/greth.c
>>>>> +++ b/c/src/libchip/network/greth.c
>>>>> @@ -339,7 +339,7 @@ greth_initialize_hardware (struct greth_softc *sc)
>>>>>                while (!(((phystatus = read_mii(phyaddr, 1))>>   5)&   1)) {
>>>>>                        if ( rtems_clock_get_tod_timeval(&tnow) != RTEMS_SUCCESSFUL )
>>>>>                          printk("rtems_clock_get_tod_timeval failed\n\r");
>>>>> -                    msecs = (tnow.tv_sec-tstart.tv_sec)*1000+(tnow.tv_usec-tstart.tv_usec)/1000;
>>>>> +                    msecs = (int)(tnow.tv_sec-tstart.tv_sec)*1000+((int)tnow.tv_usec-(int)tstart.tv_usec)/1000;
>>>>>                        if ( msecs>   GRETH_AUTONEGO_TIMEOUT_MS ){
>>>>>                                sc->auto_neg_time = msecs;
>>>>>                                sc->auto_neg = -1; /* Failed */
>>>>> -- 
>>>>> 1.7.0.4
>>>>>
>>>>> _______________________________________________
>>>>> rtems-devel mailing list
>>>>> rtems-devel at rtems.org
>>>>> http://www.rtems.org/mailman/listinfo/rtems-devel
>>>>>
>>>>> _______________________________________________
>>>>> rtems-devel mailing list
>>>>> rtems-devel at rtems.org
>>>>> http://www.rtems.org/mailman/listinfo/rtems-devel
>>>>
>>> _______________________________________________
>>> rtems-devel mailing list
>>> rtems-devel at rtems.org
>>> http://www.rtems.org/mailman/listinfo/rtems-devel
>>>
>>>
>
>