[GIT PATCHES] V4L/DVB updates

[GIT PATCHES] V4L/DVB updates

[Mauro Carvalho Chehab]

Linus,

Please pull from:
        ssh://master.kernel.org/pub/scm/linux/kernel/git/mchehab/v4l-dvb.git master

For the following:

   - Fix Kernel Bugzilla #8301: spinlock fix for flexcop-pci
   - Fix 1/3 for bug 7819: fixed frontend hotplug issue
   - Fix 2/3 for bug 7819: demux and dvr
   - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
   - Usbvision: fix TDA9887 detection
   - Bt878: prevent probing wrong card entry

The fix for Bugzilla #7819 is bigger, since the current lock approach 
doesn't allow hotunplugging an USB DVB device. The other patches are 
trivial fixes.

Cheers,
Mauro.

---

 Documentation/logo.gif                             |  Bin
 drivers/media/dvb/b2c2/flexcop-pci.c               |    9 ++--
 drivers/media/dvb/bt8xx/bt878.c                    |    4 +-
 drivers/media/dvb/dvb-core/dmxdev.c                |   56 +++++++++++++++++++-
 drivers/media/dvb/dvb-core/dmxdev.h                |    2 +
 drivers/media/dvb/dvb-core/dvb_frontend.c          |   20 ++++++-
 drivers/media/dvb/dvb-core/dvb_net.c               |   32 +++++++++++-
 drivers/media/dvb/dvb-core/dvb_net.h               |    1 +
 drivers/media/dvb/dvb-core/dvbdev.c                |    1 +
 drivers/media/dvb/dvb-core/dvbdev.h                |    1 +
 drivers/media/video/usbvision/usbvision-i2c.c      |    1 +
 11 files changed, 115 insertions(+), 12 deletions(-)

Akinobu Mita (1):
      V4L/DVB (5513): Bt878: prevent probing wrong card entry

Hendrik Borghorst (1):
      V4L/DVB (5505): Fix Kernel Bugzilla #8301: spinlock fix for flexcop-pci

Markus Rechberger (3):
      V4L/DVB (5510): Fix 1/3 for bug 7819: fixed frontend hotplug issue
      V4L/DVB (5511): Fix 2/3 for bug 7819: demux and dvr
      V4L/DVB (5512): Fix 3/3 for bug 7819: fixed hotplugging for dvbnet

Thierry MERLE (1):
      V4L/DVB (5524): Usbvision: fix TDA9887 detection

---------------------------------------------------
V4L/DVB development is hosted at http://linuxtv.org

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Michael Krufky]

Mauro,

I've been out of town for the past few days... I just got home and saw this:


Mauro Carvalho Chehab wrote:
>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
>    - Fix 2/3 for bug 7819: demux and dvr
>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
I don't think that this is 2.6.21 material.  These patches have not yet
received
enough testing to be sent to mainline.

I have tested them, and they seem to work for my cxusb device, but we have
yet to hear test results from users of usb dvb devices that do not use the
dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)

The bug that these patches fix has been around throughout the entire kernel
history of the dvb subsystem.  The bug is not a regression -- it has
always been
there.  In my opinion, it is too late in 2.6.21 development to apply
this change.
Because these fixes are not obvious, I think we should let them get some
more testing, and have them queued for 2.6.22 .


Regards,

Michael

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Manu Abraham]

Michael Krufky wrote:
> Mauro,
> 
> I've been out of town for the past few days... I just got home and saw this:
> 
> 
> Mauro Carvalho Chehab wrote:
>>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
>>    - Fix 2/3 for bug 7819: demux and dvr
>>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> I don't think that this is 2.6.21 material.  These patches have not yet
> received
> enough testing to be sent to mainline.
> 
> I have tested them, and they seem to work for my cxusb device, but we have
> yet to hear test results from users of usb dvb devices that do not use the
> dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
> 
> The bug that these patches fix has been around throughout the entire kernel
> history of the dvb subsystem.  The bug is not a regression -- it has
> always been
> there.  In my opinion, it is too late in 2.6.21 development to apply
> this change.
> Because these fixes are not obvious, I think we should let them get some
> more testing, and have them queued for 2.6.22 .


I am not arguing about the veracity of the patches, but how things are
handled.

Agreed to all the mentioned above. There is one more aspect. The
mentioned patches, do not have any ACK/SOB from any DVB
developer/maintainer for the same.

Huge regressions are created this way. One more time the regression
creator is caught.

Manu

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Markus Rechberger]

On 4/16/07, Manu Abraham <abraham.manu@gmail.com> wrote:
> Michael Krufky wrote:
> > Mauro,
> >
> > I've been out of town for the past few days... I just got home and saw
> this:
> >
> >
> > Mauro Carvalho Chehab wrote:
> >>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> >>    - Fix 2/3 for bug 7819: demux and dvr
> >>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> > I don't think that this is 2.6.21 material.  These patches have not yet
> > received
> > enough testing to be sent to mainline.
> >
> > I have tested them, and they seem to work for my cxusb device, but we have
> > yet to hear test results from users of usb dvb devices that do not use the
> > dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
> >
> > The bug that these patches fix has been around throughout the entire
> kernel
> > history of the dvb subsystem.  The bug is not a regression -- it has
> > always been
> > there.  In my opinion, it is too late in 2.6.21 development to apply
> > this change.
> > Because these fixes are not obvious, I think we should let them get some
> > more testing, and have them queued for 2.6.22 .
>
>
> I am not arguing about the veracity of the patches, but how things are
> handled.
>
> Agreed to all the mentioned above. There is one more aspect. The
> mentioned patches, do not have any ACK/SOB from any DVB
> developer/maintainer for the same.
>
> Huge regressions are created this way. One more time the regression
> creator is caught.
>

The patch has been around on the dvb mailinglist ([PATCH][RFC] DVB
Hotplug Fix, 5. April 2007), Michael looked at it when I told him
about it again separatly (12. April 2007)
Between that there was enough time to respond on it or review it.

Mauro told me at the beginning if noone responds he'll go forward
somehow because it fixes hotplugging, and after 2 weeks I wouldn't
expect anyone commenting that thread anymore.

It would be better to look and respond to that patch first before
responding about that you don't like how it's handled, Mauro also
wrote in his announcement that people should look at the recent
mercurial tree and he'll revert it if something doesn't work by
accident before submitting it to his git tree... still noone
complained about the hotplugging patch in there.

Markus

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Mauro Carvalho Chehab]

Em Seg, 2007-04-16 às 10:17 +0200, Markus Rechberger escreveu:
> On 4/16/07, Manu Abraham <abraham.manu@gmail.com> wrote:
> > Michael Krufky wrote:
> > > Mauro,
> > >
> > > I've been out of town for the past few days... I just got home and saw
> > this:
> > >
> > >
> > > Mauro Carvalho Chehab wrote:
> > >>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> > >>    - Fix 2/3 for bug 7819: demux and dvr
> > >>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> > > I don't think that this is 2.6.21 material.  These patches have not yet
> > > received
> > > enough testing to be sent to mainline.
> > >
> > > I have tested them, and they seem to work for my cxusb device, but we have
> > > yet to hear test results from users of usb dvb devices that do not use the
> > > dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
> > >
> > > The bug that these patches fix has been around throughout the entire
> > kernel
> > > history of the dvb subsystem.  The bug is not a regression -- it has
> > > always been
> > > there.  In my opinion, it is too late in 2.6.21 development to apply
> > > this change.
> > > Because these fixes are not obvious, I think we should let them get some
> > > more testing, and have them queued for 2.6.22 .
> >
> >
> > I am not arguing about the veracity of the patches, but how things are
> > handled.
> >
> > Agreed to all the mentioned above. There is one more aspect. The
> > mentioned patches, do not have any ACK/SOB from any DVB
> > developer/maintainer for the same.
> >
> > Huge regressions are created this way. One more time the regression
> > creator is caught.
> >
> 
> The patch has been around on the dvb mailinglist ([PATCH][RFC] DVB
> Hotplug Fix, 5. April 2007), Michael looked at it when I told him
> about it again separatly (12. April 2007)
> Between that there was enough time to respond on it or review it.
> 
> Mauro told me at the beginning if noone responds he'll go forward
> somehow because it fixes hotplugging, and after 2 weeks I wouldn't
> expect anyone commenting that thread anymore.
> 
> It would be better to look and respond to that patch first before
> responding about that you don't like how it's handled, Mauro also
> wrote in his announcement that people should look at the recent
> mercurial tree and he'll revert it if something doesn't work by
> accident before submitting it to his git tree... still noone
> complained about the hotplugging patch in there.
Ditto.

