Re: DVBv5 test report

2012-01-23 Thread Antti Palosaari 

On 01/19/2012 03:31 PM, Mauro Carvalho Chehab wrote:

[PATCH] dvb-usb: Don't abort stop on -EAGAIN/-EINTR

Note: this patch is not complete. if the DVB demux device is opened on
block mode, it should instead be returning -EAGAIN.

Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index ddf282f..215ce75 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -30,7 +30,9 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
*dvbdmxfeed, int onoff)
usb_urb_kill(adap-fe_adap[adap-active_fe].stream);

if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
-   ret = 
adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
+   do {
+   ret = 
adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
+   } while ((ret == -EAGAIN) || (ret == -EINTR));
if (ret  0) {
err(error while stopping stream.);
return ret;



That fixes it. But it loops do {...} while around 100 times every I stop 
zap. Over 100 times is rather much...


And I think -EINTR is the only code to look, -EAGAIN is maybe for I2C 
and can be switched to native -EINTR also.


regards
Antti
--
http://palosaari.fi/
--
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: DVBv5 test report

2012-01-23 Thread Mauro Carvalho Chehab 
Em 23-01-2012 14:23, Antti Palosaari escreveu:
 On 01/19/2012 03:31 PM, Mauro Carvalho Chehab wrote:
 [PATCH] dvb-usb: Don't abort stop on -EAGAIN/-EINTR

 Note: this patch is not complete. if the DVB demux device is opened on
 block mode, it should instead be returning -EAGAIN.

 Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com

 diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
 b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 index ddf282f..215ce75 100644
 --- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 +++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 @@ -30,7 +30,9 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
 *dvbdmxfeed, int onoff)
   usb_urb_kill(adap-fe_adap[adap-active_fe].stream);

   if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
 -ret = adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
 +do {
 +ret = adap-props.fe[adap-active_fe].streaming_ctrl(adap, 
 0);
 +} while ((ret == -EAGAIN) || (ret == -EINTR));
   if (ret  0) {
   err(error while stopping stream.);
   return ret;

 
 That fixes it. But it loops do {...} while around 100 times every I stop zap. 
 Over 100 times is rather much...

Yes, this sounds too much. 

The issue here is caused by the usage of mutex_lock_interruptible() inside the
streaming_ctrl() callbacks, when the stream should stop.

The new wait_queue wakeup inside the code made the issue more visible, but it
could still happen without it, as a break could be hit during stream_ctl()
stop call anyway.

There are two possible fixes for it:

1) The above solution.

Eventually, a schedule() could be added there:
do {
ret = adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
if (ret == -EINTR)
shedule();
} while (ret == -EINTR);

2)

Don't use mutex_lock_interruptible inside the driver's streaming_ctrl, 
if the second parameter is 0 (stop).


IMHO, (1) is cleaner, due to a few reasons:

- inside the drivers, the code will be symmetrical: it will call the 
same function for
both onoff = 1 and onoff = 0.

- The patch is on just one place;

- with (2), extra care is needed when merging patches, as regressions 
and
broken drivers could pass unnoticed.

 
 And I think -EINTR is the only code to look, -EAGAIN is maybe for I2C and can 
 be switched to native -EINTR also.

Drivers need to be checked, if only -EINTR is added there, as drivers may
be doing things like:

if (mutex_lock_interruptible(...))
return -EAGAIN;

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: DVBv5 test report

2012-01-19 Thread Mauro Carvalho Chehab 
Em 18-01-2012 20:05, Antti Palosaari escreveu:
 I tested almost all DVB-T/T2/C devices I have and all seems to be working, 
 excluding Anysee models when using legacy zap.
 
 Anysee  anysee_streaming_ctrl() will fail because mutex_lock_interruptible() 
 returns -EINTR in anysee_ctrl_msg() function when zap is killed using ctrl+c. 
 This will led error returned to DVB-USB-core and log writing dvb-usb: error 
 while stopping stream.
 
 http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/anysee.c
 
 http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/dvb-usb-urb.c
 
 If I change mutex_lock_interruptible() = mutex_lock() it will work. I think 
 it gets SIGINT (ctrl+c) from userland, but how this haven't been issue 
 earlier?
 
 Anyone have idea what's wrong/reason here?

