Patch: linux-2.5.45/drivers/base/bus.c - new field to consolidate memory allocation in many drivers

Patch: linux-2.5.45/drivers/base/bus.c - new field to consolidate memory allocation in many drivers

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 3561 bytes --]

	I believe that the following little change will enable
elimination of a kmalloc/kfree pair from many device drivers,
and, more importantly, eliminate the rarely tested often buggy
error leg for dealing with that memory allocation failure.

	This patch allows device drivers to tell the generic device
code to handle allocating the per-device blob of memory that is
normally stored in device.driver_data.  It does so by adding a new
field device_driver.drvdata_size, with the following semantics:

	0	- Do not attempt to allocate or free device.driver_data.
		  This is compatible with previous behavior (although we
		  now initialize driver_data to NULL before calling the
		  probe routine).

	>0	- Allocate the specified number of bytes, fill it with
		  all zeroes, and store the address in device.driver_data
		  before calling the device's probe routine.  Abort the
		  probe with -ENOMEM if memory allocation fails.  Free the
		  storage if the probe fails or when the driver is
		  removed.  (The allocation and freeing mechanisms are
		  not specified, although the current mechanism uses
		  kmalloc/kfree).

	-1	- Set device.driver_data to NULL before calling the probe
		  routine.  When the probe routine returns failure and
		  when the driver is removed, check the value of
		  device.driver_data.  If it is non-NULL, kfree it.
		  This is handy for drivers that want to do their own
		  memory allocation.  This is handy if the driver needs
		  a variable sized block or really really really does not
		  want the allocated memory initialized to all zeroes.

	You may wonder about the trade-off of initializing the
allocated memory to all zeroes (in the drvdata_size > 0 case).  Driver
probes are executed relatively rarely (as opposed to an IO path that
might get executed a thousand times per second) and it is hard to
identify bugs due to uninitialized values.  The structures that the
probles fill in often gain new parameters over time, so it is easy to
make initialization bugs that gcc cannot easily detect.  It also makes
it easier to design new fields in these structures where a value of
zero will provide backward compatible behavior.  A small bonus is that
driver initialization code only needs to fill in non-zero values
explicitly.

	If there is a case where the performance hit from clearing the
memory is sustantial, you could use drvdata_size = -1.  If it turns
out that the situation is common, we could have a flag to skip the
clearing of the memory, but I don't think that such cases will be
common.

	I realize that many drivers store the result of some high
level allocate routine in their private data (for example,
scsi_register).  Later, I intend to make versions of those routines
that take pointer to a block of zero-filled memory rather than calling
kmalloc themselves.

	Anyhow, here is the patch.  I would like to start writing
driver clean-ups that use this patch soon, so I would like to see this
addition integrated into the kernel as sson as possible.  I am running
a kernel with this patch compiled in now, although I have not yet
changed any drivers to take advantage of it.

	Pat, are you the person I should be submitting this patch
to?  Is there someone else I should be submitting this patch to?
Please let me know.  Thanks in advance.

-- 
Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

