Re: [parisc-linux] Untested port of parisc_device to generic device interface

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Adam J. Richter]

James Bottomley wrote:
>3. Machine is partially consistent.  consistent allocation may fail because 
>we're out of consistent memory so we have to fall back to the old.

	I'd like to know more about what these machines look
like in the real world.  Specifically, I am interested in the
trade-off of having a parameter to wback_fake_consistent
so that it could be enabled or disabled on an individual basis.

	I suspect that the parameter is not worth the clutter because
these "partially consistent" machines either have a large amount of
consistent memory, so the case of the allocation failing in the is not
worth supporting, or it is easy to check for consistent memory on them
with something like "if ((unsigned long) vaddr < 0xwhatever)", but I'm
just guessing.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Miplitas, 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: [parisc-linux] Untested port of parisc_device to generic device interface

[J.E.J. Bottomley]

adam@yggdrasil.com said:
> 
> 	I'd like to know more about what these machines look like in the real
> world.  Specifically, I am interested in the trade-off of having a
> parameter to wback_fake_consistent so that it could be enabled or
> disabled on an individual basis.

Actually, so would I.  I can suspect why there might exist machines like this 
(say the consistent attribute is settable at the pgd level)

> 	I suspect that the parameter is not worth the clutter because these
> "partially consistent" machines either have a large amount of
> consistent memory, so the case of the allocation failing in the is not
> worth supporting, or it is easy to check for consistent memory on them
> with something like "if ((unsigned long) vaddr < 0xwhatever)", but I'm
> just guessing. 

Well, if it has to be done, it can be done by making alloc_consistent return a 
handle rather than an address and making wback/invalidate take the handle (but 
it's certainly not ideal).

James

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Matthew Wilcox]

On Sat, Nov 09, 2002 at 09:01:29PM -0500, J.E.J. Bottomley wrote:
> Actually, so would I.  I can suspect why there might exist machines like this 
> (say the consistent attribute is settable at the pgd level)

well, there's a limited amount of space available for consistent mappings
on some machines.  it's basically the same as the vmalloc space.  i think
the best way to handle this is simply to fail to initialise if you can't
get the consistent memory you need.

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Adam J. Richter]

On Sat, Nov 09, 2002 at 05:21:50AM +0000, Matthew Wilcox wrote about
generic device models:
> And, for gods sake, don't fuck it up by integrating it with USB too early
> in the game.

	Without rewriting everything, I could see creating a common
device container for devices connected directly to the CPU's memory,
IO or interrupt busses (PCI, ISA, sbus, etc.) so as not to increase
the memory size of device and device_driver for non-mapped device
types (USB, firewire, SCSI, etc.).  More importantly, this change
would provide some type checking for operations that are really only
meaningful on CPU mapped devices, such as the "generic"
dma_{alloc,free}_consistent() and related functions.  I imagine
something like this:


struct cpu_mapped_device {
	dma_addr_t		dma_mask;
	/* Initialized from parent->dma_mask and driver->dma_restrictions. */
	void			*prealloc_vaddr;
	void 			*prealloc_busaddr;
	struct device		device;
};

struct cpu_mapped_driver {
	struct io_restrictions	*dma_restrictions;
	int			prealloc_consistent;
	int			fake_consistent_ok : 1;
	struct device_driver	driver;
};

struct device {
	...
	struct resource_list		*resources;
	/* For automatic release of memory ranges, IO ports, DMA channels,
	   IRQ's, SCSI ID's, SCSI LUN's within an ID, USB device ID's,
	   etc.  (but not USB hub ports, and PCMCIA slots, which we can
	   released before ->remove() is called). */

	dma_pool_t			dma_stub_pool[2];
	struct cpu_mapped_device	*dma_dev;
	/* Points to the cpu_mapped_device that we are embedded in for PCI,
	   ISA, and other cpu mapped devices.  Points to the parent
	   cpu mapped device for others, such as the USB controller for a
	   USB network interface.   Will be used for allocating DMA gather
	   scatter stubs when allocating skbuff's, bio's, scsi_request's,
	   USB request blocks, etc., eliminating another set of error
	   branches. */
};

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Miplitas, 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: [parisc-linux] Untested port of parisc_device to generic device interface

[Adam J. Richter]

Grant Grundler writes:
>Documentation/driver-model/overview.txt:
>| Note also that it is at the _end_ of struct pci_dev. This is
>| to make people think about what they're doing when switching between the bus
>| driver and the global driver; and to prevent against mindless casts between
>| the two.
>
>Until this changes, I don't see this as a useful replacement for
>either PCI or parisc devices. The "mindless casts" can be fixed.
>But without the ability to easily go from generic device type to
>bus specific type, people will just get lost in the maze of pointers.

	linux-2.5.46/include/linux/kernel.h already defines
container_of(ptr_to_element, parent_struct, element_name).

>From <linux/pci.h>:
#define to_pci_dev(n) container_of(n, struct pci_dev, dev)

>From <linux/usb.h>:
#define to_usb_device(d) container_of(d, struct usb_device, dev)

>From <asm-parisc/hardware.h> with my parisc device patch:
static inline struct parisc_device *to_parisc_dev(struct device *dev)
{
        return container_of(dev, struct parisc_device, device);
}

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Miplitas, 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: [parisc-linux] Untested port of parisc_device to generic device interface

[Adam J. Richter]

Matthew Wilcox writes:
>On Fri, Nov 08, 2002 at 08:51:28PM -0800, Adam J. Richter wrote:
>> My patch is a net deletion of 57 lines and will allow simplification
>> of parisc DMA allocation.
>
>57 lines of clean elegant code, replacing them with overly generic ugly
>code and bloated data structures.

	You'd really have to gerrymander to pick a standard by which
my code is "ugly" and what it deletes is "clean and elegant."  I leave
it to other interested readers to look at my patch and judge.

>struct device is a round 256 bytes
>on x86.  more on 64-bit architectures.

	Here is an updated version of my completely untested patch
that replaces parisc_device.name[] with parisc_device.device.name[].

	sizeof(struct device), at least on x86	244 bytes
	5 pointers removed from parisc_device   -40 bytes
	parisc_device.name moved to .device	-80 bytes
						---------
	Size cost of using struct device	124 bytes per parisc_device

	There are ten drivers in drivers/parisc and arch/parisc that
define a struct parisc_driver.  Perhaps you'll have an average of ~1.5
of each such device in a parisc system or maybe ten struct
parisc_device's and a similar number of parisc_driver's.  So, you're
talking about perhaps 2kB of additional space for parisc_device's,
perhaps 3kB including the parisc_driver's.

	This space cost would be reduced somewhat by the shrinkage to
arch/parisc/kernel/drivers.c, and this code will enable other
simplifications.  Even without those other simplificatins, this code
may be a net space savings on some systems.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Miplitas, 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."

diff -u -r linux-2.5.46/include/asm-parisc/hardware.h linux/include/asm-parisc/hardware.h
--- linux-2.5.46/include/asm-parisc/hardware.h	Wed Oct 30 21:32:15 2002
+++ linux/include/asm-parisc/hardware.h	Sat Nov  9 03:07:23 2002
@@ -1,6 +1,7 @@
 #ifndef _PARISC_HARDWARE_H
 #define _PARISC_HARDWARE_H
 
