[PATCH] ne.c fix for hibernate and rmmod oops fix

[PATCH] ne.c fix for hibernate and rmmod oops fix

[David Fries]

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 <p_gortmaker@yahoo.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/net/ne.c |  277 +++++++++++++++++++++++++++++++++--------------------
 1 files changed, 172 insertions(+), 105 deletions(-)

diff --git a/drivers/net/ne.c b/drivers/net/ne.c
index 42443d6..7980cf0 100644
--- a/drivers/net/ne.c
+++ b/drivers/net/ne.c
@@ -64,6 +64,27 @@ 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 BAD_STR "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, 0);
+module_param_array(irq, int, NULL, 0);
+module_param_array(bad, int, NULL, 0);
+MODULE_PARM_DESC(io, "I/O base address(es),required");
+MODULE_PARM_DESC(irq, "IRQ number(s)");
+MODULE_PARM_DESC(bad, "Accept card(s) with bad signatures");
+MODULE_DESCRIPTION("NE1000/NE2000 ISA/PnP Ethernet driver");
+MODULE_LICENSE("GPL");
+#endif /* MODULE */
 
 /* Do we perform extra sanity checks on stuff ? */
 /* #define NE_SANITY_CHECK */
@@ -162,7 +183,6 @@ static void ne_block_input(struct net_device *dev, int count,
 static void ne_block_output(struct net_device *dev, const int count,
 		const unsigned char *buf, const int start_page);
 
-
 /*  Probe for various non-shared-memory ethercards.
 
    NEx000-clone boards have a Station Address PROM (SAPROM) in the packet
@@ -192,8 +212,13 @@ static int __init do_ne_probe(struct net_device *dev)
 #endif
 
 	/* First check any supplied i/o locations. User knows best. <cough> */
-	if (base_addr > 0x1ff)	/* Check a single specified location. */
-		return ne_probe1(dev, base_addr);
+	if (base_addr > 0x1ff) {	/* Check a single specified location. */
+		int ret = ne_probe1(dev, base_addr);
+		if (ret)
+			printk(KERN_WARNING "ne.c: No NE*000 card found at "
+				"i/o = %#lx\n", base_addr);
+		return ret;
+	}
 	else if (base_addr != 0)	/* Don't probe at all. */
 		return -ENXIO;
 
@@ -214,28 +239,6 @@ static int __init do_ne_probe(struct net_device *dev)
 	return -ENODEV;
 }
 
-#ifndef MODULE
-struct net_device * __init ne_probe(int unit)
-{
-	struct net_device *dev = alloc_eip_netdev();
-	int err;
-
-	if (!dev)
-		return ERR_PTR(-ENOMEM);
-
-	sprintf(dev->name, "eth%d", unit);
-	netdev_boot_setup_check(dev);
-
-	err = do_ne_probe(dev);
-	if (err)
-		goto out;
-	return dev;
-out:
-	free_netdev(dev);
-	return ERR_PTR(err);
-}
-#endif
-
 static int __init ne_probe_isapnp(struct net_device *dev)
 {
 	int i;
@@ -329,7 +332,7 @@ static int __init ne_probe1(struct net_device *dev, unsigned long ioaddr)
 	   with an otherwise unused dev->mem_end value of "0xBAD" will
 	   cause the driver to skip these parts of the probe. */
 
-	bad_card = ((dev->base_addr != 0) && (dev->mem_end == 0xbad));
+	bad_card = ((dev->base_addr != 0) && (dev->mem_end == BAD));
 
 	/* Reset card. Who knows what dain-bramaged state it was left in. */
 
@@ -472,7 +475,7 @@ static int __init ne_probe1(struct net_device *dev, unsigned long ioaddr)
 		outb_p(0x00, ioaddr + EN0_RCNTLO);
 		outb_p(0x00, ioaddr + EN0_RCNTHI);
 		outb_p(E8390_RREAD+E8390_START, ioaddr); /* Trigger it... */
-		mdelay(10);		/* wait 10ms for interrupt to propagate */
+		mdelay(10);	/* wait 10ms for interrupt to propagate */
 		outb_p(0x00, ioaddr + EN0_IMR); 		/* Mask it again. */
 		dev->irq = probe_irq_off(cookie);
 		if (ei_debug > 2)
@@ -807,18 +810,20 @@ static int __init ne_drv_probe(struct platform_device *pdev)
 {
 	struct net_device *dev;
 	struct resource *res;
-	int err, irq;
+	int err, r_irq;
 
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	irq = platform_get_irq(pdev, 0);
-	if (!res || irq < 0)
+	r_irq = platform_get_irq(pdev, 0);
+	if (!res || r_irq < 0)
 		return -ENODEV;
 
 	dev = alloc_eip_netdev();
 	if (!dev)
 		return -ENOMEM;
-	dev->irq = irq;
+	dev->irq = r_irq;
 	dev->base_addr = res->start;
+	if (!strcmp(res->name, BAD_STR))
+		dev->mem_end = BAD;
 	err = do_ne_probe(dev);
 	if (err) {
 		free_netdev(dev);
@@ -828,24 +833,56 @@ static int __init ne_drv_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int __exit ne_drv_remove(struct platform_device *pdev)
+static int ne_drv_remove(struct platform_device *pdev)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
 
-	unregister_netdev(dev);
-	free_irq(dev->irq, dev);
-	release_region(dev->base_addr, NE_IO_EXTENT);
-	free_netdev(dev);
+	if (dev) {
+		struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
+		netif_device_detach(dev);
+		unregister_netdev(dev);
+		if (idev)
+			pnp_device_detach(idev);
+		/* Careful ne_drv_remove can be called twice, once from
+		 * the platform_driver.remove and again when the
+		 * platform_device is being removed.
+		 */
+		ei_status.priv = 0;
+		free_irq(dev->irq, dev);
+		release_region(dev->base_addr, NE_IO_EXTENT);
+		free_netdev(dev);
+		platform_set_drvdata(pdev, NULL);
+	}
 	return 0;
 }
 
+/* Remove unused devices or all if true. */
+static void ne_loop_rm_unreg(int all)
+{
+	int this_dev;
+	struct platform_device *pdev;
+	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
+		pdev = pdev_ne[this_dev];
+		/* No network device == unused */
+		if (pdev && (!platform_get_drvdata(pdev) || all)) {
+			ne_drv_remove(pdev);
+			platform_device_unregister(pdev);
+			pdev_ne[this_dev] = NULL;
+		}
+	}
+}
+
 #ifdef CONFIG_PM
 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;
 }
 