No idea. That part of the code wasn't changed recently, AFAIK, and
for sure it weren't affected by the frontend changes.

I suspect that the bug was already there, but it weren't noticed
before.

The fix seems to be as simple as:

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index ddf282f..6e707b5 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -32,7 +32,8 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
*dvbdmxfeed, int onoff)
if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
ret = 
adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
if (ret  0) {
-   err(error while stopping stream.);
+   if (ret != -EAGAIN)
+   err(error while stopping stream.);
return ret;
}
}

And make sure to remap -EINTR as -EAGAIN, leaving to the
userspace to retry it. Alternatively, the dvb frontend core 
or the anysee could retry it after a while for streaming
stop.

Another alternative that would likely work better would
be to just use mutex_lock() for streaming stop, but this
would require the review of all implementations for
streaming_ctrl

 
 
 here are tested drivers, working fine:
 dvb_usb_ec168,ec100,mxl5005s
 dvb_usb_au6610,zl10353,qt101
 dvb_usb_af9015,af9013,tda18218
 dvb_usb_af9015,af9013,tda18218
 dvb_usb_af9015,af9013,qt1010
 dvb_usb_af9015,af9013,mxl5005s
 dvb_usb_af9015,af9013,mxl5007t
 dvb_usb_gl861,zl10353,qt1010
 dvb_usb_ce6230,zl10353,mxl5005s
 em28xx_dvb,tda10023,tuner_simple
 dvb_ttusb_budget,stv0297
 dvb_usb_mxl111sf
 em28xx_dvb,cxd2820r,tda18271

Thanks for testing it!

Regards,
Mauro.
 
 
 Antti

--
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: DVBv5 test report

2012-01-19 Thread Antti Palosaari 

On 01/19/2012 01:33 PM, Mauro Carvalho Chehab wrote:

Em 18-01-2012 20:05, Antti Palosaari escreveu:

I tested almost all DVB-T/T2/C devices I have and all seems to be working, 
excluding Anysee models when using legacy zap.

Anysee  anysee_streaming_ctrl() will fail because mutex_lock_interruptible() returns 
-EINTR in anysee_ctrl_msg() function when zap is killed using ctrl+c. This will led error 
returned to DVB-USB-core and log writing dvb-usb: error while stopping stream.

http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/anysee.c

http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/dvb-usb-urb.c

If I change mutex_lock_interruptible() =  mutex_lock() it will work. I think 
it gets SIGINT (ctrl+c) from userland, but how this haven't been issue earlier?

Anyone have idea what's wrong/reason here?


No idea. That part of the code wasn't changed recently, AFAIK, and
for sure it weren't affected by the frontend changes.

I suspect that the bug was already there, but it weren't noticed
before.


Yeah, that's what I suspect too. But it still looks weird since DVB USB 
generic dvb_usb_generic_rw() function uses same mutex logic and it is 
very widely used about all DVB USB drivers. The reason Anysee driver 
have own mutex is weird USB message sequence that is 1xSEND 2xRECEIVE, 
instead normal 1xSEND 1xRECEIVE.


I did skeleton code below clear the issue.

dvb_usb_generic_rw() {
   if ((ret = mutex_lock_interruptible(d-usb_mutex)))
  return ret;

   usb_bulk_msg(SEND BULK USB MESSAGE);
   usb_bulk_msg(RECEIVE BULK USB MESSAGE);

   mutex_unlock(d-usb_mutex);
}

anysee_ctrl_msg() {
   if (mutex_lock_interruptible(anysee_usb_mutex)  0)
  return -EAGAIN;

   dvb_usb_generic_rw();
   usb_bulk_msg(RECEIVE BULK USB MESSAGE); // really!

   mutex_unlock(anysee_usb_mutex);
}




The fix seems to be as simple as:

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index ddf282f..6e707b5 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -32,7 +32,8 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
*dvbdmxfeed, int onoff)
if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
ret = 
adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
if (ret  0) {
-   err(error while stopping stream.);
+   if (ret != -EAGAIN)
+   err(error while stopping stream.);
return ret;
}
}