+#include <linux/device.h>
 #include <asm/pdc.h>
 
 struct parisc_device_id {
@@ -26,14 +27,9 @@
 struct parisc_device {
 	unsigned long   hpa;		/* Hard Physical Address */
 	struct parisc_device_id id;
-	struct parisc_device *parent;
-	struct parisc_device *sibling;
-	struct parisc_device *child;
-	struct parisc_driver *driver;	/* Driver for this device */
-	void		*sysdata;	/* Driver instance private data */
-	char		name[80];	/* The hardware description */
 	int		irq;
 
+	struct device	device;
 	char		hw_path;        /* The module number on this bus */
 	unsigned int	num_addrs;	/* some devices have additional address ranges. */
 	unsigned long	*addr;          /* which will be stored here */
@@ -66,10 +62,9 @@
 extern char *cpu_name_version[][2]; /* mapping from enum cpu_type to strings */
 
 struct parisc_driver {
-	struct parisc_driver *next;
-	char *name; 
 	const struct parisc_device_id *id_table;
 	int (*probe) (struct parisc_device *dev); /* New device discovered */
+	struct device_driver driver;
 };
 
 struct io_module {
@@ -128,6 +123,26 @@
 #define HPHW_FAULTY    31
 
 
+static inline struct parisc_device *to_parisc_dev(struct device *dev)
+{
+	return container_of(dev, struct parisc_device, device);
+}
+
+static inline struct parisc_driver *to_parisc_drv(struct driver *dev)
+{
+	return container_of(drv, struct parisc_driver, driver);
+}
+
+static inline void *parisc_get_drvdata(struct parisc_device *pa_dev)
+{
+	return dev_get_drvdata(pa_dev->device);
+}
+
+static inline void parisc_set_drvdata(struct parisc_device *pa_dev, void *ptr)
+{
+	dev_set_drvdata(pa_dev->device, ptr);
+}
+
 /* hardware.c: */
 extern const char *parisc_hardware_description(struct parisc_device_id *id);
 extern enum cpu_type parisc_get_cpu_type(unsigned long hversion);
@@ -135,12 +150,13 @@
 struct pci_dev;
 
 /* drivers.c: */
+extern struct bus_type parisc_bus_type;
 extern struct parisc_device *alloc_pa_dev(unsigned long hpa,
 		struct hardware_path *path);
 extern int register_parisc_device(struct parisc_device *dev);
 extern int register_parisc_driver(struct parisc_driver *driver);
 extern int count_parisc_driver(struct parisc_driver *driver);
-extern int unregister_parisc_driver(struct parisc_driver *driver);
+extern void unregister_parisc_driver(struct parisc_driver *driver);
 extern void walk_central_bus(void);
 extern void fixup_child_irqs(struct parisc_device *parent, int irqbase,
 		int (*choose)(struct parisc_device *parent));
diff -u -r linux-2.5.46/arch/parisc/kernel/drivers.c linux/arch/parisc/kernel/drivers.c
--- linux-2.5.46/arch/parisc/kernel/drivers.c	Mon Nov  4 21:35:43 2002
+++ linux/arch/parisc/kernel/drivers.c	Sat Nov  9 03:21:25 2002
@@ -27,17 +27,8 @@
 /* See comments in include/asm-parisc/pci.h */
 struct pci_dma_ops *hppa_dma_ops;
 
-static struct parisc_driver *pa_drivers;
 static struct parisc_device root;
 
-/* This lock protects the pa_drivers list _only_ since all parisc_devices
- * are registered before smp_init() is called.  If you wish to add devices
- * after that, this muct be serialised somehow.  I recommend a semaphore
- * rather than a spinlock since driver ->probe functions are allowed to
- * sleep (for example when allocating memory).
- */
-static spinlock_t pa_lock = SPIN_LOCK_UNLOCKED;
-
 #define for_each_padev(dev) \
 	for (dev = root.child; dev != NULL; dev = next_dev(dev))
 
@@ -53,20 +44,13 @@
  */
 struct parisc_device *next_dev(struct parisc_device *dev)
 {
-	if (dev->child) {
-		return check_dev(dev->child);
-	} else if (dev->sibling) {
-		return dev->sibling;
-	}
+	struct device *next =
+		list_entry(dev->device.next, struct device, g_list);
 
-	/* Exhausted tree at this level, time to go up. */
-	do {
-		dev = dev->parent;
-		if (dev && dev->sibling)
-			return dev->sibling;
-	} while (dev != &root);
-
-	return NULL;
+	if (next->parent == NULL) /* end of list.  back at root */
+		return NULL;
+	else
+		return container_of(next, struct parisc_device, device);
 }
 
 /**
@@ -74,8 +58,11 @@
  * @driver: the PA-RISC driver to try
  * @dev: the PA-RISC device to try
  */
-static int match_device(struct parisc_driver *driver, struct parisc_device *dev)
+static int
+parisc_match_device(struct device_driver *gendrv, struct device *gendev)
 {
+	struct parisc_driver *driver = to_parisc_driver(gendrv);
+	struct parisc_device *dev = to_parisc_device(gendev);
 	const struct parisc_device_id *ids;
 
 	for (ids = driver->id_table; ids->sversion; ids++) {
@@ -96,10 +83,17 @@
 	return 0;
 }
 
-static void claim_device(struct parisc_driver *driver, struct parisc_device *dev)
+static int parisc_device_probe(struct device *dev)
 {
-	dev->driver = driver;
-	request_mem_region(dev->hpa, 0x1000, driver->name);
+	struct parisc_device *pa_dev = to_parisc_device(dev);
+	struct device_driver *drv = dev->driver;
+	struct parisc_driver *pa_drv = to_parisc_driver(drv);
+	int err = (*pa_drv->probe)(pa_dev);
+
+	if (!err)
+		request_mem_region(dev->hpa, 0x1000, drv->name);
+
+	return err;
 }
 
 /**
@@ -108,37 +102,22 @@
  */
 int register_parisc_driver(struct parisc_driver *driver)
 {
-	struct parisc_device *device;
-
-	if (driver->next) {
-		printk(KERN_WARNING 
-		       "BUG: Skipping previously registered driver: %s\n",
-		       driver->name);
-		return 1;
-	}
-
-	for_each_padev(device) {
-		if (device->driver)
-			continue;
-		if (!match_device(driver, device))
-			continue;
-
-		if (driver->probe(device) < 0)
-			continue;
-		claim_device(driver, device);
-	}
+	driver->driver.bus_type = &parisc_bus_type;
+	driver->driver.probe = parisc_device_probe;
+	return driver_register(&driver->driver);
+}
 
-	/* Note that the list is in reverse order of registration.  This
-	 * may be significant if we ever actually support hotplug and have
-	 * multiple drivers capable of claiming the same chip.
-	 */
-
-	spin_lock(&pa_lock);
-	driver->next = pa_drivers;
-	pa_drivers = driver;
-	spin_unlock(&pa_lock);
+struct count_arg {
+	struct driver *driver;
+	int count;
+};
 
-	return 0;
+static void count_callback(struct device *dev, void *data)
+{
+	struct count_arg *arg = data;
+	
+	if (parisc_match_device(&arg->driver, dev))
+		(arg->count)++;
 }
 
 /**
@@ -148,17 +127,14 @@
  * Use by IOMMU support to "guess" the right size IOPdir.
  * Formula is something like memsize/(num_iommu * entry_size).
  */
-int count_parisc_driver(struct parisc_driver *driver)
+int count_parisc_driver(struct parisc_driver *pa_driver)
 {
-	struct parisc_device *device;
-	int cnt = 0;
-
-	for_each_padev(device) {
-		if (match_device(driver, device))
-			cnt++;
-	}
-
-	return cnt;
+	struct count_arg arg;
+  
+	arg.driver = &pa_driver->driver;
+	arg.count = 0;
+	bus_for_each_dev(arg.driver->bus, &arg, count_callback);
+	return arg.count;
 }
 
 
@@ -167,42 +143,9 @@
  * unregister_parisc_driver - Unregister this driver from the list of drivers
  * @driver: the PA-RISC driver to unregister
  */
-int unregister_parisc_driver(struct parisc_driver *driver)
+void unregister_parisc_driver(struct parisc_driver *driver)
 {
-	struct parisc_device *dev;
-
-	spin_lock(&pa_lock);
-
-	if (pa_drivers == driver) {
-		/* was head of list - update head */
-		pa_drivers = driver->next;
-	} else {
-		struct parisc_driver *prev = pa_drivers;
-
-		while (prev && driver != prev->next) {
-			prev = prev->next;
-		}
-
-		if (!prev) {
-			printk(KERN_WARNING "unregister_parisc_driver: %s wasn't registered\n", driver->name);
-		} else {
-			/* Drop driver from list */
-			prev->next = driver->next;
-			driver->next = NULL;
-		}
-
-	}
-
-	spin_unlock(&pa_lock);
-
-	for_each_padev(dev) {
-		if (dev->driver != driver)
-			continue;
-		dev->driver = NULL;
-		release_mem_region(dev->hpa, 0x1000);
-	}
-
-	return 0;
+	driver_unregister(&driver->driver);
 }
 
 static struct parisc_device *find_device_by_addr(unsigned long hpa)
@@ -335,7 +278,8 @@
 	memset(dev, 0, sizeof(*dev));
 	dev->hw_path = id;
 	dev->id.hw_type = HPHW_FAULTY;
-	dev->parent = parent;
+	dev->device.bus_type = &parisc_bus_type;
+	dev->device.parent = &parent->device;
 	dev->sibling = *insert;
 	*insert = dev;
 	return dev;
@@ -417,7 +361,7 @@
 	dev->hpa = hpa;
 	name = parisc_hardware_description(&dev->id);
 	if (name) {
-		strncpy(dev->name, name, sizeof(dev->name)-1);
+		strncpy(dev->device.name, name, sizeof(dev->device.name)-1);
 	}
 
 	return dev;
@@ -429,32 +373,11 @@
  *
  * Search the driver list for a driver that is willing to manage
  * this device.
+ * WARNING: This routine now returns 0 on success. --Adam J. Richter 2002.11.08
  */
 int register_parisc_device(struct parisc_device *dev)
 {
-	struct parisc_driver *driver;
-
-	if (!dev)
-		return 0;
-
-	if (dev->driver)
-		return 1;
-	
-	spin_lock(&pa_lock);
-
-	/* Locate a driver which agrees to manage this device.  */
-	for (driver = pa_drivers; driver; driver = driver->next) {
-		if (!match_device(driver,dev))
-			continue;
-		if (driver->probe(dev) == 0)
-			break;
-	}
-
-	if (driver != NULL) {
-		claim_device(driver, dev);
-	}
-	spin_unlock(&pa_lock);
-	return driver != NULL;
+	return device_register(&dev->device);
 }
 
 #define BC_PORT_MASK 0x8
@@ -588,3 +511,8 @@
 		print_parisc_device(dev);
 	}
 }
+
+struct bus_type parisc_bus_type = {
+	.name =		"parisc",
+	.match =	parisc_match_device,
+};
diff -u -r linux-2.5.46/arch/parisc/kernel/perf.c linux/arch/parisc/kernel/perf.c
--- linux-2.5.46/arch/parisc/kernel/perf.c	Mon Nov  4 21:35:43 2002
+++ linux/arch/parisc/kernel/perf.c	Sat Nov  9 03:13:23 2002
@@ -527,7 +527,7 @@
 	/* TODO: this only lets us access the first cpu.. what to do for SMP? */
 	cpu_device = cpu_data[0].dev;
 	printk("Performance monitoring counters enabled for %s\n",
-		cpu_data[0].dev->name);
+		cpu_data[0].dev->device.name);
 
 	return 0;
 }
