Re: [Y2038] [PATCH 16/16] firewire: ohci: stop using get_seconds() for BUS_TIME
On Nov 08 Arnd Bergmann wrote:
> The ohci driver uses the get_seconds() function to implement the 32-bit
> CSR_BUS_TIME register. This was added in 2010 commit a48777e03ad5
> ("firewire: add CSR BUS_TIME support").
>
> As get_seconds() returns a 32-bit value (on 32-bit architectures), it
> seems like a good fit for that register, but it is also deprecated because
> of the y2038/y2106 overflow problem, and should be replaced throughout
> the kernel with either ktime_get_real_seconds() or ktime_get_seconds().
>
> I'm using the latter here, which uses monotonic time. This has the
> advantage of behaving better during concurrent settimeofday() updates
> or leap second adjustments and won't overflow a 32-bit integer, but
> the downside of using CLOCK_MONOTONIC instead of CLOCK_REALTIME is
> that the observed values are not related to external clocks.
>
> If we instead need UTC but can live with clock jumps or overflows,
> then we should use ktime_get_real_seconds() instead, retaining the
> existing behavior.
>
> Reviewed-by: Clemens Ladisch
> Cc: Stefan Richter
> Link: https://lore.kernel.org/lkml/20180711124923.1205200-1-a...@arndb.de/
> Signed-off-by: Arnd Bergmann
Committed to linux1394.git:for-next.
Thank you for the patch and your extraordinary patience.
> ---
> drivers/firewire/ohci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 522f3addb5bd..33269316f111 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -1752,7 +1752,7 @@ static u32 update_bus_time(struct fw_ohci *ohci)
>
> if (unlikely(!ohci->bus_time_running)) {
> reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_cycle64Seconds);
> - ohci->bus_time = (lower_32_bits(get_seconds()) & ~0x7f) |
> + ohci->bus_time = (lower_32_bits(ktime_get_seconds()) & ~0x7f) |
>(cycle_time_seconds & 0x40);
> ohci->bus_time_running = true;
> }
--
Stefan Richter
-==---== =-== -==-=
http://arcgraph.de/sr/
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH] [RESEND] firewire: ohci: stop using get_seconds() for BUS_TIME
On Jul 11 Arnd Bergmann wrote:
> The ohci driver uses the get_seconds() function to implement the 32-bit
> CSR_BUS_TIME register. This was added in 2010 commit a48777e03ad5
> ("firewire: add CSR BUS_TIME support").
>
> As get_seconds() returns a 32-bit value (on 32-bit architectures), it
> seems like a good fit for that register, but it is also deprecated because
> of the y2038/y2106 overflow problem, and should be replaced throughout
> the kernel with either ktime_get_real_seconds() or ktime_get_seconds().
>
> I'm using the latter here, which uses monotonic time. This has the
> advantage of behaving better during concurrent settimeofday() updates
> or leap second adjustments and won't overflow a 32-bit integer, but
> the downside of using CLOCK_MONOTONIC instead of CLOCK_REALTIME is
> that the observed values are not related to external clocks.
>
> If we instead need UTC but can live with clock jumps or overflows,
> then we should use ktime_get_real_seconds() instead, retaining the
> existing behavior.
>
> Reviewed-by: Clemens Ladisch
> Signed-off-by: Arnd Bergmann
> ---
> I notice that Stefan Richter has not been active on the mailing lists
> since February 2018.
Thanks Arnd and Clemens.
I resurrected and updated one of my FireWire enabled PCs, and try to get
pack to reasonable response times to firewire driver patches.
The switch from CLOCK_REALTIME to CLOCK_MONOTONIC looks good to me, but
I'll have another look at the context.
> Andrew, could you pick it up in the meantime?
> ---
> drivers/firewire/ohci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 45c048751f3b..5125841ea338 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -1765,7 +1765,7 @@ static u32 update_bus_time(struct fw_ohci *ohci)
>
> if (unlikely(!ohci->bus_time_running)) {
> reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_cycle64Seconds);
> - ohci->bus_time = (lower_32_bits(get_seconds()) & ~0x7f) |
> + ohci->bus_time = (lower_32_bits(ktime_get_seconds()) & ~0x7f) |
>(cycle_time_seconds & 0x40);
> ohci->bus_time_running = true;
> }
--
Stefan Richter
-==---=- -=== =-===
http://arcgraph.de/sr/
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH] firewire: nosy: Replace timeval with timespec64
On Mar 21 Arnd Bergmann wrote: >>> On Sunday 20 March 2016 22:59:11 Tina Ruchandani wrote: >>>> 'struct timeval' uses a 32 bit field for its 'seconds' value which >>>> will overflow in year 2038 and beyond. This patch replaces the use >>>> of timeval in nosy.c with timespec64 which doesn't suffer from y2038 >>>> issue. The code is correct as is - since it is only using the >>>> microseconds portion of timeval. However, this patch does the >>>> replacement as part of a larger effort to remove all instances of >>>> 'struct timeval' from the kernel (that would help identify cases >>>> where the code is actually broken). >>>> >>>> Signed-off-by: Tina Ruchandani <ruchandani.t...@gmail.com> [...] > Reviewed-by: Arnd Bergmann <a...@arndb.de> Committed to linux1394.git. I will try to get this pulled upstream in the next few days. -- Stefan Richter -==- --== =-==- http://arcgraph.de/sr/ ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH] firewire: nosy: Replace timeval with timespec64
On Mar 21 Arnd Bergmann wrote: > On Sunday 20 March 2016 22:59:11 Tina Ruchandani wrote: > > 'struct timeval' uses a 32 bit field for its 'seconds' value which > > will overflow in year 2038 and beyond. This patch replaces the use > > of timeval in nosy.c with timespec64 which doesn't suffer from y2038 > > issue. The code is correct as is - since it is only using the > > microseconds portion of timeval. However, this patch does the > > replacement as part of a larger effort to remove all instances of > > 'struct timeval' from the kernel (that would help identify cases > > where the code is actually broken). > > > > Signed-off-by: Tina Ruchandani <ruchandani.t...@gmail.com> > > --- > > drivers/firewire/nosy.c | 8 +--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > The patch looks correct to me, but it seems the same one has > just been merged into mainline Linux on Saturday (the patch > was posted back in October). > > commit 2ae4b6b20e2004dccf80d804ae52b073377c2f5b [...] No, Amitoj's patch from October changed nosy.c::packet_irq_hander, whereas Tina' patch changes nosy.c::bus_reset_irq_handler. IOW the new patch completes what the former patch (and us reviewers) missed. -- Stefan Richter -==- --== =-=-= http://arcgraph.de/sr/ ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2] firewire: Replace timeval with timespec64
On Oct 22 Arnd Bergmann wrote: > On Thursday 22 October 2015 04:05:00 Amitoj Kaur Chawla wrote: [...] > Reviewed-by: Arnd Bergmann <a...@arndb.de> > > (adding the y2038 mailing list as well) > > > Changes in v2: > > -Replaced timespec with timspec64 > > -Modified commit message > > -Used ktime_get_real_ts64() instead of getnstimeofday64() [...] > > --- a/drivers/firewire/nosy.c > > +++ b/drivers/firewire/nosy.c [...] > > @@ -413,17 +414,18 @@ static void > > packet_irq_handler(struct pcilynx *lynx) [...] > > - do_gettimeofday(); > > - lynx->rcv_buffer[0] = (__force __le32)tv.tv_usec; > > + ktime_get_real_ts64(); > > + timestamp = ts64.tv_nsec / NSEC_PER_USEC; > > + lynx->rcv_buffer[0] = (__force __le32)timestamp; Looks fine to me, but I have a question. It was possibly already discussed at patch v1, though that was apparently not posted to an open list. include/linux/timekeeping.h says: #define ktime_get_real_ts64(ts) getnstimeofday64(ts) kernel/time/timekeeping.c says: /** * do_gettimeofday - Returns the time of day in a timeval * @tv: pointer to the timeval to be set * * NOTE: Users should be converted to using getnstimeofday() */ So what is the reason for calling ktime_get_real_ts64() instead of getnstimeofday[64]()? PS, note to self: Independently of this patch, I need to check whether CLOCK_REALTIME was really the right clock here, in contrast to CLOCK_MONOTONIC. -- Stefan Richter -=-= =-=- =-==- http://arcgraph.de/sr/ ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
