All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
@ 2012-07-31 22:22 Juergen Lock
  2012-08-12  2:15 ` Fwd: " Mauro Carvalho Chehab
  2012-09-26 20:11 ` [PATCH, RFC, updated] " Juergen Lock
  0 siblings, 2 replies; 9+ messages in thread
From: Juergen Lock @ 2012-07-31 22:22 UTC (permalink / raw)
  To: linux-media; +Cc: hselasky

That likely fxes this MythTV ticket:

	http://code.mythtv.org/trac/ticket/10830

(which btw affects all usb tuners I tested as well, pctv452e,
dib0700, af9015)  pctv452e is still possibly broken with MythTV
even after this fix; it does work with VDR here tho despite I2C
errors.

Reduced testcase:

	http://people.freebsd.org/~nox/tmp/ioctltst.c

Thanx to devinheitmueller and crope from #linuxtv for helping with
this fix! :)

Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>

--- a/drivers/media/dvb/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
@@ -604,6 +604,7 @@ static int dvb_frontend_thread(void *dat
 	enum dvbfe_algo algo;
 
 	bool re_tune = false;
+	bool semheld = false;
 
 	dprintk("%s\n", __func__);
 
@@ -627,6 +628,8 @@ restart:
 
 		if (kthread_should_stop() || dvb_frontend_is_exiting(fe)) {
 			/* got signal or quitting */
+			if (!down_interruptible (&fepriv->sem))
+				semheld = true;
 			fepriv->exit = DVB_FE_NORMAL_EXIT;
 			break;
 		}
@@ -742,6 +745,8 @@ restart:
 		fepriv->exit = DVB_FE_NO_EXIT;
 	mb();
 
+	if (semheld)
+		up(&fepriv->sem);
 	dvb_frontend_wakeup(fe);
 	return 0;
 }
@@ -1804,16 +1809,20 @@ static int dvb_frontend_ioctl(struct fil
 
 	dprintk("%s (%d)\n", __func__, _IOC_NR(cmd));
 
-	if (fepriv->exit != DVB_FE_NO_EXIT)
+	if (down_interruptible (&fepriv->sem))
+		return -ERESTARTSYS;
+
+	if (fepriv->exit != DVB_FE_NO_EXIT) {
+		up(&fepriv->sem);
 		return -ENODEV;
+	}
 
 	if ((file->f_flags & O_ACCMODE) == O_RDONLY &&
 	    (_IOC_DIR(cmd) != _IOC_READ || cmd == FE_GET_EVENT ||
-	     cmd == FE_DISEQC_RECV_SLAVE_REPLY))
+	     cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
+		up(&fepriv->sem);
 		return -EPERM;
-
-	if (down_interruptible (&fepriv->sem))
-		return -ERESTARTSYS;
+	}
 
 	if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY))
 		err = dvb_frontend_ioctl_properties(file, cmd, parg);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Fwd: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
  2012-07-31 22:22 [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast Juergen Lock
@ 2012-08-12  2:15 ` Mauro Carvalho Chehab
  2012-08-12  3:06   ` Devin Heitmueller
  2012-09-01 15:51   ` Fwd: " poma
  2012-09-26 20:11 ` [PATCH, RFC, updated] " Juergen Lock
  1 sibling, 2 replies; 9+ messages in thread
From: Mauro Carvalho Chehab @ 2012-08-12  2:15 UTC (permalink / raw)
  To: Devin Heitmueller, Antti Palosaari
  Cc: Linux Media Mailing List, Juergen Lock, hselasky

Devin/Antti,

As Juergen mentioned your help on this patch, do you mind helping reviewing
and testing it?

Touching on those semaphores can be very tricky, as anything wrong may cause
a driver hangup. So, it is great to have more pair of eyes looking on it.

While I didn't test the code (too busy trying to clean up my long queue -
currently with still 200+ patches left), I did a careful review at the
semaphore code there, and it seems this approach will work.

At least, the first hunk looks perfect for me. The second hunk seems
a little more worrying, as the dvb core might be waiting forever for
a lock on a device that was already removed.

In order to test it in practice, I think we need to remove an USB device
by hand while tuning it, and see if the core will not lock the device 
forever.

What do you think?
Mauro

-------- Mensagem original --------
Assunto: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
Data: Wed, 1 Aug 2012 00:22:16 +0200
De: Juergen Lock <nox@jelal.kn-bremen.de>
Para: linux-media@vger.kernel.org
CC: hselasky@c2i.net