diff -u -r linux-2.5.46/arch/parisc/kernel/processor.c linux/arch/parisc/kernel/processor.c
--- linux-2.5.46/arch/parisc/kernel/processor.c	Mon Nov  4 21:35:44 2002
+++ linux/arch/parisc/kernel/processor.c	Sat Nov  9 03:13:04 2002
@@ -344,7 +344,7 @@
 				"model name\t: %s\n",
 				 boot_cpu_data.pdc.sys_model_name,
 				 cpu_data[n].dev ? 
-				 cpu_data[n].dev->name : "Unknown" );
+				 cpu_data[n].dev->device.name : "Unknown" );
 
 		seq_printf(m, "hversion\t: 0x%08x\n"
 			        "sversion\t: 0x%08x\n",
@@ -370,9 +370,11 @@
 };
 
 static struct parisc_driver cpu_driver = {
-	name:		"CPU",
-	id_table:	processor_tbl,
-	probe:		processor_probe
+	.id_table =	processor_tbl,
+	.probe =	processor_probe
+	.driver = {
+		.name =		"CPU",
+	},
 };
 
 /**
diff -u -r linux-2.5.46/arch/parisc/kernel/setup.c linux/arch/parisc/kernel/setup.c
--- linux-2.5.46/arch/parisc/kernel/setup.c	Mon Nov  4 21:35:44 2002
+++ linux/arch/parisc/kernel/setup.c	Sat Nov  9 03:10:26 2002
@@ -216,24 +216,24 @@
 }
 
 static struct resource central_bus = {
-	name:	"Central Bus",
-	start:	(unsigned long)0xfffffffffff80000,
-	end:    (unsigned long)0xfffffffffffaffff,
-	flags:	IORESOURCE_MEM,
+	.name =		"Central Bus",
+	.start =	(unsigned long)0xfffffffffff80000,
+	.end =		(unsigned long)0xfffffffffffaffff,
+	.flags =	IORESOURCE_MEM,
 };
 
 static struct resource local_broadcast = {
-	name:	"Local Broadcast",
-	start:	(unsigned long)0xfffffffffffb0000,
-	end:	(unsigned long)0xfffffffffffdffff,
-	flags:	IORESOURCE_MEM,
+	.name =		"Local Broadcast",
+	.start =	(unsigned long)0xfffffffffffb0000,
+	.end =		(unsigned long)0xfffffffffffdffff,
+	.flags =	IORESOURCE_MEM,
 };
 
 static struct resource global_broadcast = {
-	name:	"Global Broadcast",
-	start:	(unsigned long)0xfffffffffffe0000,
-	end:	(unsigned long)0xffffffffffffffff,
-	flags:	IORESOURCE_MEM,
+	.name =		"Global Broadcast",
+	.start =	(unsigned long)0xfffffffffffe0000,
+	.end =		(unsigned long)0xffffffffffffffff,
+	.flags =	IORESOURCE_MEM,
 };
 
 int __init parisc_init_resources(void)
diff -u -r linux-2.5.46/drivers/parisc/sba_iommu.c linux/drivers/parisc/sba_iommu.c
--- linux-2.5.46/drivers/parisc/sba_iommu.c	Fri Nov  8 01:28:15 2002
+++ linux/drivers/parisc/sba_iommu.c	Fri Nov  8 01:43:35 2002
@@ -1988,7 +1988,7 @@
 		return(1);
 	}
 
-	dev->sysdata = (void *) sba_dev;
+	parisc_set_drvdata(dev, sba_dev);
 	memset(sba_dev, 0, sizeof(struct sba_device));
 
 	for(i=0; i<MAX_IOC; i++)
@@ -2041,7 +2041,7 @@
  */
 void * sba_get_iommu(struct parisc_device *pci_hba)
 {
-	struct sba_device *sba = (struct sba_device *) pci_hba->parent->sysdata;
+	struct sba_device *sba = parisc_get_drvdata(pci_hba->parent);
 	char t = pci_hba->parent->id.hw_type;
 	int iocnum = (pci_hba->hw_path >> 3);	/* rope # */
 

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Adam J. Richter]

Matthew Wilcox wrote:
>Actually I think the generic device model is crap. [...]

My patch is a net deletion of 57 lines and will allow simplification
of parisc DMA allocation.

Although I agree with most of your criticisms about the generic device
model, most of the problems with it are the way people use it (the
first thing everyone wants to do is a driverfs file system) and some
conventions that I disagree with, such as the idea that drivers that
embed struct device and struct device_driver should not initialize
those fields directly, but should have xxx_register_device copy them
in.  parisc can use the generic driver API without getting fat.

Problems specific to the generic device API can be incrementally
improved and nobody is treating it as set in stone.  I think the
generic device API is close enough already so that it's worth porting
to, even if future clean-ups will then require some small changes to
the code that is ported to it.

Please do not throw the baby out with the bath water.  The generic
driver interface in its present form really can make parisc smaller
and cleaner.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Miplitas, 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: [parisc-linux] Untested port of parisc_device to generic device interface

[Matthew Wilcox]

On Fri, Nov 08, 2002 at 08:51:28PM -0800, Adam J. Richter wrote:
> My patch is a net deletion of 57 lines and will allow simplification
> of parisc DMA allocation.

57 lines of clean elegant code, replacing them with overly generic ugly
code and bloated data structures.  struct device is a round 256 bytes
on x86.  more on 64-bit architectures.

> in.  parisc can use the generic driver API without getting fat.

no.  it can't.

> Problems specific to the generic device API can be incrementally
> improved and nobody is treating it as set in stone.  I think the
> generic device API is close enough already so that it's worth porting
> to, even if future clean-ups will then require some small changes to
> the code that is ported to it.

Everyone's saying "ra!  ra!  generic device model!" without asking
what the cost is.  Don't you think it's reasonable that _as the most
common device type_, struct device should be able to support PCI in a
clean manner?  Don't you think that the fact that it fails to do so is
a problem?  Don't you look at the locks sprinkled all over the struct
device system and wonder what they're all _for_?

Don't get me wrong.  I want a generic device model.  But I think it's
clear the current one has failed to show anything more than eye candy.
Perhaps it's time to start over, with something small and sane -- maybe
kobject (it's not quite what we need, but it's close).  Put one of those
in struct pci_dev.  Remove duplicate fields.  Now maybe grow kobject a
little, or perhaps start a new struct with a kobject as its first member.

And, for gods sake, don't fuck it up by integrating it with USB too early
in the game.  Let's get it right for PCI, maybe some other internal busses
(i'm gagging to write an EISA subsystem ;-).  SCSI is more interesting
than USB.  Above all, don't fall into the trap of "It's a bus and it
has devices on it, therefore it must be a part of devicefs".

*sigh*.  halloween was a week ago.

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Greg KH]

On Sat, Nov 09, 2002 at 05:21:50AM +0000, Matthew Wilcox wrote:
> 
> Everyone's saying "ra!  ra!  generic device model!" without asking
> what the cost is.  Don't you think it's reasonable that _as the most
> common device type_, struct device should be able to support PCI in a
> clean manner?

No I do not.

> Don't you think that the fact that it fails to do so is a problem?

Yes I do.

> Don't you look at the locks sprinkled all over the struct device
> system and wonder what they're all _for_?

Nope :)
(yes, I do wonder, and yes, they will be cleaned up...)

> Don't get me wrong.  I want a generic device model.  But I think it's
> clear the current one has failed to show anything more than eye candy.
> Perhaps it's time to start over, with something small and sane -- maybe
> kobject (it's not quite what we need, but it's close).  Put one of those
> in struct pci_dev.  Remove duplicate fields.  Now maybe grow kobject a
> little, or perhaps start a new struct with a kobject as its first member.

No, lets start pulling stuff out of pci_dev and relying on struct
device.  The reason this hasn't happened yet is no one has been willing
to break all of the PCI drivers, yet.

I know Pat is going to be doing this soon, and if he doesn't get to it,
I will.  But as Adam said, don't throw away the idea because it looks
crufty now.  This has been a _constantly_ evolving model as we work to
get it right.  It will take time, and we are still getting there.

> And, for gods sake, don't fuck it up by integrating it with USB too early
> in the game.

In my defense, USB was the _only_ bus willing to step up and try to do
the integration to work the initial kinks out.  The SCSI people are
being drug kicking and screaming into it, _finally_ now (hell, SCSI is
still not using the updated PCI interface, those people _never_ update
their drivers if they can avoid it.)

> Let's get it right for PCI, maybe some other internal busses
> (i'm gagging to write an EISA subsystem ;-).  SCSI is more interesting
> than USB.  Above all, don't fall into the trap of "It's a bus and it
> has devices on it, therefore it must be a part of devicefs".

Sure SCSI's more interesting, to you :)

By having USB be one of the first adopters (after PCI), we have found a
_lot_ of issues and bugs that happened due to devices showing up and
disappearing at odd times.  Which was _much_ easier to debug than PCI
would have been.  SCSI can't even do hotplug devices _yet_.  How would
we have debugged this stuff then?

And yes, USB belongs in the model, if for no other reason, that "it's a
bus and it has devices on it" :)

> *sigh*.  halloween was a week ago.

Patches for this stuff are going to be happening for quite some time
now, don't despair.

And they are greatly appreciated, and welcomed from everyone :)