And make sure to remap -EINTR as -EAGAIN, leaving to the
userspace to retry it. Alternatively, the dvb frontend core
or the anysee could retry it after a while for streaming
stop.

Another alternative that would likely work better would
be to just use mutex_lock() for streaming stop, but this
would require the review of all implementations for
streaming_ctrl


I think some changes for DVB USB are needed because after 
.streaming_ctrl() fail it will not stream anything later attempts until 
device is re-plugged. Having this kind of effect in case of single 
driver callback failure is not acceptable.


regards
Antti
--
http://palosaari.fi/
--
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: DVBv5 test report

2012-01-19 Thread Mauro Carvalho Chehab 
Em 19-01-2012 09:57, Antti Palosaari escreveu:
 On 01/19/2012 01:33 PM, Mauro Carvalho Chehab wrote:
 Em 18-01-2012 20:05, Antti Palosaari escreveu:
 I tested almost all DVB-T/T2/C devices I have and all seems to be working, 
 excluding Anysee models when using legacy zap.

 Anysee  anysee_streaming_ctrl() will fail because 
 mutex_lock_interruptible() returns -EINTR in anysee_ctrl_msg() function 
 when zap is killed using ctrl+c. This will led error returned to 
 DVB-USB-core and log writing dvb-usb: error while stopping stream.

 http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/anysee.c

 http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/dvb-usb-urb.c

 If I change mutex_lock_interruptible() =  mutex_lock() it will work. I 
 think it gets SIGINT (ctrl+c) from userland, but how this haven't been 
 issue earlier?

 Anyone have idea what's wrong/reason here?

 No idea. That part of the code wasn't changed recently, AFAIK, and
 for sure it weren't affected by the frontend changes.

 I suspect that the bug was already there, but it weren't noticed
 before.
 
 Yeah, that's what I suspect too. But it still looks weird since DVB USB 
 generic 
 dvb_usb_generic_rw() function uses same mutex logic and it is very widely 
 used 
 about all DVB USB drivers. The reason Anysee driver have own mutex is weird 
 USB
 message sequence that is 1xSEND 2xRECEIVE, instead normal 1xSEND 1xRECEIVE.

I think that this is a sort of race issue: as you're taking more time at anysee,
it is more likely to receive the break while stopping the stream.

 I did skeleton code below clear the issue.
 
 dvb_usb_generic_rw() {
if ((ret = mutex_lock_interruptible(d-usb_mutex)))
   return ret;
 
usb_bulk_msg(SEND BULK USB MESSAGE);
usb_bulk_msg(RECEIVE BULK USB MESSAGE);
 
mutex_unlock(d-usb_mutex);
 }
 
 anysee_ctrl_msg() {
if (mutex_lock_interruptible(anysee_usb_mutex)  0)
   return -EAGAIN;
 
dvb_usb_generic_rw();
usb_bulk_msg(RECEIVE BULK USB MESSAGE); // really!
 
mutex_unlock(anysee_usb_mutex);
 }
 
 

 The fix seems to be as simple as:

 diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
 b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 index ddf282f..6e707b5 100644
 --- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 +++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 @@ -32,7 +32,8 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
 *dvbdmxfeed, int onoff)
   if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
   ret = adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
   if (ret  0) {
 -err(error while stopping stream.);
 +if (ret != -EAGAIN)
 +err(error while stopping stream.);
   return ret;
   }
   }

 And make sure to remap -EINTR as -EAGAIN, leaving to the
 userspace to retry it. Alternatively, the dvb frontend core
 or the anysee could retry it after a while for streaming
 stop.

 Another alternative that would likely work better would
 be to just use mutex_lock() for streaming stop, but this
 would require the review of all implementations for
 streaming_ctrl
 
 I think some changes for DVB USB are needed because after .streaming_ctrl() 
 fail it will not stream anything later attempts until device is re-plugged.
 Having this kind of effect in case of single driver callback failure is not 
 acceptable.