That likely fxes this MythTV ticket:

	http://code.mythtv.org/trac/ticket/10830

(which btw affects all usb tuners I tested as well, pctv452e,
dib0700, af9015)  pctv452e is still possibly broken with MythTV
even after this fix; it does work with VDR here tho despite I2C
errors.

Reduced testcase:

	http://people.freebsd.org/~nox/tmp/ioctltst.c

Thanx to devinheitmueller and crope from #linuxtv for helping with
this fix! :)

Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>

--- a/drivers/media/dvb/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
@@ -604,6 +604,7 @@ static int dvb_frontend_thread(void *dat
 	enum dvbfe_algo algo;
 
 	bool re_tune = false;
+	bool semheld = false;
 
 	dprintk("%s\n", __func__);
 
@@ -627,6 +628,8 @@ restart:
 
 		if (kthread_should_stop() || dvb_frontend_is_exiting(fe)) {
 			/* got signal or quitting */
+			if (!down_interruptible (&fepriv->sem))
+				semheld = true;
 			fepriv->exit = DVB_FE_NORMAL_EXIT;
 			break;
 		}
@@ -742,6 +745,8 @@ restart:
 		fepriv->exit = DVB_FE_NO_EXIT;
 	mb();
 
+	if (semheld)
+		up(&fepriv->sem);
 	dvb_frontend_wakeup(fe);
 	return 0;
 }
@@ -1804,16 +1809,20 @@ static int dvb_frontend_ioctl(struct fil
 
 	dprintk("%s (%d)\n", __func__, _IOC_NR(cmd));
 
-	if (fepriv->exit != DVB_FE_NO_EXIT)
+	if (down_interruptible (&fepriv->sem))
+		return -ERESTARTSYS;
+
+	if (fepriv->exit != DVB_FE_NO_EXIT) {
+		up(&fepriv->sem);
 		return -ENODEV;
+	}
 
 	if ((file->f_flags & O_ACCMODE) == O_RDONLY &&
 	    (_IOC_DIR(cmd) != _IOC_READ || cmd == FE_GET_EVENT ||
-	     cmd == FE_DISEQC_RECV_SLAVE_REPLY))
+	     cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
+		up(&fepriv->sem);
 		return -EPERM;
-
-	if (down_interruptible (&fepriv->sem))
-		return -ERESTARTSYS;
+	}
 
 	if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY))
 		err = dvb_frontend_ioctl_properties(file, cmd, parg);
--
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] 9+ messages in thread

* Re: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
  2012-08-12  2:15 ` Fwd: " Mauro Carvalho Chehab
@ 2012-08-12  3:06   ` Devin Heitmueller
  2012-08-28 20:56     ` Hans Petter Selasky
  2012-09-01 15:51   ` Fwd: " poma
  1 sibling, 1 reply; 9+ messages in thread
From: Devin Heitmueller @ 2012-08-12  3:06 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Antti Palosaari, Linux Media Mailing List, Juergen Lock, hselasky

On Sat, Aug 11, 2012 at 10:15 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Devin/Antti,
>
> As Juergen mentioned your help on this patch, do you mind helping reviewing
> and testing it?

I guided Juergen through the creation of the patch via #linuxtv a
couple of weeks ago.  While I'm generally confident that it should
work (and it does address his basic issue), I hadn't had the time to
stare at the code long enough to see what other side effects it might
produce.

I'm tied up in other projects right now and am not confident I will
have cycles to look at this closer.  Antti, if you want to give it
some cycles, this would be a good fix to get upstream (and you've
already been looking at dvb_frontend.c for quite a while, so I believe
you would be able to spot a problem if one exists).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
  2012-08-12  3:06   ` Devin Heitmueller
@ 2012-08-28 20:56     ` Hans Petter Selasky
  0 siblings, 0 replies; 9+ messages in thread
From: Hans Petter Selasky @ 2012-08-28 20:56 UTC (permalink / raw)
  To: Devin Heitmueller
  Cc: Mauro Carvalho Chehab, Antti Palosaari, Linux Media Mailing List,
	Juergen Lock

On Sunday 12 August 2012 05:06:49 Devin Heitmueller wrote:
> On Sat, Aug 11, 2012 at 10:15 PM, Mauro Carvalho Chehab
> 
> <mchehab@redhat.com> wrote:
> > Devin/Antti,
> > 
> > As Juergen mentioned your help on this patch, do you mind helping
> > reviewing and testing it?
> 
> I guided Juergen through the creation of the patch via #linuxtv a
> couple of weeks ago.  While I'm generally confident that it should
> work (and it does address his basic issue), I hadn't had the time to
> stare at the code long enough to see what other side effects it might
> produce.
> 
> I'm tied up in other projects right now and am not confident I will
> have cycles to look at this closer.  Antti, if you want to give it
> some cycles, this would be a good fix to get upstream (and you've
> already been looking at dvb_frontend.c for quite a while, so I believe
> you would be able to spot a problem if one exists).
> 
> Devin

