All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 3/3] media: check status of dmxdev->exit in poll functions of demux&dvr
@ 2014-08-21  2:05 Changbing Xiong
  2014-09-01 23:58 ` Antti Palosaari
  0 siblings, 1 reply; 5+ messages in thread
From: Changbing Xiong @ 2014-08-21  2:05 UTC (permalink / raw)
  To: linux-media; +Cc: m.chehab, crope

when usb-type tuner is pulled out, user applications did not close device's FD,
and go on polling the device, we should return POLLERR directly.

Signed-off-by: Changbing Xiong <cb.xiong@samsung.com>
---
 drivers/media/dvb-core/dmxdev.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c
index 7a5c070..42b5e70 100755
--- a/drivers/media/dvb-core/dmxdev.c
+++ b/drivers/media/dvb-core/dmxdev.c
@@ -1085,9 +1085,10 @@ static long dvb_demux_ioctl(struct file *file, unsigned int cmd,
 static unsigned int dvb_demux_poll(struct file *file, poll_table *wait)
 {
 	struct dmxdev_filter *dmxdevfilter = file->private_data;
+	struct dmxdev *dmxdev = dmxdevfilter->dev;
 	unsigned int mask = 0;

-	if (!dmxdevfilter)
+	if ((!dmxdevfilter) || (dmxdev->exit))
 		return POLLERR;

 	poll_wait(file, &dmxdevfilter->buffer.queue, wait);
@@ -1181,6 +1182,9 @@ static unsigned int dvb_dvr_poll(struct file *file, poll_table *wait)

 	dprintk("function : %s\n", __func__);

+	if (dmxdev->exit)
+		return POLLERR;
+
 	poll_wait(file, &dmxdev->dvr_buffer.queue, wait);

 	if ((file->f_flags & O_ACCMODE) == O_RDONLY) {
--
1.7.9.5


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: Re: [PATCH 3/3] media: check status of dmxdev->exit in poll functions of demux&dvr
@ 2014-09-02  3:16 Changbing Xiong
  2014-09-02  6:00 ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 5+ messages in thread
From: Changbing Xiong @ 2014-09-02  3:16 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Antti Palosaari; +Cc: linux-media@vger.kernel.org


> Well, we may start returning -ENODEV when such event happens. 

> At the frontend, we could use fe->exit = DVB_FE_DEVICE_REMOVED to
> signalize it. I don't think that the demod frontend has something
> similar.

> Yet, it should be up to the userspace application to properly handle 
> the error codes and close the devices on fatal non-recovery errors like
> ENODEV. 

> So, what we can do, at Kernel level, is to always return -ENODEV when
> the device is known to be removed, and double check libdvbv5 if it
> handles such error properly.

 well, we do not use libdvbv5, and  -ENODEV can be returned by read syscall,  
but for poll syscall,  -ENODEV can never be returned to user, as negative number
 is invalid  type for poll returned value. please refer to my second patch.

and in our usage, whether to read the device is up to the poll result. if tuner is plugged out, 
and there is no data in dvr ringbuffer. then user code will still go on polling the dvr device and never stop.
if POLLERR is returned, then user will perform read dvr, and then -ENODEV can be got, and 
user will stop polling dvr device.

the first patch is enough to fix the deadlock issue.
the second patch is used to correct the wrong type of returned value.
the third patch is used to provide user a better controlling logic.



^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: Re: [PATCH 3/3] media: check status of dmxdev->exit in poll functions of demux&dvr
@ 2014-09-02  6:42 Changbing Xiong
  2014-09-02  7:15 ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 5+ messages in thread
From: Changbing Xiong @ 2014-09-02  6:42 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Antti Palosaari, linux-media@vger.kernel.org

> Actually, poll() may return an error as well (from poll() manpage):

> "RETURN VALUE
>        On success, a positive number is returned; this is the number of struc‐
>        tures which have nonzero revents fields (in other words, those descrip‐
>        tors  with events or errors reported).  A value of 0 indicates that the
>        call timed out and no file descriptors were ready.   On  error,  -1  is
>        returned, and errno is set appropriately."

> So, if the Kernel returns -ENODEV, the glibc poll() wrapper would return -1
> and errno will be ENODEV. Never actually tested if this works on poll()
> though.

> Actually, poll() may return an error as well (from poll() manpage):

> "RETURN VALUE
>        On success, a positive number is returned; this is the number of struc‐
>        tures which have nonzero revents fields (in other words, those descrip‐
>        tors  with events or errors reported).  A value of 0 indicates that the
>        call timed out and no file descriptors were ready.   On  error,  -1  is
>        returned, and errno is set appropriately."

> So, if the Kernel returns -ENODEV, the glibc poll() wrapper would return -1
> and errno will be ENODEV. Never actually tested if this works on poll()
> though.

maybe the poll() manpage is wrong.
The standard system call poll() can not get -ENODEV from errno. 
My experiment has proved that I was right(return -ENODEV directly in dvb_dvr_poll).  
and you can also check code of do_poll() and do_sys_poll() in select.c file, it also shows that -ENODEV is invalid.

please also check that.

thanks!
Xiong changbing

------- Original Message -------
Sender : Mauro Carvalho Chehab<m.chehab@samsung.com> Director/SRBR-Open Source/삼성전자
Date : 九月 02, 2014 15:00 (GMT+09:00)
Title : Re: [PATCH 3/3] media: check status of dmxdev->exit in poll functions of demux&dvr

Em Tue, 02 Sep 2014 03:16:00 +0000 (GMT)
Changbing Xiong escreveu:

> 
> > Well, we may start returning -ENODEV when such event happens. 
> 
> > At the frontend, we could use fe->exit = DVB_FE_DEVICE_REMOVED to
> > signalize it. I don't think that the demod frontend has something
> > similar.
> 
> > Yet, it should be up to the userspace application to properly handle 
> > the error codes and close the devices on fatal non-recovery errors like
> > ENODEV. 
> 
> > So, what we can do, at Kernel level, is to always return -ENODEV when
> > the device is known to be removed, and double check libdvbv5 if it
> > handles such error properly.
> 
>  well, we do not use libdvbv5,

The upstream stuff I maintain, related to it, are the media subsystems
and libdvbv5. Of course, other apps will need to be patched as well.

> and  -ENODEV can be returned by read syscall,  
> but for poll syscall,

Actually, poll() may return an error as well (from poll() manpage):

"RETURN VALUE
       On success, a positive number is returned; this is the number of struc‐
       tures which have nonzero revents fields (in other words, those descrip‐
       tors  with events or errors reported).  A value of 0 indicates that the
       call timed out and no file descriptors were ready.   On  error,  -1  is
       returned, and errno is set appropriately."

So, if the Kernel returns -ENODEV, the glibc poll() wrapper would return -1
and errno will be ENODEV. Never actually tested if this works on poll()
though.

>  -ENODEV can never be returned to user, as negative number
>  is invalid  type for poll returned value. please refer to my second patch.
> 
> and in our usage, whether to read the device is up to the poll result. if tuner is plugged out, 
> and there is no data in dvr ringbuffer. then user code will still go on polling the dvr device and never stop.
> if POLLERR is returned, then user will perform read dvr, and then -ENODEV can be got, and 
> user will stop polling dvr device.

Your app should be also be handling poll() errors, as there are already
other errors that poll() can return.

> the first patch is enough to fix the deadlock issue.
> the second patch is used to correct the wrong type of returned value.
> the third patch is used to provide user a better controlling logic.

I'll take a deeper look and do some tests on your patches likely
tomorrow. 

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-02  7:15 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-21  2:05 [PATCH 3/3] media: check status of dmxdev->exit in poll functions of demux&dvr Changbing Xiong
2014-09-01 23:58 ` Antti Palosaari
2014-09-02  1:42   ` Mauro Carvalho Chehab
  -- strict thread matches above, loose matches on Subject: below --
2014-09-02  3:16 Changbing Xiong
2014-09-02  6:00 ` Mauro Carvalho Chehab
2014-09-02  6:42 Changbing Xiong
2014-09-02  7:15 ` Mauro Carvalho Chehab

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.