After thinking a little about it, I think that the best thing to do here is to 
retry automatically, like the enclosed patch.

The patch doesn't take into account the device mode. If it were opened
in non-block mode, the right behaviour would likely to return -EAGAIN,
and let userspace to retry it.

 regards
 Antti

[PATCH] dvb-usb: Don't abort stop on -EAGAIN/-EINTR

Note: this patch is not complete. if the DVB demux device is opened on
block mode, it should instead be returning -EAGAIN.

Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index ddf282f..215ce75 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -30,7 +30,9 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
*dvbdmxfeed, int onoff)
usb_urb_kill(adap-fe_adap[adap-active_fe].stream);
 
if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
-   ret = 
adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
+   do {
+   ret = 
adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
+   } while ((ret == -EAGAIN) || (ret == -EINTR));
if (ret  0) {
err(error while stopping stream.);
return ret;

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the

Re: DVBv5 test report

2012-01-19 Thread Antti Palosaari 

On 01/19/2012 03:31 PM, Mauro Carvalho Chehab wrote:

Em 19-01-2012 09:57, Antti Palosaari escreveu:

On 01/19/2012 01:33 PM, Mauro Carvalho Chehab wrote:

Em 18-01-2012 20:05, Antti Palosaari escreveu:

I tested almost all DVB-T/T2/C devices I have and all seems to be working, 
excluding Anysee models when using legacy zap.

Anysee  anysee_streaming_ctrl() will fail because mutex_lock_interruptible() returns 
-EINTR in anysee_ctrl_msg() function when zap is killed using ctrl+c. This will led error 
returned to DVB-USB-core and log writing dvb-usb: error while stopping stream.

http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/anysee.c

http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/dvb-usb-urb.c

If I change mutex_lock_interruptible() =   mutex_lock() it will work. I think 
it gets SIGINT (ctrl+c) from userland, but how this haven't been issue earlier?

Anyone have idea what's wrong/reason here?


No idea. That part of the code wasn't changed recently, AFAIK, and
for sure it weren't affected by the frontend changes.

I suspect that the bug was already there, but it weren't noticed
before.


Yeah, that's what I suspect too. But it still looks weird since DVB USB generic
dvb_usb_generic_rw() function uses same mutex logic and it is very widely used
about all DVB USB drivers. The reason Anysee driver have own mutex is weird USB
message sequence that is 1xSEND 2xRECEIVE, instead normal 1xSEND 1xRECEIVE.


I think that this is a sort of race issue: as you're taking more time at anysee,
it is more likely to receive the break while stopping the stream.


I installed latest 3.2.1 Kernel and no that error seen so it is regression.
Linux localhost.localdomain 3.2.1 #1 SMP Thu Jan 19 16:07:04 EET 2012 
x86_64 x86_64 x86_64 GNU/Linux


Hardware does not stop streaming since streaming command is newer send - 
it is mutex protecting control messages that returns EINTR and command 
was terminated before it happens.




I did skeleton code below clear the issue.

dvb_usb_generic_rw() {
if ((ret = mutex_lock_interruptible(d-usb_mutex)))
   return ret;

usb_bulk_msg(SEND BULK USB MESSAGE);
usb_bulk_msg(RECEIVE BULK USB MESSAGE);

mutex_unlock(d-usb_mutex);
}

anysee_ctrl_msg() {
if (mutex_lock_interruptible(anysee_usb_mutex)  0)
   return -EAGAIN;

dvb_usb_generic_rw();
usb_bulk_msg(RECEIVE BULK USB MESSAGE); // really!

mutex_unlock(anysee_usb_mutex);
}




The fix seems to be as simple as:

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index ddf282f..6e707b5 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -32,7 +32,8 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
*dvbdmxfeed, int onoff)
   if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
   ret = adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
   if (ret   0) {
-err(error while stopping stream.);
+if (ret != -EAGAIN)
+err(error while stopping stream.);
   return ret;
   }
   }

And make sure to remap -EINTR as -EAGAIN, leaving to the
userspace to retry it. Alternatively, the dvb frontend core
or the anysee could retry it after a while for streaming
stop.

