| Message ID | 20180309112149.9801-1-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lucas, Please retain my authorship of my patch, which was sent on 23 Oct 2017. The patch you have below is 100% identical to that which I sent. You should also point out, as per the follow-on discussion, that using clock_gettime() on 32-bit systems will not work once the time it reports wraps - so something like the comment I suggested in a follow up patch: + * Etnaviv timeouts are specified wrt CLOCK_MONOTONIC, not jiffies. + * We need to calculate the timeout in terms of number of jiffies + * between the specified timeout and the current CLOCK_MONOTONIC time. + * Note: clock_gettime() is 32-bit on 32-bit arch. Using 64-bit + * timespec math here just means that when a wrap occurs, the + * specified timeout goes into the past and we can't request a + * timeout in the future: IOW, the code breaks. would be sensible either in the commit message or the code. On Fri, Mar 09, 2018 at 12:21:49PM +0100, Lucas Stach wrote: > The old way did clamp the jiffy conversion and thus caused the timeouts > to become negative after some time. Also it didn't work with userspace > which actually fills the upper 32bits of the 64bit timestamp value. > > Fix this by using the solution developed and tested by Russell. > > Suggested-by: Russell King <linux@armlinux.org.uk> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > drivers/gpu/drm/etnaviv/etnaviv_drv.h | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > index ddb17ee565e9..17a43da98fb9 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > @@ -26,6 +26,7 @@ > #include <linux/pm_runtime.h> > #include <linux/slab.h> > #include <linux/list.h> > +#include <linux/time64.h> > #include <linux/types.h> > #include <linux/sizes.h> > > @@ -132,19 +133,27 @@ static inline bool fence_after_eq(u32 a, u32 b) > return (s32)(a - b) >= 0; > } > > +/* > + * Etnaviv timeouts are specified wrt CLOCK_MONOTONIC, not jiffies. > + * We need to calculate the timeout in terms of number of jiffies > + * between the specified timeout and the current CLOCK_MONOTONIC time. > + */ > static inline unsigned long etnaviv_timeout_to_jiffies( > const struct timespec *timeout) > { > - unsigned long timeout_jiffies = timespec_to_jiffies(timeout); > - unsigned long start_jiffies = jiffies; > - unsigned long remaining_jiffies; > + struct timespec64 ts, to; > + > + to = timespec_to_timespec64(*timeout); > + > + ktime_get_ts64(&ts); > + > + /* timeouts before "now" have already expired */ > + if (timespec64_compare(&to, &ts) <= 0) > + return 0; > > - if (time_after(start_jiffies, timeout_jiffies)) > - remaining_jiffies = 0; > - else > - remaining_jiffies = timeout_jiffies - start_jiffies; > + ts = timespec64_sub(to, ts); > > - return remaining_jiffies; > + return timespec64_to_jiffies(&ts); > } > > #endif /* __ETNAVIV_DRV_H__ */ > -- > 2.16.1 >
Hi Russell, Am Freitag, den 09.03.2018, 11:44 +0000 schrieb Russell King - ARM Linux: > Hi Lucas, > > Please retain my authorship of my patch, which was sent on 23 Oct 2017. > The patch you have below is 100% identical to that which I sent. I'll gladly do so if you provide me a proper Signed-off-by for this patch, which was missing from your 23 Oct 2017 submission. > You should also point out, as per the follow-on discussion, that using > clock_gettime() on 32-bit systems will not work once the time it > reports wraps - so something like the comment I suggested in a follow > up patch: > > + * Etnaviv timeouts are specified wrt CLOCK_MONOTONIC, not jiffies. > + * We need to calculate the timeout in terms of number of jiffies > + * between the specified timeout and the current CLOCK_MONOTONIC time. > + * Note: clock_gettime() is 32-bit on 32-bit arch. Using 64-bit > + * timespec math here just means that when a wrap occurs, the > + * specified timeout goes into the past and we can't request a > + * timeout in the future: IOW, the code breaks. > > would be sensible either in the commit message or the code. I'll add it to the commit message, as I think the discussion with Arnd established that this is a very theoretical risk, not likely to be hit in practice. Regards, Lucas > On Fri, Mar 09, 2018 at 12:21:49PM +0100, Lucas Stach wrote: > > The old way did clamp the jiffy conversion and thus caused the timeouts > > to become negative after some time. Also it didn't work with userspace > > which actually fills the upper 32bits of the 64bit timestamp value. > > > > Fix this by using the solution developed and tested by Russell. > > > > > > Suggested-by: Russell King <linux@armlinux.org.uk> > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > drivers/gpu/drm/etnaviv/etnaviv_drv.h | 25 +++++++++++++++++-------- > > 1 file changed, 17 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > > index ddb17ee565e9..17a43da98fb9 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > > @@ -26,6 +26,7 @@ > > #include <linux/pm_runtime.h> > > #include <linux/slab.h> > > #include <linux/list.h> > > +#include <linux/time64.h> > > #include <linux/types.h> > > #include <linux/sizes.h> > > > > @@ -132,19 +133,27 @@ static inline bool fence_after_eq(u32 a, u32 b) > > > > return (s32)(a - b) >= 0; > > } > > > > +/* > > + * Etnaviv timeouts are specified wrt CLOCK_MONOTONIC, not jiffies. > > + * We need to calculate the timeout in terms of number of jiffies > > + * between the specified timeout and the current CLOCK_MONOTONIC time. > > + */ > > static inline unsigned long etnaviv_timeout_to_jiffies( > > > > const struct timespec *timeout) > > { > > > > - unsigned long timeout_jiffies = timespec_to_jiffies(timeout); > > > > - unsigned long start_jiffies = jiffies; > > > > - unsigned long remaining_jiffies; > > > > + struct timespec64 ts, to; > > + > > > > + to = timespec_to_timespec64(*timeout); > > + > > > > + ktime_get_ts64(&ts); > > + > > > > + /* timeouts before "now" have already expired */ > > > > + if (timespec64_compare(&to, &ts) <= 0) > > > > + return 0; > > > > > > - if (time_after(start_jiffies, timeout_jiffies)) > > > > - remaining_jiffies = 0; > > > > - else > > > > - remaining_jiffies = timeout_jiffies - start_jiffies; > > > > + ts = timespec64_sub(to, ts); > > > > > > - return remaining_jiffies; > > > > + return timespec64_to_jiffies(&ts); > > } > > > > #endif /* __ETNAVIV_DRV_H__ */ > > -- > > 2.16.1 > > > >
On Fri, Mar 09, 2018 at 12:52:40PM +0100, Lucas Stach wrote: > Hi Russell, > > Am Freitag, den 09.03.2018, 11:44 +0000 schrieb Russell King - ARM Linux: > > Hi Lucas, > > > > Please retain my authorship of my patch, which was sent on 23 Oct 2017. > > The patch you have below is 100% identical to that which I sent. > > I'll gladly do so if you provide me a proper Signed-off-by for this > patch, which was missing from your 23 Oct 2017 submission. It wasn't a submission, but was for discussion and I provided two variants. However, by doing what you've done, you're effectively claiming authorship of my work, and as works are copyrighted, that's really not nice. Unfortunately, the Linux community has tied itself in knots over the "author must sign-off on the patch" despite DCO (b) existing, and DCO (b) specifically allows for patches that are not authored by the submitter to be incorporated into the kernel. If you take the authorship and the manufactured sign-off requirements together, then effectively no one has the ability to submit code that they did not author. Nevertheless, for this patch, Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > You should also point out, as per the follow-on discussion, that using > > clock_gettime() on 32-bit systems will not work once the time it > > reports wraps - so something like the comment I suggested in a follow > > up patch: > > > > + * Etnaviv timeouts are specified wrt CLOCK_MONOTONIC, not jiffies. > > + * We need to calculate the timeout in terms of number of jiffies > > + * between the specified timeout and the current CLOCK_MONOTONIC time. > > + * Note: clock_gettime() is 32-bit on 32-bit arch. Using 64-bit > > + * timespec math here just means that when a wrap occurs, the > > + * specified timeout goes into the past and we can't request a > > + * timeout in the future: IOW, the code breaks. > > > > would be sensible either in the commit message or the code. > > I'll add it to the commit message, as I think the discussion with Arnd > established that this is a very theoretical risk, not likely to be hit > in practice. True, it's only likely to be hit if a system is up for 68 years, but it's still worth documenting.
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h index ddb17ee565e9..17a43da98fb9 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h @@ -26,6 +26,7 @@ #include <linux/pm_runtime.h> #include <linux/slab.h> #include <linux/list.h> +#include <linux/time64.h> #include <linux/types.h> #include <linux/sizes.h> @@ -132,19 +133,27 @@ static inline bool fence_after_eq(u32 a, u32 b) return (s32)(a - b) >= 0; } +/* + * Etnaviv timeouts are specified wrt CLOCK_MONOTONIC, not jiffies. + * We need to calculate the timeout in terms of number of jiffies + * between the specified timeout and the current CLOCK_MONOTONIC time. + */ static inline unsigned long etnaviv_timeout_to_jiffies( const struct timespec *timeout) { - unsigned long timeout_jiffies = timespec_to_jiffies(timeout); - unsigned long start_jiffies = jiffies; - unsigned long remaining_jiffies; + struct timespec64 ts, to; + + to = timespec_to_timespec64(*timeout); + + ktime_get_ts64(&ts); + + /* timeouts before "now" have already expired */ + if (timespec64_compare(&to, &ts) <= 0) + return 0; - if (time_after(start_jiffies, timeout_jiffies)) - remaining_jiffies = 0; - else - remaining_jiffies = timeout_jiffies - start_jiffies; + ts = timespec64_sub(to, ts); - return remaining_jiffies; + return timespec64_to_jiffies(&ts); } #endif /* __ETNAVIV_DRV_H__ */
The old way did clamp the jiffy conversion and thus caused the timeouts to become negative after some time. Also it didn't work with userspace which actually fills the upper 32bits of the 64bit timestamp value. Fix this by using the solution developed and tested by Russell. Suggested-by: Russell King <linux@armlinux.org.uk> Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)