[Ndiswrapper-general] Re: rmmod oops final patch

[Giridhar Pemmasani <giri@lm...>]

On Thu, 1 Jan 2004 18:19:31 +0100, Tomas Szepe <szepe@...> said:

    Tomas> It seems to me that your doing of a) has somehow resolved
    Tomas> b), which frankly I don't understand.

Third possibility is that the code is still not correct! I have gone
through NDIS documentation on this, but it is not easy to understand
it. So for now I am going to leave the code as it is, until someone
complains it or if we understand it better.

    Tomas> I have not seen the posts, probably because those people
    Tomas> replied off-list, which is unfortunate; All such posts need
    Tomas> to go to the mailing list.

Yes, as pointed out earlier, I don't always use the mailing list
(probably not a good idea); I am a bit worried that people may not
want to get all the mails.

    Tomas> But the interface might not be downed before rmmod is
    Tomas> called (yes, this can be done, at least in 2.6.x), and
    Tomas> that's why I believe your reversal of call_halt() and
    Tomas> unregister_netdev() is wrong.

Will look at it.

Thanks again for your testing and patches.

BTW, fix for 'timer added twice' bug is in cvs now. With this, all
major bugs/problems are resolved. If you have complained about a
bug/problem before that is not resolved in cvs, post again.

-- 
Giri

[Ndiswrapper-general] rmmod oops final patch

[Tomas Szepe <szepe@pi...>]

Hi,

I've thought things over again and have concluded that the the NDIS drivers
that we're seeing the rmmod OOPS with are broken, as they ought to disable
the IRQ on the device prior to calling NdisMDeregisterInterrupt().

The idea I had with calling a function within the NDIS driver that would
do this explicitly was me faulting w/ -ENOCAFFEINE again, forget it.

Anyway, here's my final patch that solves the issue, please apply.

It is against current CVS and I must say I'm very disappointed that
I had to rediff, because you've applied your untested version of the
fix in the meantime.

-- 
Tomas Szepe <szepe@...>


diff -urN ndiswrapper.cvs/driver/ndis.c ndiswrapper/driver/ndis.c
--- ndiswrapper.cvs/driver/ndis.c	2004-01-01 15:06:40.000000000 +0100
+++ ndiswrapper/driver/ndis.c	2004-01-01 15:21:56.000000000 +0100
@@ -907,17 +907,30 @@
 	DBGTRACE("%s sp:%08x\n", __FUNCTION__ , getSp());
 }
 
+/* the following flag is set when ndiswrapper is being removed from the
+ * kernel so as to prevent servicing bh interrupts w/ broken NDIS drivers
+ * that will call DeregisterInterrupt w/o actually disabling the interrupt
+ * on the device.	-- Tomas Szepe <szepe@...>, January 2004
+ */
+static int ndiswrp_being_unloaded;	/* implicitly initialized to 0 */
+static int broken_ndis_driver;		/* implicitly initialized to 0 */
 
-static int ndis_irq_enabled;
 /*
  *  bottom half of the irq handler
  *
  */
 void ndis_irq_bh(void *data)
 {
-	struct ndis_handle *handle = (struct ndis_handle *) data;
-	if (ndis_irq_enabled)
-		handle->driver->miniport_char.handle_interrupt(handle->adapter_ctx);
+	struct ndis_handle *handle;
+
+	if(ndiswrp_being_unloaded)
+	{
+		broken_ndis_driver = 1;
+		return;
+	}
+
+	handle = (struct ndis_handle *) data;
+	handle->driver->miniport_char.handle_interrupt(handle->adapter_ctx);
 }
 
 /*
@@ -980,7 +993,6 @@
 		kfree(ndis_irq);
 		return NDIS_STATUS_FAILURE;
 	}
-	ndis_irq_enabled = 1;
 	INIT_WORK(&handle->irq_bh, &ndis_irq_bh, handle);
 	return NDIS_STATUS_SUCCESS;
 }
@@ -997,10 +1009,21 @@
 
 	if(ndis_irq)
 	{
-		ndis_irq_enabled = 0;
+		/* work around broken NDIS drivers: bh part */
+		ndiswrp_being_unloaded = 1;
+
 		free_irq(ndis_irq->irq, ndis_irq);
 		kfree(ndis_irq);
+
+		/* work around broken NDIS drivers: th part */
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		schedule_timeout(HZ >> 2);
 	}
