Fix DVB ioctls failing if frontend open/closed too fast

Devin/Antti,

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

On Sat, Aug 11, 2012 at 10:15 PM, Mauro Carvalho Chehab

Quoted 4 lines · <[email protected]> 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/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 <[email protected]>
Para: [email protected]
CC: [email protected]

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 <[email protected]>

drivers/media/dvb/dvb-core/dvb_frontend.c +14 -6

@@ -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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 108 lines · Mauro Carvalho Chehab

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 <[email protected]>
Para: [email protected]
CC: [email protected]

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 <[email protected]>

drivers/media/dvb/dvb-core/dvb_frontend.c +14 -6

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
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).

Quoted 5 lines · Antti Palosaari

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

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! :)