thanks,

greg k-h

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[J.E.J. Bottomley]

First off, lets remember that this discussion started over two different, but 
related, problem sets.  One was the DMA api and the other was the device model.

As far as the DMA API goes, consistent memory allocation has always annoyed me 
because there are three separate cases

1. machine is consistent, consistent allocation always succeeds (unless we're 
out of memory)

2. machine is fully inconsistent, consistent allocation always fails.

3. Machine is partially consistent.  consistent allocation may fail because 
we're out of consistent memory so we have to fall back to the old.

What I'd like is an improvement to the DMA API where drivers can advertise 
levels of conformance (I only work with consistent memory or I work correctly 
with any dma'able memory and do all the sync points), and where all the sync 
point stuff optimises out for a machine architecture which is recognisably 
fully consistent at compile time.

Ok, I'll get off my soapbox now.  I never quite recovered from the awful 
#ifdef mess that doing the above correctly for the parisc introduced into the 
53c700 driver...

As far as the device model goes:

greg@kroah.com said:
> No, lets start pulling stuff out of pci_dev and relying on struct
> device.  The reason this hasn't happened yet is no one has been
> willing to break all of the PCI drivers, yet. 

I'd like to see that.  It's always annoyed me that my MCA machines have to 
bounce highmem just because they don't have a pci_dev to put the bounce mask 
in.

> The SCSI people are being drug kicking and screaming into it,
> _finally_ now (hell, SCSI is still not using the updated PCI
> interface, those people _never_ update their drivers if they can avoid
> it.)

That't not entirely fair.  Most of the unbroken drivers in the tree (those 
with active 2.5 maintainers) are using the up to date pci/dma interface.  The 
mid layer is `sort of' using the device api.

Where I'd like to see the device model go for SCSI is:

- we have a device node for every struct scsi_device (even unattached ones)

- unattached devices are really minimal entities with as few resources 
allocated as we can get away with, so we can have lazy attachment more easily.

- on attachment, the device node gets customised by the attachment type (and 
remember, we can have more than one attachment).

- whatever the permanent `name' field for the device is going to be needs to 
be writeable from user level, that way it can eventually be determined by the 
user level and simply written there as a string (rather than having all the 
wwn fallback cruft in the mid-layer).

- Ultimately, I'd like us to dump the host/channel/target numbering scheme in 
favour of the unique device node name (we may still number them in the 
mid-layer for convenience) so that we finesse the FC mapping problems---FC 
devices can embed the necessary identification in the target strings.

- Oh, and of course, we move to a hotplug/coldplug model where the root device 
is attached in initramfs and everything else is discovered in user space from 
the boot process.

> Patches for this stuff are going to be happening for quite some time
> now, don't despair.

> And they are greatly appreciated, and welcomed from everyone :) 

As far as extending the generic device model goes, I'll do it for the MCA bus. 
 I have looked at doing it previously, but giving the MCA bus a struct pci_dev 
is a real pain because of the underlying assumptions when one of these exists 
in an x86 machine.

But, while were on the subject of sorting out the device model abstraction, 
does the `power' node entry really belong at the top level?  It serves no 
purpose for something like a legacy MCA bus.

James

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Greg KH]

On Sat, Nov 09, 2002 at 10:33:56AM -0500, J.E.J. Bottomley wrote:
> 
> > The SCSI people are being drug kicking and screaming into it,
> > _finally_ now (hell, SCSI is still not using the updated PCI
> > interface, those people _never_ update their drivers if they can avoid
> > it.)
> 
> That't not entirely fair.  Most of the unbroken drivers in the tree (those 
> with active 2.5 maintainers) are using the up to date pci/dma interface.  The 
> mid layer is `sort of' using the device api.

I was referring to the pci_module_init() model of PCI drivers, which, as
of 2.5.47, is only implemented in the ips, nsp32 and aic7xxx drivers.
Every other PCI SCSI controller driver will crash and burn a nasty death
if placed in a machine with a PCI hotplug controller, and someone tries
to remove it.  Hopefully someday this will be fixed... :)

> Where I'd like to see the device model go for SCSI is:
> 
> - we have a device node for every struct scsi_device (even unattached ones)
> 
> - unattached devices are really minimal entities with as few resources 
> allocated as we can get away with, so we can have lazy attachment more easily.
> 
> - on attachment, the device node gets customised by the attachment type (and 
> remember, we can have more than one attachment).
> 
> - whatever the permanent `name' field for the device is going to be needs to 
> be writeable from user level, that way it can eventually be determined by the 
> user level and simply written there as a string (rather than having all the 
> wwn fallback cruft in the mid-layer).
> 
> - Ultimately, I'd like us to dump the host/channel/target numbering scheme in 
> favour of the unique device node name (we may still number them in the 
> mid-layer for convenience) so that we finesse the FC mapping problems---FC 
> devices can embed the necessary identification in the target strings.
> 
> - Oh, and of course, we move to a hotplug/coldplug model where the root device 
> is attached in initramfs and everything else is discovered in user space from 
> the boot process.

All of that sounds very reasonable, and would be nice to see
implemented.

> > Patches for this stuff are going to be happening for quite some time
> > now, don't despair.
> 
> > And they are greatly appreciated, and welcomed from everyone :) 
> 
> As far as extending the generic device model goes, I'll do it for the MCA bus. 
>  I have looked at doing it previously, but giving the MCA bus a struct pci_dev 
> is a real pain because of the underlying assumptions when one of these exists 
> in an x86 machine.

What is the real reason for needing this, pci_alloc_consistent()?  We
have talked about renaming that to dev_alloc_consistent() in the past,
which I think will work for you, right?

thanks,

greg k-h

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Miles Bader]

Greg KH <greg@kroah.com> writes:
> What is the real reason for needing this, pci_alloc_consistent()?  We
> have talked about renaming that to dev_alloc_consistent() in the past,
> which I think will work for you, right?

This this would end up [or have the ability to] invoking a bus-specific
routine at some point, right?  [so that a truly PCI-specific definition
could be still be had]

Thanks,

-Miles
-- 
[|nurgle|]  ddt- demonic? so quake will have an evil kinda setting? one that 
            will  make every christian in the world foamm at the mouth? 
[iddt]      nurg, that's the goal 

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Greg KH]

On Wed, Nov 13, 2002 at 04:46:58PM +0900, Miles Bader wrote:
> Greg KH <greg@kroah.com> writes:
> > What is the real reason for needing this, pci_alloc_consistent()?  We
> > have talked about renaming that to dev_alloc_consistent() in the past,
> > which I think will work for you, right?
> 
> This this would end up [or have the ability to] invoking a bus-specific
> routine at some point, right?  [so that a truly PCI-specific definition
> could be still be had]

If that was needed, yes, we should not break that functionality.

Are there any existing archs that need more than just dma_mask moved to
struct device out of pci_dev?  Hm, ppc might need a bit more...

thanks,

greg k-h

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Miles Bader]

Greg KH <greg@kroah.com> writes:
> > This this would end up [or have the ability to] invoking a bus-specific
> > routine at some point, right?  [so that a truly PCI-specific definition
> > could be still be had]
> 
> If that was needed, yes, we should not break that functionality.
> 
> Are there any existing archs that need more than just dma_mask moved to
> struct device out of pci_dev?  Hm, ppc might need a bit more...

I can't speak for `real machines,' but on my wierd embedded board,
pci_alloc_consistent allocates from a special area of memory (not
located at 0) that is the only shared memory between PCI devices and the
CPU.  pci_alloc_consistent happens to fit this situation quite well, but
I don't think a bitmask is enough to express the situation.