[-- Attachment #2: devalloc.diff --]
[-- Type: text/plain, Size: 1100 bytes --]

--- linux-2.5.45/include/linux/device.h	2002-10-30 16:43:40.000000000 -0800
+++ linux/include/linux/device.h	2002-11-02 05:20:36.000000000 -0800
@@ -114,6 +114,7 @@
 	char			* name;
 	struct bus_type		* bus;
 	struct device_class	* devclass;
+	int			drvdata_size;
 
 	rwlock_t		lock;
 	atomic_t		refcount;
--- linux-2.5.45/drivers/base/bus.c	2002-10-30 16:42:20.000000000 -0800
+++ linux/drivers/base/bus.c	2002-11-02 05:21:37.000000000 -0800
@@ -98,6 +98,17 @@
 {
 	int error = -ENODEV;
 	if (dev->bus->match(dev,drv)) {
+
+		if (drv->drvdata_size > 0) {
+			dev->driver_data = kmalloc(drv->drvdata_size);
+			if (dev->driver_data)
+				memset(dev->driver_data, 0, drv->drvdata_size);
+			else
+				return -ENOMEM;
+		}
+		else
+			dev->driver_data = NULL;
+
 		dev->driver = drv;
 		if (drv->probe) {
 			if (!(error = drv->probe(dev)))
@@ -166,6 +177,12 @@
 		devclass_remove_device(dev);
 		if (drv->remove)
 			drv->remove(dev);
+		if (drv->drvdata_size) {
+			if (dev->driver_data)
+				kfree(dev->driver_data);
+			else
+				BUG_ON(drv->drvdata_size != -1);
+		}
 		dev->driver = NULL;
 	}
 }

Re: Patch: linux-2.5.45/drivers/base/bus.c - new field to consolidate memory allocation in many drivers

[Greg KH]

On Sat, Nov 02, 2002 at 11:29:51AM -0800, Adam J. Richter wrote:
> 
> 	This patch allows device drivers to tell the generic device
> code to handle allocating the per-device blob of memory that is
> normally stored in device.driver_data.  It does so by adding a new
> field device_driver.drvdata_size, with the following semantics:

Ugh, have you tried to use this patch in real life with the busses that
use driver_data today?  I didn't think so :)

In short, only the driver's individual probe function knows how big of a
data chunk it needs, and that probe function isn't known until probe()
is called.  Hm, well ok, match() could set this, but then it would have
to call into the function found by match to get the size.  By then it's
just really not worth adding this extra complexity to the code.

So in short, I don't like this, and don't really see where it buys you
anything.

But I did like your pci private data cleanup patch from the other day,
mind if I add that into the next round of pci cleanups I'm going to be
sending to Linus?

thanks,

greg k-h

Re: Patch: linux-2.5.45/drivers/base/bus.c - new field to consolidate memory allocation in many drivers

[Adam J. Richter]

>But I did like your pci private data cleanup patch from the other day,
>mind if I add that into the next round of pci cleanups I'm going to be
>sending to Linus?

	By all means, please integrate it and get it into Linus's tree
if you can.  Thanks for your assistance.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: Patch: linux-2.5.45/drivers/base/bus.c - new field to consolidate memory allocation in many drivers

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 2740 bytes --]

On Sat, Nov 02, 2002 at 12:42:58PM -0800, Greg KH wrote:
> On Sat, Nov 02, 2002 at 11:29:51AM -0800, Adam J. Richter wrote:
> > 
> > 	This patch allows device drivers to tell the generic device
> > code to handle allocating the per-device blob of memory that is
> > normally stored in device.driver_data.  It does so by adding a new
> > field device_driver.drvdata_size, with the following semantics:
> 
> Ugh, have you tried to use this patch in real life with the busses that
> use driver_data today?  I didn't think so :)
> 
> In short, only the driver's individual probe function knows how big of a
> data chunk it needs, and that probe function isn't known until probe()
> is called.  Hm, well ok, match() could set this, but then it would have
> to call into the function found by match to get the size.  By then it's
> just really not worth adding this extra complexity to the code.

	Although I had not converted any drivers when I wrote my
previous message, I am now writing this message on a machine using a
via-rhine ethernet driver that uses this facility.  The driver is
three lines smaller and, more importantly, has one less probably
untested error branch.  I think this example should address you
concerns.

	Note that because struct net_device may need to persist after
->remove() returns, I had to make a net_device.destructor function
(sharable with all similar network device drivers) rather than allow
drivers/base/bus.c to do the kfree.  If the same turns out to be true
for Scsi_Host, gendisk, etc., then I will scrap the kfree part of my
proposal.

	Drivers that really want to do a single variable-length
kmalloc for their private data will not use this facility, or will use
drvpriv_size = -1 if they want to just eliminate the kfree.  I think
those drivers will be rare if the allocators for the underlying
software abstractions (Scsi_Host, net_device, gendisk, etc.) are made
available that can just take a pointer to zero-filled memory rather
than doing the kmalloc themselves.

	This change may seem like small fry, but it's important to
understand that it will potentially eliminate a lot of untested error
branches and also that I plan some similar optional consolidation of
other driver resource allocations, especially those that can return
failure (DMA consistent memory, streaming DMA mappings, request_region
in ISA drivers, etc., but I'd like to do this incrementally).
Ultimately, this should produce smaller and more reliable drivers.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 4980 bytes --]

--- linux-2.5.45/include/linux/device.h	2002-10-30 16:43:40.000000000 -0800
+++ linux/include/linux/device.h	2002-11-02 05:20:36.000000000 -0800
@@ -114,6 +114,7 @@
 	char			* name;
 	struct bus_type		* bus;
 	struct device_class	* devclass;
+	int			drvdata_size;
 
 	rwlock_t		lock;
 	atomic_t		refcount;
--- linux-2.5.45/drivers/base/bus.c	2002-10-30 16:42:20.000000000 -0800
+++ linux/drivers/base/bus.c	2002-11-02 21:24:12.000000000 -0800
@@ -12,6 +12,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/errno.h>
+#include <linux/slab.h>
 #include "base.h"
 
 static LIST_HEAD(bus_driver_list);
@@ -94,16 +95,35 @@
 	list_add_tail(&dev->driver_list,&dev->driver->devices);
 }
 
