| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 9 Nov 2019 13:12:07 +0100
From: Arnd Bergmann <arnd@...db.de>
To: Lucas Stach <l.stach@...gutronix.de>
Cc: y2038 Mailman List <y2038@...ts.linaro.org>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Guido Günther <agx@...xcpu.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
dri-devel <dri-devel@...ts.freedesktop.org>,
The etnaviv authors <etnaviv@...ts.freedesktop.org>,
Russell King <linux+etnaviv@...linux.org.uk>,
Sam Ravnborg <sam@...nborg.org>,
Christian König <christian.koenig@....com>,
Emil Velikov <emil.velikov@...labora.com>
Subject: Re: [PATCH 15/16] drm/etnaviv: use ktime_t for timeouts
On Sat, Nov 9, 2019 at 12:03 AM Lucas Stach <l.stach@...gutronix.de> wrote:
>
> Am Freitag, den 08.11.2019, 22:32 +0100 schrieb Arnd Bergmann:
> > struct timespec is being removed from the kernel because it often leads
> > to code that is not y2038-safe.
> >
> > In the etnaviv driver, monotonic timestamps are used, which do not suffer
> > from overflow, but using ktime_t still leads to better code overall.
> >
> > The conversion is straightforward for the most part, except for
> > etnaviv_timeout_to_jiffies(), which needs to handle arguments larger
> > than MAX_JIFFY_OFFSET on 32-bit architectures.
> >
> > Signed-off-by: Arnd Bergmann <arnd@...db.de>
> > @@ -368,7 +366,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data,
> > return -ENXIO;
> >
> > if (args->flags & ETNA_WAIT_NONBLOCK)
> > - timeout = NULL;
> > + timeout = ktime_set(0, 0);
>
> This is a change in behavior, as far as I can see. After this change
> the called internal function is not able to differentiate between a
> NONBLOCK call and a blocking call with 0 timeout. The difference being
> that on a busy object the NONBLOCK call will return -EBUSY, while a
> blocking call will return -ETIMEDOUT.
Ah, good point. I created this patch a long time ago (cherry-picked it out
of an older branch I had done), so I don't remember how I concluded this
was a safe conversion, of if I missed that difference originally.
> But then CLOCK_MONOTONIC starts at 0 and should not never wrap, right?
Yes, that is correct.
> If that's the case then we should never encounter a genuine 0 timeout
> and this change would be okay.
That's quite likely, I'd say any program passing {0,0} as a timeout without
ETNA_WAIT_NONBLOCK is already broken, but if we leave it like that,
it would be best to describe the reasoning in the changelog.
Should I change the changelog, or change the patch to restore the
current behavior instead?
I guess I could fold the change below into my patch to make it transparent
to the application again.
Arnd
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 1250c5e06329..162cedfb7f72 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -354,6 +354,7 @@ static int etnaviv_ioctl_wait_fence(struct
drm_device *dev, void *data,
ktime_t timeout = ktime_set(args->timeout.tv_sec,
args->timeout.tv_nsec);
struct etnaviv_gpu *gpu;
+ int ret;
if (args->flags & ~(ETNA_WAIT_NONBLOCK))
return -EINVAL;
@@ -365,8 +366,12 @@ static int etnaviv_ioctl_wait_fence(struct
drm_device *dev, void *data,
if (!gpu)
return -ENXIO;
- if (args->flags & ETNA_WAIT_NONBLOCK)
- timeout = ktime_set(0, 0);
+ if (args->flags & ETNA_WAIT_NONBLOCK) {
+ ret = etnaviv_gpu_wait_fence_interruptible(gpu, args->fence,
+ ktime_set(0, 0));
+
+ return (ret == -ETIMEDOUT) ? -EBUSY : ret;
+ }
return etnaviv_gpu_wait_fence_interruptible(gpu, args->fence,
timeout);
@@ -421,10 +426,13 @@ static int etnaviv_ioctl_gem_wait(struct
drm_device *dev, void *data,
if (!obj)
return -ENOENT;
- if (args->flags & ETNA_WAIT_NONBLOCK)
- timeout = ktime_set(0, 0);
-
- ret = etnaviv_gem_wait_bo(gpu, obj, timeout);
+ if (args->flags & ETNA_WAIT_NONBLOCK) {
+ ret = etnaviv_gem_wait_bo(gpu, obj, ktime_set(0, 0));
+ if (ret == -ETIMEDOUT)
+ ret = -EBUSY;
+ } else {
+ ret = etnaviv_gem_wait_bo(gpu, obj, timeout);
+ }
drm_gem_object_put_unlocked(obj);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index e42b1c4d902c..fa6986c5a5fe 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1135,6 +1135,7 @@ int etnaviv_gpu_wait_fence_interruptible(struct
etnaviv_gpu *gpu,
u32 id, ktime_t timeout)
{
struct dma_fence *fence;
+ unsigned long remaining;
int ret;
/*
@@ -1151,12 +1152,12 @@ int
etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu,
if (!fence)
return 0;
- if (!timeout) {
- /* No timeout was requested: just test for completion */
- ret = dma_fence_is_signaled(fence) ? 0 : -EBUSY;
+ if (!timeout ||
+ (remaining = etnaviv_timeout_to_jiffies(timeout)) == 0) {
+ /* No timeout was requested, or timeout is already expired,
+ * just test for completion */
+ ret = dma_fence_is_signaled(fence) ? 0 : -ETIMEDOUT;
} else {
- unsigned long remaining = etnaviv_timeout_to_jiffies(timeout);
-
ret = dma_fence_wait_timeout(fence, true, remaining);
if (ret == 0)
ret = -ETIMEDOUT;
@@ -1185,7 +1186,7 @@ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu,
long ret;
if (!timeout)
- return !is_active(etnaviv_obj) ? 0 : -EBUSY;
+ return !is_active(etnaviv_obj) ? 0 : -ETIMEDOUT;
remaining = etnaviv_timeout_to_jiffies(timeout);
Powered by blists - more mailing lists