-Miles
-- 
Ich bin ein Virus. Mach' mit und kopiere mich in Deine .signature.

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Greg KH]

On Wed, Nov 13, 2002 at 05:02:39PM +0900, Miles Bader wrote:
> Greg KH <greg@kroah.com> writes:
> > > This this would end up [or have the ability to] invoking a bus-specific
> > > routine at some point, right?  [so that a truly PCI-specific definition
> > > could be still be had]
> > 
> > If that was needed, yes, we should not break that functionality.
> > 
> > Are there any existing archs that need more than just dma_mask moved to
> > struct device out of pci_dev?  Hm, ppc might need a bit more...
> 
> I can't speak for `real machines,' but on my wierd embedded board,
> pci_alloc_consistent allocates from a special area of memory (not
> located at 0) that is the only shared memory between PCI devices and the
> CPU.  pci_alloc_consistent happens to fit this situation quite well, but
> I don't think a bitmask is enough to express the situation.

What does your pci_alloc_consistent() function need from the pci_dev
structure in order to do what you need it to do?  Anything other than
the dma_mask value?

thanks,

greg k-h

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Miles Bader]

Greg KH <greg@kroah.com> writes:
> > I can't speak for `real machines,' but on my wierd embedded board,
> > pci_alloc_consistent allocates from a special area of memory (not
> > located at 0) that is the only shared memory between PCI devices and the
> > CPU.  pci_alloc_consistent happens to fit this situation quite well, but
> > I don't think a bitmask is enough to express the situation.
> 
> What does your pci_alloc_consistent() function need from the pci_dev
> structure in order to do what you need it to do?  Anything other than
> the dma_mask value?

Currently, it ignores the pci_dev argument entirely (I've never had a
device that needed the mask, so I haven't bothered with it).  It just
allocates a block from the special memory region and returns the result.

-Miles
-- 
自らを空にして、心を開く時、道は開かれる

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Greg KH]

On Wed, Nov 13, 2002 at 05:26:34PM +0900, Miles Bader wrote:
> Greg KH <greg@kroah.com> writes:
> > > I can't speak for `real machines,' but on my wierd embedded board,
> > > pci_alloc_consistent allocates from a special area of memory (not
> > > located at 0) that is the only shared memory between PCI devices and the
> > > CPU.  pci_alloc_consistent happens to fit this situation quite well, but
> > > I don't think a bitmask is enough to express the situation.
> > 
> > What does your pci_alloc_consistent() function need from the pci_dev
> > structure in order to do what you need it to do?  Anything other than
> > the dma_mask value?
> 
> Currently, it ignores the pci_dev argument entirely (I've never had a
> device that needed the mask, so I haven't bothered with it).  It just
> allocates a block from the special memory region and returns the result.

So merely renaming that function to dev_alloc_consistent(), changing the
first paramater to be a struct device, and proving a macro for all of
the pci drivers for the old pci_alloc_consistent() name would work just
fine for you?

greg k-h

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Miles Bader]

Greg KH <greg@kroah.com> writes:
> > Currently, it ignores the pci_dev argument entirely (I've never had a
> > device that needed the mask, so I haven't bothered with it).  It just
> > allocates a block from the special memory region and returns the result.
> 
> So merely renaming that function to dev_alloc_consistent(), changing the
> first paramater to be a struct device, and proving a macro for all of
> the pci drivers for the old pci_alloc_consistent() name would work just
> fine for you?

Except that this function doesn't make any sense except for PCI devices.

I don't know whether there will ever be any devices that (1) call
`dev_alloc_consistent', (2) aren't PCI devices, and (3) would stand a
chance of ever working on this platform -- probably not.

Never-the-less, it provides (a non-artificial) example of a case where
it's wrong to assume that all busses are the same, and I think that
merits some attention.

-Miles
-- 
97% of everything is grunge

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Grant Grundler]

Miles Bader wrote:
> I can't speak for `real machines,' but on my wierd embedded board,
> pci_alloc_consistent allocates from a special area of memory (not
> located at 0) that is the only shared memory between PCI devices and the
> CPU.  pci_alloc_consistent happens to fit this situation quite well, but
> I don't think a bitmask is enough to express the situation.

HP PARISC V-Class do that as well. The "consistent" memory lives
on the PCI Bus Controller - not in host mem.
Note that parisc-linux does not (yet) support V-class.

grant

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[J.E.J. Bottomley]

Miles Bader wrote:
> I can't speak for `real machines,' but on my wierd embedded board,
> pci_alloc_consistent allocates from a special area of memory (not
> located at 0) that is the only shared memory between PCI devices and the
> CPU.  pci_alloc_consistent happens to fit this situation quite well, but
> I don't think a bitmask is enough to express the situation.

grundler@dsl2.external.hp.com said:
> HP PARISC V-Class do that as well. The "consistent" memory lives on
> the PCI Bus Controller - not in host mem. Note that parisc-linux does
> not (yet) support V-class. 

Actually, I think dma_mask and consistent memory are orthogonal problems.  
dma_masks are used by the I/O subsystem to determine whether direct DMA to a 
memory region containing an I/O buffer is possible or whether it has to be 
bounced.  Consistent memory is usually allocated for driver specific 
transfers.  The I/O subsystem doesn't usually require the actual I/O buffers 
to be in consistent memory.

James

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Grant Grundler]

"J.E.J. Bottomley" wrote:
> Actually, I think dma_mask and consistent memory are orthogonal problems.  

No. consistent memory needs to be reachable by the device as well.
dma_mask constrains which memory pci_alloc_consistent() can use.

> dma_masks are used by the I/O subsystem to determine whether direct DMA to a 
> memory region containing an I/O buffer is possible or whether it has to be 
> bounced.  Consistent memory is usually allocated for driver specific 
> transfers.  The I/O subsystem doesn't usually require the actual I/O buffers 
> to be in consistent memory.

right.

grant

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Ivan Kokshaysky]

On Tue, Nov 12, 2002 at 11:52:23PM -0800, Greg KH wrote:
> On Wed, Nov 13, 2002 at 04:46:58PM +0900, Miles Bader wrote:
> > This this would end up [or have the ability to] invoking a bus-specific
> > routine at some point, right?  [so that a truly PCI-specific definition
> > could be still be had]
> 
> If that was needed, yes, we should not break that functionality.
> 
> Are there any existing archs that need more than just dma_mask moved to
> struct device out of pci_dev?  Hm, ppc might need a bit more...

Add alpha, parisc, sparc and so on. ;-)

pci_dev->sysdata needs to be moved as well, but not only.
It seems that two things are fundamentally missing in generic
device model:
1. clean way to detect the type of container structure from arbitrary
   struct device *;