+static void free_driver_data(struct device *dev)
+{
+	if (dev->driver->drvdata_size && dev->driver_data)
+		kfree(dev->driver_data);
+	dev->driver = NULL;
+}
+
 static int bus_match(struct device * dev, struct device_driver * drv)
 {
 	int error = -ENODEV;
 	if (dev->bus->match(dev,drv)) {
+
+		if (drv->drvdata_size > 0) {
+			dev->driver_data = kmalloc(drv->drvdata_size,
+						   GFP_KERNEL);
+			if (dev->driver_data)
+				memset(dev->driver_data, 0, drv->drvdata_size);
+			else
+				return -ENOMEM;
+		}
+		else
+			dev->driver_data = NULL;
+
 		dev->driver = drv;
 		if (drv->probe) {
 			if (!(error = drv->probe(dev)))
 				attach(dev);
 			else
-				dev->driver = NULL;
+				free_driver_data(dev);
 		} else 
 			attach(dev);
 	}
@@ -166,7 +186,7 @@
 		devclass_remove_device(dev);
 		if (drv->remove)
 			drv->remove(dev);
-		dev->driver = NULL;
+		free_driver_data(dev);
 	}
 }
 
--- linux-2.5.45/include/linux/netdevice.h	2002-10-30 16:43:43.000000000 -0800
+++ linux/include/linux/netdevice.h	2002-11-02 20:08:04.000000000 -0800
@@ -615,6 +623,8 @@
 	/* WILL BE REMOVED IN 2.5.0 */
 }
 
+extern void netdev_kfree_priv(struct net_device *dev);
+
 extern int netdev_finish_unregister(struct net_device *dev);
 
 static inline void dev_put(struct net_device *dev)
--- linux-2.5.45/net/core/dev.c	2002-10-30 16:42:56.000000000 -0800
+++ linux/net/core/dev.c	2002-11-02 20:08:16.000000000 -0800
@@ -2488,6 +2679,19 @@
 }
 
 /**
+ *	netdev_kfree_priv - Simple dev.destructor to free private data
+ *	@dev: device
+ *
+ *	Simply calls kfree(dev->priv).
+ */
+void
+netdev_kfree_priv(struct net_device *dev)
+{
+	kfree(dev->priv);
+}
+EXPORT_SYMBOL(netdev_kfree_priv);
+
+/**
  *	netdev_finish_unregister - complete unregistration
  *	@dev: device
  *
--- linux-2.5.45/drivers/net/via-rhine.c	2002-10-30 16:43:44.000000000 -0800
+++ linux/drivers/net/via-rhine.c	2002-11-02 21:47:02.000000000 -0800
@@ -501,6 +501,7 @@
 	unsigned int mii_cnt;			/* number of MIIs found, but only the first one is used */
 	u16 mii_status;						/* last read MII status */
 	struct mii_if_info mii_if;
+	struct net_device netdev;
 };
 
 static int  mdio_read(struct net_device *dev, int phy_id, int location);