Hi,

What is the status of this PATCH? Submitted or under testing ??

--HPS

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fwd: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
  2012-08-12  2:15 ` Fwd: " Mauro Carvalho Chehab
  2012-08-12  3:06   ` Devin Heitmueller
@ 2012-09-01 15:51   ` poma
  2012-09-01 16:13     ` Antti Palosaari
  1 sibling, 1 reply; 9+ messages in thread
From: poma @ 2012-09-01 15:51 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Devin Heitmueller, Antti Palosaari, Linux Media Mailing List,
	Juergen Lock, hselasky

On 08/12/2012 04:15 AM, Mauro Carvalho Chehab wrote:
> Devin/Antti,
> 
> As Juergen mentioned your help on this patch, do you mind helping reviewing
> and testing it?
> 
> Touching on those semaphores can be very tricky, as anything wrong may cause
> a driver hangup. So, it is great to have more pair of eyes looking on it.
> 
> While I didn't test the code (too busy trying to clean up my long queue -
> currently with still 200+ patches left), I did a careful review at the
> semaphore code there, and it seems this approach will work.
> 
> At least, the first hunk looks perfect for me. The second hunk seems
> a little more worrying, as the dvb core might be waiting forever for
> a lock on a device that was already removed.
> 
> In order to test it in practice, I think we need to remove an USB device
> by hand while tuning it, and see if the core will not lock the device 
> forever.
> 
> What do you think?
> Mauro
> 
> -------- Mensagem original --------
> Assunto: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
> Data: Wed, 1 Aug 2012 00:22:16 +0200
> De: Juergen Lock <nox@jelal.kn-bremen.de>
> Para: linux-media@vger.kernel.org
> CC: hselasky@c2i.net
> 
> That likely fxes this MythTV ticket:
> 
> 	http://code.mythtv.org/trac/ticket/10830
> 
> (which btw affects all usb tuners I tested as well, pctv452e,
> dib0700, af9015)  pctv452e is still possibly broken with MythTV
> even after this fix; it does work with VDR here tho despite I2C
> errors.
> 
> Reduced testcase:
> 
> 	http://people.freebsd.org/~nox/tmp/ioctltst.c
> 
> Thanx to devinheitmueller and crope from #linuxtv for helping with
> this fix! :)
> 
> Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
> 
> --- a/drivers/media/dvb/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
> @@ -604,6 +604,7 @@ static int dvb_frontend_thread(void *dat
>  	enum dvbfe_algo algo;
>  
>  	bool re_tune = false;
> +	bool semheld = false;
>  
>  	dprintk("%s\n", __func__);
>  
> @@ -627,6 +628,8 @@ restart:
>  
>  		if (kthread_should_stop() || dvb_frontend_is_exiting(fe)) {
>  			/* got signal or quitting */
> +			if (!down_interruptible (&fepriv->sem))
> +				semheld = true;
>  			fepriv->exit = DVB_FE_NORMAL_EXIT;
>  			break;
>  		}
> @@ -742,6 +745,8 @@ restart:
>  		fepriv->exit = DVB_FE_NO_EXIT;
>  	mb();
>  
> +	if (semheld)
> +		up(&fepriv->sem);
>  	dvb_frontend_wakeup(fe);
>  	return 0;
>  }
> @@ -1804,16 +1809,20 @@ static int dvb_frontend_ioctl(struct fil
>  
>  	dprintk("%s (%d)\n", __func__, _IOC_NR(cmd));
>  
> -	if (fepriv->exit != DVB_FE_NO_EXIT)
> +	if (down_interruptible (&fepriv->sem))
> +		return -ERESTARTSYS;
> +
> +	if (fepriv->exit != DVB_FE_NO_EXIT) {
> +		up(&fepriv->sem);
>  		return -ENODEV;
> +	}
>  
>  	if ((file->f_flags & O_ACCMODE) == O_RDONLY &&
>  	    (_IOC_DIR(cmd) != _IOC_READ || cmd == FE_GET_EVENT ||
> -	     cmd == FE_DISEQC_RECV_SLAVE_REPLY))
> +	     cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
> +		up(&fepriv->sem);
>  		return -EPERM;
> -
> -	if (down_interruptible (&fepriv->sem))
> -		return -ERESTARTSYS;
> +	}
>  
>  	if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY))
>  		err = dvb_frontend_ioctl_properties(file, cmd, parg);
> --
> 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
> 
> 
> --
> 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
> 

