V4L/DVB updates

- 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

Mauro,

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

Quoted 3 lines · Mauro Carvalho Chehab

- 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 .

Quoted 3 lines · Michael Krufky

Mauro,

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

this:

Quoted 10 lines

Quoted 3 lines · Mauro Carvalho Chehab

- 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.

Quoted 5 lines

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

Quoted 6 lines

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.

Quoted 32 lines

Quoted 3 lines · Michael Krufky

Mauro,

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

this:

Quoted 10 lines

Quoted 3 lines · Mauro Carvalho Chehab

- 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.

Quoted 5 lines

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

Quoted 6 lines

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.

Quoted 7 lines

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.

Mauro,

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

Quoted 3 lines · Mauro Carvalho Chehab

- 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

Quoted 18 lines

Mauro,

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

Quoted 3 lines · Mauro Carvalho Chehab

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

Quoted 7 lines

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).

Quoted 3 lines

Regards,

Michael

cu
Adrian

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

Quoted 18 lines

Mauro,

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

Quoted 3 lines · Mauro Carvalho Chehab

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

Quoted 7 lines

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).

Quoted 3 lines

Regards,

Michael

cu
Adrian

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

Quoted 17 lines · Michael Krufky

Mauro,

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

Quoted 3 lines · Mauro Carvalho Chehab

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

Quoted 7 lines

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).

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

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.

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.

Quoted 4 lines · Adrian Bunk

Quoted 3 lines · Michael Krufky

Mauro,

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

this:

Quoted 11 lines

Quoted 11 lines

Quoted 3 lines · Mauro Carvalho Chehab

- 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

Quoted 1 line

Quoted 1 line

yet to hear test results from users of usb dvb devices that do not use

the

Quoted 3 lines

Quoted 3 lines

dvb-usb framework. (ttusb, flexcop-usb, cinergyT2, for example)

The bug that these patches fix has been around throughout the entire

kernel

Quoted 7 lines

Quoted 6 lines

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 .

Quoted 5 lines

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.

Quoted 2 lines · Markus Rechberger

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.

Quoted 4 lines · Mauro Carvalho Chehab

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.

Quoted 11 lines · 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.

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
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/v4l-dvb-maintainer

Quoted 8 lines · Michael Krufky

Quoted 4 lines · Adrian Bunk

Quoted 3 lines · Michael Krufky

Mauro,

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

this:

Quoted 2 lines

Quoted 2 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines · Mauro Carvalho Chehab

- 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.

Quoted 6 lines · Markus Rechberger

Quoted 5 lines · Michael Krufky

Quoted 4 lines · Adrian Bunk

Quoted 3 lines · Michael Krufky

Mauro,

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

saw

Quoted 8 lines

Quoted 8 lines

this:

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines · Mauro Carvalho Chehab

- 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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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:

(quoted diff) +7 -0

+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():

(quoted diff) +5 -0

+       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?

Quoted 11 lines · Trent Piepho

Quoted 1 line · Markus Rechberger

Quoted 1 line

Quoted 1 line

Quoted 1 line

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.

Quoted 4 lines

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..

Quoted 5 lines

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.

Quoted 10 lines

There is also a bug:

(quoted diff) +7 -0

+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;

Quoted 4 lines

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?

Quoted 10 lines

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?

Quoted 16 lines

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);
	[...]
}

Quoted 5 lines

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.

Quoted 13 lines

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

(quoted diff) +5 -0

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

Quoted 2 lines

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.

Quoted 5 lines · Markus Rechberger

Quoted 4 lines · Trent Piepho

Quoted 1 line · Markus Rechberger

Quoted 1 line

Quoted 1 line

Quoted 1 line

enough testing to be sent to mainline.

I wish these patches had more comments about how they worked, because

it's

Quoted 8 lines

Quoted 8 lines

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

Quoted 5 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 5 lines

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?

Quoted 2 lines

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.

Quoted 11 lines

Quoted 11 lines

There is also a bug:

(quoted diff) +7 -0

+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

Quoted 8 lines

Quoted 3 lines

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?

Quoted 1 line

Is the check for !dvbdev unnecessary paranoia? Or can dvbdev actually

be

Quoted 5 lines

Quoted 5 lines

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

Quoted 3 lines

Quoted 3 lines

while there is still a user of the device?

Would it be possible to keep dvb_net_release() from getting called in

the

Quoted 1 line

Quoted 1 line

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?

Quoted 16 lines

Quoted 16 lines

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);
	[...]
}

Quoted 3 lines

Quoted 3 lines

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

Quoted 24 lines

Quoted 2 lines

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.

Quoted 14 lines

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

(quoted diff) +5 -0

+       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?

Quoted 2 lines

Quoted 1 line

Quoted 3 lines

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().

Quoted 27 lines · Adrian Bunk

Quoted 18 lines · Michael Krufky

Mauro,

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

Quoted 3 lines · Mauro Carvalho Chehab

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

Quoted 7 lines

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 .

Quoted 5 lines

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.

Quoted 2 lines · Markus Rechberger

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.

Quoted 4 lines · Mauro Carvalho Chehab

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.

Quoted 11 lines · 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.

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 ?

Dne pondělí 16 duben 2007 17:34 Michael Krufky napsal(a):

Quoted 105 lines

Quoted 29 lines · Adrian Bunk

Quoted 19 lines · Michael Krufky

Mauro,

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

Quoted 4 lines · Mauro Carvalho Chehab

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

Quoted 8 lines

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 .

Quoted 6 lines

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.

Quoted 3 lines · Markus Rechberger

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.

Quoted 5 lines · Mauro Carvalho Chehab

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.

Quoted 4 lines · CIJOML

Hi,

I have tested these patches with:

Quoted 8 lines

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.

Quoted 8 lines

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?

Quoted 116 lines · CIJOML

Dne pondělí 16 duben 2007 17:34 Michael Krufky napsal(a):

Quoted 105 lines

Quoted 29 lines · Adrian Bunk

Quoted 19 lines · Michael Krufky

Mauro,

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

Quoted 4 lines · Mauro Carvalho Chehab

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

Quoted 8 lines

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 .

Quoted 6 lines

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.

Quoted 3 lines · Markus Rechberger

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.

Quoted 5 lines · Mauro Carvalho Chehab

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.

Quoted 4 lines · CIJOML

Hi,

I have tested these patches with:

Quoted 8 lines

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.

Quoted 8 lines

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 ?

Quoted 6 lines

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

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 3 lines

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.

Quoted 2 lines

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.

- Fix Kernel Bugzilla #8301: spinlock fix for flexcop-pci

Hi Mauro,

Quoted 1 line · Mauro Carvalho Chehab

- 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?