@@ -571,8 +572,8 @@
 static int __devinit via_rhine_init_one (struct pci_dev *pdev,
 					 const struct pci_device_id *ent)
 {
-	struct net_device *dev;
-	struct netdev_private *np;
+	struct netdev_private *np = pci_get_drvdata(pdev);
+	struct net_device *dev = &np->netdev;
 	int i, option;
 	int chip_id = (int) ent->driver_data;
 	static int card_idx = -1;
@@ -618,15 +619,13 @@
 	if (pci_flags & PCI_USES_MASTER)
 		pci_set_master (pdev);
 
-	dev = alloc_etherdev(sizeof(*np));
-	if (dev == NULL) {
-		printk (KERN_ERR "init_ethernet failed for card #%d\n", card_idx);
+	if (pci_request_regions(pdev, shortname))
 		goto err_out;
-	}
+
+	init_etherdev(dev, 0);
 	SET_MODULE_OWNER(dev);
-	
-	if (pci_request_regions(pdev, shortname))
-		goto err_out_free_netdev;
+	dev->priv = np;
+	dev->destructor = netdev_kfree_priv;
 
 #ifdef USE_MEM
 	ioaddr0 = ioaddr;
@@ -707,7 +706,6 @@
 
 	dev->irq = pdev->irq;
 
-	np = dev->priv;
 	spin_lock_init (&np->lock);
 	np->chip_id = chip_id;
 	np->drv_flags = via_rhine_chip_info[chip_id].drv_flags;
@@ -760,8 +758,6 @@
 			printk("%2.2x:", dev->dev_addr[i]);
 	printk("%2.2x, IRQ %d.\n", dev->dev_addr[i], pdev->irq);
 
-	pci_set_drvdata(pdev, dev);
-
 	if (np->drv_flags & CanHaveMII) {
 		int phy, phy_idx = 0;
 		np->phys[0] = 1;		/* Standard for this chip. */
@@ -812,8 +808,6 @@
 err_out_free_res:
 #endif
 	pci_release_regions(pdev);
-err_out_free_netdev:
-	kfree (dev);
 err_out:
 	return -ENODEV;
 }
@@ -1730,7 +1724,8 @@
 
 static void __devexit via_rhine_remove_one (struct pci_dev *pdev)
 {
-	struct net_device *dev = pci_get_drvdata(pdev);
+	struct netdev_private *np = pci_get_drvdata(pdev);
+	struct net_device *dev = &np->netdev;
 	
 	unregister_netdev(dev);
 
@@ -1740,7 +1735,6 @@
 	iounmap((char *)(dev->base_addr));
 #endif
 
-	kfree(dev);
 	pci_disable_device(pdev);
 	pci_set_drvdata(pdev, NULL);
 }
@@ -1751,6 +1745,9 @@
 	.id_table	= via_rhine_pci_tbl,
 	.probe		= via_rhine_init_one,
 	.remove		= __devexit_p(via_rhine_remove_one),
+	.driver		= {
+		.drvdata_size = sizeof(struct netdev_private)
+	},
 };
 
 

Re: Patch: linux-2.5.45/drivers/base/bus.c - new field to consolidate memory allocation in many drivers

[Alan Cox]

On Sun, 2002-11-03 at 07:45, Adam J. Richter wrote:
> 	Note that because struct net_device may need to persist after
> ->remove() returns, I had to make a net_device.destructor function
> (sharable with all similar network device drivers) rather than allow
> drivers/base/bus.c to do the kfree.  If the same turns out to be true
> for Scsi_Host, gendisk, etc., then I will scrap the kfree part of my
> proposal.

It is certainly true for scsi devices, it will be true (once I get
around to it) for IDE devices

Re: Patch: linux-2.5.45/drivers/base/bus.c - new field to consolidate memory allocation in many drivers

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 1432 bytes --]

	Here is a new version of my patch that also allows drivers to
ask the generic driver code to allocate a fixed-size per-device blob of
consistent DMA memory.  It includes an example patch to via-rhine.c,
removing another 14 lines, including another memory allocation failure
error branch.  I am running the resulting via-rhine driver now.

	If I can eliminate an average of 5 line from each of the 557
modules in my modules.pcimap (never mind other busses for the moment),
that would be over 2000 lines of code.  More importantly, I'm sure it
would eliminate a lot of bugs in untested error branches.

	Dave M.: I am cc'ing this to because you asked for a