@@ -854,6 +891,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);
@@ -866,7 +906,7 @@ static int ne_drv_resume(struct platform_device *pdev)
 #endif
 
 static struct platform_driver ne_driver = {
-	.remove		= __exit_p(ne_drv_remove),
+	.remove		= ne_drv_remove,
 	.suspend	= ne_drv_suspend,
 	.resume		= ne_drv_resume,
 	.driver		= {
@@ -875,91 +915,118 @@ static struct platform_driver ne_driver = {
 	},
 };
 
-static int __init ne_init(void)
-{
-	return platform_driver_probe(&ne_driver, ne_drv_probe);
-}
-
-static void __exit ne_exit(void)
-{
-	platform_driver_unregister(&ne_driver);
-}
-
-#ifdef MODULE
-#define MAX_NE_CARDS	4	/* Max number of NE cards per module */
-static struct net_device *dev_ne[MAX_NE_CARDS];
-static int io[MAX_NE_CARDS];
-static int irq[MAX_NE_CARDS];
-static int bad[MAX_NE_CARDS];	/* 0xbad = bad sig or no reset ack */
-
-module_param_array(io, int, NULL, 0);
-module_param_array(irq, int, NULL, 0);
-module_param_array(bad, int, NULL, 0);
-MODULE_PARM_DESC(io, "I/O base address(es),required");
-MODULE_PARM_DESC(irq, "IRQ number(s)");
-MODULE_PARM_DESC(bad, "Accept card(s) with bad signatures");
-MODULE_DESCRIPTION("NE1000/NE2000 ISA/PnP Ethernet driver");
-MODULE_LICENSE("GPL");
-
 /* This is set up so that no ISA autoprobe takes place. We can't guarantee
 that the ne2k probe is the last 8390 based probe to take place (as it
 is at boot) and so the probe will get confused by any other 8390 cards.
 ISA device autoprobes on a running machine are not recommended anyway. */
 
-int __init init_module(void)
+/* Register platform devices for each non-zero io[] (and one 0 io for probe
+ * or pnp probing), then probe to see if we found any.
+ */
+static void __init ne_add_devices(void)
 {
-	int this_dev, found = 0;
-	int plat_found = !ne_init();
+	int this_dev;
+	int probed_zero = 0;
+	struct platform_device *pdev;
 
 	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
-		struct net_device *dev = alloc_eip_netdev();
-		if (!dev)
-			break;
-		dev->irq = irq[this_dev];
-		dev->mem_end = bad[this_dev];
-		dev->base_addr = io[this_dev];
-		if (do_ne_probe(dev) == 0) {
-			dev_ne[found++] = dev;
-			continue;
+		struct resource r[2] = {
+			{
+				.start = io[this_dev],
+				.end = io[this_dev]+NE_IO_EXTENT-1,
+				.flags = IORESOURCE_IO},
+			{
+				.start = irq[this_dev],
+				.flags = IORESOURCE_IRQ} };
+		/* probe zero once */
+		if (io[this_dev] == 0) {
+			if (probed_zero)
+				continue;
+			probed_zero = 1;
 		}
-		free_netdev(dev);
-		if (found || plat_found)
-			break;
-		if (io[this_dev] != 0)
-			printk(KERN_WARNING "ne.c: No NE*000 card found at i/o = %#x\n", io[this_dev]);
-		else
-			printk(KERN_NOTICE "ne.c: You must supply \"io=0xNNN\" value(s) for ISA cards.\n");
-		return -ENXIO;
+		if (pdev_ne[this_dev])
+			continue;
+		if (bad[this_dev] == BAD)
+			r[0].name = BAD_STR;
+		pdev = platform_device_register_simple(
+			DRV_NAME, this_dev, r, ARRAY_SIZE(r));
+		if (IS_ERR(pdev))
+			continue;
+		pdev_ne[this_dev] = pdev;
 	}
-	if (found || plat_found)
-		return 0;
-	return -ENODEV;
 }
 
-static void cleanup_card(struct net_device *dev)
+#ifdef MODULE
+int __init init_module()
 {
-	struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
-	if (idev)
-		pnp_device_detach(idev);
-	free_irq(dev->irq, dev);
-	release_region(dev->base_addr, NE_IO_EXTENT);
+	int retval;
+	ne_add_devices();
+	retval = platform_driver_probe(&ne_driver, ne_drv_probe);
+	if (retval) {
+		if (io[0] == 0)
+			printk(KERN_NOTICE "ne.c: You must supply \"io=0xNNN\""
+				" value(s) for ISA cards.\n");
+		ne_loop_rm_unreg(1);
+		return retval;
+	}
+
+	/* Unregister unused platform_devices. */
+	ne_loop_rm_unreg(0);
+	return retval;
+}
+#else /* MODULE */
+static int __init ne_init(void)
+{
+	int retval = platform_driver_probe(&ne_driver, ne_drv_probe);
+
+	/* Unregister unused platform_devices. */
+	ne_loop_rm_unreg(0);
+	return retval;
 }
+module_init(ne_init);
 
-void __exit cleanup_module(void)
+struct net_device * __init ne_probe(int unit)
 {
 	int this_dev;
+	struct net_device *dev;
+	struct platform_device *pdev;
 
-	ne_exit();
-	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
-		struct net_device *dev = dev_ne[this_dev];
-		if (dev) {
-			unregister_netdev(dev);
-			cleanup_card(dev);
-			free_netdev(dev);
-		}
+	/* Find an empty slot. */
+	this_dev = 0;
+	while (pdev_ne[this_dev] || io[this_dev]) {
+		if (++this_dev == MAX_NE_CARDS)
+			return ERR_PTR(-ENOMEM);
 	}
+
+	/* Get irq, io from kernel command line */
+	dev = alloc_eip_netdev();
+	if (!dev)
+		return ERR_PTR(-ENOMEM);
+
+	sprintf(dev->name, "eth%d", unit);
+	netdev_boot_setup_check(dev);
+
+	io[this_dev] = dev->base_addr;
+	irq[this_dev] = dev->irq;
+	bad[this_dev] = dev->mem_end;
+
+	free_netdev(dev);
+
+	ne_add_devices();
+
+	/* return the first device found */
+	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++)
+		if ((pdev = pdev_ne[this_dev]) &&
+			(dev = platform_get_drvdata(pdev)))
+			return dev;
+
+	return ERR_PTR(-ENODEV);
 }
-#else /* MODULE */
-module_init(ne_init);
-module_exit(ne_exit);
 #endif /* MODULE */
+
+static void __exit ne_exit(void)
+{
+	platform_driver_unregister(&ne_driver);
+	ne_loop_rm_unreg(1);
+}
+module_exit(ne_exit);
-- 
1.4.4.4

Re: [PATCH] ne.c fix for hibernate and rmmod oops fix

[Jeff Garzik]

David Fries wrote:
> 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.

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

Re: [PATCH] ne.c fix for hibernate and rmmod oops fix

[Paul Gortmaker]

On Wed, Sep 3, 2008 at 11:02 PM, David Fries <david@fries.net> wrote:
> Andrew Morton,
>
> This patch is ready to be merged.

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

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

The suspend/resume fix looks to be a candidate for a simple
fix (i.e. separate patch) in its own right.

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

This too sounds like it could be an independent fix on its own. I'd
say do those 1st and then the platform patch won't be as big.

>
> 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>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  drivers/net/ne.c |  277 +++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 172 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/net/ne.c b/drivers/net/ne.c
> index 42443d6..7980cf0 100644
> --- a/drivers/net/ne.c
> +++ b/drivers/net/ne.c
> @@ -64,6 +64,27 @@ 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 BAD_STR "0xbad"

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
like some resource management bug to an end user.

> +
> +#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, 0);
> +module_param_array(irq, int, NULL, 0);
> +module_param_array(bad, int, NULL, 0);
> +MODULE_PARM_DESC(io, "I/O base address(es),required");
> +MODULE_PARM_DESC(irq, "IRQ number(s)");
> +MODULE_PARM_DESC(bad, "Accept card(s) with bad signatures");
> +MODULE_DESCRIPTION("NE1000/NE2000 ISA/PnP Ethernet driver");
> +MODULE_LICENSE("GPL");
> +#endif /* MODULE */

The relocation of all this makes it harder to read through the patch and
see what the significant changes are.  Is it necessary?