Another alternative that would likely work better would
be to just use mutex_lock() for streaming stop, but this
would require the review of all implementations for
streaming_ctrl


I think some changes for DVB USB are needed because after .streaming_ctrl()
fail it will not stream anything later attempts until device is re-plugged.
Having this kind of effect in case of single driver callback failure is not 
acceptable.


After thinking a little about it, I think that the best thing to do here is to
retry automatically, like the enclosed patch.

The patch doesn't take into account the device mode. If it were opened
in non-block mode, the right behaviour would likely to return -EAGAIN,
and let userspace to retry it.


regards
Antti


[PATCH] dvb-usb: Don't abort stop on -EAGAIN/-EINTR

Note: this patch is not complete. if the DVB demux device is opened on
block mode, it should instead be returning -EAGAIN.

Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index ddf282f..215ce75 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -30,7 +30,9 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
*dvbdmxfeed, int onoff)
usb_urb_kill(adap-fe_adap[adap-active_fe].stream);

if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
-   ret = 
adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
+   do {
+   ret

Re: DVBv5 test report

2012-01-19 Thread Mauro Carvalho Chehab 
Em 19-01-2012 13:53, Antti Palosaari escreveu:
 On 01/19/2012 03:31 PM, Mauro Carvalho Chehab wrote:
 Em 19-01-2012 09:57, Antti Palosaari escreveu:
 On 01/19/2012 01:33 PM, Mauro Carvalho Chehab wrote:
 Em 18-01-2012 20:05, Antti Palosaari escreveu:
 I tested almost all DVB-T/T2/C devices I have and all seems to be 
 working, excluding Anysee models when using legacy zap.

 Anysee  anysee_streaming_ctrl() will fail because 
 mutex_lock_interruptible() returns -EINTR in anysee_ctrl_msg() function 
 when zap is killed using ctrl+c. This will led error returned to 
 DVB-USB-core and log writing dvb-usb: error while stopping stream.

 http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/anysee.c

 http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/dvb-usb-urb.c

 If I change mutex_lock_interruptible() =   mutex_lock() it will work. I 
 think it gets SIGINT (ctrl+c) from userland, but how this haven't been 
 issue earlier?

 Anyone have idea what's wrong/reason here?

 No idea. That part of the code wasn't changed recently, AFAIK, and
 for sure it weren't affected by the frontend changes.

 I suspect that the bug was already there, but it weren't noticed
 before.

 Yeah, that's what I suspect too. But it still looks weird since DVB USB 
 generic
 dvb_usb_generic_rw() function uses same mutex logic and it is very widely 
 used
 about all DVB USB drivers. The reason Anysee driver have own mutex is weird 
 USB
 message sequence that is 1xSEND 2xRECEIVE, instead normal 1xSEND 1xRECEIVE.

 I think that this is a sort of race issue: as you're taking more time at 
 anysee,
 it is more likely to receive the break while stopping the stream.
 
 I installed latest 3.2.1 Kernel and no that error seen so it is regression.
 Linux localhost.localdomain 3.2.1 #1 SMP Thu Jan 19 16:07:04 EET 2012 x86_64 
 x86_64 x86_64 GNU/Linux

Maybe, but it can still be some race condition.

The error is at the dmx part, so it is very unlikely that the frontend changes
would cause any harm.

 Hardware does not stop streaming since streaming command is newer send - 
 it is mutex protecting control messages that returns EINTR and command was 
 terminated before it happens.
 
 
 I did skeleton code below clear the issue.

 dvb_usb_generic_rw() {q
 if ((ret = mutex_lock_interruptible(d-usb_mutex)))
return ret;

 usb_bulk_msg(SEND BULK USB MESSAGE);
 usb_bulk_msg(RECEIVE BULK USB MESSAGE);

 mutex_unlock(d-usb_mutex);
 }

 anysee_ctrl_msg() {
 if (mutex_lock_interruptible(anysee_usb_mutex)  0)
return -EAGAIN;

 dvb_usb_generic_rw();
 usb_bulk_msg(RECEIVE BULK USB MESSAGE); // really!

 mutex_unlock(anysee_usb_mutex);
 }



 The fix seems to be as simple as:

 diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
 b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 index ddf282f..6e707b5 100644
 --- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 +++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 @@ -32,7 +32,8 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
 *dvbdmxfeed, int onoff)