bus-independent API for allocating DMA consistent memory when I
discusssed consolidating some other driver DMA mapping operations
about six months ago.  This patch adds the wrapper functions
dma_{alloc,free}_consistent() for this purpose which are not optimized
right now, but provide an API and could be expanded to support more
the the DMA allocation API as it is actually used.  The implementation
can be optimized later, but I think it addresses your concern about
propagating artificial PCI dependence.

-- 
Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

[-- Attachment #2: dma.diff --]
[-- Type: text/plain, Size: 10402 bytes --]

--- linux-2.5.45/include/linux/dma-map.h	1969-12-31 16:00:00.000000000 -0800
+++ linux/include/linux/dma-map.h	2002-11-03 05:27:04.000000000 -0800
@@ -0,0 +1,53 @@
+/*
+ * Generic DMA region interface.
+ */
+
+#ifndef LINUX_DMA_MAP_H
+#define LINUX_DMA_MAP_H
+
+#include <asm/types.h>
+#include <linux/device.h>
+
+struct device;
+
+struct dma_ops {
+	void *(*alloc_consistent)(struct device *dev, size_t size,
+				  dma_addr_t *dma_handle);
+
+	void (*free_consistent)(struct device *dev, size_t size,
+				void *vaddr, dma_addr_t dma_handle);
+
+	/* Other operations (map_page, map_single, etc.) will be added
+	   when they are actually used. */
+};
+
+/* So we only have one place to change if dma_ops is moved: */
+static inline struct dma_ops *DEV_DMA_OPS(struct device *dev)
+{
+	return dev->bus->dma_ops;
+}
+
+static inline void *
+dma_alloc_consistent(struct device *dev, size_t size, dma_addr_t *dma_handle)
+{
+	void *ptr;
+	ptr = (*DEV_DMA_OPS(dev)->alloc_consistent)(dev, size, dma_handle);
+
+#if 0
+	/* Currently every alloc_consistent implementation replicates this.
+	   In the future, only do the memset here. */
+	if (ptr)
+		memset(ptr, 0, size);
+#endif
+
+	return ptr;
+}
+
+static inline void dma_free_consistent(struct device *dev, size_t size,
+				       void *vaddr, dma_addr_t dma_handle)
+{
+	(*DEV_DMA_OPS(dev)->free_consistent)(dev, size, vaddr, dma_handle);
+}
+
+
+#endif /* LINUX_DMA_MAP_H */
--- linux-2.5.45/drivers/pci/dma-ops.c	1969-12-31 16:00:00.000000000 -0800
+++ linux/drivers/pci/dma-ops.c	2002-11-03 05:31:09.000000000 -0800
@@ -0,0 +1,22 @@
+#include <linux/dma-map.h>
+#include <linux/pci.h>
+
+static void *_pci_alloc_consistent(struct device *dev, size_t size,
+				   dma_addr_t *dma_handle)
+{
+	struct pci_dev *pcidev = container_of(dev, struct pci_dev, dev);
+	return pci_alloc_consistent(pcidev, size, dma_handle);
+}
+
+static void _pci_free_consistent(struct device *dev, size_t size,
+				 void *vaddr, dma_addr_t dma_handle)
+{
+	struct pci_dev *pcidev = container_of(dev, struct pci_dev, dev);
+	pci_free_consistent(pcidev, size, vaddr, dma_handle);
+}
+
+
+struct dma_ops pci_dma_ops = {
+	.alloc_consistent =	_pci_alloc_consistent,
+	.free_consistent =	_pci_free_consistent,
+};
--- linux-2.5.45/include/linux/device.h	2002-10-30 16:43:40.000000000 -0800
+++ linux/include/linux/device.h	2002-11-03 05:04:52.000000000 -0800
@@ -58,6 +58,7 @@
 struct device;
 struct device_driver;
 struct device_class;
+struct dma_ops;
 
 struct bus_type {
 	char			* name;
@@ -68,6 +69,7 @@
 	struct list_head	node;
 	struct list_head	devices;
 	struct list_head	drivers;
+	struct dma_ops		*dma_ops; /* For CPU busses: pci, isa, sbus */
 
 	struct driver_dir_entry	dir;
 	struct driver_dir_entry	device_dir;
@@ -114,6 +116,8 @@
 	char			* name;
 	struct bus_type		* bus;
 	struct device_class	* devclass;
+	int			drv_alloc;
+	int			dma_alloc;
 
 	rwlock_t		lock;
 	atomic_t		refcount;
@@ -290,6 +294,8 @@
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
 	void		*driver_data;	/* data private to the driver */
+	void		*dma_vaddr;
+	dma_addr_t	dma_busaddr;
 
 	u32		class_num;	/* class-enumerated value */
 	void		* class_data;	/* class-specific data */
--- linux-2.5.45/drivers/base/bus.c	2002-10-30 16:42:20.000000000 -0800
+++ linux/drivers/base/bus.c	2002-11-03 06:21:16.000000000 -0800
@@ -12,6 +12,8 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/dma-map.h>
 #include "base.h"
 
 static LIST_HEAD(bus_driver_list);
@@ -94,16 +96,57 @@
 	list_add_tail(&dev->driver_list,&dev->driver->devices);
 }
 
+static void free_driver_data(struct device *dev, struct device_driver *drv)
+{
+	if (drv->drv_alloc && dev->driver_data)
+		kfree(dev->driver_data);
+
+	if (drv->dma_alloc && dev->dma_vaddr)
+		dma_free_consistent(dev, drv->dma_alloc,
+				    dev->dma_vaddr, dev->dma_busaddr);
+
+	dev->driver = NULL;
+}
+
+static int alloc_driver_data(struct device *dev, struct device_driver *drv)
+{
+	dev->driver_data = NULL;
+	dev->dma_vaddr = NULL;
+
+	if (drv->dma_alloc != 0) {
+		dev->dma_vaddr = dma_alloc_consistent(dev, drv->dma_alloc,
+						      &dev->dma_busaddr);
+		if (!dev->dma_vaddr)
+			return -ENOMEM;
+	}
+	if (drv->drv_alloc > 0) {
+		dev->driver_data = kmalloc(drv->drv_alloc,  GFP_KERNEL);
+		if (dev->driver_data)
+			memset(dev->driver_data, 0, drv->drv_alloc);
+		else {
+			free_driver_data(dev, drv);
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
 static int bus_match(struct device * dev, struct device_driver * drv)
 {
 	int error = -ENODEV;
 	if (dev->bus->match(dev,drv)) {
+		int alloc_err = alloc_driver_data(dev, drv);
+
+		if (alloc_err)
+			return alloc_err;
+
 		dev->driver = drv;
 		if (drv->probe) {
 			if (!(error = drv->probe(dev)))
 				attach(dev);
 			else
-				dev->driver = NULL;
+				free_driver_data(dev, drv);
 		} else 
 			attach(dev);
 	}
@@ -166,9 +209,9 @@
 		devclass_remove_device(dev);
 		if (drv->remove)
 			drv->remove(dev);
-		dev->driver = NULL;
+		free_driver_data(dev, drv);
 	}
 }
 
--- linux-2.5.45/include/linux/netdevice.h	2002-10-30 16:43:43.000000000 -0800
+++ linux/include/linux/netdevice.h	2002-11-02 20:08:04.000000000 -0800
@@ -615,8 +623,10 @@
 	/* WILL BE REMOVED IN 2.5.0 */
 }
 
+extern void netdev_kfree_priv(struct net_device *dev);
+
 extern int netdev_finish_unregister(struct net_device *dev);
 
 static inline void dev_put(struct net_device *dev)
--- linux-2.5.45/net/core/dev.c	2002-10-30 16:42:56.000000000 -0800
+++ linux/net/core/dev.c	2002-11-03 00:15:23.000000000 -0800
@@ -2488,6 +2679,19 @@
 }
 
 /**
+ *	netdev_kfree_priv - Simple dev.destructor to free private data
+ *	@dev: device
+ *
+ *	Simply calls kfree(dev->priv).
+ */
+void
+netdev_kfree_priv(struct net_device *dev)
+{
+	kfree(dev->priv);
+}
+EXPORT_SYMBOL(netdev_kfree_priv);
+
+/**
  *	netdev_finish_unregister - complete unregistration
  *	@dev: device
  *
@@ -2511,7 +2715,7 @@
 #endif
 	if (dev->destructor)
 		dev->destructor(dev);
-	if (dev->features & NETIF_F_DYNALLOC)
+	else if (dev->features & NETIF_F_DYNALLOC)
 		kfree(dev);
 	return 0;
 }
--- linux-2.5.45/drivers/net/via-rhine.c	2002-10-30 16:43:44.000000000 -0800
+++ linux/drivers/net/via-rhine.c	2002-11-03 06:09:29.000000000 -0800
@@ -143,7 +143,6 @@
 #define TX_QUEUE_LEN	10		/* Limit ring entries actually used.  */
 #define RX_RING_SIZE	16
 
-
 /* Operational parameters that usually are not changed. */
 
 /* Time in jiffies before concluding the transmitter is hung. */
@@ -441,6 +440,9 @@
 	u32 next_desc;
 };
 
