Home » Mailing list archives » linux-kernel » 2008 » September » 13

Re: [PATCH 2/2] ne.c Fix suspend and resume for ISA PnP cards.

Previous thread: reiserfs do_journal_end unnecessary hd wake up? by Bráulio Barros de Oliveira on Wednesday, September 3, 2008 - 7:56 pm. (1 message)

Next thread: [PATCH] bq27x00_battery: use unaligned access helper by Harvey Harrison on Wednesday, September 3, 2008 - 8:39 pm. (1 message)
[view original message]
From: David Fries
Subject: [PATCH] ne.c fix for hibernate and rmmod oops fix
Date: Wednesday, September 3, 2008 - 8:02 pm

Andrew Morton, 

This patch is ready to be merged.



This ne2000 and the previous cr4 problem found when enabling suspend to
disk on my i486 with a PnP ISA NE2000 and Creative SB Live in the
trunk of my car.  It is providing in car podcast entertainment.  Must
be a unique setup.

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)


From c2307cd616ccbe4b8eb1bb65e46aea11da7787a2 Mon Sep 17 00:00:00 2001
Message-Id: <c2307cd616ccbe4b8eb1bb65e46aea11da7787a2.1220402838.git.david@fries.net>
From: David Fries <david@fries.net>
Date: Wed, 27 Aug 2008 22:49:16 -0500
Subject: [PATCH 1/1] [PATCH] ne.c fix for hibernate and rmmod oops fix

Removing the module would cause a kernel oops as platform_driver_probe
failed to detect a device and unregistered the platform driver on
module init, and cleanup_module would unregister the already
unregistered driver.  The suspend and resume functions weren't being
called and the network card didn't function across a suspend to disk
followed by a power cycle.

platform_driver support was added earlier, but without any
platform_device_register* calls I don't think it was being used.  Now
all devices are registered using platform_device_register_simple and
pointers are kept to unregister the ones that the probe failed for or
unregister all devices on module shutdown.  init_module no longer
calls ne_init to reduce confusion (and multiple unregister paths that
caused the rmmod oops).

With the devices now registered they are added to the platform driver
and get suspend and resume events.  A call to pnp_stop_dev and
pnp_start_dev now shutsdown and initializes plug and play devices.

netif_device_detach(dev) was added before unregister_netdev(dev) when
removing the region as occationally I would see a race condition where
the device was still being used in unregister_netdev.

Signed-off-by: David Fries <david@fries.net>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Cc: Paul Gortmaker ...
[ message continues ]
[view original message]
From: Jeff Garzik
Subject: Re: [PATCH] ne.c fix for hibernate and rmmod oops fix
Date: Thursday, September 4, 2008 - 12:48 am

Please send a correct patch format.  The comments quoted above are all 
that appears when I run the official patch import tools ('git am').

It should be a simple matter of making sure your patch email body starts 
with "Removing the module would cause a kernel oops as 
platform_driver_probe" as described in Documentation/SubmittingPatches.

	Jeff


--


[ message continues ]
[view original message]
From: Paul Gortmaker
Subject: Re: [PATCH] ne.c fix for hibernate and rmmod oops fix
Date: Friday, September 5, 2008 - 1:01 am

Hi David,

I think there are some things that should be done here still.

Generally speaking, this is an old driver for hardware that most of
us will never see again, so the concern is that any changes may
break some existing configurations we can't easily test.  In light
of that, the change footprint should be minimized and/or be
bisectable wherever possible.

For example, if I'm reading it correctly, you've changed the module
behaviour so that on module load an autoprobe takes place, where
that was not the case before, because the probe is invasive.

It looks like you've got several semi-independent fixes here, plus
some shuffling of code blocks and a couple of needless white
space changes which all add up to a pretty big patch to an old
legacy driver.  I think it would be good if this was broken down
a bit and had a reduced footprint.  It would also be good to know
if you have tested both module and non-module (build and/or boot).

Additional comments inline.

Thanks,

The suspend/resume fix looks to be a candidate for a simple

This too sounds like it could be an independent fix on its own. I'd

I think your change puts the above as the human readable name in
/proc/ioports when you have a bad clone.  As such it probably should
be something like "ne2000(bad sig)" or similar, so it doesn't look

The relocation of all this makes it harder to read through the patch and