if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
ret = adap-props.fe[adap-active_fe].streaming_ctrl(adap, 
 0);
if (ret   0) {
 -err(error while stopping stream.);
 +if (ret != -EAGAIN)
 +err(error while stopping stream.);
return ret;
}
}

 And make sure to remap -EINTR as -EAGAIN, leaving to the
 userspace to retry it. Alternatively, the dvb frontend core
 or the anysee could retry it after a while for streaming
 stop.

 Another alternative that would likely work better would
 be to just use mutex_lock() for streaming stop, but this
 would require the review of all implementations for
 streaming_ctrl

 I think some changes for DVB USB are needed because after .streaming_ctrl()
 fail it will not stream anything later attempts until device is re-plugged.
 Having this kind of effect in case of single driver callback failure is not 
 acceptable.

 After thinking a little about it, I think that the best thing to do here is 
 to
 retry automatically, like the enclosed patch.

 The patch doesn't take into account the device mode. If it were opened
 in non-block mode, the right behaviour would likely to return -EAGAIN,
 and let userspace to retry it.

 regards
 Antti

 [PATCH] dvb-usb: Don't abort stop on -EAGAIN/-EINTR

 Note: this patch is not complete. if the DVB demux device is opened on
 block mode, it should instead be returning -EAGAIN.

 Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com

 diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
 b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 index ddf282f..215ce75 100644
 --- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 +++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
 @@ -30,7 +30,9 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
 *dvbdmxfeed, int onoff)

Re: DVBv5 test report

2012-01-19 Thread Antti Palosaari 

On 01/19/2012 06:08 PM, Mauro Carvalho Chehab wrote:

Em 19-01-2012 13:53, Antti Palosaari escreveu:

On 01/19/2012 03:31 PM, Mauro Carvalho Chehab wrote:

Em 19-01-2012 09:57, Antti Palosaari escreveu:

On 01/19/2012 01:33 PM, Mauro Carvalho Chehab wrote:

Em 18-01-2012 20:05, Antti Palosaari escreveu:

I tested almost all DVB-T/T2/C devices I have and all seems to be working, 
excluding Anysee models when using legacy zap.

Anysee  anysee_streaming_ctrl() will fail because mutex_lock_interruptible() returns 
-EINTR in anysee_ctrl_msg() function when zap is killed using ctrl+c. This will led error 
returned to DVB-USB-core and log writing dvb-usb: error while stopping stream.

http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/anysee.c

http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/dvb-usb-urb.c

If I change mutex_lock_interruptible() =mutex_lock() it will work. I think 
it gets SIGINT (ctrl+c) from userland, but how this haven't been issue earlier?

Anyone have idea what's wrong/reason here?


No idea. That part of the code wasn't changed recently, AFAIK, and
for sure it weren't affected by the frontend changes.

I suspect that the bug was already there, but it weren't noticed
before.


Yeah, that's what I suspect too. But it still looks weird since DVB USB generic
dvb_usb_generic_rw() function uses same mutex logic and it is very widely used
about all DVB USB drivers. The reason Anysee driver have own mutex is weird USB
message sequence that is 1xSEND 2xRECEIVE, instead normal 1xSEND 1xRECEIVE.


I think that this is a sort of race issue: as you're taking more time at anysee,
it is more likely to receive the break while stopping the stream.


I installed latest 3.2.1 Kernel and no that error seen so it is regression.
Linux localhost.localdomain 3.2.1 #1 SMP Thu Jan 19 16:07:04 EET 2012 x86_64 
x86_64 x86_64 GNU/Linux


Maybe, but it can still be some race condition.

The error is at the dmx part, so it is very unlikely that the frontend changes
would cause any harm.


Hardware does not stop streaming since streaming command is newer send -
it is mutex protecting control messages that returns EINTR and command was 
terminated before it happens.