>
>  /* Do we perform extra sanity checks on stuff ? */
>  /* #define NE_SANITY_CHECK */
> @@ -162,7 +183,6 @@ static void ne_block_input(struct net_device *dev, int count,
>  static void ne_block_output(struct net_device *dev, const int count,
>                const unsigned char *buf, const int start_page);
>
> -
>  /*  Probe for various non-shared-memory ethercards.
>

Gratuitous whitespace change.

>    NEx000-clone boards have a Station Address PROM (SAPROM) in the packet
> @@ -192,8 +212,13 @@ static int __init do_ne_probe(struct net_device *dev)
>  #endif
>
>        /* First check any supplied i/o locations. User knows best. <cough> */
> -       if (base_addr > 0x1ff)  /* Check a single specified location. */
> -               return ne_probe1(dev, base_addr);
> +       if (base_addr > 0x1ff) {        /* Check a single specified location. */
> +               int ret = ne_probe1(dev, base_addr);
> +               if (ret)
> +                       printk(KERN_WARNING "ne.c: No NE*000 card found at "
> +                               "i/o = %#lx\n", base_addr);
> +               return ret;
> +       }
>        else if (base_addr != 0)        /* Don't probe at all. */
>                return -ENXIO;
>
> @@ -214,28 +239,6 @@ static int __init do_ne_probe(struct net_device *dev)
>        return -ENODEV;
>  }
>
> -#ifndef MODULE
> -struct net_device * __init ne_probe(int unit)
> -{
> -       struct net_device *dev = alloc_eip_netdev();
> -       int err;
> -
> -       if (!dev)
> -               return ERR_PTR(-ENOMEM);
> -
> -       sprintf(dev->name, "eth%d", unit);
> -       netdev_boot_setup_check(dev);
> -
> -       err = do_ne_probe(dev);
> -       if (err)
> -               goto out;
> -       return dev;
> -out:
> -       free_netdev(dev);
> -       return ERR_PTR(err);
> -}
> -#endif
> -
>  static int __init ne_probe_isapnp(struct net_device *dev)
>  {
>        int i;
> @@ -329,7 +332,7 @@ static int __init ne_probe1(struct net_device *dev, unsigned long ioaddr)
>           with an otherwise unused dev->mem_end value of "0xBAD" will
>           cause the driver to skip these parts of the probe. */
>
> -       bad_card = ((dev->base_addr != 0) && (dev->mem_end == 0xbad));
> +       bad_card = ((dev->base_addr != 0) && (dev->mem_end == BAD));
>
>        /* Reset card. Who knows what dain-bramaged state it was left in. */
>
> @@ -472,7 +475,7 @@ static int __init ne_probe1(struct net_device *dev, unsigned long ioaddr)
>                outb_p(0x00, ioaddr + EN0_RCNTLO);
>                outb_p(0x00, ioaddr + EN0_RCNTHI);
>                outb_p(E8390_RREAD+E8390_START, ioaddr); /* Trigger it... */
> -               mdelay(10);             /* wait 10ms for interrupt to propagate */
> +               mdelay(10);     /* wait 10ms for interrupt to propagate */

Gratuitous whitespace change.

>                outb_p(0x00, ioaddr + EN0_IMR);                 /* Mask it again. */
>                dev->irq = probe_irq_off(cookie);
>                if (ei_debug > 2)
> @@ -807,18 +810,20 @@ static int __init ne_drv_probe(struct platform_device *pdev)
>  {
>        struct net_device *dev;
>        struct resource *res;
> -       int err, irq;
> +       int err, r_irq;
>
>        res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> -       irq = platform_get_irq(pdev, 0);
> -       if (!res || irq < 0)
> +       r_irq = platform_get_irq(pdev, 0);
> +       if (!res || r_irq < 0)
>                return -ENODEV;
>
>        dev = alloc_eip_netdev();
>        if (!dev)
>                return -ENOMEM;
> -       dev->irq = irq;
> +       dev->irq = r_irq;
>        dev->base_addr = res->start;
> +       if (!strcmp(res->name, BAD_STR))
> +               dev->mem_end = BAD;
>        err = do_ne_probe(dev);
>        if (err) {
>                free_netdev(dev);
> @@ -828,24 +833,56 @@ static int __init ne_drv_probe(struct platform_device *pdev)
>        return 0;
>  }
>
> -static int __exit ne_drv_remove(struct platform_device *pdev)
> +static int ne_drv_remove(struct platform_device *pdev)

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
they might care about saving on driver size while built-in...

>  {
>        struct net_device *dev = platform_get_drvdata(pdev);
>
> -       unregister_netdev(dev);
> -       free_irq(dev->irq, dev);
> -       release_region(dev->base_addr, NE_IO_EXTENT);
> -       free_netdev(dev);
> +       if (dev) {
> +               struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
> +               netif_device_detach(dev);
> +               unregister_netdev(dev);
> +               if (idev)
> +                       pnp_device_detach(idev);
> +               /* Careful ne_drv_remove can be called twice, once from
> +                * the platform_driver.remove and again when the
> +                * platform_device is being removed.
> +                */
> +               ei_status.priv = 0;
> +               free_irq(dev->irq, dev);
> +               release_region(dev->base_addr, NE_IO_EXTENT);
> +               free_netdev(dev);
> +               platform_set_drvdata(pdev, NULL);
> +       }
>        return 0;
>  }
>
> +/* Remove unused devices or all if true. */
> +static void ne_loop_rm_unreg(int all)
> +{
> +       int this_dev;
> +       struct platform_device *pdev;
> +       for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
> +               pdev = pdev_ne[this_dev];
> +               /* No network device == unused */
> +               if (pdev && (!platform_get_drvdata(pdev) || all)) {
> +                       ne_drv_remove(pdev);
> +                       platform_device_unregister(pdev);
> +                       pdev_ne[this_dev] = NULL;
> +               }
> +       }
> +}
> +
>  #ifdef CONFIG_PM
>  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;
>  }
>
> @@ -854,6 +891,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);

The two chunks above look to be what could be bundled together
as a separate, stand-alone fix.