It is really bad to have an OOPS that can be generated by simply
removing a device at the usb port. This can open a door for DoS attacks.
IMO, this kind of bug should be fixed as soon as possible. 

If you take a look at the history of this bug, it were reported back on
Jan, 14. Also, Adrian sent several regression reports c/c to
v4l-dvb-mailing list (so copying all V4L/DVB maintainers, including you,
me, Markus and the others), warning about the bug, and pointing that a
patch from Markus were already available.

I also explicitly warned at DVB ML that I were about to send this patch,
together with other fixes, asking the community for more tests. After
that, I received two positive answers on my mailbox from people that
tested and noticed that this really fixed the issue.

I don't think that 3 months is a short period of time for us to work on
a solution for the bug and have it done to be included on Kernel.

Cheers,
Mauro

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Adrian Bunk]

On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
> Mauro,
> 
> I've been out of town for the past few days... I just got home and saw this:
> 
> 
> Mauro Carvalho Chehab wrote:
> >    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> >    - Fix 2/3 for bug 7819: demux and dvr
> >    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> I don't think that this is 2.6.21 material.  These patches have not yet
> received
> enough testing to be sent to mainline.
> 
> I have tested them, and they seem to work for my cxusb device, but we have
> yet to hear test results from users of usb dvb devices that do not use the
> dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
> 
> The bug that these patches fix has been around throughout the entire kernel
> history of the dvb subsystem.  The bug is not a regression -- it has
> always been
> there.  In my opinion, it is too late in 2.6.21 development to apply
> this change.
> Because these fixes are not obvious, I think we should let them get some
> more testing, and have them queued for 2.6.22 .

Unless I misunderstand anything, this should fix [1].

And this is a bug that was reported to be present in 2.6.21-rc but not 
in 2.6.20 (and it's therefore a regression, no matter whether the 
underlying problem was older and only exposed by some other change).

> Regards,
> 
> Michael

cu
Adrian

[1] http://lkml.org/lkml/2007/3/9/212

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[CIJOML]

Hi,

I have tested these patches with:

Freecom DVB-T dongle
Pluto2 pcmcia card
Leadtek WinFast DTV dongle 1st generation
Leadtek WinFast DTV dongle 2nd generation

These are 4 different devices with 4 different hw and modules.
All works. Please apply.

Michal

Dne pondělí 16 duben 2007 16:15 Adrian Bunk napsal(a):
> On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
> > Mauro,
> >
> > I've been out of town for the past few days... I just got home and saw
> > this:
> >
> > Mauro Carvalho Chehab wrote:
> > >    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> > >    - Fix 2/3 for bug 7819: demux and dvr
> > >    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> >
> > I don't think that this is 2.6.21 material.  These patches have not yet
> > received
> > enough testing to be sent to mainline.
> >
> > I have tested them, and they seem to work for my cxusb device, but we
> > have yet to hear test results from users of usb dvb devices that do not
> > use the dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
> >
> > The bug that these patches fix has been around throughout the entire
> > kernel history of the dvb subsystem.  The bug is not a regression -- it
> > has always been
> > there.  In my opinion, it is too late in 2.6.21 development to apply
> > this change.
> > Because these fixes are not obvious, I think we should let them get some
> > more testing, and have them queued for 2.6.22 .
>
> Unless I misunderstand anything, this should fix [1].
>
> And this is a bug that was reported to be present in 2.6.21-rc but not
> in 2.6.20 (and it's therefore a regression, no matter whether the
> underlying problem was older and only exposed by some other change).
>
> > Regards,
> >
> > Michael
>
> cu
> Adrian
>
> [1] http://lkml.org/lkml/2007/3/9/212

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[CIJOML]

One more addiction:

As a side effect this fixes also kernel crash, when you swsusp/swresume with 
USB dongle and playing DVB stream. 

Michal

Dne pondělí 16 duben 2007 16:15 Adrian Bunk napsal(a):
> On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
> > Mauro,
> >
> > I've been out of town for the past few days... I just got home and saw
> > this:
> >
> > Mauro Carvalho Chehab wrote:
> > >    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> > >    - Fix 2/3 for bug 7819: demux and dvr
> > >    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> >
> > I don't think that this is 2.6.21 material.  These patches have not yet
> > received
> > enough testing to be sent to mainline.
> >
> > I have tested them, and they seem to work for my cxusb device, but we
> > have yet to hear test results from users of usb dvb devices that do not
> > use the dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
> >
> > The bug that these patches fix has been around throughout the entire
> > kernel history of the dvb subsystem.  The bug is not a regression -- it
> > has always been
> > there.  In my opinion, it is too late in 2.6.21 development to apply
> > this change.
> > Because these fixes are not obvious, I think we should let them get some
> > more testing, and have them queued for 2.6.22 .
>
> Unless I misunderstand anything, this should fix [1].
>
> And this is a bug that was reported to be present in 2.6.21-rc but not
> in 2.6.20 (and it's therefore a regression, no matter whether the
> underlying problem was older and only exposed by some other change).
>
> > Regards,
> >
> > Michael
>
> cu
> Adrian
>
> [1] http://lkml.org/lkml/2007/3/9/212

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Michael Krufky]

Adrian Bunk wrote:
> On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
>> Mauro,
>>
>> I've been out of town for the past few days... I just got home and saw this:
>>
>>
>> Mauro Carvalho Chehab wrote:
>>>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
>>>    - Fix 2/3 for bug 7819: demux and dvr
>>>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
>> I don't think that this is 2.6.21 material.  These patches have not yet
>> received
>> enough testing to be sent to mainline.
>>
>> I have tested them, and they seem to work for my cxusb device, but we have
>> yet to hear test results from users of usb dvb devices that do not use the
>> dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
>>
>> The bug that these patches fix has been around throughout the entire kernel
>> history of the dvb subsystem.  The bug is not a regression -- it has
>> always been
>> there.  In my opinion, it is too late in 2.6.21 development to apply
>> this change.
>> Because these fixes are not obvious, I think we should let them get some
>> more testing, and have them queued for 2.6.22 .
> 
> Unless I misunderstand anything, this should fix [1].
> 
> And this is a bug that was reported to be present in 2.6.21-rc but not 
> in 2.6.20 (and it's therefore a regression, no matter whether the 
> underlying problem was older and only exposed by some other change).

Not true.  The DVB subsystem has NEVER been hot-unpluggable.  I confirm that the
patches SEEM to be correct, but this has not yet been verified.  None of the
authors of dvb-core gave their ACK on these changesets.

The DVB hotplug issue has been around since the very beginning.  I assure you,
that I consider this fix to be very important, and I really would love to see it
hit mainline.  However, given the situation, it is not appropriate to push these
in during -rc7

I have doubts on CIJOML's testing method -- there is no way he could have
unplugged the device while in use, while running 2.6.20.y and not receive an
OOPS.  CIJOML, please see the bottom of this email for

Sure, this will prevent an OOPS on some, and hopefully all devices...  but what
if it causes a regression for those untested?

