| Message ID | 502711BE.4020701@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- 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
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 -- 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
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
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
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
--- 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);
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> -- 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