I did skeleton code below clear the issue.

dvb_usb_generic_rw() {q
 if ((ret = mutex_lock_interruptible(d-usb_mutex)))
return ret;

 usb_bulk_msg(SEND BULK USB MESSAGE);
 usb_bulk_msg(RECEIVE BULK USB MESSAGE);

 mutex_unlock(d-usb_mutex);
}

anysee_ctrl_msg() {
 if (mutex_lock_interruptible(anysee_usb_mutex)   0)
return -EAGAIN;

 dvb_usb_generic_rw();
 usb_bulk_msg(RECEIVE BULK USB MESSAGE); // really!

 mutex_unlock(anysee_usb_mutex);
}




The fix seems to be as simple as:

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index ddf282f..6e707b5 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -32,7 +32,8 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
*dvbdmxfeed, int onoff)
if (adap-props.fe[adap-active_fe].streaming_ctrl != NULL) {
ret = adap-props.fe[adap-active_fe].streaming_ctrl(adap, 0);
if (ret0) {
-err(error while stopping stream.);
+if (ret != -EAGAIN)
+err(error while stopping stream.);
return ret;
}
}

And make sure to remap -EINTR as -EAGAIN, leaving to the
userspace to retry it. Alternatively, the dvb frontend core
or the anysee could retry it after a while for streaming
stop.

Another alternative that would likely work better would
be to just use mutex_lock() for streaming stop, but this
would require the review of all implementations for
streaming_ctrl


I think some changes for DVB USB are needed because after .streaming_ctrl()
fail it will not stream anything later attempts until device is re-plugged.
Having this kind of effect in case of single driver callback failure is not 
acceptable.


After thinking a little about it, I think that the best thing to do here is to
retry automatically, like the enclosed patch.

The patch doesn't take into account the device mode. If it were opened
in non-block mode, the right behaviour would likely to return -EAGAIN,
and let userspace to retry it.


regards
Antti


[PATCH] dvb-usb: Don't abort stop on -EAGAIN/-EINTR

Note: this patch is not complete. if the DVB demux device is opened on
block mode, it should instead be returning -EAGAIN.

Signed-off-by: Mauro Carvalho Chehabmche...@redhat.com

diff --git a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c 
b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
index ddf282f..215ce75 100644
--- a/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/dvb/dvb-usb/dvb-usb-dvb.c
@@ -30,7 +30,9 @@ static int dvb_usb_ctrl_feed(struct dvb_demux_feed 
*dvbdmxfeed, int onoff)

DVBv5 test report

2012-01-18 Thread Antti Palosaari 
I tested almost all DVB-T/T2/C devices I have and all seems to be 
working, excluding Anysee models when using legacy zap.


Anysee  anysee_streaming_ctrl() will fail because 
mutex_lock_interruptible() returns -EINTR in anysee_ctrl_msg() function 
when zap is killed using ctrl+c. This will led error returned to 
DVB-USB-core and log writing dvb-usb: error while stopping stream.


http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/anysee.c

http://git.linuxtv.org/media_tree.git/blob/refs/heads/master:/drivers/media/dvb/dvb-usb/dvb-usb-urb.c

If I change mutex_lock_interruptible() = mutex_lock() it will work. I 
think it gets SIGINT (ctrl+c) from userland, but how this haven't been 
issue earlier?


Anyone have idea what's wrong/reason here?


here are tested drivers, working fine:
dvb_usb_ec168,ec100,mxl5005s
dvb_usb_au6610,zl10353,qt101
dvb_usb_af9015,af9013,tda18218
dvb_usb_af9015,af9013,tda18218
dvb_usb_af9015,af9013,qt1010
dvb_usb_af9015,af9013,mxl5005s
dvb_usb_af9015,af9013,mxl5007t
dvb_usb_gl861,zl10353,qt1010
dvb_usb_ce6230,zl10353,mxl5005s
em28xx_dvb,tda10023,tuner_simple
dvb_ttusb_budget,stv0297
dvb_usb_mxl111sf
em28xx_dvb,cxd2820r,tda18271


Antti
--
http://palosaari.fi/
--
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