Re: [PATCH 3/3] media: check status of dmxdev-exit in poll functions of demuxdvr

[Mauro Carvalho Chehab]

Em Tue, 02 Sep 2014 03:16:00 + (GMT)
Changbing Xiong cb.xi...@samsung.com 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH 3/3] media: check status of dmxdev-exit in poll functions of demuxdvr

[Changbing Xiong]

 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 Chehabm.che...@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 demuxdvr

Em Tue, 02 Sep 2014 03:16:00 + (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 majord...@vger.kernel.org
More majordomo info at  
http://vger.kernel.org/majordomo-info.htmlN�r��yb�X��ǧv�^�)޺{.n�+{���bj)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥

Re: [PATCH 3/3] media: check status of dmxdev-exit in poll functions of demuxdvr

[Mauro Carvalho Chehab]

Em Tue, 02 Sep 2014 06:42:42 + (GMT)
Changbing Xiong cb.xi...@samsung.com escreveu:

  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. 

Well, it would likely put ENODEV at errno (and not -ENODEV).

 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.

Yeah, I'll check it there. Poll have some different handling, so
it is possible that it only accepts a subset of error codes.

Btw, I'm not against returning POLLERR there. Just not sure at
this point what would be the best approach. Anyway, read and
all other syscalls should return -ENODEV.

 please also check that.

Sure I will.
 
 thanks!
 Xiong changbing
 
 --- Original Message ---
 Sender : Mauro Carvalho Chehabm.che...@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 demuxdvr
 
 Em Tue, 02 Sep 2014 03:16:00 + (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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this 

Re: [PATCH 3/3] media: check status of dmxdev-exit in poll functions of demuxdvr

[Antti Palosaari]

Moikka Changbing and thanks to working that.

I reviewed the first patch and tested all these patches. It does not 
deadlock USB device anymore because of patch #1 so it is improvement. 
However, what I expect that patch, it should force device unregister but 
when I use tzap and unplug running device, it does not stop tzap, but 
continues zapping until app is killed using ctrl-c.

I used same(?) WinTV Aero for my tests.
$ tzap -r YLE TV1 -a 0 -f 1 -c ~/.tzap/channels.conf_

Sep 02 02:50:38 localhost.localdomain kernel: usb 1-2: USB disconnect, 
device number 6
Sep 02 02:50:38 localhost.localdomain kernel: [57] dvb_usbv2_disconnect: 
usb 1-2: dvb_usbv2_disconnect: bInterfaceNumber=0
Sep 02 02:50:38 localhost.localdomain kernel: [57] dvb_usbv2_exit: usb 
1-2: dvb_usbv2_exit:
Sep 02 02:50:38 localhost.localdomain kernel: [57] 
dvb_usbv2_remote_exit: usb 1-2: dvb_usbv2_remote_exit:
Sep 02 02:50:38 localhost.localdomain kernel: [57] 
dvb_usbv2_adapter_exit: usb 1-2: dvb_usbv2_adapter_exit:
Sep 02 02:50:38 localhost.localdomain kernel: [57] 
dvb_usbv2_adapter_dvb_exit: usb 1-2: dvb_usbv2_adapter_dvb_exit: adap=0
Sep 02 02:50:38 localhost.localdomain kernel: [24239] 
dvb_usb_v2_generic_io: usb 1-2: dvb_usb_v2_generic_io:  aa 28
Sep 02 02:50:38 localhost.localdomain kernel: usb 1-2: dvb_usb_v2: 
usb_bulk_msg() failed=-19
Sep 02 02:50:38 localhost.localdomain kernel: 
mxl1x1sf_demod_get_rs_lock_status: error -19 on line 232
Sep 02 02:50:38 localhost.localdomain kernel: 
mxl111sf_demod_read_status: error -19 on line 452
Sep 02 02:50:39 localhost.localdomain kernel: [24238] 
dvb_usb_v2_generic_io: usb 1-2: dvb_usb_v2_generic_io:  aa 28
Sep 02 02:50:39 localhost.localdomain kernel: usb 1-2: dvb_usb_v2: 
usb_bulk_msg() failed=-19
Sep 02 02:50:39 localhost.localdomain kernel: 
mxl1x1sf_demod_get_rs_lock_status: error -19 on line 232
Sep 02 02:50:39 localhost.localdomain kernel: 
mxl111sf_demod_read_status: error -19 on line 452
Sep 02 02:50:39 localhost.localdomain kernel: [24238] 
dvb_usb_v2_generic_io: usb 1-2: dvb_usb_v2_generic_io:  aa 28

[.]
Sep 02 02:50:42 localhost.localdomain kernel: [24238] 
dvb_usb_v2_generic_io: usb 1-2: dvb_usb_v2_generic_io:  aa 2e
Sep 02 02:50:42 localhost.localdomain kernel: usb 1-2: dvb_usb_v2: 
usb_bulk_msg() failed=-19
Sep 02 02:50:42 localhost.localdomain kernel: 
mxl111sf_demod_read_ucblocks: error -19 on line 350
Sep 02 02:50:42 localhost.localdomain kernel: [24238] dvb_usb_stop_feed: 
usb 1-2: dvb_usb_stop_feed: adap=0 active_fe=1 feed_type=0 setting pid 
[no]: 0200 (0512) at index 0
Sep 02 02:50:42 localhost.localdomain kernel: [24239] dvb_usb_fe_sleep: 
usb 1-2: dvb_usb_fe_sleep: adap=0 fe=1
Sep 02 02:50:42 localhost.localdomain kernel: [24238] dvb_usb_stop_feed: 
usb 1-2: dvb_usb_stop_feed: adap=0 active_fe=1 feed_type=0 setting pid 
[no]: 028a (0650) at index 1
Sep 02 02:50:42 localhost.localdomain kernel: [24238] usb_urb_killv2: 
usb 1-2: usb_urb_killv2: kill urb=0
Sep 02 02:50:42 localhost.localdomain kernel: [24238] usb_urb_killv2: 
usb 1-2: usb_urb_killv2: kill urb=1
Sep 02 02:50:42 localhost.localdomain kernel: [24238] usb_urb_killv2: 
usb 1-2: usb_urb_killv2: kill urb=2
Sep 02 02:50:42 localhost.localdomain kernel: [24238] usb_urb_killv2: 
usb 1-2: usb_urb_killv2: kill urb=3
Sep 02 02:50:42 localhost.localdomain kernel: [24238] usb_urb_killv2: 
usb 1-2: usb_urb_killv2: kill urb=4
Sep 02 02:50:42 localhost.localdomain kernel: [24239] 
dvb_usbv2_device_power_ctrl: usb 1-2: dvb_usbv2_device_power_ctrl: power=0
Sep 02 02:50:42 localhost.localdomain kernel: [24239] dvb_usb_fe_sleep: 
usb 1-2: dvb_usb_fe_sleep: ret=0
Sep 02 02:50:42 localhost.localdomain kernel: [57] 
dvb_usbv2_adapter_stream_exit: usb 1-2: dvb_usbv2_adapter_stream_exit: 
adap=0
Sep 02 02:50:42 localhost.localdomain kernel: [57] usb_urb_free_urbs: 
usb 1-2: usb_urb_free_urbs: free urb=4
Sep 02 02:50:42 localhost.localdomain kernel: [57] usb_urb_free_urbs: 
usb 1-2: usb_urb_free_urbs: free urb=3
Sep 02 02:50:42 localhost.localdomain kernel: [57] usb_urb_free_urbs: 
usb 1-2: usb_urb_free_urbs: free urb=2
Sep 02 02:50:42 localhost.localdomain kernel: [57] usb_urb_free_urbs: 
usb 1-2: usb_urb_free_urbs: free urb=1
Sep 02 02:50:42 localhost.localdomain kernel: [57] usb_urb_free_urbs: 
usb 1-2: usb_urb_free_urbs: free urb=0
Sep 02 02:50:42 localhost.localdomain kernel: [57] 
usb_free_stream_buffers: usb 1-2: usb_free_stream_buffers: free buf=4
Sep 02 02:50:42 localhost.localdomain kernel: [57] 
usb_free_stream_buffers: usb 1-2: usb_free_stream_buffers: free buf=3
Sep 02 02:50:42 localhost.localdomain kernel: [57] 
usb_free_stream_buffers: usb 1-2: usb_free_stream_buffers: free buf=2
Sep 02 02:50:42 localhost.localdomain kernel: [57] 
usb_free_stream_buffers: usb 1-2: usb_free_stream_buffers: free buf=1
Sep 02 02:50:42 localhost.localdomain kernel: [57] 
usb_free_stream_buffers: usb 1-2: usb_free_stream_buffers: free buf=0
Sep 02 02:50:42 localhost.localdomain kernel: [57] 
dvb_usbv2_adapter_frontend_exit: usb 

Re: [PATCH 3/3] media: check status of dmxdev-exit in poll functions of demuxdvr

[Mauro Carvalho Chehab]

Em Tue, 02 Sep 2014 02:58:50 +0300
Antti Palosaari cr...@iki.fi escreveu:

 Moikka Changbing and thanks to working that.
 
 I reviewed the first patch and tested all these patches. It does not 
 deadlock USB device anymore because of patch #1 so it is improvement. 
 However, what I expect that patch, it should force device unregister but 
 when I use tzap and unplug running device, it does not stop tzap, but 
 continues zapping until app is killed using ctrl-c.
 I used same(?) WinTV Aero for my tests.

...

 Is there any change to close all those /dev file handles when device 
 disappears?

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.

Regards,
Mauro

 
 regards
 Antti
 
 
 On 08/21/2014 05:05 AM, Changbing Xiong wrote:
  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.xi...@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
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH 3/3] media: check status of dmxdev-exit in poll functions of demuxdvr

[Changbing Xiong]

 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.

[PATCH 3/3] media: check status of dmxdev-exit in poll functions of demuxdvr

[Changbing Xiong]

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.xi...@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

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