+#define RX_RING_BYTES	(RX_RING_SIZE * sizeof(struct rx_desc))
+#define TX_RING_BYTES   (TX_RING_SIZE * sizeof(struct tx_desc))
+
 enum rx_status_bits {
 	RxOK=0x8000, RxWholePkt=0x0300, RxErr=0x008F
 };
@@ -501,6 +503,7 @@
 	unsigned int mii_cnt;			/* number of MIIs found, but only the first one is used */
 	u16 mii_status;						/* last read MII status */
 	struct mii_if_info mii_if;
+	struct net_device netdev;
 };
 
 static int  mdio_read(struct net_device *dev, int phy_id, int location);
@@ -571,8 +574,8 @@
 static int __devinit via_rhine_init_one (struct pci_dev *pdev,
 					 const struct pci_device_id *ent)
 {
-	struct net_device *dev;
-	struct netdev_private *np;
+	struct netdev_private *np = pci_get_drvdata(pdev);
+	struct net_device *dev = &np->netdev;
 	int i, option;
 	int chip_id = (int) ent->driver_data;
 	static int card_idx = -1;
@@ -618,15 +621,13 @@
 	if (pci_flags & PCI_USES_MASTER)
 		pci_set_master (pdev);
 
-	dev = alloc_etherdev(sizeof(*np));
-	if (dev == NULL) {
-		printk (KERN_ERR "init_ethernet failed for card #%d\n", card_idx);
+	if (pci_request_regions(pdev, shortname))
 		goto err_out;
-	}
+
+	init_etherdev(dev, 0);
 	SET_MODULE_OWNER(dev);
-	
-	if (pci_request_regions(pdev, shortname))
-		goto err_out_free_netdev;
+	dev->priv = np;
+	dev->destructor = netdev_kfree_priv;
 
 #ifdef USE_MEM
 	ioaddr0 = ioaddr;
@@ -707,7 +708,6 @@
 
 	dev->irq = pdev->irq;
 
-	np = dev->priv;
 	spin_lock_init (&np->lock);
 	np->chip_id = chip_id;
 	np->drv_flags = via_rhine_chip_info[chip_id].drv_flags;
@@ -760,8 +760,6 @@
 			printk("%2.2x:", dev->dev_addr[i]);
 	printk("%2.2x, IRQ %d.\n", dev->dev_addr[i], pdev->irq);
 
-	pci_set_drvdata(pdev, dev);
-
 	if (np->drv_flags & CanHaveMII) {
 		int phy, phy_idx = 0;
 		np->phys[0] = 1;		/* Standard for this chip. */
@@ -812,8 +810,6 @@
 err_out_free_res:
 #endif
 	pci_release_regions(pdev);
-err_out_free_netdev:
-	kfree (dev);
 err_out:
 	return -ENODEV;
 }
@@ -821,27 +817,14 @@
 static int alloc_ring(struct net_device* dev)
 {
 	struct netdev_private *np = dev->priv;
-	void *ring;
-	dma_addr_t ring_dma;
+	void *ring = np->pdev->dev.dma_vaddr;
+	dma_addr_t ring_dma = np->pdev->dev.dma_busaddr;
 
-	ring = pci_alloc_consistent(np->pdev, 
-				    RX_RING_SIZE * sizeof(struct rx_desc) +
-				    TX_RING_SIZE * sizeof(struct tx_desc),
-				    &ring_dma);
-	if (!ring) {
-		printk(KERN_ERR "Could not allocate DMA memory.\n");
-		return -ENOMEM;
-	}
 	if (np->drv_flags & ReqTxAlign) {
 		np->tx_bufs = pci_alloc_consistent(np->pdev, PKT_BUF_SZ * TX_RING_SIZE,
 								   &np->tx_bufs_dma);
-		if (np->tx_bufs == NULL) {
-			pci_free_consistent(np->pdev, 
-				    RX_RING_SIZE * sizeof(struct rx_desc) +
-				    TX_RING_SIZE * sizeof(struct tx_desc),
-				    ring, ring_dma);
+		if (np->tx_bufs == NULL)
 			return -ENOMEM;
-		}
 	}
 
 	np->rx_ring = ring;
@@ -856,10 +839,6 @@
 {
 	struct netdev_private *np = dev->priv;
 
-	pci_free_consistent(np->pdev, 
-			    RX_RING_SIZE * sizeof(struct rx_desc) +
-			    TX_RING_SIZE * sizeof(struct tx_desc),
-			    np->rx_ring, np->rx_ring_dma);
 	np->tx_ring = NULL;
 
 	if (np->tx_bufs)
@@ -1730,7 +1709,8 @@
 
 static void __devexit via_rhine_remove_one (struct pci_dev *pdev)
 {
-	struct net_device *dev = pci_get_drvdata(pdev);
+	struct netdev_private *np = pci_get_drvdata(pdev);
+	struct net_device *dev = &np->netdev;
 	
 	unregister_netdev(dev);
 
@@ -1740,7 +1720,6 @@
 	iounmap((char *)(dev->base_addr));
 #endif
 
-	kfree(dev);
 	pci_disable_device(pdev);
 	pci_set_drvdata(pdev, NULL);
 }
@@ -1751,6 +1730,10 @@
 	.id_table	= via_rhine_pci_tbl,
 	.probe		= via_rhine_init_one,
 	.remove		= __devexit_p(via_rhine_remove_one),
+	.driver		= {
+		.drv_alloc = sizeof(struct netdev_private),
+		.dma_alloc = RX_RING_BYTES + TX_RING_BYTES,
+	},
 };
 
 

Re: Patch: linux-2.5.45/drivers/base/bus.c - new field to consolidate memory allocation in many drivers

[David S. Miller]

   From: "Adam J. Richter" <adam@yggdrasil.com>
   Date: Sun, 3 Nov 2002 14:40:22 -0800
   
   	Dave M.: I am cc'ing this to because you asked for a
   bus-independent API for allocating DMA consistent memory when I
   discusssed consolidating some other driver DMA mapping operations
   about six months ago.

I don't know how much I like the DMA memory being allocated
transparently based upon some structure initialization values.

I'd rather the DMA alloc/free be explicit in the drivers.

Otherwise, the ->ops->alloc_consistent et al. abstraction
looks ok.

Re: Patch: linux-2.5.45/drivers/base/bus.c - new field to consolidate memory allocation in many drivers

[Adam J. Richter]

Dave Miller writes:
>I don't know how much I like the DMA memory being allocated
>transparently based upon some structure initialization values.
>
>I'd rather the DMA alloc/free be explicit in the drivers.

	I think I roughly understand your attitude.  Certainly I am of
wary adding more abstraction.  Programmers tend to get lost in it.  In
spite of the best intentions, code is often made less readable and
maintainable, bugs less apparent.

	However, at some point, abstraction can be worth it.  The
resulting code actually shirnking, accelerating, being clearer,
when the abstraction results in the removal of bugs, are all
metrics that I would look at.

	Lifting most of the memory allocation and DMA mapping out of
the drivers will remove thousands of lines from Linux drivers in
aggregate and remove hundreds of potentially buggy error branches.  It
will be a little easier see the hardware from reading the driver.
In comparison, the CPU costs are small and may be negative if
some of that consolidation allows for additional optimizations.

	I'd be interested in knowing how you quantify this trade-off
and what you think might persuade you to support or at least be
neutral toward this type of facility (results of converting drivers,
examples of buggy error branches?).  Please keep in mind that not all
drivers necessarily need to use these facilities.

>Otherwise, the ->ops->alloc_consistent et al. abstraction
>looks ok.

	Thanks.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."