+1
This one resolve annoying mythtv-setup output:
"E FE_GET_INFO ioctl failed (/dev/dvb/adapter0/frontend0)
eno: No such device (19)"
http://www.gossamer-threads.com/lists/mythtv/dev/513410

But, there is a new one!
Just had a little déjà vu :)

Cheers,
poma



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fwd: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
  2012-09-01 15:51   ` Fwd: " poma
@ 2012-09-01 16:13     ` Antti Palosaari
  2012-09-01 16:26       ` Devin Heitmueller
  0 siblings, 1 reply; 9+ messages in thread
From: Antti Palosaari @ 2012-09-01 16:13 UTC (permalink / raw)
  To: poma
  Cc: Mauro Carvalho Chehab, Devin Heitmueller,
	Linux Media Mailing List, Juergen Lock, hselasky

On 09/01/2012 06:51 PM, poma wrote:
> On 08/12/2012 04:15 AM, Mauro Carvalho Chehab wrote:
>> Devin/Antti,
>>
>> As Juergen mentioned your help on this patch, do you mind helping reviewing
>> and testing it?
>>
>> Touching on those semaphores can be very tricky, as anything wrong may cause
>> a driver hangup. So, it is great to have more pair of eyes looking on it.
>>
>> While I didn't test the code (too busy trying to clean up my long queue -
>> currently with still 200+ patches left), I did a careful review at the
>> semaphore code there, and it seems this approach will work.
>>
>> At least, the first hunk looks perfect for me. The second hunk seems
>> a little more worrying, as the dvb core might be waiting forever for
>> a lock on a device that was already removed.
>>
>> In order to test it in practice, I think we need to remove an USB device
>> by hand while tuning it, and see if the core will not lock the device
>> forever.
>>
>> What do you think?
>> Mauro
>>
>> -------- Mensagem original --------
>> Assunto: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
>> Data: Wed, 1 Aug 2012 00:22:16 +0200
>> De: Juergen Lock <nox@jelal.kn-bremen.de>
>> Para: linux-media@vger.kernel.org
>> CC: hselasky@c2i.net
>>
>> That likely fxes this MythTV ticket:
>>
>> 	http://code.mythtv.org/trac/ticket/10830
>>
>> (which btw affects all usb tuners I tested as well, pctv452e,
>> dib0700, af9015)  pctv452e is still possibly broken with MythTV
>> even after this fix; it does work with VDR here tho despite I2C
>> errors.
>>
>> Reduced testcase:
>>
>> 	http://people.freebsd.org/~nox/tmp/ioctltst.c
>>
>> Thanx to devinheitmueller and crope from #linuxtv for helping with
>> this fix! :)
>>
>> Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>
>>
>> --- a/drivers/media/dvb/dvb-core/dvb_frontend.c
>> +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
>> @@ -604,6 +604,7 @@ static int dvb_frontend_thread(void *dat
>>   	enum dvbfe_algo algo;
>>
>>   	bool re_tune = false;
>> +	bool semheld = false;
>>
>>   	dprintk("%s\n", __func__);
>>
>> @@ -627,6 +628,8 @@ restart:
>>
>>   		if (kthread_should_stop() || dvb_frontend_is_exiting(fe)) {
>>   			/* got signal or quitting */
>> +			if (!down_interruptible (&fepriv->sem))
>> +				semheld = true;
>>   			fepriv->exit = DVB_FE_NORMAL_EXIT;
>>   			break;
>>   		}
>> @@ -742,6 +745,8 @@ restart:
>>   		fepriv->exit = DVB_FE_NO_EXIT;
>>   	mb();
>>
>> +	if (semheld)
>> +		up(&fepriv->sem);
>>   	dvb_frontend_wakeup(fe);
>>   	return 0;
>>   }
>> @@ -1804,16 +1809,20 @@ static int dvb_frontend_ioctl(struct fil
>>
>>   	dprintk("%s (%d)\n", __func__, _IOC_NR(cmd));
>>
>> -	if (fepriv->exit != DVB_FE_NO_EXIT)
>> +	if (down_interruptible (&fepriv->sem))
>> +		return -ERESTARTSYS;
>> +
>> +	if (fepriv->exit != DVB_FE_NO_EXIT) {
>> +		up(&fepriv->sem);
>>   		return -ENODEV;
>> +	}
>>
>>   	if ((file->f_flags & O_ACCMODE) == O_RDONLY &&
>>   	    (_IOC_DIR(cmd) != _IOC_READ || cmd == FE_GET_EVENT ||
>> -	     cmd == FE_DISEQC_RECV_SLAVE_REPLY))
>> +	     cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
>> +		up(&fepriv->sem);
>>   		return -EPERM;
>> -
>> -	if (down_interruptible (&fepriv->sem))
>> -		return -ERESTARTSYS;
>> +	}
>>
>>   	if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY))
>>   		err = dvb_frontend_ioctl_properties(file, cmd, parg);
>> --
>> 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
>>
>>
>> --
>> 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
>>
>
> +1
> This one resolve annoying mythtv-setup output:
> "E FE_GET_INFO ioctl failed (/dev/dvb/adapter0/frontend0)
> eno: No such device (19)"
> http://www.gossamer-threads.com/lists/mythtv/dev/513410
>
> But, there is a new one!
> Just had a little déjà vu :)


Is there anyone caring to review that carefully?

I am quite out with semaphores (up/down_interruptible) and also frontend 
is so complex... I would rather design / write whole dvb-frontend from 
the scratch :] (not doing that as no time).

regards
Antti

-- 
http://palosaari.fi/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fwd: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
  2012-09-01 16:13     ` Antti Palosaari
