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

From: Adam J. Richter (adam@yggdrasil.com)
Date: Sat Nov 09 2002 - 07:22:42 EST


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 # */
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Fri Nov 15 2002 - 22:00:17 EST