At Thu, 4 Dec 2014 12:13:46 +0000, Chris Wilson wrote: > > On Thu, Dec 04, 2014 at 12:56:38PM +0100, Takashi Iwai wrote: > > At Thu, 4 Dec 2014 11:51:14 +0000, > > Chris Wilson wrote: > > > > > > On Thu, Dec 04, 2014 at 11:56:42AM +0100, Takashi Iwai wrote: > > > > Signed-off-by: Takashi Iwai <tiwai at suse.de> > > > > --- > > > > drivers/gpu/drm/drm_fops.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > > > > index ed7bc68f7e87..a82dc28d54f3 100644 > > > > --- a/drivers/gpu/drm/drm_fops.c > > > > +++ b/drivers/gpu/drm/drm_fops.c > > > > @@ -525,6 +525,7 @@ ssize_t drm_read(struct file *filp, char __user > > > > *buffer, > > > > if (copy_to_user(buffer + total, > > > > e->event, e->event->length)) { > > > > total = -EFAULT; > > > > + e->destroy(e); > > > > > > We shouldn't just be throwing away the event here, but put the event > > > back at the front of the queue. Poses an interesting race issue. Seems > > > like we want to hold the spinlock until the copy is complete so that we > > > can fix up the failure correctly. > > > > Yeah, I thought of it while writing this, but as a starter, I tried > > the simpler one. (And I didn't realize your comment referring to the > > already existing fix in kernel, sorry!) > > > > The problem to hold the spinlock for the whole is that you can't it > > with copy_to_user(). So it'd be a bit tricky. > > We can use access_ok(VERFIY_WRITE) and then copy_to_user_inatomic(). > Doable with a bit of code rearrangement.
Yes, but this is really not necessarily atomic. We can simply prepend the event back in the error case, instead, too. > > And, why not using event_wait.lock instead of the extra event_lock? > > Then we can use wait_event_lock_*() variant that covers more race > > between dequeuing and wait_event. > > The tricky part would appear to be then protecting > dev->vblank_event_list. That looks like it could be an RCU list instead? Sounds interesting. Takashi