@ 2012-09-01 16:26       ` Devin Heitmueller
  2012-09-01 16:38         ` Antti Palosaari
  0 siblings, 1 reply; 9+ messages in thread
From: Devin Heitmueller @ 2012-09-01 16:26 UTC (permalink / raw)
  To: Antti Palosaari
  Cc: poma, Mauro Carvalho Chehab, Linux Media Mailing List,
	Juergen Lock, hselasky

On Sat, Sep 1, 2012 at 12:13 PM, Antti Palosaari <crope@iki.fi> wrote:
> Is there anyone caring to review that carefully?
>
> I am quite out with semaphores (up/down_interruptible) and also frontend is
> so complex... I would rather design / write whole dvb-frontend from the
> scratch :] (not doing that as no time).

If you're not willing to take the time to understand why the existing
dvb-frontend is so complex, how could you possibly suggest that you
could do a better job rewriting it from scratch?  :-)

Like most things, the devil is in the details.  The threading model is
complicated not because it was done poorly, but because there are lots
of complexity that is not obvious (combined with it having evolved
over time to adapt to hardware bugs).  It's only when you run it
against a half dozen cards with different behavior that you begin to
see why certain things were done the way they were.

In this case, I think the race condition in question has become more
obvious because of more aggressive use of power management for the
tuner and demod.  Because powering down the frontend now takes actual
time (due to i2c), users are now starting to hit the race condition.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fwd: [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast
  2012-09-01 16:26       ` Devin Heitmueller
@ 2012-09-01 16:38         ` Antti Palosaari
  0 siblings, 0 replies; 9+ messages in thread
From: Antti Palosaari @ 2012-09-01 16:38 UTC (permalink / raw)
  To: Devin Heitmueller
  Cc: poma, Mauro Carvalho Chehab, Linux Media Mailing List,
	Juergen Lock, hselasky

On 09/01/2012 07:26 PM, Devin Heitmueller wrote:
> On Sat, Sep 1, 2012 at 12:13 PM, Antti Palosaari <crope@iki.fi> wrote:
>> Is there anyone caring to review that carefully?
>>
>> I am quite out with semaphores (up/down_interruptible) and also frontend is
>> so complex... I would rather design / write whole dvb-frontend from the
>> scratch :] (not doing that as no time).
>
> If you're not willing to take the time to understand why the existing
> dvb-frontend is so complex, how could you possibly suggest that you
> could do a better job rewriting it from scratch?  :-)

Writing and designing things from the scratch is more motivated that 
trying to learn this much complex stuff. You surely know I has a kinda 
understanding with current dvb-frontend but I hate there is so much 
hacks which makes it even worse. Writing from the scratch means you 
could get rid of hacks - slowly.

See all the module parameters and think twice are those really needed? 
Also re-initialization stuff and what more. Unfortunately those are 
allowed at some phase as easy solution and now it is impossible to get 
rid. Hacks which belongs to individual drivers instead of core.