Why do we have a merge window, if new changesets are going to be rushed into
late -rc kernels without proper testing, and without the ack of a dvb subsystem
maintainer?

Are we prepared to go for another -rc and 3 or 4 weeks of testing to confirm
that this fix doesn't cause new regressions?  I don't think so.


Markus Rechberger wrote:
> The patch has been around on the dvb mailinglist ([PATCH][RFC] DVB
> Hotplug Fix, 5. April 2007),

The patch was merged into the development repository at the same time the pull
request was issued to Linus.  This has NOT been tested on a wide scale.  It
should go to -mm for a while before being merged to mainline.

Mauro Carvalho Chehab wrote:
> I also explicitly warned at DVB ML that I were about to send this patch,
> together with other fixes, asking the community for more tests. After
> that, I received two positive answers on my mailbox from people that
> tested and noticed that this really fixed the issue.

One of those positive answers was me -  I explained that it worked for me, but
we need others to test.

You waited ONE DAY after sending this "warning" to the dvb mailing list?  (
http://linuxtv.org/pipermail/linux-dvb/2007-April/017204.html ) I saw that email
after seeing the pull request to Linus.  We dont have users testing the
repositories after each commit -- you _really_ need to give some more time to
allow for such testing.

CIJOML wrote:
> Hi,
>
> I have tested these patches with:
>
> Freecom DVB-T dongle
> Pluto2 pcmcia card
> Leadtek WinFast DTV dongle 1st generation
> Leadtek WinFast DTV dongle 2nd generation
>
> These are 4 different devices with 4 different hw and modules.
> All works. Please apply.

Well, that helps...  But it would still be nice to hear test results on a
CinergyT2 or flexcop-usb.

Which driver supports those Winfast dongles?  We already know for sure that the
patches work correctly for any driver based on the dvb-usb framework.

If you had the device open, and then disconnect it from the usb bus, no matter
what kernel version you're running, you should hit the OOPS.  I confirm that
these patches prevent that OOPS from occurring, but I have trouble believing
that you did NOT experience such an OOPS in 2.6.20.y

Could you please describe the method in which your test caused an OOPS using
2.6.21-rc and did NOT cause an oops in 2.6.20.y ?

-- 
Michael Krufky

Re: [GIT PATCHES] V4L/DVB updates

[Dmitry Torokhov]

Hi Mauro,

On 4/15/07, Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
>   - Fix Kernel Bugzilla #8301: spinlock fix for flexcop-pci

While move of spin_lock_init before request_irq is obviously correct I
wonder what is the reason behind changing spin_lock_irq() into
spin_lock_irqsave() as I do not see flexcop_pci_isr being called from
anywhere but IRQ context.

BTW, is irq_lock needed at all?

-- 
Dmitry

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Markus Rechberger]

On 4/16/07, Michael Krufky <mkrufky@linuxtv.org> wrote:
> Adrian Bunk wrote:
> > On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
> >> Mauro,
> >>
> >> I've been out of town for the past few days... I just got home and saw
> this:
> >>
> >>
> >> Mauro Carvalho Chehab wrote:
> >>>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> >>>    - Fix 2/3 for bug 7819: demux and dvr
> >>>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> >> I don't think that this is 2.6.21 material.  These patches have not yet
> >> received
> >> enough testing to be sent to mainline.
> >>
> >> I have tested them, and they seem to work for my cxusb device, but we
> have
> >> yet to hear test results from users of usb dvb devices that do not use
> the
> >> dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
> >>
> >> The bug that these patches fix has been around throughout the entire
> kernel
> >> history of the dvb subsystem.  The bug is not a regression -- it has
> >> always been
> >> there.  In my opinion, it is too late in 2.6.21 development to apply
> >> this change.
> >> Because these fixes are not obvious, I think we should let them get some
> >> more testing, and have them queued for 2.6.22 .
> >
> > Unless I misunderstand anything, this should fix [1].
> >
> > And this is a bug that was reported to be present in 2.6.21-rc but not
> > in 2.6.20 (and it's therefore a regression, no matter whether the
> > underlying problem was older and only exposed by some other change).
>
> Not true.  The DVB subsystem has NEVER been hot-unpluggable.  I confirm that
> the
> patches SEEM to be correct, but this has not yet been verified.  None of the
> authors of dvb-core gave their ACK on these changesets.
>
> The DVB hotplug issue has been around since the very beginning.  I assure
> you,
> that I consider this fix to be very important, and I really would love to
> see it
> hit mainline.  However, given the situation, it is not appropriate to push
> these
> in during -rc7
>
> I have doubts on CIJOML's testing method -- there is no way he could have
> unplugged the device while in use, while running 2.6.20.y and not receive an
> OOPS.  CIJOML, please see the bottom of this email for
>
> Sure, this will prevent an OOPS on some, and hopefully all devices...  but
> what
> if it causes a regression for those untested?
>
> Why do we have a merge window, if new changesets are going to be rushed into
> late -rc kernels without proper testing, and without the ack of a dvb
> subsystem
> maintainer?
>
> Are we prepared to go for another -rc and 3 or 4 weeks of testing to confirm
> that this fix doesn't cause new regressions?  I don't think so.
>

The problem I see with the cinergyT2 is that this driver reimplements
the frontend code and doesn't even use the dvb-core template, so a
proper fix would also be to reimplement that fix in the cinergyT2
device.
I don't think Daniel Mack or Holger Waechtler manage this driver
anymore because they didn't comment the last few fixes that were
applied to that driver (though I put them into CC here) As far as I
remember someone wrote that they didn't want to use the dvb_frontend
code back then for any reason.

So the only driver I'd take into account would be the flexcop driver.

Currently tested drivers are: dvb-usb based devices, and em28xx/em2880
based devices

Markus