I guess the above is because of your addition of ne_loop_rm?
If someone is unfortunate enough to be using an ISA ne2000, then

The two chunks above look to be what could be bundled together

This is where I think you now have the module doing an
--


[ message continues ]
[view original message]
From: David Fries
Subject: Re: [PATCH] ne.c fix for hibernate and rmmod oops fix
Date: Monday, September 8, 2008 - 7:54 pm

Two new patches will follow,
Notable changes since the last patch:

Register all four platform devices, previously I would stop at one
with a zero io port address, but if there were multiple ISA PnP
devices or autoprobed devices on bootup, it would not look for the
second.  This way it will look for up to MAX_NE_CARDS.

Removed BAD_STR.  The platform_device structure holds id, which is
this_dev which is the index into the io, irq, and bad arrays.
ne_drv_probe can find the 0xbad flag in the bad array directly, no
need to figure out a way to pass it through the resources.

Now that the io port and irq can be looked up in the arrays the probe
code doesn't even need the resources passed by the platform device.
The ne.c driver already registers the resources it needs and there
isn't any good way to update the resources when the io port or irq are
zero and probed at runtime.

The "This is set up so that no ISA autoprobe takes place." comment was
moved up to #define NEEDS_PORTLIST, it was above the init_module
function, but there isn't any logic related to autoprobing.


Not that I'm aware of,

#if !defined(MODULE) && (defined(CONFIG_ISA) || defined(CONFIG_M32R))
/* Do we need a portlist for the ISA auto-probe ? */
#define NEEDS_PORTLIST
#endif

static int __init do_ne_probe(struct net_device *dev)
#ifdef NEEDS_PORTLIST
        /* Last resort. The semi-risky ISA auto-probe. */

testbeeper:~# modprobe ne
ne.c: You must supply "io=0xNNN" value(s) for ISA cards.  FATAL: Error inserting ne
(/lib/modules/2.6.27-rc3/kernel/drivers/net/ne.ko): No such device

Looks to have not autoprobed to me.

Previously init_module would read the module parameter for irq, io,
and bad and stop the first probe that failed.  I would expect it to

The final patch was tested in the following configurations,
qemu 32bit mode, fixed io & irq, modular and built in the kernel
i486 hardware, kernel ISA PnP, modular

I've tested built in the kernel on the i486 hardware as well, along
with other ...
[ message continues ]
[view original message]
From: David Fries
Subject: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements
Date: Monday, September 8, 2008 - 7:58 pm

From: David Fries <david@fries.net>

Removing the module would cause a kernel oops as platform_driver_probe
failed to detect a device and unregistered the platform driver on
module init, and cleanup_module would unregister the already
unregistered driver.  The suspend and resume functions weren't being
called.

platform_driver support was added earlier, but without any
platform_device_register* calls I don't think it was being used.  Now
all devices are registered using platform_device_register_simple and
pointers are kept to unregister the ones that the probe failed for or
unregister all devices on module shutdown.  init_module no longer
calls ne_init to reduce confusion (and multiple unregister paths that
caused the rmmod oops).  With the devices now registered they are
added to the platform driver and get suspend and resume events.

netif_device_detach(dev) was added before unregister_netdev(dev) when
removing the region as occationally I would see a race condition where
the device was still being used in unregister_netdev.

Signed-off-by: David Fries <david@fries.net>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Cc: Paul Gortmaker <p_gortmaker@yahoo.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Jeff Garzik <jeff@garzik.org>
---
 drivers/net/ne.c |  255 +++++++++++++++++++++++++++++++-----------------------
 1 files changed, 146 insertions(+), 109 deletions(-)

diff --git a/drivers/net/ne.c b/drivers/net/ne.c
index 42443d6..2bece66 100644
--- a/drivers/net/ne.c
+++ b/drivers/net/ne.c
@@ -64,6 +64,25 @@ static const char version2[] =
 
 /* Do we support clones that don't adhere to 14,15 of the SAprom ? */
 #define SUPPORT_NE_BAD_CLONES