> Like most things, the devil is in the details.  The threading model is
> complicated not because it was done poorly, but because there are lots
> of complexity that is not obvious (combined with it having evolved
> over time to adapt to hardware bugs).  It's only when you run it
> against a half dozen cards with different behavior that you begin to
> see why certain things were done the way they were.
>
> In this case, I think the race condition in question has become more
> obvious because of more aggressive use of power management for the
> tuner and demod.  Because powering down the frontend now takes actual
> time (due to i2c), users are now starting to hit the race condition.
>
> Devin
>

regards
Antti


-- 
http://palosaari.fi/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH, RFC, updated] Fix DVB ioctls failing if frontend open/closed too fast
  2012-07-31 22:22 [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast Juergen Lock
  2012-08-12  2:15 ` Fwd: " Mauro Carvalho Chehab
@ 2012-09-26 20:11 ` Juergen Lock
  1 sibling, 0 replies; 9+ messages in thread
From: Juergen Lock @ 2012-09-26 20:11 UTC (permalink / raw)
  To: Juergen Lock; +Cc: linux-media, hselasky

On Wed, Aug 01, 2012 at 12:22:16AM +0200, Juergen Lock wrote:
> That likely fixes this MythTV ticket:
> 
> 	http://code.mythtv.org/trac/ticket/10830
> 
> (which btw affects all usb tuners I tested as well, pctv452e,
> dib0700, af9015)  pctv452e is still possibly broken with MythTV
> even after this fix; it does work with VDR here tho despite I2C
> errors.
> 
> Reduced testcase:
> 
> 	http://people.freebsd.org/~nox/tmp/ioctltst.c
> 
> Thanx to devinheitmueller and crope from #linuxtv for helping with
> this fix! :)
> 
Updated patch for media tree for_3.7:

Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>

--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -603,6 +603,7 @@ static int dvb_frontend_thread(void *dat
 	enum dvbfe_algo algo;
 
 	bool re_tune = false;
+	bool semheld = false;
 
 	dev_dbg(fe->dvb->device, "%s:\n", __func__);
 
@@ -626,6 +627,8 @@ restart:
 
 		if (kthread_should_stop() || dvb_frontend_is_exiting(fe)) {
 			/* got signal or quitting */
+			if (!down_interruptible (&fepriv->sem))
+				semheld = true;
 			fepriv->exit = DVB_FE_NORMAL_EXIT;
 			break;
 		}
@@ -741,6 +744,8 @@ restart:
 		fepriv->exit = DVB_FE_NO_EXIT;
 	mb();
 
+	if (semheld)
+		up(&fepriv->sem);
 	dvb_frontend_wakeup(fe);
 	return 0;
 }
@@ -1819,16 +1824,20 @@ static int dvb_frontend_ioctl(struct fil
 	int err = -ENOTTY;
 
 	dev_dbg(fe->dvb->device, "%s: (%d)\n", __func__, _IOC_NR(cmd));
-	if (fepriv->exit != DVB_FE_NO_EXIT)
+	if (down_interruptible (&fepriv->sem))
+		return -ERESTARTSYS;
+
+	if (fepriv->exit != DVB_FE_NO_EXIT) {
+		up(&fepriv->sem);
 		return -ENODEV;
+	}
 
 	if ((file->f_flags & O_ACCMODE) == O_RDONLY &&
 	    (_IOC_DIR(cmd) != _IOC_READ || cmd == FE_GET_EVENT ||
-	     cmd == FE_DISEQC_RECV_SLAVE_REPLY))
+	     cmd == FE_DISEQC_RECV_SLAVE_REPLY)) {
+		up(&fepriv->sem);
 		return -EPERM;
-
-	if (down_interruptible (&fepriv->sem))
-		return -ERESTARTSYS;
+	}
 
 	if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY))
 		err = dvb_frontend_ioctl_properties(file, cmd, parg);

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-09-26 20:23 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-31 22:22 [PATCH, RFC] Fix DVB ioctls failing if frontend open/closed too fast Juergen Lock
2012-08-12  2:15 ` Fwd: " Mauro Carvalho Chehab
2012-08-12  3:06   ` Devin Heitmueller
2012-08-28 20:56     ` Hans Petter Selasky
2012-09-01 15:51   ` Fwd: " poma
2012-09-01 16:13     ` Antti Palosaari
2012-09-01 16:26       ` Devin Heitmueller
2012-09-01 16:38         ` Antti Palosaari
2012-09-26 20:11 ` [PATCH, RFC, updated] " Juergen Lock

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.