2. parent/child relationship between devices of different bus types.

Example (not exactly real life, but close enough):
to do DMA mapping properly for, say, some legacy device, I need to know
that it's sitting behind ISA-to-PCI bridge X belonging in PCI domain Y of
the root-level IO controller Z.

Ivan.

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Marc Zyngier]

>>>>> "Ivan" == Ivan Kokshaysky <ink@jurassic.park.msu.ru> writes:

Ivan> It seems that two things are fundamentally missing in generic
Ivan> device model:
Ivan> 1. clean way to detect the type of container structure from arbitrary
Ivan>    struct device *;

Indeed.

I'm using the following stuff in some EISA drivers :

#ifdef CONFIG_EISA
#define DEVICE_EISA(dev) (((dev)->bus == &eisa_bus_type) ? to_eisa_device((dev)) : NULL)
#else
#define DEVICE_EISA(dev) NULL
#endif

and frankly, it's really awful. On drivers which are both EISA and
PCI (3c59x, aic7xxx), this is a major pain.

        M.
-- 
Places change, faces change. Life is so very strange.

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Bjorn Helgaas]

On Wednesday 13 November 2002 12:52 am, Greg KH wrote:
> On Wed, Nov 13, 2002 at 04:46:58PM +0900, Miles Bader wrote:
> > Greg KH <greg@kroah.com> writes:
> > > What is the real reason for needing this, pci_alloc_consistent()?  We
> > > have talked about renaming that to dev_alloc_consistent() in the past,
> > > which I think will work for you, right?
> > 
> > This this would end up [or have the ability to] invoking a bus-specific
> > routine at some point, right?  [so that a truly PCI-specific definition
> > could be still be had]
> 
> If that was needed, yes, we should not break that functionality.
> 
> Are there any existing archs that need more than just dma_mask moved to
> struct device out of pci_dev?  Hm, ppc might need a bit more...

Absolutely.  Boxes with multiple IOMMUs (at least ia64, sparc64, parisc)
need to look up the correct IOMMU with which to map the allocated buffer.
Typically this is in the pci_dev sysdata.

Bjorn

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[J.E.J. Bottomley]

> Absolutely.  Boxes with multiple IOMMUs (at least ia64, sparc64,
> parisc) need to look up the correct IOMMU with which to map the
> allocated buffer. Typically this is in the pci_dev sysdata. 

Actually, I think all of the DMA mapping api needs to become bus independent 
and take a struct device * instead of a pci_dev.  How this lookup/mapping is 
done could be abstracted per architecture inside the DMA api internals.

We should also allow devices to do all the setup through bus generic 
functions, but leave open the possibility that the driver may (once it knows 
the bus type) obtain the pci_dev (or whatever) from the struct device if it 
really, really has to muck with bus specific registers.

As far as the SCSI mid layer goes, all we really need from struct device is 
the dma_mask for setting up the I/O bounce buffers.

The simplest way to do all of this is probably to add a pointer to the 
dma_mask in struct device and make it point to the same thing in pci_dev.  If 
we find we need more than this per device, it could become a pointer to a 
generic dma information structure later on.

Drivers need to advertise DMA conformance (at the moment, requires consistent 
allocation, or fully writeback/invalidate compliant)

We should also adopt Adam's pointer approach to the sync/invalidate points so 
we can treat a dma_alloc_consistent failure as a real failure and not clutter 
the code with writeback/invalidate fallbacks.

The above changes would allow me to yank all of the pci_dev specific code out 
of the scsi mid layer, and also introduce a mca_dev type, convert the 53c700 
driver to using the generic dma API and *finally* get us to the point where I 
don't have to use bounce buffers for highmem access on the MCA bus.

Since the 53c700 is also used by parisc (including some machines with 
IOMMUs---which, unfortunately, I don't have access to), it probably makes an 
ideal conversion test case.

This can probably all be wrappered so the current SCSI pci drivers don't 
notice anything wrong.

James

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Grant Grundler]

"J.E.J. Bottomley" wrote:
> We should also allow devices to do all the setup through bus generic 
> functions, but leave open the possibility that the driver may (once it knows 
> the bus type) obtain the pci_dev (or whatever) from the struct device if it 
> really, really has to muck with bus specific registers.

For device discovery and initialization, the generic PCI code has to muck
with PCI specific resources (IO Port, MMIO, and IRQ related stuff primarily).

> As far as the SCSI mid layer goes, all we really need from struct device is 
> the dma_mask for setting up the I/O bounce buffers.
> 
> The simplest way to do all of this is probably to add a pointer to the 
> dma_mask in struct device and make it point to the same thing in pci_dev.
> If we find we need more than this per device, it could become a pointer
> to a generic dma information structure later on.

uhmm...If we are going to touch dma_mask in pci_dev, then just move it
to struct device and be done with it. Then fixup pci_set_dma_mask()
to do the right thing.