>
> Markus Rechberger wrote:
> > The patch has been around on the dvb mailinglist ([PATCH][RFC] DVB
> > Hotplug Fix, 5. April 2007),
>
> The patch was merged into the development repository at the same time the
> pull
> request was issued to Linus.  This has NOT been tested on a wide scale.  It
> should go to -mm for a while before being merged to mainline.
>
> Mauro Carvalho Chehab wrote:
> > I also explicitly warned at DVB ML that I were about to send this patch,
> > together with other fixes, asking the community for more tests. After
> > that, I received two positive answers on my mailbox from people that
> > tested and noticed that this really fixed the issue.
>
> One of those positive answers was me -  I explained that it worked for me,
> but
> we need others to test.
>
> You waited ONE DAY after sending this "warning" to the dvb mailing list?  (
> http://linuxtv.org/pipermail/linux-dvb/2007-April/017204.html ) I saw that
> email
> after seeing the pull request to Linus.  We dont have users testing the
> repositories after each commit -- you _really_ need to give some more time
> to
> allow for such testing.
>
> CIJOML wrote:
> > Hi,
> >
> > I have tested these patches with:
> >
> > Freecom DVB-T dongle
> > Pluto2 pcmcia card
> > Leadtek WinFast DTV dongle 1st generation
> > Leadtek WinFast DTV dongle 2nd generation
> >
> > These are 4 different devices with 4 different hw and modules.
> > All works. Please apply.
>
> Well, that helps...  But it would still be nice to hear test results on a
> CinergyT2 or flexcop-usb.
>
> Which driver supports those Winfast dongles?  We already know for sure that
> the
> patches work correctly for any driver based on the dvb-usb framework.
>
> If you had the device open, and then disconnect it from the usb bus, no
> matter
> what kernel version you're running, you should hit the OOPS.  I confirm that
> these patches prevent that OOPS from occurring, but I have trouble believing
> that you did NOT experience such an OOPS in 2.6.20.y
>
> Could you please describe the method in which your test caused an OOPS using
> 2.6.21-rc and did NOT cause an oops in 2.6.20.y ?
>
> --
> Michael Krufky
>
>
> _______________________________________________
> v4l-dvb-maintainer mailing list
> v4l-dvb-maintainer@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/v4l-dvb-maintainer
>


-- 
Markus Rechberger

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[CIJOML]

Dne pondělí 16 duben 2007 17:34 Michael Krufky napsal(a):
> Adrian Bunk wrote:
> > On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
> >> Mauro,
> >>
> >> I've been out of town for the past few days... I just got home and saw
> >> this:
> >>
> >> Mauro Carvalho Chehab wrote:
> >>>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> >>>    - Fix 2/3 for bug 7819: demux and dvr
> >>>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> >>
> >> I don't think that this is 2.6.21 material.  These patches have not yet
> >> received
> >> enough testing to be sent to mainline.
> >>
> >> I have tested them, and they seem to work for my cxusb device, but we
> >> have yet to hear test results from users of usb dvb devices that do not
> >> use the dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
> >>
> >> The bug that these patches fix has been around throughout the entire
> >> kernel history of the dvb subsystem.  The bug is not a regression -- it
> >> has always been
> >> there.  In my opinion, it is too late in 2.6.21 development to apply
> >> this change.
> >> Because these fixes are not obvious, I think we should let them get some
> >> more testing, and have them queued for 2.6.22 .
> >
> > Unless I misunderstand anything, this should fix [1].
> >
> > And this is a bug that was reported to be present in 2.6.21-rc but not
> > in 2.6.20 (and it's therefore a regression, no matter whether the
> > underlying problem was older and only exposed by some other change).
>
> Not true.  The DVB subsystem has NEVER been hot-unpluggable.  I confirm
> that the patches SEEM to be correct, but this has not yet been verified. 
> None of the authors of dvb-core gave their ACK on these changesets.
>
> The DVB hotplug issue has been around since the very beginning.  I assure
> you, that I consider this fix to be very important, and I really would love
> to see it hit mainline.  However, given the situation, it is not
> appropriate to push these in during -rc7
>
> I have doubts on CIJOML's testing method -- there is no way he could have
> unplugged the device while in use, while running 2.6.20.y and not receive
> an OOPS.  CIJOML, please see the bottom of this email for
>
> Sure, this will prevent an OOPS on some, and hopefully all devices...  but
> what if it causes a regression for those untested?
>
> Why do we have a merge window, if new changesets are going to be rushed
> into late -rc kernels without proper testing, and without the ack of a dvb
> subsystem maintainer?
>
> Are we prepared to go for another -rc and 3 or 4 weeks of testing to
> confirm that this fix doesn't cause new regressions?  I don't think so.
>
> Markus Rechberger wrote:
> > The patch has been around on the dvb mailinglist ([PATCH][RFC] DVB
> > Hotplug Fix, 5. April 2007),
>
> The patch was merged into the development repository at the same time the
> pull request was issued to Linus.  This has NOT been tested on a wide
> scale.  It should go to -mm for a while before being merged to mainline.
>
> Mauro Carvalho Chehab wrote:
> > I also explicitly warned at DVB ML that I were about to send this patch,
> > together with other fixes, asking the community for more tests. After
> > that, I received two positive answers on my mailbox from people that
> > tested and noticed that this really fixed the issue.
>
> One of those positive answers was me -  I explained that it worked for me,
> but we need others to test.
>
> You waited ONE DAY after sending this "warning" to the dvb mailing list?  (
> http://linuxtv.org/pipermail/linux-dvb/2007-April/017204.html ) I saw that
> email after seeing the pull request to Linus.  We dont have users testing
> the repositories after each commit -- you _really_ need to give some more
> time to allow for such testing.
>
> CIJOML wrote:
> > Hi,
> >
> > I have tested these patches with:
> >
> > Freecom DVB-T dongle
> > Pluto2 pcmcia card
> > Leadtek WinFast DTV dongle 1st generation
> > Leadtek WinFast DTV dongle 2nd generation
> >
> > These are 4 different devices with 4 different hw and modules.
> > All works. Please apply.
>
> Well, that helps...  But it would still be nice to hear test results on a
> CinergyT2 or flexcop-usb.
>
> Which driver supports those Winfast dongles?  We already know for sure that
> the patches work correctly for any driver based on the dvb-usb framework.
>
> If you had the device open, and then disconnect it from the usb bus, no
> matter what kernel version you're running, you should hit the OOPS.  I
> confirm that these patches prevent that OOPS from occurring, but I have
> trouble believing that you did NOT experience such an OOPS in 2.6.20.y
>
> Could you please describe the method in which your test caused an OOPS
> using 2.6.21-rc and did NOT cause an oops in 2.6.20.y ?

Hi,

I have tested these patches with 2.6.20-mh1 + v4l-dvb-b5be3479f070 patchset.
I also tried 2.6.21-rc6 + v4l-dvb-b5be3479f070 patchset and this combination 
also works without OOPS.

Winfast dongles are both dvb-usb based (DiBcom 3000M-C and DiBcom DiB7000P), 
but pluto2 is cardbus (pci) based.

I think we can include these patches into 2.6.21 and if we receive any 
problem, we still have 2.6.21.Z for fixing, don't we?

Michal

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Michael Krufky]

CIJOML wrote:
> Dne pondělí 16 duben 2007 17:34 Michael Krufky napsal(a):
>   
>> Adrian Bunk wrote:
>>     
>>> On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
>>>       
>>>> Mauro,
>>>>
>>>> I've been out of town for the past few days... I just got home and saw
>>>> this:
>>>>
>>>> Mauro Carvalho Chehab wrote:
>>>>         
>>>>>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
>>>>>    - Fix 2/3 for bug 7819: demux and dvr
>>>>>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
>>>>>           
>>>> I don't think that this is 2.6.21 material.  These patches have not yet
>>>> received
>>>> enough testing to be sent to mainline.
>>>>
>>>> I have tested them, and they seem to work for my cxusb device, but we
>>>> have yet to hear test results from users of usb dvb devices that do not
>>>> use the dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
>>>>
>>>> The bug that these patches fix has been around throughout the entire
>>>> kernel history of the dvb subsystem.  The bug is not a regression -- it
>>>> has always been
>>>> there.  In my opinion, it is too late in 2.6.21 development to apply
>>>> this change.
>>>> Because these fixes are not obvious, I think we should let them get some
>>>> more testing, and have them queued for 2.6.22 .
>>>>         
>>> Unless I misunderstand anything, this should fix [1].
>>>
>>> And this is a bug that was reported to be present in 2.6.21-rc but not
>>> in 2.6.20 (and it's therefore a regression, no matter whether the
>>> underlying problem was older and only exposed by some other change).
>>>       
>> Not true.  The DVB subsystem has NEVER been hot-unpluggable.  I confirm
>> that the patches SEEM to be correct, but this has not yet been verified. 
>> None of the authors of dvb-core gave their ACK on these changesets.
>>
>> The DVB hotplug issue has been around since the very beginning.  I assure
>> you, that I consider this fix to be very important, and I really would love
>> to see it hit mainline.  However, given the situation, it is not
>> appropriate to push these in during -rc7
>>
>> I have doubts on CIJOML's testing method -- there is no way he could have
>> unplugged the device while in use, while running 2.6.20.y and not receive
>> an OOPS.  CIJOML, please see the bottom of this email for
>>
>> Sure, this will prevent an OOPS on some, and hopefully all devices...  but
>> what if it causes a regression for those untested?
>>
>> Why do we have a merge window, if new changesets are going to be rushed
>> into late -rc kernels without proper testing, and without the ack of a dvb
>> subsystem maintainer?
>>
>> Are we prepared to go for another -rc and 3 or 4 weeks of testing to
>> confirm that this fix doesn't cause new regressions?  I don't think so.
>>
>> Markus Rechberger wrote:
>>     
>>> The patch has been around on the dvb mailinglist ([PATCH][RFC] DVB
>>> Hotplug Fix, 5. April 2007),
>>>       
>> The patch was merged into the development repository at the same time the
>> pull request was issued to Linus.  This has NOT been tested on a wide
>> scale.  It should go to -mm for a while before being merged to mainline.
>>
>> Mauro Carvalho Chehab wrote:
>>     
>>> I also explicitly warned at DVB ML that I were about to send this patch,
>>> together with other fixes, asking the community for more tests. After
>>> that, I received two positive answers on my mailbox from people that
>>> tested and noticed that this really fixed the issue.
>>>       
>> One of those positive answers was me -  I explained that it worked for me,
>> but we need others to test.
>>
>> You waited ONE DAY after sending this "warning" to the dvb mailing list?  (
>> http://linuxtv.org/pipermail/linux-dvb/2007-April/017204.html ) I saw that
>> email after seeing the pull request to Linus.  We dont have users testing
>> the repositories after each commit -- you _really_ need to give some more
>> time to allow for such testing.
>>
>> CIJOML wrote:
>>     
>>> Hi,
>>>
>>> I have tested these patches with:
>>>
>>> Freecom DVB-T dongle
>>> Pluto2 pcmcia card
>>> Leadtek WinFast DTV dongle 1st generation
>>> Leadtek WinFast DTV dongle 2nd generation
>>>
>>> These are 4 different devices with 4 different hw and modules.
>>> All works. Please apply.
>>>       
>> Well, that helps...  But it would still be nice to hear test results on a
>> CinergyT2 or flexcop-usb.
>>
>> Which driver supports those Winfast dongles?  We already know for sure that
>> the patches work correctly for any driver based on the dvb-usb framework.
>>
>> If you had the device open, and then disconnect it from the usb bus, no
>> matter what kernel version you're running, you should hit the OOPS.  I
>> confirm that these patches prevent that OOPS from occurring, but I have
>> trouble believing that you did NOT experience such an OOPS in 2.6.20.y
>>
>> Could you please describe the method in which your test caused an OOPS
>> using 2.6.21-rc and did NOT cause an oops in 2.6.20.y ?
>>     
>
> Hi,
>
> I have tested these patches with 2.6.20-mh1 + v4l-dvb-b5be3479f070 patchset.
> I also tried 2.6.21-rc6 + v4l-dvb-b5be3479f070 patchset and this combination 
> also works without OOPS.
>   
Yes, that shows that the changesets prevent the oops, but it says
nothing about vanilla 2.6.20.y
> Winfast dongles are both dvb-usb based (DiBcom 3000M-C and DiBcom DiB7000P), 
> but pluto2 is cardbus (pci) based.
>   
just as I figured.  The pluto2 test results are great to hear, though --
thank you.
> I think we can include these patches into 2.6.21 and if we receive any 
> problem, we still have 2.6.21.Z for fixing, don't we?

The stable kernel series is not there for that purpose.  It is not there
to encourage a rush of patches into a final kernel release, only to
cause potential problems, with the 2.6.x.y series as a fallback for
fixes.  We should avoid the need for such last-minute fixes wherever
possible.

-- 
Michael Krufky

Re: [v4l-dvb-maintainer] Re: [GIT PATCHES] V4L/DVB updates

[Trent Piepho]

On Mon, 16 Apr 2007, Dmitry Torokhov wrote:
> Hi Mauro,
>
> On 4/15/07, Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> >   - Fix Kernel Bugzilla #8301: spinlock fix for flexcop-pci
>
> While move of spin_lock_init before request_irq is obviously correct I
> wonder what is the reason behind changing spin_lock_irq() into
> spin_lock_irqsave() as I do not see flexcop_pci_isr being called from
> anywhere but IRQ context.
>
> BTW, is irq_lock needed at all?

There was some more discussion on the linux-dvb list
http://www.linuxtv.org/pipermail/linux-dvb/2007-April/017024.html , and I
think we came to the conclusion that irq_lock isn't needed at all.  It does
nothing but serialize the ISR and ISRs are automatically serialized by the
kernel.

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[hermann pitton]

Am Montag, den 16.04.2007, 12:25 -0400 schrieb Michael Krufky:
> CIJOML wrote:
> > Dne pondělí 16 duben 2007 17:34 Michael Krufky napsal(a):
> >   
> >> Adrian Bunk wrote:
> >>     
> >>> On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
> >>>       
> >>>> Mauro,
> >>>>
> >>>> I've been out of town for the past few days... I just got home and saw
> >>>> this:
> >>>>
> >>>> Mauro Carvalho Chehab wrote:
> >>>>         
> >>>>>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> >>>>>    - Fix 2/3 for bug 7819: demux and dvr
> >>>>>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> >>>>>           
> >>>> I don't think that this is 2.6.21 material.  These patches have not yet
> >>>> received
> >>>> enough testing to be sent to mainline.
> >>>>
> >>>> I have tested them, and they seem to work for my cxusb device, but we
> >>>> have yet to hear test results from users of usb dvb devices that do not
> >>>> use the dvb-usb framework.  (ttusb, flexcop-usb, cinergyT2, for example)
> >>>>
> >>>> The bug that these patches fix has been around throughout the entire
> >>>> kernel history of the dvb subsystem.  The bug is not a regression -- it
> >>>> has always been
> >>>> there.  In my opinion, it is too late in 2.6.21 development to apply
> >>>> this change.
> >>>> Because these fixes are not obvious, I think we should let them get some
> >>>> more testing, and have them queued for 2.6.22 .
> >>>>         
> >>> Unless I misunderstand anything, this should fix [1].
> >>>
> >>> And this is a bug that was reported to be present in 2.6.21-rc but not
> >>> in 2.6.20 (and it's therefore a regression, no matter whether the
> >>> underlying problem was older and only exposed by some other change).
> >>>       
> >> Not true.  The DVB subsystem has NEVER been hot-unpluggable.  I confirm
> >> that the patches SEEM to be correct, but this has not yet been verified. 
> >> None of the authors of dvb-core gave their ACK on these changesets.
> >>
> >> The DVB hotplug issue has been around since the very beginning.  I assure
> >> you, that I consider this fix to be very important, and I really would love
> >> to see it hit mainline.  However, given the situation, it is not
> >> appropriate to push these in during -rc7
> >>
> >> I have doubts on CIJOML's testing method -- there is no way he could have
> >> unplugged the device while in use, while running 2.6.20.y and not receive
> >> an OOPS.  CIJOML, please see the bottom of this email for
> >>
> >> Sure, this will prevent an OOPS on some, and hopefully all devices...  but
> >> what if it causes a regression for those untested?
> >>
> >> Why do we have a merge window, if new changesets are going to be rushed
> >> into late -rc kernels without proper testing, and without the ack of a dvb
> >> subsystem maintainer?
> >>
> >> Are we prepared to go for another -rc and 3 or 4 weeks of testing to
> >> confirm that this fix doesn't cause new regressions?  I don't think so.
> >>
> >> Markus Rechberger wrote:
> >>     
> >>> The patch has been around on the dvb mailinglist ([PATCH][RFC] DVB
> >>> Hotplug Fix, 5. April 2007),
> >>>       
> >> The patch was merged into the development repository at the same time the
> >> pull request was issued to Linus.  This has NOT been tested on a wide
> >> scale.  It should go to -mm for a while before being merged to mainline.
> >>
> >> Mauro Carvalho Chehab wrote:
> >>     
> >>> I also explicitly warned at DVB ML that I were about to send this patch,
> >>> together with other fixes, asking the community for more tests. After
> >>> that, I received two positive answers on my mailbox from people that
> >>> tested and noticed that this really fixed the issue.
> >>>       
> >> One of those positive answers was me -  I explained that it worked for me,
> >> but we need others to test.
> >>
> >> You waited ONE DAY after sending this "warning" to the dvb mailing list?  (
> >> http://linuxtv.org/pipermail/linux-dvb/2007-April/017204.html ) I saw that
> >> email after seeing the pull request to Linus.  We dont have users testing
> >> the repositories after each commit -- you _really_ need to give some more
> >> time to allow for such testing.
> >>
> >> CIJOML wrote:
> >>     
> >>> Hi,
> >>>
> >>> I have tested these patches with:
> >>>
> >>> Freecom DVB-T dongle
> >>> Pluto2 pcmcia card
> >>> Leadtek WinFast DTV dongle 1st generation
> >>> Leadtek WinFast DTV dongle 2nd generation
> >>>
> >>> These are 4 different devices with 4 different hw and modules.
> >>> All works. Please apply.
> >>>       
> >> Well, that helps...  But it would still be nice to hear test results on a
> >> CinergyT2 or flexcop-usb.
> >>
> >> Which driver supports those Winfast dongles?  We already know for sure that
> >> the patches work correctly for any driver based on the dvb-usb framework.
> >>
> >> If you had the device open, and then disconnect it from the usb bus, no
> >> matter what kernel version you're running, you should hit the OOPS.  I
> >> confirm that these patches prevent that OOPS from occurring, but I have
> >> trouble believing that you did NOT experience such an OOPS in 2.6.20.y
> >>
> >> Could you please describe the method in which your test caused an OOPS
> >> using 2.6.21-rc and did NOT cause an oops in 2.6.20.y ?
> >>     
> >
> > Hi,
> >
> > I have tested these patches with 2.6.20-mh1 + v4l-dvb-b5be3479f070 patchset.
> > I also tried 2.6.21-rc6 + v4l-dvb-b5be3479f070 patchset and this combination 
> > also works without OOPS.
> >   
> Yes, that shows that the changesets prevent the oops, but it says
> nothing about vanilla 2.6.20.y
> > Winfast dongles are both dvb-usb based (DiBcom 3000M-C and DiBcom DiB7000P), 
> > but pluto2 is cardbus (pci) based.
> >   
> just as I figured.  The pluto2 test results are great to hear, though --
> thank you.
> > I think we can include these patches into 2.6.21 and if we receive any 
> > problem, we still have 2.6.21.Z for fixing, don't we?
> 
> The stable kernel series is not there for that purpose.  It is not there
> to encourage a rush of patches into a final kernel release, only to
> cause potential problems, with the 2.6.x.y series as a fallback for
> fixes.  We should avoid the need for such last-minute fixes wherever
> possible.
> 

Hi,

there is still this simple word missing.

This was/is the most annoying always ever reported bug I can remember of
through aeons.

             the simple word is

"thanks" Markus, for trying to fix it.

Cheers,
Hermann

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Mauro Carvalho Chehab]

> > I have tested these patches with 2.6.20-mh1 + v4l-dvb-b5be3479f070 patchset.
> > I also tried 2.6.21-rc6 + v4l-dvb-b5be3479f070 patchset and this combination 
> > also works without OOPS.
> >   
> Yes, that shows that the changesets prevent the oops, but it says
> nothing about vanilla 2.6.20.y
> > Winfast dongles are both dvb-usb based (DiBcom 3000M-C and DiBcom DiB7000P), 
> > but pluto2 is cardbus (pci) based.
> >   
> just as I figured.  The pluto2 test results are great to hear, though --
> thank you.
> > I think we can include these patches into 2.6.21 and if we receive any 
> > problem, we still have 2.6.21.Z for fixing, don't we?
> 
> The stable kernel series is not there for that purpose.  It is not there
> to encourage a rush of patches into a final kernel release, only to
> cause potential problems, with the 2.6.x.y series as a fallback for
> fixes.  We should avoid the need for such last-minute fixes wherever
> possible.
For sure we should do the best to avoid regressions. But, IMO, a driver
for a hotpluggable device (USB) that can't support device hot plug is a
serious issue. 

If nobody have an issue pointing regressions on this, we should really
apply the fix for 2.6.21.

-- 
Cheers,
Mauro

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Trent Piepho]

On Mon, 16 Apr 2007, Markus Rechberger wrote:
> On 4/16/07, Michael Krufky <mkrufky@linuxtv.org> wrote:
> > Adrian Bunk wrote:
> > > On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
> > >> Mauro,
> > >>
> > >> I've been out of town for the past few days... I just got home and saw
> > this:
> > >>
> > >>
> > >> Mauro Carvalho Chehab wrote:
> > >>>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> > >>>    - Fix 2/3 for bug 7819: demux and dvr
> > >>>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> > >> I don't think that this is 2.6.21 material.  These patches have not yet
> > >> received
> > >> enough testing to be sent to mainline.

I wish these patches had more comments about how they worked, because it's
not clear to me exactly what they are doing or why they do it.

Fix 3/3 for bug 7819: fixed hotplugging for dvbnet

I'm just going to talk about this one patch, since it appears to be the
simplest.

This patch removes the 'wait_queue' memory of dvb_demux.  Why does a patch
to dvbnet have anything to do with the demux device?  Should this change
have been part of the previous patch, the one what fixes demux and dvr?

The patch replace the (file_operations)->release pointer with the new
function dvb_net_close() in place of dvb_generic_release().  The first
part of dvb_net_close() is just a cut&paste from dvb_generic_release(),
so maybe it would be better to just call dvb_generic_release() instead?

There is also a bug:

+static int dvb_net_close(struct inode *inode, struct file *file)
+{
+       struct dvb_device *dvbdev = file->private_data;
+       struct dvb_net *dvbnet = dvbdev->priv;
+
+       if (!dvbdev)
+               return -ENODEV;

The check for !dvbdev is too late, dvbdev was already dereferenced on the
previous line to get dvbnet, so if dvbdev is NULL you will have already
crashed.

Is the check for !dvbdev unnecessary paranoia?  Or can dvbdev actually be
NULL is some cases?

So, w.r.t. dvbnet, my understanding is that somehow dvb_net_release() is
called while the net device is still in use.  Is that correct?
dvb_net_release() will end up deleting some stuff that needs to be around
while there is still a user of the device?

Would it be possible to keep dvb_net_release() from getting called in the
first place until there are no users of the device?

When looking at this code, keep in mind that users starts at 1 and goes
down as the device is used.

void dvb_net_release (struct dvb_net *dvbnet)
{
        int i;

        dvbnet->exit = 1;
        if (dvbnet->dvbdev->users < 1)		/* line A */
                wait_event(dvbnet->dvbdev->wait_queue,  /* line B */
                                dvbnet->dvbdev->users==1);
	/* line C */
        dvb_unregister_device(dvbnet->dvbdev);
	[...]
}

Isn't there a race condition here?  What if there are no users of the
device at the time line A runs, but the device is opened before line C
runs?  After the wait_even() on line B returns there are no users, but what
if new users opens the device after the after line B?

Is seems like you need something to prevent new users from opening the
device, i.e. make it so that dvbnet->exit=1 prevents the device from
getting opened, but if that's the intention, I don't see where it's
happening.

In dvb_net_close():

+       if(dvbdev->users == 1 && dvbnet->exit==1) {
+               fops_put(file->f_op);
+               file->f_op = NULL;
+               wake_up(&dvbdev->wait_queue);
+       }

What is the purpose of the fops_put() call and setting file->f_op to NULL?

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Markus Rechberger]

On 4/27/07, Trent Piepho <xyzzy@speakeasy.org> wrote:
> On Mon, 16 Apr 2007, Markus Rechberger wrote:
> > On 4/16/07, Michael Krufky <mkrufky@linuxtv.org> wrote:
> > > Adrian Bunk wrote:
> > > > On Sun, Apr 15, 2007 at 08:33:38PM -0400, Michael Krufky wrote:
> > > >> Mauro,
> > > >>
> > > >> I've been out of town for the past few days... I just got home and
> saw
> > > this:
> > > >>
> > > >>
> > > >> Mauro Carvalho Chehab wrote:
> > > >>>    - Fix 1/3 for bug 7819: fixed frontend hotplug issue
> > > >>>    - Fix 2/3 for bug 7819: demux and dvr
> > > >>>    - Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> > > >> I don't think that this is 2.6.21 material.  These patches have not
> yet
> > > >> received
> > > >> enough testing to be sent to mainline.
>
> I wish these patches had more comments about how they worked, because it's
> not clear to me exactly what they are doing or why they do it.
>
> Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
>
> I'm just going to talk about this one patch, since it appears to be the
> simplest.
>
> This patch removes the 'wait_queue' memory of dvb_demux.  Why does a patch
> to dvbnet have anything to do with the demux device?  Should this change
> have been part of the previous patch, the one what fixes demux and dvr?
>

The waitqueue got added in a previous patch just ignore it..

> The patch replace the (file_operations)->release pointer with the new
> function dvb_net_close() in place of dvb_generic_release().  The first
> part of dvb_net_close() is just a cut&paste from dvb_generic_release(),
> so maybe it would be better to just call dvb_generic_release() instead?
>

no because it needs the dvb_net structure which cannot be used in the
dvb_generic_release function.

> There is also a bug:
>
> +static int dvb_net_close(struct inode *inode, struct file *file)
> +{
> +       struct dvb_device *dvbdev = file->private_data;
> +       struct dvb_net *dvbnet = dvbdev->priv;
> +
> +       if (!dvbdev)
> +               return -ENODEV;
>
> The check for !dvbdev is too late, dvbdev was already dereferenced on the
> previous line to get dvbnet, so if dvbdev is NULL you will have already
> crashed.
>

I think this check isn't needed at all actually, why should someone
release dvbdev before already?

> Is the check for !dvbdev unnecessary paranoia?  Or can dvbdev actually be
> NULL is some cases?
>
> So, w.r.t. dvbnet, my understanding is that somehow dvb_net_release() is
> called while the net device is still in use.  Is that correct?
> dvb_net_release() will end up deleting some stuff that needs to be around
> while there is still a user of the device?
>
> Would it be possible to keep dvb_net_release() from getting called in the
> first place until there are no users of the device?
>
> When looking at this code, keep in mind that users starts at 1 and goes
> down as the device is used.
>
> void dvb_net_release (struct dvb_net *dvbnet)
> {
>         int i;
>
>         dvbnet->exit = 1;
>         if (dvbnet->dvbdev->users < 1)		/* line A */
>                 wait_event(dvbnet->dvbdev->wait_queue,  /* line B */
>                                 dvbnet->dvbdev->users==1);
> 	/* line C */
>         dvb_unregister_device(dvbnet->dvbdev);
> 	[...]
> }
>
> Isn't there a race condition here?  What if there are no users of the
> device at the time line A runs, but the device is opened before line C
> runs?  After the wait_even() on line B returns there are no users, but what
> if new users opens the device after the after line B?
>

yes this is a valid argument, practically it will never occure though.
Since I also wrote that I didn't have a dvb signal at the time of
writing this I don't even know if the dvb-net implementation is ok at
all.
I don't even know if that part of the framework works and if someone
really uses it.

> Is seems like you need something to prevent new users from opening the
> device, i.e. make it so that dvbnet->exit=1 prevents the device from
> getting opened, but if that's the intention, I don't see where it's
> happening.
>
> In dvb_net_close():
>
> +       if(dvbdev->users == 1 && dvbnet->exit==1) {
> +               fops_put(file->f_op);
> +               file->f_op = NULL;
> +               wake_up(&dvbdev->wait_queue);
> +       }
>
> What is the purpose of the fops_put() call and setting file->f_op to NULL?
>

fops_put lowers the usecount on the inode, after wake_up the f_op
structure is invalid because it's just allocated memory which gets
freed by the dvb release function.

thanks for reviewing,
Markus

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Trent Piepho]

On Sat, 28 Apr 2007, Markus Rechberger wrote:
> On 4/27/07, Trent Piepho <xyzzy@speakeasy.org> wrote:
> > On Mon, 16 Apr 2007, Markus Rechberger wrote:
> > > > >> enough testing to be sent to mainline.
> >
> > I wish these patches had more comments about how they worked, because it's
> > not clear to me exactly what they are doing or why they do it.
> >
> > Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> >
> > I'm just going to talk about this one patch, since it appears to be the
> > simplest.
> >
> > This patch removes the 'wait_queue' memory of dvb_demux.  Why does a patch
> > to dvbnet have anything to do with the demux device?  Should this change
> > have been part of the previous patch, the one what fixes demux and dvr?
> >
>
> The waitqueue got added in a previous patch just ignore it..

Do the patches need to be applied all together?  If I apply just the first
two, do I get something that's broken?

> > The patch replace the (file_operations)->release pointer with the new
> > function dvb_net_close() in place of dvb_generic_release().  The first
> > part of dvb_net_close() is just a cut&paste from dvb_generic_release(),
> > so maybe it would be better to just call dvb_generic_release() instead?
> >
>
> no because it needs the dvb_net structure which cannot be used in the
> dvb_generic_release function.

I mean, have dvb_net_close() call dvb_generic_release() and then do the
extra stuff.

> > There is also a bug:
> >
> > +static int dvb_net_close(struct inode *inode, struct file *file)
> > +{
> > +       struct dvb_device *dvbdev = file->private_data;
> > +       struct dvb_net *dvbnet = dvbdev->priv;
> > +
> > +       if (!dvbdev)
> > +               return -ENODEV;
> >
> > The check for !dvbdev is too late, dvbdev was already dereferenced on the
> > previous line to get dvbnet, so if dvbdev is NULL you will have already
> > crashed.
> >
>
> I think this check isn't needed at all actually, why should someone
> release dvbdev before already?
>
> > Is the check for !dvbdev unnecessary paranoia?  Or can dvbdev actually be
> > NULL is some cases?
> >
> > So, w.r.t. dvbnet, my understanding is that somehow dvb_net_release() is
> > called while the net device is still in use.  Is that correct?
> > dvb_net_release() will end up deleting some stuff that needs to be around
> > while there is still a user of the device?
> >
> > Would it be possible to keep dvb_net_release() from getting called in the
> > first place until there are no users of the device?

What about this?  I don't have any hot pluggable hardware, so how does
dvb_net_release() get called when the device disconnects?

> > When looking at this code, keep in mind that users starts at 1 and goes
> > down as the device is used.
> >
> > void dvb_net_release (struct dvb_net *dvbnet)
> > {
> >         int i;
> >
> >         dvbnet->exit = 1;
> >         if (dvbnet->dvbdev->users < 1)		/* line A */
> >                 wait_event(dvbnet->dvbdev->wait_queue,  /* line B */
> >                                 dvbnet->dvbdev->users==1);
> > 	/* line C */
> >         dvb_unregister_device(dvbnet->dvbdev);
> > 	[...]
> > }
> >
> > Isn't there a race condition here?  What if there are no users of the
> > device at the time line A runs, but the device is opened before line C
> > runs?  After the wait_even() on line B returns there are no users, but what
> > if new users opens the device after the after line B?
> >
>
> yes this is a valid argument, practically it will never occure though.
> Since I also wrote that I didn't have a dvb signal at the time of
> writing this I don't even know if the dvb-net implementation is ok at
> all.
> I don't even know if that part of the framework works and if someone
> really uses it.
>
> > Is seems like you need something to prevent new users from opening the
> > device, i.e. make it so that dvbnet->exit=1 prevents the device from
> > getting opened, but if that's the intention, I don't see where it's
> > happening.
> >
> > In dvb_net_close():
> >
> > +       if(dvbdev->users == 1 && dvbnet->exit==1) {
> > +               fops_put(file->f_op);
> > +               file->f_op = NULL;
> > +               wake_up(&dvbdev->wait_queue);
> > +       }
> >
> > What is the purpose of the fops_put() call and setting file->f_op to NULL?
> >
>
> fops_put lowers the usecount on the inode, after wake_up the f_op
> structure is invalid because it's just allocated memory which gets
> freed by the dvb release function.

fopts_put() lowers the usecount of the module that owns the inode.  I don't
see how it works to add this when it wasn't here before?  Isn't it going to
mess up the module usecount by doing a fopts_put() that's not matched by a
fopts_get()?  I didn't see anything in your patch that added a fops_put().

Re: [v4l-dvb-maintainer] [GIT PATCHES] V4L/DVB updates

[Markus Rechberger]

On 4/28/07, Trent Piepho <xyzzy@speakeasy.org> wrote:
> On Sat, 28 Apr 2007, Markus Rechberger wrote:
> > On 4/27/07, Trent Piepho <xyzzy@speakeasy.org> wrote:
> > > On Mon, 16 Apr 2007, Markus Rechberger wrote:
> > > > > >> enough testing to be sent to mainline.
> > >
> > > I wish these patches had more comments about how they worked, because
> it's
> > > not clear to me exactly what they are doing or why they do it.
> > >
> > > Fix 3/3 for bug 7819: fixed hotplugging for dvbnet
> > >
> > > I'm just going to talk about this one patch, since it appears to be the
> > > simplest.
> > >
> > > This patch removes the 'wait_queue' memory of dvb_demux.  Why does a
> patch
> > > to dvbnet have anything to do with the demux device?  Should this change
> > > have been part of the previous patch, the one what fixes demux and dvr?
> > >
> >
> > The waitqueue got added in a previous patch just ignore it..
>
> Do the patches need to be applied all together?  If I apply just the first
> two, do I get something that's broken?
>

yes very likely only because of this wait_queue..

> > > The patch replace the (file_operations)->release pointer with the new
> > > function dvb_net_close() in place of dvb_generic_release().  The first
> > > part of dvb_net_close() is just a cut&paste from dvb_generic_release(),
> > > so maybe it would be better to just call dvb_generic_release() instead?
> > >
> >
> > no because it needs the dvb_net structure which cannot be used in the
> > dvb_generic_release function.
>
> I mean, have dvb_net_close() call dvb_generic_release() and then do the
> extra stuff.
>
dvb_generic_release is really generic and has no information about the
dvb net structure if it's used for eg. the frontend

for example the dvb_frontend release function:
static int dvb_frontend_release(struct inode *inode, struct file *file)
{
        struct dvb_device *dvbdev = file->private_data;
        struct dvb_frontend *fe = dvbdev->priv;
        struct dvb_frontend_private *fepriv = fe->frontend_priv;

it would be possible to use dvb_release_generic if there wouldn't be
any reference to dvb_frontend and dvb_frontend_private.
In case of dvb-net it's a dvb_net specific release function by adding
the dvb_net structure.

If you look at dvb_generic_release it only uses dvb_device which is
really generic and used by all the dvb modules (frontend/dmx/..)

> > > There is also a bug:
> > >
> > > +static int dvb_net_close(struct inode *inode, struct file *file)
> > > +{
> > > +       struct dvb_device *dvbdev = file->private_data;
> > > +       struct dvb_net *dvbnet = dvbdev->priv;
> > > +
> > > +       if (!dvbdev)
> > > +               return -ENODEV;
> > >
> > > The check for !dvbdev is too late, dvbdev was already dereferenced on
> the
> > > previous line to get dvbnet, so if dvbdev is NULL you will have already
> > > crashed.
> > >
> >
> > I think this check isn't needed at all actually, why should someone
> > release dvbdev before already?
> >
> > > Is the check for !dvbdev unnecessary paranoia?  Or can dvbdev actually
> be
> > > NULL is some cases?
> > >
> > > So, w.r.t. dvbnet, my understanding is that somehow dvb_net_release() is
> > > called while the net device is still in use.  Is that correct?
> > > dvb_net_release() will end up deleting some stuff that needs to be
> around
> > > while there is still a user of the device?
> > >
> > > Would it be possible to keep dvb_net_release() from getting called in
> the
> > > first place until there are no users of the device?
>
> What about this?  I don't have any hot pluggable hardware, so how does
> dvb_net_release() get called when the device disconnects?
>

I don't understand, it's inode based if you open the file it will also
release the file if you close it.
If it's not hotpluggable it will not use the wait_queue because the
usecount would be 0 already when unloading the module.

> > > When looking at this code, keep in mind that users starts at 1 and goes
> > > down as the device is used.
> > >
> > > void dvb_net_release (struct dvb_net *dvbnet)
> > > {
> > >         int i;
> > >
> > >         dvbnet->exit = 1;
> > >         if (dvbnet->dvbdev->users < 1)		/* line A */
> > >                 wait_event(dvbnet->dvbdev->wait_queue,  /* line B */
> > >                                 dvbnet->dvbdev->users==1);
> > > 	/* line C */
> > >         dvb_unregister_device(dvbnet->dvbdev);
> > > 	[...]
> > > }
> > >
> > > Isn't there a race condition here?  What if there are no users of the
> > > device at the time line A runs, but the device is opened before line C
> > > runs?  After the wait_even() on line B returns there are no users, but
> what
> > > if new users opens the device after the after line B?
> > >
> >
> > yes this is a valid argument, practically it will never occure though.
> > Since I also wrote that I didn't have a dvb signal at the time of
> > writing this I don't even know if the dvb-net implementation is ok at
> > all.
> > I don't even know if that part of the framework works and if someone
> > really uses it.
> >
> > > Is seems like you need something to prevent new users from opening the
> > > device, i.e. make it so that dvbnet->exit=1 prevents the device from
> > > getting opened, but if that's the intention, I don't see where it's
> > > happening.
> > >
> > > In dvb_net_close():
> > >
> > > +       if(dvbdev->users == 1 && dvbnet->exit==1) {
> > > +               fops_put(file->f_op);
> > > +               file->f_op = NULL;
> > > +               wake_up(&dvbdev->wait_queue);
> > > +       }
> > >
> > > What is the purpose of the fops_put() call and setting file->f_op to
> NULL?
> > >
> >
> > fops_put lowers the usecount on the inode, after wake_up the f_op
> > structure is invalid because it's just allocated memory which gets
> > freed by the dvb release function.
>
> fopts_put() lowers the usecount of the module that owns the inode.  I don't
> see how it works to add this when it wasn't here before?  Isn't it going to
> mess up the module usecount by doing a fopts_put() that's not matched by a
> fopts_get()?  I didn't see anything in your patch that added a fops_put().
>

fops_put is nullpointer safe, the close() function remember the
allocated memory is gone after the release function deallocates the
memory.
What other module should own a usecount of it?

If you look at file_table.c:
        if (file->f_op && file->f_op->release)
                file->f_op->release(inode, file);
        security_file_free(file);
        if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL))
                cdev_put(inode->i_cdev);
        fops_put(file->f_op); <- this would be invalid in our case
because the f_op structure gets deallocated earlier already so setting
it to NULL would prevent to lower the usecount.

first it calls file->f_op->release (our close/release function)
in the close/release function, while there's still a user the fops
deallocation is on hold.
As soon as the wake up function gets called the process will very
likely switch to the waked up procedure and it will immediatelly free
the f_ops structure. After cycling back to the close function we have
invalid memory and an invalid f_ops pointer.

Markus