> @@ -866,7 +906,7 @@ static int ne_drv_resume(struct platform_device *pdev)
>  #endif
>
>  static struct platform_driver ne_driver = {
> -       .remove         = __exit_p(ne_drv_remove),
> +       .remove         = ne_drv_remove,
>        .suspend        = ne_drv_suspend,
>        .resume         = ne_drv_resume,
>        .driver         = {
> @@ -875,91 +915,118 @@ static struct platform_driver ne_driver = {
>        },
>  };
>
> -static int __init ne_init(void)
> -{
> -       return platform_driver_probe(&ne_driver, ne_drv_probe);
> -}
> -
> -static void __exit ne_exit(void)
> -{
> -       platform_driver_unregister(&ne_driver);
> -}
> -
> -#ifdef MODULE
> -#define MAX_NE_CARDS   4       /* Max number of NE cards per module */
> -static struct net_device *dev_ne[MAX_NE_CARDS];
> -static int io[MAX_NE_CARDS];
> -static int irq[MAX_NE_CARDS];
> -static int bad[MAX_NE_CARDS];  /* 0xbad = bad sig or no reset ack */
> -
> -module_param_array(io, int, NULL, 0);
> -module_param_array(irq, int, NULL, 0);
> -module_param_array(bad, int, NULL, 0);
> -MODULE_PARM_DESC(io, "I/O base address(es),required");
> -MODULE_PARM_DESC(irq, "IRQ number(s)");
> -MODULE_PARM_DESC(bad, "Accept card(s) with bad signatures");
> -MODULE_DESCRIPTION("NE1000/NE2000 ISA/PnP Ethernet driver");
> -MODULE_LICENSE("GPL");
> -
>  /* This is set up so that no ISA autoprobe takes place. We can't guarantee
>  that the ne2k probe is the last 8390 based probe to take place (as it
>  is at boot) and so the probe will get confused by any other 8390 cards.
>  ISA device autoprobes on a running machine are not recommended anyway. */
>
> -int __init init_module(void)
> +/* Register platform devices for each non-zero io[] (and one 0 io for probe
> + * or pnp probing), then probe to see if we found any.
> + */
> +static void __init ne_add_devices(void)
>  {
> -       int this_dev, found = 0;
> -       int plat_found = !ne_init();
> +       int this_dev;
> +       int probed_zero = 0;
> +       struct platform_device *pdev;
>
>        for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
> -               struct net_device *dev = alloc_eip_netdev();
> -               if (!dev)
> -                       break;
> -               dev->irq = irq[this_dev];
> -               dev->mem_end = bad[this_dev];
> -               dev->base_addr = io[this_dev];
> -               if (do_ne_probe(dev) == 0) {
> -                       dev_ne[found++] = dev;
> -                       continue;
> +               struct resource r[2] = {
> +                       {
> +                               .start = io[this_dev],
> +                               .end = io[this_dev]+NE_IO_EXTENT-1,
> +                               .flags = IORESOURCE_IO},
> +                       {
> +                               .start = irq[this_dev],
> +                               .flags = IORESOURCE_IRQ} };
> +               /* probe zero once */
> +               if (io[this_dev] == 0) {
> +                       if (probed_zero)
> +                               continue;
> +                       probed_zero = 1;
>                }

This is where I think you now have the module doing an
autoprobe that wasn''t being done before.

> -               free_netdev(dev);
> -               if (found || plat_found)
> -                       break;
> -               if (io[this_dev] != 0)
> -                       printk(KERN_WARNING "ne.c: No NE*000 card found at i/o = %#x\n", io[this_dev]);
> -               else
> -                       printk(KERN_NOTICE "ne.c: You must supply \"io=0xNNN\" value(s) for ISA cards.\n");
> -               return -ENXIO;
> +               if (pdev_ne[this_dev])
> +                       continue;
> +               if (bad[this_dev] == BAD)
> +                       r[0].name = BAD_STR;
> +               pdev = platform_device_register_simple(
> +                       DRV_NAME, this_dev, r, ARRAY_SIZE(r));
> +               if (IS_ERR(pdev))
> +                       continue;
> +               pdev_ne[this_dev] = pdev;
>        }
> -       if (found || plat_found)
> -               return 0;
> -       return -ENODEV;
>  }
>
> -static void cleanup_card(struct net_device *dev)
> +#ifdef MODULE
> +int __init init_module()
>  {
> -       struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
> -       if (idev)
> -               pnp_device_detach(idev);
> -       free_irq(dev->irq, dev);
> -       release_region(dev->base_addr, NE_IO_EXTENT);
> +       int retval;
> +       ne_add_devices();
> +       retval = platform_driver_probe(&ne_driver, ne_drv_probe);
> +       if (retval) {
> +               if (io[0] == 0)
> +                       printk(KERN_NOTICE "ne.c: You must supply \"io=0xNNN\""
> +                               " value(s) for ISA cards.\n");
> +               ne_loop_rm_unreg(1);
> +               return retval;
> +       }
> +
> +       /* Unregister unused platform_devices. */
> +       ne_loop_rm_unreg(0);
> +       return retval;
> +}
> +#else /* MODULE */
> +static int __init ne_init(void)
> +{
> +       int retval = platform_driver_probe(&ne_driver, ne_drv_probe);
> +
> +       /* Unregister unused platform_devices. */
> +       ne_loop_rm_unreg(0);
> +       return retval;
>  }
> +module_init(ne_init);
>
> -void __exit cleanup_module(void)
> +struct net_device * __init ne_probe(int unit)
>  {
>        int this_dev;
> +       struct net_device *dev;
> +       struct platform_device *pdev;
>
> -       ne_exit();
> -       for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
> -               struct net_device *dev = dev_ne[this_dev];
> -               if (dev) {
> -                       unregister_netdev(dev);
> -                       cleanup_card(dev);
> -                       free_netdev(dev);
> -               }
> +       /* Find an empty slot. */
> +       this_dev = 0;
> +       while (pdev_ne[this_dev] || io[this_dev]) {
> +               if (++this_dev == MAX_NE_CARDS)
> +                       return ERR_PTR(-ENOMEM);
>        }
> +
> +       /* Get irq, io from kernel command line */
> +       dev = alloc_eip_netdev();
> +       if (!dev)
> +               return ERR_PTR(-ENOMEM);
> +
> +       sprintf(dev->name, "eth%d", unit);
> +       netdev_boot_setup_check(dev);
> +
> +       io[this_dev] = dev->base_addr;
> +       irq[this_dev] = dev->irq;
> +       bad[this_dev] = dev->mem_end;
> +
> +       free_netdev(dev);
> +
> +       ne_add_devices();
> +
> +       /* return the first device found */
> +       for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++)
> +               if ((pdev = pdev_ne[this_dev]) &&
> +                       (dev = platform_get_drvdata(pdev)))
> +                       return dev;
> +
> +       return ERR_PTR(-ENODEV);
>  }
> -#else /* MODULE */
> -module_init(ne_init);
> -module_exit(ne_exit);
>  #endif /* MODULE */
> +
> +static void __exit ne_exit(void)
> +{
> +       platform_driver_unregister(&ne_driver);
> +       ne_loop_rm_unreg(1);
> +}
> +module_exit(ne_exit);
> --
> 1.4.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] ne.c fix for hibernate and rmmod oops fix

[David Fries]

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.

On Fri, Sep 05, 2008 at 04:01:16AM -0400, Paul Gortmaker wrote:
> On Wed, Sep 3, 2008 at 11:02 PM, David Fries <david@fries.net> wrote:
>
> 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.

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
always probe with a zero port address.

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

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 configurations, just probably not with the final
configuration.  Some of the others were multiple ne2000's in qemu,
user space ISA PnP with i486 etc.

Auto detecting the irq only works once per boot, remove the module and
insert it again and it fails.  I didn't change any of that code, and I
didn't need it to work, so I didn't look into it.

Note the user space ISA PnP doesn't work for suspend to disk, because
of another kernel bug.  Boot, run isapnp to init the board, insert
module, works fine, suspend to disk, power cycle, restore, isapnp
hasn't run, so the card doesn't work (as expected).  Run isapnp, only
it fails.  /proc/interrupts has the interrupt listed, even after rmmod
ne, so it can't initialize the board.  After rmmod ne, the interrupt
has an empty driver field.

It's not related to ISA PnP, boot, insert module, suspend, resume,
rmmod, /proc/interrupts lists an empty driver for that module.  The
irq isn't claimed, as the module can be inserted and claim the irq
again.  If suspend and resume are left out the irq isn't listed 
after rmmod.

> Additional comments inline.

> > 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.
> 
> The suspend/resume fix looks to be a candidate for a simple
> fix (i.e. separate patch) in its own right.
> 
> >
> > 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.
> 
> This too sounds like it could be an independent fix on its own. I'd
> say do those 1st and then the platform patch won't be as big.