+
+	if(broken_ndis_driver)
+		printk("ndiswrapper: warning: your NDIS driver"
+			" calls NdisMDeregisterInterrupt() without"
+			" first disabling IRQ on the device.\n");
 }
 
 
diff -urN ndiswrapper.cvs/driver/wrapper.c ndiswrapper/driver/wrapper.c
--- ndiswrapper.cvs/driver/wrapper.c	2004-01-01 15:06:40.000000000 +0100
+++ ndiswrapper/driver/wrapper.c	2004-01-01 15:24:45.000000000 +0100
@@ -1753,14 +1753,16 @@
 	ndis_remove_proc(handle);
 
 #ifndef DEBUG_CRASH_ON_INIT
-	set_int(handle, NDIS_OID_DISASSOCIATE, 0);
-	call_halt(handle);
-	unregister_netdev(handle->net_dev);
-
-	if(handle->net_dev)
+	if(handle->net_dev)
+	{
+		unregister_netdev(handle->net_dev);
 		free_netdev(handle->net_dev);
+	}
 
+	set_int(handle, NDIS_OID_DISASSOCIATE, 0);
+	call_halt(handle);	/* this call frees the associated IRQ */
 #endif
+
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 	pci_set_drvdata(pdev, NULL);

[Ndiswrapper-general] Re: rmmod oops final patch

[Giridhar Pemmasani <giri@lm...>]

Have you tested the current cvs code and still experienced oops? 

I am sorry you are very disappointed, but your assumption that I put
in untested code is wrong; two people who have complained earlier
about rmmor oops have confirmed that the patch doesn't oops anymore.

BTW, I rediff all the time!

As for the analysis you posted in earlier posting: ndis_close is
called when the interface goes down, which stops the queue. So by the
time ndis_remove_one is called, the queue is already stopped.

Giri

[Ndiswrapper-general] Re: rmmod oops final patch

[Tomas Szepe <szepe@pi...>]

On Jan-01 2004, Thu, 11:55 -0500
Giridhar Pemmasani <giri@...> wrote:

> Have you tested the current cvs code and still experienced oops? 

Yes, tested, and no OOPS for me either, but the current code

a) reverses the calls to call_halt() and unregister_netdev(),
	which I believe is wrong.

b) does not seem to handle the case when the top-half interrupt routine
	is invoked between the time we free the irq to the OS and return
	to the NDIS driver.

c) can call unregister_netdev(handle->net_dev) when handle->net_dev is NULL

It seems to me that your doing of a) has somehow resolved b), which
frankly I don't understand.

> I am sorry you are very disappointed, but your assumption that I put
> in untested code is wrong; two people who have complained earlier
> about rmmor oops have confirmed that the patch doesn't oops anymore.

I have not seen the posts, probably because those people replied
off-list, which is unfortunate;  All such posts need to go to the
mailing list.

> As for the analysis you posted in earlier posting: ndis_close is
> called when the interface goes down, which stops the queue. So by
> the time ndis_remove_one is called, the queue is already stopped.

But the interface might not be downed before rmmod is called (yes,
this can be done, at least in 2.6.x), and that's why I believe your
reversal of call_halt() and unregister_netdev() is wrong.

-- 
Tomas Szepe <szepe@...>

[Ndiswrapper-general] Re: rmmod oops final patch

[Giridhar Pemmasani <giri@lm...>]

On Thu, 1 Jan 2004 18:19:31 +0100, Tomas Szepe <szepe@...> said:

    Tomas> It seems to me that your doing of a) has somehow resolved
    Tomas> b), which frankly I don't understand.

Third possibility is that the code is still not correct! I have gone
through NDIS documentation on this, but it is not easy to understand
it. So for now I am going to leave the code as it is, until someone
complains it or if we understand it better.

    Tomas> I have not seen the posts, probably because those people
    Tomas> replied off-list, which is unfortunate; All such posts need
    Tomas> to go to the mailing list.

Yes, as pointed out earlier, I don't always use the mailing list
(probably not a good idea); I am a bit worried that people may not
want to get all the mails.

    Tomas> But the interface might not be downed before rmmod is
    Tomas> called (yes, this can be done, at least in 2.6.x), and
    Tomas> that's why I believe your reversal of call_halt() and
    Tomas> unregister_netdev() is wrong.

Will look at it.