...
> Since the 53c700 is also used by parisc (including some machines with 
> IOMMUs---which, unfortunately, I don't have access to), it probably makes an 
> ideal conversion test case.

Duck! (that's going to get fixed it seems) ;^)

thanks,
grant

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[J.E.J. Bottomley]

grundler@dsl2.external.hp.com said:
> For device discovery and initialization, the generic PCI code has to
> muck with PCI specific resources (IO Port, MMIO, and IRQ related stuff
> primarily). 

Oh, I agree.  If we conduct a phased approach to this, what happens initially 
is that the pci drivers simply pull pci_dev out of the struct device and use 
it as previously.

However, I think the ultimate destination is to see how much of the bus 
specific stuff we can abstract by throwing an API around it.  I think IRQ, 
port and mmio are feasible.  Specific knowledge of bus posting et al may not 
be.

> uhmm...If we are going to touch dma_mask in pci_dev, then just move it
> to struct device and be done with it. Then fixup pci_set_dma_mask() to
> do the right thing. 

Well...OK.  The advantage of a pointer in struct device is that the code can 
be converted as is, and no-one has to muck with the direct accessors of the 
pci_dev->dma_mask.  However, I'll see how many of them there actually are, its 
probably just the drivers that transfer the information to 
blk_queue_bounce_limit.

> Duck! (that's going to get fixed it seems) ;^) 

I thought the 53c700 was working OK?  Richard Hirst did some extensive testing 
on a parisc with an IO-MMU for me (he caught a lot of early mapping leaks 
which I fixed).

James

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Grant Grundler]

"J.E.J. Bottomley" wrote:
> However, I think the ultimate destination is to see how much of the bus 
> specific stuff we can abstract by throwing an API around it.  I think IRQ, 
> port and mmio are feasible.  Specific knowledge of bus posting et al may not 
> be.

I was thinking how many BARs are present/used is PCI specific.

arch code already handles most of the IRQ fixups anyway and it
doesn't really matter where IRQ info is stored as long as the
device driver knows where to find it.

> > Duck! (that's going to get fixed it seems) ;^) 
> 
> I thought the 53c700 was working OK?

sorry - "going to get fixed" meant we are looking for a C180
or similar machine to send you.

thanks!
grant

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Grant Grundler]

Greg KH wrote:
> Are there any existing archs that need more than just dma_mask moved to
> struct device out of pci_dev?  Hm, ppc might need a bit more...

"out of pci_dev" and into struct device?
I think that's all parisc port would need now.

At some point I'd like to propose "dma_hint" field.
But I don't have a specific proposal yet.
Something to help drivers communicate DMA characteristics to
the IOMMU support code. ie bandwidth needed, cacheline line aware,
MWLI support, etc.

grant

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Marc Zyngier]

>>>>> "Matthew" == Matthew Wilcox <willy@debian.org> writes:

Matthew> (i'm gagging to write an EISA subsystem ;-).

Humm, please don't... :-)

maz@midlife-crisis:~$ ls -R /sys/bus/eisa/ 
/sys/bus/eisa/:
devices  drivers

/sys/bus/eisa/devices:
00:00  00:01  00:02

/sys/bus/eisa/drivers:
3c509

/sys/bus/eisa/drivers/3c509:
00:02
maz@midlife-crisis:~$ 

I have it working on x86 and Alpha, will test parisc and mips over the
week-end.

        M.
-- 
Places change, faces change. Life is so very strange.

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Grant Grundler]

"Adam J. Richter" wrote:
> Please do not throw the baby out with the bath water.  The generic
> driver interface in its present form really can make parisc smaller
> and cleaner.

I hope that's true. I was just as disappointed as willy.

Documentation/driver-model/overview.txt:
| Note also that it is at the _end_ of struct pci_dev. This is
| to make people think about what they're doing when switching between the bus
| driver and the global driver; and to prevent against mindless casts between
| the two.

Until this changes, I don't see this as a useful replacement for
either PCI or parisc devices. The "mindless casts" can be fixed.
But without the ability to easily go from generic device type to
bus specific type, people will just get lost in the maze of pointers.

Common code needs to take a common parameter.  Operations on the tree
(eg probe) often require calls to bus specific (or arch or platform
specific) code which may in turn need to make other IO tree operations.
Those code paths convert back and forth between types regularly.
That's why I want to make it as simple as possible at the risk
a few people will get it wrong.

HPUX has had a "unified" IO tree since 10.0 in ~1994. Previous
releases had an IO tree for "Server IO" but not the PA-RISC
workstations. I've work on HPUX IO subsystem 6 years (PCI Code owner for
2 years) and it had several key features:
  (a) traverse the complete tree (from "central bus" to SCSI LUN)
      with shared code,
  (b) determine which type of bus any node was "on",
  (c) associate arbitrary local data with any node.
       (this includes bus *operations*! eg probe, dma, irq setup)

Maybe I'm not seeing it, but (b) and (c) are missing from basic
design or not well described in driver-model/overview.txt.

BTW, I couldn't find Documentation/filesystems/driverfs.txt.

Lastly, the example of an "irq" entry in overview.txt is interesting.
iosapic code "owns" the IRQ. And it could make visible other info
regarding IRQs - eg type and which CPU it's directed at.
But I get the feeling only bus specific code can do that since
it "owns" the directory. Do I misunderstand?

thanks,
grant

Re: [parisc-linux] Untested port of parisc_device to generic device interface

[Matthew Wilcox]

On Fri, Nov 08, 2002 at 05:28:05PM -0800, Adam J. Richter wrote:
> 	To start with, here is another completely untested patch that
> I haven't even tried to compile that attempts to port the parisc device
> type to the generic device model.  I have deliberately not included any
> changes that would make it dependent on my generic DMA routine facility.

Actually I think the generic device model is crap.  It's failed to live
up to its promise of removing common fields from structs, it's introduced
a new composite filesystem and it's not helped in any concrete way.

Just look at pci_dev:

struct pci_dev {
        struct list_head global_list;   /* node in list of all PCI devices */
        struct list_head bus_list;      /* node in per-bus list */
        struct pci_bus  *bus;           /* bus this device is on */
        struct pci_bus  *subordinate;   /* bus this device bridges to */
        struct proc_dir_entry *procent; /* device entry in /proc/bus/pci */

        unsigned int    devfn;          /* encoded device & function index */
        struct pci_driver *driver;      /* which driver has allocated this devic
e */
        void            *driver_data;   /* data private to the driver */
        struct  device  dev;            /* Generic device interface */
        struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regio
ns + expansion ROMs */
        struct resource dma_resource[DEVICE_COUNT_DMA];
        struct resource irq_resource[DEVICE_COUNT_IRQ];
        char            name[90];       /* device name */
}

(there may be some more duplicate fields i've missed)

here's the duplicate fields in struct device:

struct device {
        struct list_head g_list;        /* node in depth-first order list */
        struct list_head node;          /* node in sibling list */
        struct list_head bus_list;      /* node in bus's list */
        struct list_head driver_list;
        struct list_head children;
        struct list_head intf_list;
        struct device   * parent;
        char    name[DEVICE_NAME_SIZE]; /* descriptive ascii string */
        char    bus_id[BUS_ID_SIZE];    /* position on parent bus */

        spinlock_t      lock;           /* lock for the device to ensure two
                                           different layers don't access it at
                                           the same time. */
        atomic_t        refcount;       /* refcount to make sure the device
                                         * persists for the right amount of time
 */

        struct bus_type * bus;          /* type of bus device is on */
        struct device_driver *driver;   /* which driver has allocated this
                                           device */
        void            *driver_data;   /* data private to the driver */
}

Oh, and _that_ embeds a struct kobject:

struct kobject {
        char                    name[KOBJ_NAME_LEN];
        atomic_t                refcount;
        struct list_head        entry;
        struct kobject          * parent;
        struct subsystem        * subsys;
        struct dentry           * dentry;
};

For fucks sake, this is ridiculous.  I haven't dared compare the relative
sizes of struct pci_dev between 2.5, 2.4 and 2.2, but this is sheer bloat.

I was hoping for something _incredibly_ simple from struct device.
Something to replace pci_alloc_consistent with device_alloc_consistent.
Something where I could look through the ancestors of a device to find
out whether it was under a CCIO or just a processor.  Something I could
query to find out whether it was an EISA, a GSC or a PCI device.

I'm disappointed this is trying to serve the needs of USB over the needs
of busses in the box.  I don't think it was even remotely smart to unify
USB with other busses.  And I think the PCI system has suffered the most.
I guess I'm so annoyed because I thought it might solve problems instead
of increasing the amount of user eyecandy.

> 	One question about the machine that has no consistent memory
> option: does it take PCI cards?  If so, then all PCI device drivers
> should theoretically use something like wback_fake_consistent.
> If not, then it sounds like the facility needs only to apply to
> generic DMA operations for "parisc" bus cards.

The only machines which can take any kind of PCI devices that don't
have consistent memory available to them are the T-class machines.
We have no plans to support these machines.  What you do need to watch
out for are machines such as the 735/755 which can take an NCR720 chip
in a non-coherent memory machine.  It is of course also used in machines
which are perfectly capable of allocating consistent memory (whether
through uncached mappings or a cache-coherent IO TLB).

-- 
Revolutions do not require corporate support.