The suspend and resume fix is somewhat isolated.  I could see that
patch after the main platform patch, but it would just be dead code
that couldn't be testable if added before.  Same with
netif_device_deatch, only it would be modifying the same code, so it's
not going to help with size.

> > ---
> >  drivers/net/ne.c |  277 +++++++++++++++++++++++++++++++++--------------------
> >  1 files changed, 172 insertions(+), 105 deletions(-)
> >
> > diff --git a/drivers/net/ne.c b/drivers/net/ne.c
> > index 42443d6..7980cf0 100644
> > --- a/drivers/net/ne.c
> > +++ b/drivers/net/ne.c
> > @@ -64,6 +64,27 @@ 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 BAD_STR "0xbad"
> 
> 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
> like some resource management bug to an end user.

So it does.

0300-031f : 0xbad
  0300-031f : ne

vs

0300-031f : ne.0
  0300-031f : ne

Will be fixed, sigh, so much for (ab)using r[0].name.

> > +
> > +#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, 0);
> > +module_param_array(irq, int, NULL, 0);
> > +module_param_array(bad, int, NULL, 0);
> > +MODULE_PARM_DESC(io, "I/O base address(es),required");
> > +MODULE_PARM_DESC(irq, "IRQ number(s)");
> > +MODULE_PARM_DESC(bad, "Accept card(s) with bad signatures");
> > +MODULE_DESCRIPTION("NE1000/NE2000 ISA/PnP Ethernet driver");
> > +MODULE_LICENSE("GPL");
> > +#endif /* MODULE */
> 
> The relocation of all this makes it harder to read through the patch and
> see what the significant changes are.  Is it necessary?

It was previously before init_module, but now the first use of pdev_ne
is ne_loop_rm_unreg which is earlier.  ne_loop_rm_unreg could have
been moved above init_module, but then it wouldn'tbe logically be
grouped with the other ne_drv_remove function, and ne_drv_remove can't
be moved down as struct platform_driver uses it.  If you're going to
move it up, might as well move it up to the more logical top.

> >  /* Do we perform extra sanity checks on stuff ? */
> >  /* #define NE_SANITY_CHECK */
> > @@ -162,7 +183,6 @@ static void ne_block_input(struct net_device *dev, int count,
> >  static void ne_block_output(struct net_device *dev, const int count,
> >                const unsigned char *buf, const int start_page);
> >
> > -
> >  /*  Probe for various non-shared-memory ethercards.
> >
> 
> Gratuitous whitespace change.

Fixed,

> > @@ -472,7 +475,7 @@ static int __init ne_probe1(struct net_device *dev, unsigned long ioaddr)
> >                outb_p(0x00, ioaddr + EN0_RCNTLO);
> >                outb_p(0x00, ioaddr + EN0_RCNTHI);
> >                outb_p(E8390_RREAD+E8390_START, ioaddr); /* Trigger it... */
> > -               mdelay(10);             /* wait 10ms for interrupt to propagate */
> > +               mdelay(10);     /* wait 10ms for interrupt to propagate */
> 
> Gratuitous whitespace change.

Andrew Morton says to use checkpatch, at one point it failed so I
fixed it, reverted.

> > -static int __exit ne_drv_remove(struct platform_device *pdev)
> > +static int ne_drv_remove(struct platform_device *pdev)
> 
> 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
> they might care about saving on driver size while built-in...

ne_loop_rm_unreg yes calls ne_drv_remove which is also called at
initialization to remove the platform drivers that failed to probe.
Unless people prefer to leave those devices around.

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

[PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements

[David Fries]

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, 0);
+module_param_array(irq, int, NULL, 0);
+module_param_array(bad, int, NULL, 0);
+MODULE_PARM_DESC(io, "I/O base address(es),required");
+MODULE_PARM_DESC(irq, "IRQ number(s)");
+MODULE_PARM_DESC(bad, "Accept card(s) with bad signatures");
+MODULE_DESCRIPTION("NE1000/NE2000 ISA/PnP Ethernet driver");
+MODULE_LICENSE("GPL");
+#endif /* MODULE */
 
 /* Do we perform extra sanity checks on stuff ? */
 /* #define NE_SANITY_CHECK */
@@ -74,6 +93,10 @@ static const char version2[] =
 /* Do we have a non std. amount of memory? (in units of 256 byte pages) */
 /* #define PACKETBUF_MEMSIZE	0x40 */
 
+/* This is set up so that no ISA autoprobe takes place. We can't guarantee
+that the ne2k probe is the last 8390 based probe to take place (as it
+is at boot) and so the probe will get confused by any other 8390 cards.
+ISA device autoprobes on a running machine are not recommended anyway. */
 #if !defined(MODULE) && (defined(CONFIG_ISA) || defined(CONFIG_M32R))
 /* Do we need a portlist for the ISA auto-probe ? */
 #define NEEDS_PORTLIST
@@ -192,8 +215,13 @@ static int __init do_ne_probe(struct net_device *dev)
 #endif
 
 	/* First check any supplied i/o locations. User knows best. <cough> */
-	if (base_addr > 0x1ff)	/* Check a single specified location. */
-		return ne_probe1(dev, base_addr);
+	if (base_addr > 0x1ff) {	/* Check a single specified location. */
+		int ret = ne_probe1(dev, base_addr);
+		if (ret)
+			printk(KERN_WARNING "ne.c: No NE*000 card found at "
+				"i/o = %#lx\n", base_addr);
+		return ret;
+	}
 	else if (base_addr != 0)	/* Don't probe at all. */
 		return -ENXIO;
 
@@ -214,28 +242,6 @@ static int __init do_ne_probe(struct net_device *dev)
 	return -ENODEV;
 }
 
-#ifndef MODULE
-struct net_device * __init ne_probe(int unit)
-{
-	struct net_device *dev = alloc_eip_netdev();
-	int err;
-
-	if (!dev)
-		return ERR_PTR(-ENOMEM);
-
-	sprintf(dev->name, "eth%d", unit);
-	netdev_boot_setup_check(dev);
-
-	err = do_ne_probe(dev);
-	if (err)
-		goto out;
-	return dev;
-out:
-	free_netdev(dev);
-	return ERR_PTR(err);
-}
-#endif
-
 static int __init ne_probe_isapnp(struct net_device *dev)
 {
 	int i;
@@ -329,7 +335,7 @@ static int __init ne_probe1(struct net_device *dev, unsigned long ioaddr)
 	   with an otherwise unused dev->mem_end value of "0xBAD" will
 	   cause the driver to skip these parts of the probe. */
 
-	bad_card = ((dev->base_addr != 0) && (dev->mem_end == 0xbad));
+	bad_card = ((dev->base_addr != 0) && (dev->mem_end == BAD));
 
 	/* Reset card. Who knows what dain-bramaged state it was left in. */
 
@@ -806,39 +812,65 @@ retry:
 static int __init ne_drv_probe(struct platform_device *pdev)
 {
 	struct net_device *dev;
-	struct resource *res;
-	int err, irq;
-
-	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	irq = platform_get_irq(pdev, 0);
-	if (!res || irq < 0)
-		return -ENODEV;
+	int err, this_dev = pdev->id;
 
 	dev = alloc_eip_netdev();
 	if (!dev)
 		return -ENOMEM;
-	dev->irq = irq;
-	dev->base_addr = res->start;
+	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;
 	return 0;
 }
 
-static int __exit ne_drv_remove(struct platform_device *pdev)
+static int ne_drv_remove(struct platform_device *pdev)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
 
-	unregister_netdev(dev);
-	free_irq(dev->irq, dev);
-	release_region(dev->base_addr, NE_IO_EXTENT);
-	free_netdev(dev);
+	if (dev) {
+		struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
+		netif_device_detach(dev);
+		unregister_netdev(dev);
+		if (idev)
+			pnp_device_detach(idev);
+		/* Careful ne_drv_remove can be called twice, once from
+		 * the platform_driver.remove and again when the
+		 * platform_device is being removed.
+		 */
+		ei_status.priv = 0;
+		free_irq(dev->irq, dev);
+		release_region(dev->base_addr, NE_IO_EXTENT);
+		free_netdev(dev);
+		platform_set_drvdata(pdev, NULL);
+	}
 	return 0;
 }
 
+/* Remove unused devices or all if true. */
+static void ne_loop_rm_unreg(int all)
+{
+	int this_dev;
+	struct platform_device *pdev;
+	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
+		pdev = pdev_ne[this_dev];
+		/* No network device == unused */
+		if (pdev && (!platform_get_drvdata(pdev) || all)) {
+			ne_drv_remove(pdev);
+			platform_device_unregister(pdev);
+			pdev_ne[this_dev] = NULL;
+		}
+	}
+}
+
 #ifdef CONFIG_PM
 static int ne_drv_suspend(struct platform_device *pdev, pm_message_t state)
 {
@@ -866,7 +898,7 @@ static int ne_drv_resume(struct platform_device *pdev)
 #endif
 
 static struct platform_driver ne_driver = {
-	.remove		= __exit_p(ne_drv_remove),
+	.remove		= ne_drv_remove,
 	.suspend	= ne_drv_suspend,
 	.resume		= ne_drv_resume,
 	.driver		= {
@@ -875,91 +907,96 @@ static struct platform_driver ne_driver = {
 	},
 };
 
-static int __init ne_init(void)
+static void __init ne_add_devices(void)
 {
-	return platform_driver_probe(&ne_driver, ne_drv_probe);
-}
+	int this_dev;
+	struct platform_device *pdev;
 
-static void __exit ne_exit(void)
-{
-	platform_driver_unregister(&ne_driver);
+	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
+		if (pdev_ne[this_dev])
+			continue;
+		pdev = platform_device_register_simple(
+			DRV_NAME, this_dev, NULL, 0);
+		if (IS_ERR(pdev))
+			continue;
+		pdev_ne[this_dev] = pdev;
+	}
 }
 
 #ifdef MODULE
-#define MAX_NE_CARDS	4	/* Max number of NE cards per module */
-static struct net_device *dev_ne[MAX_NE_CARDS];
-static int io[MAX_NE_CARDS];
-static int irq[MAX_NE_CARDS];
-static int bad[MAX_NE_CARDS];	/* 0xbad = bad sig or no reset ack */
-
-module_param_array(io, int, NULL, 0);
-module_param_array(irq, int, NULL, 0);
-module_param_array(bad, int, NULL, 0);
-MODULE_PARM_DESC(io, "I/O base address(es),required");
-MODULE_PARM_DESC(irq, "IRQ number(s)");
-MODULE_PARM_DESC(bad, "Accept card(s) with bad signatures");
-MODULE_DESCRIPTION("NE1000/NE2000 ISA/PnP Ethernet driver");
-MODULE_LICENSE("GPL");
-
-/* This is set up so that no ISA autoprobe takes place. We can't guarantee
-that the ne2k probe is the last 8390 based probe to take place (as it
-is at boot) and so the probe will get confused by any other 8390 cards.
-ISA device autoprobes on a running machine are not recommended anyway. */
-
-int __init init_module(void)
+int __init init_module()
 {
-	int this_dev, found = 0;
-	int plat_found = !ne_init();
-
-	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
-		struct net_device *dev = alloc_eip_netdev();
-		if (!dev)
-			break;
-		dev->irq = irq[this_dev];
-		dev->mem_end = bad[this_dev];
-		dev->base_addr = io[this_dev];
-		if (do_ne_probe(dev) == 0) {
-			dev_ne[found++] = dev;
-			continue;
-		}
-		free_netdev(dev);
-		if (found || plat_found)
-			break;
-		if (io[this_dev] != 0)
-			printk(KERN_WARNING "ne.c: No NE*000 card found at i/o = %#x\n", io[this_dev]);
-		else
-			printk(KERN_NOTICE "ne.c: You must supply \"io=0xNNN\" value(s) for ISA cards.\n");
-		return -ENXIO;
+	int retval;
+	ne_add_devices();
+	retval = platform_driver_probe(&ne_driver, ne_drv_probe);
+	if (retval) {
+		if (io[0] == 0)
+			printk(KERN_NOTICE "ne.c: You must supply \"io=0xNNN\""
+				" value(s) for ISA cards.\n");
+		ne_loop_rm_unreg(1);
+		return retval;
 	}
-	if (found || plat_found)
-		return 0;
-	return -ENODEV;
-}
 
-static void cleanup_card(struct net_device *dev)
+	/* Unregister unused platform_devices. */
+	ne_loop_rm_unreg(0);
+	return retval;
+}
+#else /* MODULE */
+static int __init ne_init(void)
 {
-	struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
-	if (idev)
-		pnp_device_detach(idev);
-	free_irq(dev->irq, dev);
-	release_region(dev->base_addr, NE_IO_EXTENT);
+	int retval = platform_driver_probe(&ne_driver, ne_drv_probe);
+
+	/* Unregister unused platform_devices. */
+	ne_loop_rm_unreg(0);
+	return retval;
 }
+module_init(ne_init);
 
-void __exit cleanup_module(void)
+struct net_device * __init ne_probe(int unit)
 {
 	int this_dev;
+	struct net_device *dev;
+
+	/* Find an empty slot, that is no net_device and zero io port. */
+	this_dev = 0;
+	while ((pdev_ne[this_dev] && platform_get_drvdata(pdev_ne[this_dev])) ||
+		io[this_dev]) {
+		if (++this_dev == MAX_NE_CARDS)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	/* Get irq, io from kernel command line */
+	dev = alloc_eip_netdev();
+	if (!dev)
+		return ERR_PTR(-ENOMEM);
+
+	sprintf(dev->name, "eth%d", unit);
+	netdev_boot_setup_check(dev);
 
-	ne_exit();
+	io[this_dev] = dev->base_addr;
+	irq[this_dev] = dev->irq;
+	bad[this_dev] = dev->mem_end;
+
+	free_netdev(dev);
+
+	ne_add_devices();
+
+	/* return the first device found */
 	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
-		struct net_device *dev = dev_ne[this_dev];
-		if (dev) {
-			unregister_netdev(dev);
-			cleanup_card(dev);
-			free_netdev(dev);
+		if (pdev_ne[this_dev]) {
+			dev = platform_get_drvdata(pdev_ne[this_dev]);
+			if (dev)
+				return dev;
 		}
 	}
+
+	return ERR_PTR(-ENODEV);
 }
-#else /* MODULE */
-module_init(ne_init);
-module_exit(ne_exit);
 #endif /* MODULE */
+
+static void __exit ne_exit(void)
+{
+	platform_driver_unregister(&ne_driver);
+	ne_loop_rm_unreg(1);
+}
+module_exit(ne_exit);
-- 
1.4.4.4

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

[David Fries]

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

Re: [PATCH] ne.c fix for hibernate and rmmod oops fix

[Atsushi Nemoto]

On Mon, 8 Sep 2008 21:54:34 -0500, David Fries <david@fries.net> wrote:
> 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.

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

Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements

[Atsushi Nemoto]

On Mon, 8 Sep 2008 21:58:21 -0500, David Fries <david@fries.net> wrote:
>  static int __init ne_drv_probe(struct platform_device *pdev)
>  {
>  	struct net_device *dev;
> -	struct resource *res;
> -	int err, irq;
> -
> -	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> -	irq = platform_get_irq(pdev, 0);
> -	if (!res || irq < 0)
> -		return -ENODEV;
> +	int err, this_dev = pdev->id;
>  
>  	dev = alloc_eip_netdev();
>  	if (!dev)
>  		return -ENOMEM;
> -	dev->irq = irq;
> -	dev->base_addr = res->start;
> +	dev->base_addr = io[this_dev];
> +	dev->irq = irq[this_dev];
> +	dev->mem_end = bad[this_dev];
>  	err = do_ne_probe(dev);

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

Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements

[David Fries]

On Thu, Sep 11, 2008 at 10:55:03PM +0900, Atsushi Nemoto wrote:
> This breaks current users of ne platform driver.  Here is a quick fix
> upon your patch.

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)

Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements

[Atsushi Nemoto]

On Sat, 13 Sep 2008 09:44:57 -0500, David Fries <david@fries.net> wrote:
> 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
...
>  	/* 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;
>  }

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

---
Atsushi Nemoto

Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements

[David Fries]

On Sun, Sep 14, 2008 at 12:13:00AM +0900, Atsushi Nemoto wrote:
> On Sat, 13 Sep 2008 09:44:57 -0500, David Fries <david@fries.net> wrote:
> > 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
> ...
> >  	/* 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;
> >  }
> 
> For now rbtx49xx ne platform devices use -1 for pdev->id, but it may
> change.  Please don't assume it.

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)

Re: [PATCH 1/2] [PATCH] ne.c fix rmmod, platform driver improvements

[Atsushi Nemoto]

On Sat, 13 Sep 2008 10:19:06 -0500, David Fries <david@fries.net> wrote:
> > For now rbtx49xx ne platform devices use -1 for pdev->id, but it may
> > change.  Please don't assume it.
> 
> Does this work?
> -	if (0 <= this_dev && this_dev < MAX_NE_CARDS) {
> +	if (!res) {

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

[PATCH 1/2] ne.c fix rmmod, platform driver improvements

[David Fries]

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, 0);
+module_param_array(irq, int, NULL, 0);
+module_param_array(bad, int, NULL, 0);
+MODULE_PARM_DESC(io, "I/O base address(es),required");
+MODULE_PARM_DESC(irq, "IRQ number(s)");
+MODULE_PARM_DESC(bad, "Accept card(s) with bad signatures");
+MODULE_DESCRIPTION("NE1000/NE2000 ISA/PnP Ethernet driver");
+MODULE_LICENSE("GPL");
+#endif /* MODULE */
 
 /* Do we perform extra sanity checks on stuff ? */
 /* #define NE_SANITY_CHECK */
@@ -74,6 +93,10 @@ static const char version2[] =
 /* Do we have a non std. amount of memory? (in units of 256 byte pages) */
 /* #define PACKETBUF_MEMSIZE	0x40 */
 
+/* This is set up so that no ISA autoprobe takes place. We can't guarantee
+that the ne2k probe is the last 8390 based probe to take place (as it
+is at boot) and so the probe will get confused by any other 8390 cards.
+ISA device autoprobes on a running machine are not recommended anyway. */
 #if !defined(MODULE) && (defined(CONFIG_ISA) || defined(CONFIG_M32R))
 /* Do we need a portlist for the ISA auto-probe ? */
 #define NEEDS_PORTLIST
@@ -192,8 +215,13 @@ static int __init do_ne_probe(struct net_device *dev)
 #endif
 
 	/* First check any supplied i/o locations. User knows best. <cough> */
-	if (base_addr > 0x1ff)	/* Check a single specified location. */
-		return ne_probe1(dev, base_addr);
+	if (base_addr > 0x1ff) {	/* Check a single specified location. */
+		int ret = ne_probe1(dev, base_addr);
+		if (ret)
+			printk(KERN_WARNING "ne.c: No NE*000 card found at "
+				"i/o = %#lx\n", base_addr);
+		return ret;
+	}
 	else if (base_addr != 0)	/* Don't probe at all. */
 		return -ENXIO;
 
@@ -214,28 +242,6 @@ static int __init do_ne_probe(struct net_device *dev)
 	return -ENODEV;
 }
 
-#ifndef MODULE
-struct net_device * __init ne_probe(int unit)
-{
-	struct net_device *dev = alloc_eip_netdev();
-	int err;
-
-	if (!dev)
-		return ERR_PTR(-ENOMEM);
-
-	sprintf(dev->name, "eth%d", unit);
-	netdev_boot_setup_check(dev);
-
-	err = do_ne_probe(dev);
-	if (err)
-		goto out;
-	return dev;
-out:
-	free_netdev(dev);
-	return ERR_PTR(err);
-}
-#endif
-
 static int __init ne_probe_isapnp(struct net_device *dev)
 {
 	int i;
@@ -329,7 +335,7 @@ static int __init ne_probe1(struct net_device *dev, unsigned long ioaddr)
 	   with an otherwise unused dev->mem_end value of "0xBAD" will
 	   cause the driver to skip these parts of the probe. */
 
-	bad_card = ((dev->base_addr != 0) && (dev->mem_end == 0xbad));
+	bad_card = ((dev->base_addr != 0) && (dev->mem_end == BAD));
 
 	/* Reset card. Who knows what dain-bramaged state it was left in. */
 
@@ -806,39 +812,84 @@ retry:
 static int __init ne_drv_probe(struct platform_device *pdev)
 {
 	struct net_device *dev;
+	int err, this_dev = pdev->id;
 	struct resource *res;
-	int err, irq;
-
-	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	irq = platform_get_irq(pdev, 0);
-	if (!res || irq < 0)
-		return -ENODEV;
 
 	dev = alloc_eip_netdev();
 	if (!dev)
 		return -ENOMEM;
-	dev->irq = irq;
-	dev->base_addr = res->start;
+
+	/* 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 {
+		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];
+	}
 	err = do_ne_probe(dev);
 	if (err) {
 		free_netdev(dev);
 		return err;
 	}
 	platform_set_drvdata(pdev, dev);
+
+	/* Update with any values found by probing, don't update if
+	 * resources were specified.
+	 */
+	if (!res) {
+		io[this_dev] = dev->base_addr;
+		irq[this_dev] = dev->irq;
+	}
 	return 0;
 }
 
-static int __exit ne_drv_remove(struct platform_device *pdev)
+static int ne_drv_remove(struct platform_device *pdev)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
 
-	unregister_netdev(dev);
-	free_irq(dev->irq, dev);
-	release_region(dev->base_addr, NE_IO_EXTENT);
-	free_netdev(dev);
+	if (dev) {
+		struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
+		netif_device_detach(dev);
+		unregister_netdev(dev);
+		if (idev)
+			pnp_device_detach(idev);
+		/* Careful ne_drv_remove can be called twice, once from
+		 * the platform_driver.remove and again when the
+		 * platform_device is being removed.
+		 */
+		ei_status.priv = 0;
+		free_irq(dev->irq, dev);
+		release_region(dev->base_addr, NE_IO_EXTENT);
+		free_netdev(dev);
+		platform_set_drvdata(pdev, NULL);
+	}
 	return 0;
 }
 
+/* Remove unused devices or all if true. */
+static void ne_loop_rm_unreg(int all)
+{
+	int this_dev;
+	struct platform_device *pdev;
+	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
+		pdev = pdev_ne[this_dev];
+		/* No network device == unused */
+		if (pdev && (!platform_get_drvdata(pdev) || all)) {
+			ne_drv_remove(pdev);
+			platform_device_unregister(pdev);
+			pdev_ne[this_dev] = NULL;
+		}
+	}
+}
+
 #ifdef CONFIG_PM
 static int ne_drv_suspend(struct platform_device *pdev, pm_message_t state)
 {
@@ -866,7 +917,7 @@ static int ne_drv_resume(struct platform_device *pdev)
 #endif
 
 static struct platform_driver ne_driver = {
-	.remove		= __exit_p(ne_drv_remove),
+	.remove		= ne_drv_remove,
 	.suspend	= ne_drv_suspend,
 	.resume		= ne_drv_resume,
 	.driver		= {
@@ -875,91 +926,96 @@ static struct platform_driver ne_driver = {
 	},
 };
 
-static int __init ne_init(void)
+static void __init ne_add_devices(void)
 {
-	return platform_driver_probe(&ne_driver, ne_drv_probe);
-}
+	int this_dev;
+	struct platform_device *pdev;
 
-static void __exit ne_exit(void)
-{
-	platform_driver_unregister(&ne_driver);
+	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
+		if (pdev_ne[this_dev])
+			continue;
+		pdev = platform_device_register_simple(
+			DRV_NAME, this_dev, NULL, 0);
+		if (IS_ERR(pdev))
+			continue;
+		pdev_ne[this_dev] = pdev;
+	}
 }
 
 #ifdef MODULE
-#define MAX_NE_CARDS	4	/* Max number of NE cards per module */
-static struct net_device *dev_ne[MAX_NE_CARDS];
-static int io[MAX_NE_CARDS];
-static int irq[MAX_NE_CARDS];
-static int bad[MAX_NE_CARDS];	/* 0xbad = bad sig or no reset ack */
-
-module_param_array(io, int, NULL, 0);
-module_param_array(irq, int, NULL, 0);
-module_param_array(bad, int, NULL, 0);
-MODULE_PARM_DESC(io, "I/O base address(es),required");
-MODULE_PARM_DESC(irq, "IRQ number(s)");
-MODULE_PARM_DESC(bad, "Accept card(s) with bad signatures");
-MODULE_DESCRIPTION("NE1000/NE2000 ISA/PnP Ethernet driver");
-MODULE_LICENSE("GPL");
-
-/* This is set up so that no ISA autoprobe takes place. We can't guarantee
-that the ne2k probe is the last 8390 based probe to take place (as it
-is at boot) and so the probe will get confused by any other 8390 cards.
-ISA device autoprobes on a running machine are not recommended anyway. */
-
-int __init init_module(void)
+int __init init_module()
 {
-	int this_dev, found = 0;
-	int plat_found = !ne_init();
-
-	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
-		struct net_device *dev = alloc_eip_netdev();
-		if (!dev)
-			break;
-		dev->irq = irq[this_dev];
-		dev->mem_end = bad[this_dev];
-		dev->base_addr = io[this_dev];
-		if (do_ne_probe(dev) == 0) {
-			dev_ne[found++] = dev;
-			continue;
-		}
-		free_netdev(dev);
-		if (found || plat_found)
-			break;
-		if (io[this_dev] != 0)
-			printk(KERN_WARNING "ne.c: No NE*000 card found at i/o = %#x\n", io[this_dev]);
-		else
-			printk(KERN_NOTICE "ne.c: You must supply \"io=0xNNN\" value(s) for ISA cards.\n");
-		return -ENXIO;
+	int retval;
+	ne_add_devices();
+	retval = platform_driver_probe(&ne_driver, ne_drv_probe);
+	if (retval) {
+		if (io[0] == 0)
+			printk(KERN_NOTICE "ne.c: You must supply \"io=0xNNN\""
+				" value(s) for ISA cards.\n");
+		ne_loop_rm_unreg(1);
+		return retval;
 	}
-	if (found || plat_found)
-		return 0;
-	return -ENODEV;
-}
 
-static void cleanup_card(struct net_device *dev)
+	/* Unregister unused platform_devices. */
+	ne_loop_rm_unreg(0);
+	return retval;
+}
+#else /* MODULE */
+static int __init ne_init(void)
 {
-	struct pnp_dev *idev = (struct pnp_dev *)ei_status.priv;
-	if (idev)
-		pnp_device_detach(idev);
-	free_irq(dev->irq, dev);
-	release_region(dev->base_addr, NE_IO_EXTENT);
+	int retval = platform_driver_probe(&ne_driver, ne_drv_probe);
+
+	/* Unregister unused platform_devices. */
+	ne_loop_rm_unreg(0);
+	return retval;
 }
+module_init(ne_init);
 
-void __exit cleanup_module(void)
+struct net_device * __init ne_probe(int unit)
 {
 	int this_dev;
+	struct net_device *dev;
+
+	/* Find an empty slot, that is no net_device and zero io port. */
+	this_dev = 0;
+	while ((pdev_ne[this_dev] && platform_get_drvdata(pdev_ne[this_dev])) ||
+		io[this_dev]) {
+		if (++this_dev == MAX_NE_CARDS)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	/* Get irq, io from kernel command line */
+	dev = alloc_eip_netdev();
+	if (!dev)
+		return ERR_PTR(-ENOMEM);
 
-	ne_exit();
+	sprintf(dev->name, "eth%d", unit);
+	netdev_boot_setup_check(dev);
+
+	io[this_dev] = dev->base_addr;
+	irq[this_dev] = dev->irq;
+	bad[this_dev] = dev->mem_end;
+
+	free_netdev(dev);
+
+	ne_add_devices();
+
+	/* return the first device found */
 	for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
-		struct net_device *dev = dev_ne[this_dev];
-		if (dev) {
-			unregister_netdev(dev);
-			cleanup_card(dev);
-			free_netdev(dev);
+		if (pdev_ne[this_dev]) {
+			dev = platform_get_drvdata(pdev_ne[this_dev]);
+			if (dev)
+				return dev;
 		}
 	}
+
+	return ERR_PTR(-ENODEV);
 }
-#else /* MODULE */
-module_init(ne_init);
-module_exit(ne_exit);
 #endif /* MODULE */
+
+static void __exit ne_exit(void)
+{
+	platform_driver_unregister(&ne_driver);
+	ne_loop_rm_unreg(1);
+}
+module_exit(ne_exit);
-- 
1.4.4.4

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

[David Fries]

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

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

[Jeff Garzik]

David Fries wrote:
> 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

applied

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

[David Fries]

On Sat, Sep 13, 2008 at 03:55:40PM -0400, Jeff Garzik wrote:
> David Fries wrote:
> >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
> 
> applied

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)

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

[Jeff Garzik]

David Fries wrote:
> On Sat, Sep 13, 2008 at 03:55:40PM -0400, Jeff Garzik wrote:
>> David Fries wrote:
>>> 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
>> applied
> 
> 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".  

Thanks for noting that...