Thanks again for your testing and patches.

BTW, fix for 'timer added twice' bug is in cvs now. With this, all
major bugs/problems are resolved. If you have complained about a
bug/problem before that is not resolved in cvs, post again.

-- 
Giri

Re: [Ndiswrapper-general] Re: rmmod oops final patch

[Tomas Szepe <szepe@pi...>]

On Jan-01 2004, Thu, 12:50 -0500
Giridhar Pemmasani <giri@...> wrote:

> On Thu, 1 Jan 2004 18:19:31 +0100, Tomas Szepe <szepe@...> said:
> 
>     Tomas> It seems to me that your doing of a) has somehow resolved
>     Tomas> b), which frankly I don't understand.
> 
> Third possibility is that the code is still not correct! I have gone
> through NDIS documentation on this, but it is not easy to understand
> it. So for now I am going to leave the code as it is, until someone
> complains it or if we understand it better.

Ok.  I will still play with the code and may report back if I find
something interesting.

> Thanks again for your testing and patches.

No problem.

> BTW, fix for 'timer added twice' bug is in cvs now. With this, all
> major bugs/problems are resolved.

Great!

-- 
Tomas Szepe <szepe@...>

Re: [Ndiswrapper-general] Re: rmmod oops final patch

[Olivier Bornet <Olivier.Bornet@pu...>]

Hello,

On Thu, Jan 01, 2004 at 12:50:38PM -0500, Giridhar Pemmasani wrote:
>     Tomas> I have not seen the posts, probably because those people
>     Tomas> replied off-list, which is unfortunate; All such posts need
>     Tomas> to go to the mailing list.
>=20
> Yes, as pointed out earlier, I don't always use the mailing list
> (probably not a good idea); I am a bit worried that people may not
> want to get all the mails.

The solution is maybe to have two mailing list: the current one for
general discution (user-oriented), another one for developers
(ndiswrapper-devel). And all the discution for developers will go in
this list.

This is what many projects have, like BlueZ for example.

Good day.

		Olivier
--=20
Olivier Bornet                 |      fran=E7ais : http://puck.ch/f
Swiss Ice Hockey Results       |      english  : http://puck.ch/e
http://puck.ch/                |      deutsch  : http://puck.ch/g
Olivier.Bornet@...         |      italiano : http://puck.ch/i
Get my PGP-key at http://puck.ch/pgp or at http://wwwkeys.pgp.net

[Ndiswrapper-general] Re: rmmod oops final patch

[Giridhar Pemmasani <giri@lm...>]

How about following patch, that will take care of issues you
mentioned? If you are okay with it, I will commit:

Re: [Ndiswrapper-general] Re: rmmod oops final patch

[Tomas Szepe <szepe@pi...>]

Giridhar Pemmasani <giri@...> wrote:

> How about following patch, that will take care of issues you
> mentioned? If you are okay with it, I will commit:

Well, this patch actually reintroduces the OOPS. :)

I'm starting to believe that the call of call_halt() actually belongs
before the call of unregister_netdev(), but I'm incapable of offering
an explanation why.  Do you have any idea what the culprit could be?

You see, if we call call_halt() (that is, the function that will
result in the IRQ being freed for the OS) after unregister_netdev(),
some drivers will invoke an IRQ after the driver has been freed,
resulting in the kernel calling an already-freed IRQ handler.

I'll keep poking around this, in the meantime, please apply just the
following bit.

-- 
Tomas Szepe <szepe@...>


diff -urN a/driver/wrapper.c ndiswrapper/driver/wrapper.c
--- a/driver/wrapper.c	2004-01-01 15:05:37.000000000 +0100
+++ ndiswrapper/driver/wrapper.c	2004-01-01 22:09:09.000000000 +0100
@@ -1755,11 +1755,12 @@
 #ifndef DEBUG_CRASH_ON_INIT
 	set_int(handle, NDIS_OID_DISASSOCIATE, 0);
 	call_halt(handle);
-	unregister_netdev(handle->net_dev);
 
 	if(handle->net_dev)
+	{
+		unregister_netdev(handle->net_dev);
 		free_netdev(handle->net_dev);
-
+	}
 #endif
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);

Re: [Ndiswrapper-general] Re: rmmod oops final patch

[Giridhar Pemmasani <giri@lm...>]

The patch is committed. 

Thanks,

-- 
Giri