Re: [Xenomai] error / bugfix in ipipe-core-3-10.28-powerpc-1.patch (from eldk 5.5, company denx)

[Philippe Gerum <rpm@xenomai.org>]

On 04/14/2014 02:10 PM, Ralf wrote:
> Am 14.04.2014 09:57, schrieb Philippe Gerum:
>> On 04/14/2014 08:55 AM, Ralf wrote:
>>> Am 28.03.2014 08:39, schrieb Ralf:
>>>> Hello,
>>>>
>>>> i have found an error in the ipipe-patch above.
>>>> Here ist the error description:
>>>>
>>>> Our system:
>>>> Linux 3.10.28 with xenomai 2.6 patch on a PowerPC MPC5200
>>>>
>>>> Error description:
>>>> We use the xenomai-(posix-skin-)function clock_gettime(
>>>> CLOCK_HOST_REALTIME, ..) to read out our system time. Thus, we set up a
>>>> NTP clock that runs on an FPGA. Later, we compare the system time and
>>>> NTP time deviations. Unfortunately we recognize in any comparison with
>>>> the nanoseconds a deviation of at least 100 million nsecs. We then
>>>> started the xenomai test suite and get the following prints on our console:
>>>>
>> [snip]
>>
>>>> tk_xtime() makes a shift to the right on the nanoseconds. This shift is
>>>> at our clock source 24. If this shift now run twice consecutively, that
>>>> explains naturally why we only get the most significant 8 bits of the
>>>> nanosecond value.
>>>>
>>>> a fix that might solve the problem, looks like this
>>>>
>>>> void ipipe_update_hostrt(struct timekeeper *tk)
>>>> {
>>>>       struct ipipe_hostrt_data data;
>>>>       struct timespec xt;
>>>>
>>>> //     xt = tk_xtime(tk);
>>>>       ipipe_root_only();
>>>>       data.live = 1;
>>>>       data.cycle_last = tk->clock->cycle_last;
>>>>       data.mask = tk->clock->mask;
>>>>       data.mult = tk->mult;
>>>>       data.shift = tk->shift;
>>>>       data.wall_time_sec = tk->xtime_sec;
>>>>       data.wall_time_nsec = (long)tk->xtime_nsec;
>>>>       data.wall_to_monotonic = tk->wall_to_monotonic;
>>>>       __ipipe_notify_kevent(IPIPE_KEVT_HOSTRT, &data);
>>>> }
>>>>
>>> Has anyone observed the same thing? Do you think that bugfix correct?
>>> How can i bring it into mainline? I would be very grateful for some help.
>>>
>> Good catch, thanks. This bug is introduced by a work-around specific to the few architectures which do not implement the newest VSYSCALL interface yet, like powerpc. However, the suggested change would break others which do, could you test the patch below instead? TIA,
>>
>> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
>> index e1214ba..5b01356 100644
>> --- a/arch/powerpc/kernel/time.c
>> +++ b/arch/powerpc/kernel/time.c
>> @@ -749,7 +749,7 @@ static cycle_t timebase_read(struct clocksource *cs)
>>    * API.
>>    */
>>   static inline void update_hostrt(struct timespec *wall_time, struct timespec *wtm,
>> -				 struct clocksource *clock, u32 mult)
>> +				 struct clocksource *clock, u32 mult, u32 shift)
>>   {
>>   	/*
>>   	 * This is a temporary work-around until powerpc implements
>> @@ -761,7 +761,7 @@ static inline void update_hostrt(struct timespec *wall_time, struct timespec *wt
>>   		.shift = clock->shift,
>>   		.mult = mult,
>>   		.xtime_sec = wall_time->tv_sec,
>> -		.xtime_nsec = wall_time->tv_nsec,
>> +		.xtime_nsec = wall_time->tv_nsec << shift,
> Unfortunately, this solution does not work on our powerpc, because
> "wall_time->tv_nsec" is only a 32 bit value and "xtime_nsec" a 64 bit
> value. It seems that the shift on the 32 bit value happens first and
> only then the assignment to the 64 bit value. So the shifted bits
> greater than 32 bit are lost.
> A cast avoids this problem:
>
>                          .xtime_nsec = (u64)wall_time->tv_nsec << shift;
>
> After that everything works well.

This make sense. I'll merge this fix, thanks.

-- 
Philippe.