+/* 0xbad = bad sig or no reset ack */
+#define BAD 0xbad
+
+#define MAX_NE_CARDS	4	/* Max number of NE cards per module */
+static struct platform_device *pdev_ne[MAX_NE_CARDS];
+static int io[MAX_NE_CARDS];
+static int irq[MAX_NE_CARDS];
+static int bad[MAX_NE_CARDS];
+
+#ifdef MODULE
+module_param_array(io, int, NULL, ...
[ message continues ]
[view original message]
From: Atsushi Nemoto
Subject: Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements
Date: Thursday, September 11, 2008 - 6:55 am

This breaks current users of ne platform driver.  Here is a quick fix
upon your patch.

diff --git a/drivers/net/ne.c b/drivers/net/ne.c
index 221e188..8b08433 100644
--- a/drivers/net/ne.c
+++ b/drivers/net/ne.c
@@ -813,10 +813,26 @@ static int __init ne_drv_probe(struct platform_device *pdev)
 {
 	struct net_device *dev;
 	int err, this_dev = pdev->id;
+	struct resource *res;
 
 	dev = alloc_eip_netdev();
 	if (!dev)
 		return -ENOMEM;
+	
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (res) {
+		dev->base_addr = res->start;
+		dev->irq = platform_get_irq(pdev, 0);
+		err = do_ne_probe(dev);
+		if (err) {
+			free_netdev(dev);
+			return err;
+		}
+		platform_set_drvdata(pdev, dev);
+		return 0;
+	}
+	if (this_dev < 0 || this_dev >= MAX_NE_CARDS)
+		return -EINVAL;
 	dev->base_addr = io[this_dev];
 	dev->irq = irq[this_dev];
 	dev->mem_end = bad[this_dev];
--


[ message continues ]
[view original message]
From: David Fries
Subject: Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements
Date: Saturday, September 13, 2008 - 7:44 am

I reformatted your patch and added a comment.  Does this work for you?

diff --git a/drivers/net/ne.c b/drivers/net/ne.c
index cd31e77..9759aec 100644
--- a/drivers/net/ne.c
+++ b/drivers/net/ne.c
@@ -813,22 +813,37 @@ static int __init ne_drv_probe(struct platform_device *pdev)
 {
 	struct net_device *dev;
 	int err, this_dev = pdev->id;
+	struct resource *res;
 
 	dev = alloc_eip_netdev();
 	if (!dev)
 		return -ENOMEM;
-	dev->base_addr = io[this_dev];
-	dev->irq = irq[this_dev];
-	dev->mem_end = bad[this_dev];
+
+	/* ne.c doesn't populate resources in platform_device, but
+	 * rbtx4927_ne_init and rbtx4938_ne_init do register devices
+	 * with resources.
+	 */
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (res) {
+		dev->base_addr = res->start;
+		dev->irq = platform_get_irq(pdev, 0);
+	} else {
+		dev->base_addr = io[this_dev];
+		dev->irq = irq[this_dev];
+		dev->mem_end = bad[this_dev];
+	}
 	err = do_ne_probe(dev);
 	if (err) {
 		free_netdev(dev);
 		return err;
 	}
 	platform_set_drvdata(pdev, dev);
+
 	/* Update with any values found by probing. */
-	io[this_dev] = dev->base_addr;
-	irq[this_dev] = dev->irq;
+	if (0 <= this_dev && this_dev < MAX_NE_CARDS) {
+		io[this_dev] = dev->base_addr;
+		irq[this_dev] = dev->irq;
+	}
 	return 0;
 }
 

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)
--


[ message continues ]
[view original message]
From: Atsushi Nemoto
Subject: Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements
Date: Saturday, September 13, 2008 - 8:13 am

For now rbtx49xx ne platform devices use -1 for pdev->id, but it may
change.  Please don't assume it.

---
Atsushi Nemoto
--


[ message continues ]
[view original message]
From: David Fries
Subject: Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements
Date: Saturday, September 13, 2008 - 8:19 am

Does this work?
-	if (0 <= this_dev && this_dev < MAX_NE_CARDS) {
+	if (!res) {

diff --git a/drivers/net/ne.c b/drivers/net/ne.c
index cd31e77..ede9b12 100644
--- a/drivers/net/ne.c
+++ b/drivers/net/ne.c
@@ -813,22 +813,37 @@ static int __init ne_drv_probe(struct platform_device *pdev)
 {
 	struct net_device *dev;
 	int err, this_dev = pdev->id;
+	struct resource *res;
 
 	dev = alloc_eip_netdev();
 	if (!dev)
 		return -ENOMEM;
-	dev->base_addr = io[this_dev];
-	dev->irq = irq[this_dev];
-	dev->mem_end = bad[this_dev];
+
+	/* ne.c doesn't populate resources in platform_device, but
+	 * rbtx4927_ne_init and rbtx4938_ne_init do register devices
+	 * with resources.
+	 */
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (res) {
+		dev->base_addr = res->start;
+		dev->irq = platform_get_irq(pdev, 0);
+	} else {
+		dev->base_addr = io[this_dev];
+		dev->irq = irq[this_dev];
+		dev->mem_end = bad[this_dev];
+	}
 	err = do_ne_probe(dev);
 	if (err) {
 		free_netdev(dev);
 		return err;
 	}
 	platform_set_drvdata(pdev, dev);
+
 	/* Update with any values found by probing. */
-	io[this_dev] = dev->base_addr;
-	irq[this_dev] = dev->irq;
+	if (!res) {
+		io[this_dev] = dev->base_addr;
+		irq[this_dev] = dev->irq;
+	}
 	return 0;
 }
 

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)
--


[ message continues ]
[view original message]
From: Atsushi Nemoto
Subject: Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements
Date: Saturday, September 13, 2008 - 8:29 am

It should work (not tested: I have no access to the board few days),
but if someone defined ne platform device with pdev->id == -1 and he
forgot to declare IO resources, but things happen.  Checking this_dev
range before accessing io[] array would be better.

---
Atsushi Nemoto
--


[ message continues ]
[view original message]
From: David Fries
Subject: [PATCH 1/2] ne.c fix rmmod, platform driver improvements
Date: Saturday, September 13, 2008 - 9:47 am

From: David Fries <david@fries.net>

Removing the module would cause a kernel oops as platform_driver_probe
failed to detect a device and unregistered the platform driver on
module init, and cleanup_module would unregister the already
unregistered driver.  The suspend and resume functions weren't being
called.

platform_driver support was added earlier, but without any
platform_device_register* calls I don't think it was being used.  Now
all devices are registered using platform_device_register_simple and
pointers are kept to unregister the ones that the probe failed for or
unregister all devices on module shutdown.  init_module no longer
calls ne_init to reduce confusion (and multiple unregister paths that
caused the rmmod oops).  With the devices now registered they are
added to the platform driver and get suspend and resume events.

netif_device_detach(dev) was added before unregister_netdev(dev) when
removing the region as occationally I would see a race condition where
the device was still being used in unregister_netdev.

Signed-off-by: David Fries <david@fries.net>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Cc: Paul Gortmaker <p_gortmaker@yahoo.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Jeff Garzik <jeff@garzik.org>
---
 drivers/net/ne.c |  272 ++++++++++++++++++++++++++++++++---------------------
 1 files changed, 164 insertions(+), 108 deletions(-)

diff --git a/drivers/net/ne.c b/drivers/net/ne.c
index 42443d6..4cb9c11 100644
--- a/drivers/net/ne.c
+++ b/drivers/net/ne.c
@@ -64,6 +64,25 @@ static const char version2[] =
 
 /* Do we support clones that don't adhere to 14,15 of the SAprom ? */
 #define SUPPORT_NE_BAD_CLONES
+/* 0xbad = bad sig or no reset ack */
+#define BAD 0xbad
+
+#define MAX_NE_CARDS	4	/* Max number of NE cards per module */
+static struct platform_device *pdev_ne[MAX_NE_CARDS];
+static int io[MAX_NE_CARDS];
+static int irq[MAX_NE_CARDS];
+static int bad[MAX_NE_CARDS];
+
+#ifdef MODULE
+module_param_array(io, int, NULL, ...
[ message continues ]
[view original message]
From: David Fries
Subject: [PATCH 2/2] ne.c Fix suspend and resume for ISA PnP cards.
Date: Saturday, September 13, 2008 - 9:54 am

From: David Fries <david@fries.net>

A call to pnp_stop_dev and pnp_start_dev now shuts down and
initializes plug and play devices for suspend and resume.

Signed-off-by: David Fries <david@fries.net>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Cc: Paul Gortmaker <p_gortmaker@yahoo.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Jeff Garzik <jeff@garzik.org>
---
This version is the same as the last PATCH 2/2, only with updated line
offsets.

 drivers/net/ne.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ne.c b/drivers/net/ne.c
index 4cb9c11..f9da162 100644
--- a/drivers/net/ne.c
+++ b/drivers/net/ne.c
@@ -895,8 +895,12 @@ static int ne_drv_suspend(struct platform_device *pdev, pm_message_t state)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
 
-	if (netif_running(dev))
+	if (netif_running(dev)) {
+		struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
 		netif_device_detach(dev);
+		if (idev)
+			pnp_stop_dev(idev);
+	}
 	return 0;
 }
 
@@ -905,6 +909,9 @@ static int ne_drv_resume(struct platform_device *pdev)
 	struct net_device *dev = platform_get_drvdata(pdev);
 
 	if (netif_running(dev)) {
+		struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
+		if (idev)
+			pnp_start_dev(idev);
 		ne_reset_8390(dev);
 		NS8390p_init(dev, 1);
 		netif_device_attach(dev);
-- 
1.4.4.4

--


[ message continues ]
[view original message]
From: David Fries
Subject: [PATCH 2/2] ne.c Fix suspend and resume for ISA PnP cards.
Date: Monday, September 8, 2008 - 8:01 pm

From: David Fries <david@fries.net>

A call to pnp_stop_dev and pnp_start_dev now shuts down and
initializes plug and play devices for suspend and resume.

Signed-off-by: David Fries <david@fries.net>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Cc: Paul Gortmaker <p_gortmaker@yahoo.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Jeff Garzik <jeff@garzik.org>
---
 drivers/net/ne.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ne.c b/drivers/net/ne.c
index 2bece66..cd31e77 100644
--- a/drivers/net/ne.c
+++ b/drivers/net/ne.c
@@ -876,8 +876,12 @@ static int ne_drv_suspend(struct platform_device *pdev, pm_message_t state)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
 
-	if (netif_running(dev))
+	if (netif_running(dev)) {
+		struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
 		netif_device_detach(dev);
+		if (idev)
+			pnp_stop_dev(idev);
+	}
 	return 0;
 }
 
@@ -886,6 +890,9 @@ static int ne_drv_resume(struct platform_device *pdev)
 	struct net_device *dev = platform_get_drvdata(pdev);
 
 	if (netif_running(dev)) {
+		struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
+		if (idev)
+			pnp_start_dev(idev);
 		ne_reset_8390(dev);
 		NS8390p_init(dev, 1);
 		netif_device_attach(dev);
-- 
1.4.4.4
--


[ message continues ]
[view original message]
From: Jeff Garzik
Subject: Re: [PATCH 2/2] ne.c Fix suspend and resume for ISA PnP cards.
Date: Saturday, September 13, 2008 - 12:55 pm

applied


--


[ message continues ]
[view original message]
From: David Fries
Subject: Re: [PATCH 2/2] ne.c Fix suspend and resume for ISA PnP cards.
Date: Saturday, September 13, 2008 - 1:32 pm

I just wanted to make it clear for anyone testing, this patch 2/2 by
itself changes suspend and resume functions which aren't executed
without patch 1/2 "ne.c fix rmmod, platform driver improvements".  

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)
--


[ message continues ]
[view original message]
From: Jeff Garzik
Subject: Re: [PATCH 2/2] ne.c Fix suspend and resume for ISA PnP cards.
Date: Saturday, September 13, 2008 - 1:36 pm

Thanks for noting that...


--


[ message continues ]
[view original message]
From: Atsushi Nemoto
Subject: Re: [PATCH] ne.c fix for hibernate and rmmod oops fix
Date: Thursday, September 11, 2008 - 6:50 am

No, please don't.  This breaks current users of ne platform driver.
For example, please look at rbtx4927_ne_init() in
arch/mips/txx9/rbtx4927/setup.c file.

---
Atsushi Nemoto
--


[ message continues ]
Previous thread: reiserfs do_journal_end unnecessary hd wake up? by Bráulio Barros de Oliveira on Wednesday, September 3, 2008 - 7:56 pm. (1 message)

Next thread: [PATCH] bq27x00_battery: use unaligned access helper by Harvey Harrison on Wednesday, September 3, 2008 - 8:39 pm. (1 message)