On Thu, Dec 04, 2014 at 04:31:15PM +0000, Chris Wilson wrote: > On Thu, Dec 04, 2014 at 01:28:39PM +0100, Daniel Vetter wrote: > > On Thu, Dec 04, 2014 at 11:51:14AM +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. > > > > I've read the manpage for read and it explicitly states that when you get > > an error it's undefined what happens to the read position. Since -EFAULT > > is really just a userspace bug I think we can happily drop the event on > > the floor, no reason to bend over in the kernel. > > Hmm. Actually the code is buggy is the provided buffer is too short for > the first event in O_NONBLOCK mode.
Well we essentially send out datagrams instead of a bytestream. If we look at recvmsg and friends then discarding the additional bytes is something that's already being done. So I'm not terribly concerned about that either. It's a bit non-pretty that we use read and not reicvmsg but since we use read already not something we can ever fix. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch