lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Wed, 24 Sep 2008 20:50:07 -0400
From:	Jeff Garzik <jeff@...zik.org>
To:	akpm@...ux-foundation.org
CC:	netdev@...r.kernel.org, david@...es.net, alan@...rguk.ukuu.org.uk,
	anemo@....ocn.ne.jp, p_gortmaker@...oo.com
Subject: Re: [patch for 2.6.27? 06/10] ne.c: fix rmmod, platform driver improvements

akpm@...ux-foundation.org wrote:
> From: David Fries <david@...es.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@...es.net>
> Cc: Atsushi Nemoto <anemo@....ocn.ne.jp>
> Cc: Paul Gortmaker <p_gortmaker@...oo.com>
> Cc: Alan Cox <alan@...rguk.ukuu.org.uk>
> Cc: Jeff Garzik <jeff@...zik.org>
> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> ---
> 
>  drivers/net/ne.c |  272 +++++++++++++++++++++++++++------------------
>  1 file changed, 164 insertions(+), 108 deletions(-)
> 
> diff -puN drivers/net/ne.c~nec-fix-rmmod-platform-driver-improvements drivers/net/ne.c
> --- a/drivers/net/ne.c~nec-fix-rmmod-platform-driver-improvements
> +++ a/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
>  #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
>  	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_d
>  	   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
>  #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;
> +
> +	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;
> +	}
>  }
>  
> -static void __exit ne_exit(void)
> +#ifdef MODULE
> +int __init init_module()
>  {
> -	platform_driver_unregister(&ne_driver);
> +	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);
>  
> -#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 */
> +	/* Unregister unused platform_devices. */
> +	ne_loop_rm_unreg(0);
> +	return retval;
> +}
> +module_init(ne_init);
>  
> -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");
> +struct net_device * __init ne_probe(int unit)
> +{
> +	int this_dev;
> +	struct net_device *dev;
>  
> -/* 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. */
> +	/* 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);
> +	}
>  
> -int __init init_module(void)
> -{
> -	int this_dev, found = 0;
> -	int plat_found = !ne_init();
> +	/* 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++) {
> -		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;
> +		if (pdev_ne[this_dev]) {
> +			dev = platform_get_drvdata(pdev_ne[this_dev]);
> +			if (dev)
> +				return dev;
>  		}
> -		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 (found || plat_found)
> -		return 0;
> -	return -ENODEV;
> -}
>  
> -static void cleanup_card(struct net_device *dev)
> -{
> -	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);
> +	return ERR_PTR(-ENODEV);
>  }
> +#endif /* MODULE */
>  
> -void __exit cleanup_module(void)
> +static void __exit ne_exit(void)
>  {
> -	int this_dev;
> -
> -	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);
> -		}
> -	}
> +	platform_driver_unregister(&ne_driver);
> +	ne_loop_rm_unreg(1);
>  }
> -#else /* MODULE */
> -module_init(ne_init);
>  module_exit(ne_exit);
> -#endif /* MODULE */

applied


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists