[RFT][PATCH] cleanup net/atm/br2684.c

[RFT][PATCH] cleanup net/atm/br2684.c

[Stephen Hemminger]

I fixed up some things in br2684 but don't have ATM hardware to test it well enough.
The patch is against 2.6.0-test3 and module loads/unloads fine.

Fixed:
	- Allocate network device with alloc_netdev and embed private data via
	  dev->priv.  This allows for future fix of rmmod race with sysfs.
	- Get rid of all the MOD_INC stuff. MOD_INC is not a spinlock or semaphore
	  and don't use it like that!  Have driver clean itself up properly on
	  unload.
	- Add required owner field for /proc interface.  Thought about converting
	  to seq_file, but existing output format and ordering makes that hard.


diff -Nru a/net/atm/br2684.c b/net/atm/br2684.c
--- a/net/atm/br2684.c	Mon Aug 11 11:42:19 2003
+++ b/net/atm/br2684.c	Mon Aug 11 11:42:19 2003
@@ -81,7 +81,7 @@
 };
 
 struct br2684_dev {
-	struct net_device net_dev;
+	struct net_device *net_dev;
 	struct list_head br2684_devs;
 	int number;
 	struct list_head brvccs; /* one device <=> one vcc (before xmas) */
@@ -137,8 +137,8 @@
 	case BR2684_FIND_BYIFNAME:
 		list_for_each(lh, &br2684_devs) {
 			brdev = list_entry_brdev(lh);
-			if (!strncmp(brdev->net_dev.name, s->spec.ifname,
-			    sizeof brdev->net_dev.name))
+			if (!strncmp(brdev->net_dev->name, s->spec.ifname,
+				     IFNAMSIZ))
 				return brdev;
 		}
 		break;
@@ -400,7 +400,6 @@
 	brvcc->atmvcc->user_back = NULL;	/* what about vcc->recvq ??? */
 	brvcc->old_push(brvcc->atmvcc, NULL);	/* pass on the bad news */
 	kfree(brvcc);
-	MOD_DEC_USE_COUNT;
 }
 
 /* when AAL5 PDU comes in: */
@@ -418,8 +417,8 @@
 			read_lock(&devs_lock);
 			list_del(&brdev->br2684_devs);
 			read_unlock(&devs_lock);
-			unregister_netdev(&brdev->net_dev);
-			kfree(brdev);
+			unregister_netdev(brdev->net_dev);
+			kfree(brdev->net_dev);
 		}
 		return;
 	}
@@ -464,7 +463,7 @@
 #endif /* CONFIG_BR2684_FAST_TRANS */
 #else
 	skb_pull(skb, plen - ETH_HLEN);
-	skb->protocol = eth_type_trans(skb, &brdev->net_dev);
+	skb->protocol = eth_type_trans(skb, brdev->net_dev);
 #endif /* FASTER_VERSION */
 #ifdef CONFIG_ATM_BR2684_IPFILTER
 	if (packet_fails_filter(skb->protocol, brvcc, skb)) {
@@ -473,11 +472,11 @@
 		return;
 	}
 #endif /* CONFIG_ATM_BR2684_IPFILTER */
-	skb->dev = &brdev->net_dev;
+	skb->dev = brdev->net_dev;
 	ATM_SKB(skb)->vcc = atmvcc;	/* needed ? */
 	DPRINTK("received packet's protocol: %x\n", ntohs(skb->protocol));
 	skb_debug(skb);
-	if (!(brdev->net_dev.flags & IFF_UP)) { /* sigh, interface is down */
+	if (!(brdev->net_dev->flags & IFF_UP)) { /* sigh, interface is down */
 		brdev->stats.rx_dropped++;
 		dev_kfree_skb(skb);
 		return;
@@ -500,9 +499,7 @@
 	struct br2684_dev *brdev;
 	struct atm_backend_br2684 be;
 
-	MOD_INC_USE_COUNT;
 	if (copy_from_user(&be, (void *) arg, sizeof be)) {
-		MOD_DEC_USE_COUNT;
 		return -EFAULT;
 	}
 	write_lock_irq(&devs_lock);
@@ -539,10 +536,10 @@
 	if (list_empty(&brdev->brvccs) && !brdev->mac_was_set) {
 		unsigned char *esi = atmvcc->dev->esi;
 		if (esi[0] | esi[1] | esi[2] | esi[3] | esi[4] | esi[5])
-			memcpy(brdev->net_dev.dev_addr, esi,
-			    brdev->net_dev.addr_len);
+			memcpy(brdev->net_dev->dev_addr, esi,
+			    brdev->net_dev->addr_len);
 		else
-			brdev->net_dev.dev_addr[2] = 1;
+			brdev->net_dev->dev_addr[2] = 1;
 	}
 	list_add(&brvcc->brvccs, &brdev->brvccs);
 	write_unlock_irq(&devs_lock);
@@ -563,77 +560,69 @@
 	return 0;
     error:
 	write_unlock_irq(&devs_lock);
-	MOD_DEC_USE_COUNT;
 	return err;
 }
 
+static void br2684_setup(struct net_device *netdev)
+{
+	struct br2684_dev *brdev = netdev->priv;
+
+	ether_setup(netdev);
+	brdev->net_dev = netdev;
+
+#ifdef FASTER_VERSION
+	my_eth_header = netdev->hard_header;
+	netdev->hard_header = br2684_header;
+	my_eth_header_cache = netdev->hard_header_cache;
+	netdev->hard_header_cache = br2684_header_cache;
+	netdev->hard_header_len = sizeof(llc_oui_pid_pad) + ETH_HLEN;	/* 10 + 14 */
+#endif
+	my_eth_mac_addr = netdev->set_mac_address;
+	netdev->set_mac_address = br2684_mac_addr;
+	netdev->hard_start_xmit = br2684_start_xmit;
+	netdev->get_stats = br2684_get_stats;
+
+	INIT_LIST_HEAD(&brdev->brvccs);
+}
+
 static int br2684_create(unsigned long arg)
 {
 	int err;
+	struct net_device *netdev;
 	struct br2684_dev *brdev;
 	struct atm_newif_br2684 ni;
 
 	DPRINTK("br2684_create\n");
-	/*
-	 * We track module use by vcc's NOT the devices they're on.  We're
-	 * protected here against module death by the kernel_lock, but if
-	 * we need to sleep we should make sure that the module doesn't
-	 * disappear under us.
-	 */
-	MOD_INC_USE_COUNT;
+
 	if (copy_from_user(&ni, (void *) arg, sizeof ni)) {
-		MOD_DEC_USE_COUNT;
 		return -EFAULT;
 	}
 	if (ni.media != BR2684_MEDIA_ETHERNET || ni.mtu != 1500) {
-		MOD_DEC_USE_COUNT;
 		return -EINVAL;
 	}
-	if ((brdev = kmalloc(sizeof(struct br2684_dev), GFP_KERNEL)) == NULL) {
-		MOD_DEC_USE_COUNT;
-		return -ENOMEM;
-	}
-	memset(brdev, 0, sizeof(struct br2684_dev));
-	INIT_LIST_HEAD(&brdev->brvccs);
 
-	write_lock_irq(&devs_lock);
-	brdev->number = list_empty(&br2684_devs) ? 1 :
-	    list_entry_brdev(br2684_devs.prev)->number + 1;
-	list_add_tail(&brdev->br2684_devs, &br2684_devs);
-	write_unlock_irq(&devs_lock);
+	netdev = alloc_netdev(sizeof(struct br2684_dev),
+			      ni.ifname[0] ? ni.ifname : "nas%d",
+			      br2684_setup);
+	if (!netdev)
+		return -ENOMEM;
 
-	if (ni.ifname[0] != '\0') {
-		memcpy(brdev->net_dev.name, ni.ifname,
-		    sizeof(brdev->net_dev.name));
-		brdev->net_dev.name[sizeof(brdev->net_dev.name) - 1] = '\0';
-	} else
-		sprintf(brdev->net_dev.name, "nas%d", brdev->number);
-	DPRINTK("registered netdev %s\n", brdev->net_dev.name);
-	ether_setup(&brdev->net_dev);
-	brdev->mac_was_set = 0;
-#ifdef FASTER_VERSION
-	my_eth_header = brdev->net_dev.hard_header;
-	brdev->net_dev.hard_header = br2684_header;
-	my_eth_header_cache = brdev->net_dev.hard_header_cache;
-	brdev->net_dev.hard_header_cache = br2684_header_cache;
-	brdev->net_dev.hard_header_len = sizeof(llc_oui_pid_pad) + ETH_HLEN;	/* 10 + 14 */
-#endif
-	my_eth_mac_addr = brdev->net_dev.set_mac_address;
-	brdev->net_dev.set_mac_address = br2684_mac_addr;
-	brdev->net_dev.hard_start_xmit = br2684_start_xmit;
-	brdev->net_dev.get_stats = br2684_get_stats;
+	brdev = netdev->priv;
 
+	DPRINTK("registered netdev %s\n", netdev->name);
 	/* open, stop, do_ioctl ? */
-	err = register_netdev(&brdev->net_dev);
-	MOD_DEC_USE_COUNT;
+	err = register_netdev(netdev);
 	if (err < 0) {
 		printk(KERN_ERR "br2684_create: register_netdev failed\n");
-		write_lock_irq(&devs_lock);
-		list_del(&brdev->br2684_devs);
-		write_unlock_irq(&devs_lock);
-		kfree(brdev);
+		kfree(netdev);
 		return err;
 	}
+
+	write_lock_irq(&devs_lock);
+	brdev->number = list_empty(&br2684_devs) ? 1 :
+	    list_entry_brdev(br2684_devs.prev)->number + 1;
+	list_add_tail(&brdev->br2684_devs, &br2684_devs);
+	write_unlock_irq(&devs_lock);
 	return 0;
 }
 
@@ -649,9 +638,7 @@
 	case ATM_SETBACKEND:
 	case ATM_NEWBACKENDIF: {
 		atm_backend_t b;
-		MOD_INC_USE_COUNT;
 		err = get_user(b, (atm_backend_t *) arg);
-		MOD_DEC_USE_COUNT;
 		if (err)
 			return -EFAULT;
 		if (b != ATM_BACKEND_BR2684)
@@ -669,9 +656,7 @@
 			return -ENOIOCTLCMD;
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
-		MOD_INC_USE_COUNT;
 		err = br2684_setfilt(atmvcc, arg);
-		MOD_DEC_USE_COUNT;
 		return err;
 #endif /* CONFIG_ATM_BR2684_IPFILTER */
 	}
@@ -688,14 +673,14 @@
 		brdev = list_entry_brdev(lhd);
 		if (pos-- == 0)
 			return sprintf(buf, "dev %.16s: num=%d, mac=%02X:%02X:"
-			    "%02X:%02X:%02X:%02X (%s)\n", brdev->net_dev.name,
+			    "%02X:%02X:%02X:%02X (%s)\n", brdev->net_dev->name,
 			    brdev->number,
-			    brdev->net_dev.dev_addr[0],
-			    brdev->net_dev.dev_addr[1],
-			    brdev->net_dev.dev_addr[2],
-			    brdev->net_dev.dev_addr[3],
-			    brdev->net_dev.dev_addr[4],
-			    brdev->net_dev.dev_addr[5],
+			    brdev->net_dev->dev_addr[0],
+			    brdev->net_dev->dev_addr[1],
+			    brdev->net_dev->dev_addr[2],
+			    brdev->net_dev->dev_addr[3],
+			    brdev->net_dev->dev_addr[4],
+			    brdev->net_dev->dev_addr[5],
 			    brdev->mac_was_set ? "set" : "auto");
 		list_for_each(lhc, &brdev->brvccs) {
 			brvcc = list_entry_brvcc(lhc);
@@ -766,15 +751,13 @@
 }
 
 static struct file_operations br2684_proc_operations = {
-	read: br2684_proc_read,
+	.owner = THIS_MODULE,
+	.read =  br2684_proc_read,
 };
 
 extern struct proc_dir_entry *atm_proc_root;	/* from proc.c */
 
-/* the following avoids some spurious warnings from the compiler */
-#define UNUSED __attribute__((unused))
-
-static int __init UNUSED br2684_init(void)
+static int __init br2684_init(void)
 {
 	struct proc_dir_entry *p;
 	if ((p = create_proc_entry("br2684", 0, atm_proc_root)) == NULL)
@@ -784,16 +767,25 @@
 	return 0;
 }
 
-static void __exit UNUSED br2684_exit(void)
+static void __exit br2684_exit(void)
 {
 	struct br2684_dev *brdev;
+	struct br2684_vcc *brvcc;
+	
 	br2684_ioctl_set(NULL);
+
 	remove_proc_entry("br2684", atm_proc_root);
+
 	while (!list_empty(&br2684_devs)) {
 		brdev = list_entry_brdev(br2684_devs.next);
-		unregister_netdev(&brdev->net_dev);
+		while (!list_empty(&brdev->brvccs)) {
+			brvcc = list_entry_brvcc(brdev->brvccs.next);
+			br2684_close_vcc(brvcc);
+		}
+
+		unregister_netdev(brdev->net_dev);
 		list_del(&brdev->br2684_devs);
-		kfree(brdev);
+		kfree(brdev->net_dev);
 	}
 }
 

Re: [RFT][PATCH] cleanup net/atm/br2684.c

[Mitchell Blank Jr]

You might also want to send this patch to the linux-atm mailing list:
  linux-atm-general@lists.sourceforge.net
Unfortunately you need to subscribe to post.
  http://lists.sourceforge.net/lists/listinfo/linux-atm-general

Not sure how many ATM users are following 2.6 yet though.  I might be
able to give it a spin soon.

> -			unregister_netdev(&brdev->net_dev);
> -			kfree(brdev);
> +			unregister_netdev(brdev->net_dev);
> +			kfree(brdev->net_dev);

Mmmm.. I see this snippet in your diff in multiplce places and it looks
wrong.  Where does the "brdev" structure get freed now?

-Mitch

Re: [RFT][PATCH] cleanup net/atm/br2684.c

[Stephen Hemminger]

On Mon, 11 Aug 2003 14:58:57 -0700
Mitchell Blank Jr <mitch@sfgoth.com> wrote:

> You might also want to send this patch to the linux-atm mailing list:
>   linux-atm-general@lists.sourceforge.net
> Unfortunately you need to subscribe to post.
>   http://lists.sourceforge.net/lists/listinfo/linux-atm-general
> 
> Not sure how many ATM users are following 2.6 yet though.  I might be
> able to give it a spin soon.
> 
> > -			unregister_netdev(&brdev->net_dev);
> > -			kfree(brdev);
> > +			unregister_netdev(brdev->net_dev);
> > +			kfree(brdev->net_dev);
> 
> Mmmm.. I see this snippet in your diff in multiplce places and it looks
> wrong.  Where does the "brdev" structure get freed now?
> 
> -Mitch
The brdev structure is contained inside the allocation of net_device,
this is how all the ether and other devices do it.

net_dev   -->+-------------------+
             | network_device    |
             +-------------------+
             |/// padding ///////|
             +-------------------+
priv      -->|     brdev         |
             +-------------------+

	

Re: [RFT][PATCH] cleanup net/atm/br2684.c

[David S. Miller]

On Mon, 11 Aug 2003 11:48:23 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:

> I fixed up some things in br2684 but don't have ATM hardware to test it well enough.
> The patch is against 2.6.0-test3 and module loads/unloads fine.

I'll wait until Chas either ACKs this or sends it to me under
seperate cover.

Thanks.

Re: [RFT][PATCH] cleanup net/atm/br2684.c

[chas williams]

>	- Get rid of all the MOD_INC stuff. MOD_INC is not a spinlock or semaph
>ore
>	  and don't use it like that!  Have driver clean itself up properly on
>	  unload.

i actually have a patch for that already.  pppoatm has the same problem.
since both are now holding semaphores during ioctl most of the MOD_* stuff
can go away.  however, one pair needs to kept around in br2684 to keep the
module from being removed while the user space process is keeping the
vcc's in place.  this will go away when everything is converted to netlink.

>+	netdev = alloc_netdev(sizeof(struct br2684_dev),
>+			      ni.ifname[0] ? ni.ifname : "nas%d",
>+			      br2684_setup);
>+	if (!netdev)
>+		return -ENOMEM;
>...
>+
>+	write_lock_irq(&devs_lock);
>+	brdev->number = list_empty(&br2684_devs) ? 1 :
>+	    list_entry_brdev(br2684_devs.prev)->number + 1;
>+	list_add_tail(&brdev->br2684_devs, &br2684_devs);
>+	write_unlock_irq(&devs_lock);
> 	return 0;

this is a little different than the old code.  if the user didnt
provide an ifname, then brdev->number is used to determine the
name of the interface instead of letting the network stack choose
the next available interface name.  not sure if this really matters
to anyone.

Re: [RFT][PATCH] cleanup net/atm/br2684.c

[Mitchell Blank Jr]

Stephen Hemminger wrote:
> > Mmmm.. I see this snippet in your diff in multiplce places and it looks
> > wrong.  Where does the "brdev" structure get freed now?
> > 
> The brdev structure is contained inside the allocation of net_device,
> this is how all the ether and other devices do it.

OK, sorry... that should have been obvious.  The reason I was confused is
that almost always the net_dev is referenced as brdev->net_dev which makes
look like brdev merely has a reference to net_dev.  I think it would be
cleaner to change the code to pass around "net_dev" instead of "brdev"
references (and just get the brdev via net_dev->priv as needed) and just
get rid of the brdev->net_dev pointer entirely.

Are you interested in doing that?  Otherwise I could take a crack at it
when I get the chance (I've done some surgery on this file before)

-Mitch