* [PATCH][ATM]: [ambassador] eliminate pci_find_device()
@ 2004-10-21 11:26 chas williams (contractor)
2004-10-21 11:38 ` Christoph Hellwig
0 siblings, 1 reply; 5+ messages in thread
From: chas williams (contractor) @ 2004-10-21 11:26 UTC (permalink / raw)
To: netdev; +Cc: davem
please apply to 2.6.
thanks!
Signed-off-by: Chas Williams <chas@....>
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2004/10/18 16:39:40-04:00 chas@relax.cmf.nrl.navy.mil
# [ATM]: [ambassador] eliminate pci_find_device()
#
# drivers/atm/ambassador.h
# 2004/10/18 16:39:25-04:00 chas@relax.cmf.nrl.navy.mil +1 -1
# [ATM]: [ambassador] eliminate pci_find_device()
#
# drivers/atm/ambassador.c
# 2004/10/18 16:39:25-04:00 chas@relax.cmf.nrl.navy.mil +65 -67
# [ATM]: [ambassador] eliminate pci_find_device()
#
diff -Nru a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
--- a/drivers/atm/ambassador.c 2004-10-18 16:52:28 -04:00
+++ b/drivers/atm/ambassador.c 2004-10-18 16:52:28 -04:00
@@ -313,7 +313,7 @@
static void do_housekeeping (unsigned long arg);
/********** globals **********/
-static amb_dev * amb_devs = NULL;
+static LIST_HEAD(amb_devs);
static struct timer_list housekeeping = TIMER_INITIALIZER(do_housekeeping, 0, 1);
static unsigned short debug = 0;
@@ -870,7 +870,8 @@
static irqreturn_t interrupt_handler(int irq, void *dev_id,
struct pt_regs *pt_regs) {
- amb_dev * dev = amb_devs;
+ amb_dev * dev = NULL;
+ struct list_head *p;
(void) pt_regs;
PRINTD (DBG_IRQ|DBG_FLOW, "interrupt_handler: %p", dev_id);
@@ -880,10 +881,11 @@
return IRQ_NONE;
}
// Did one of our cards generate the interrupt?
- while (dev) {
+ list_for_each(p, &amb_devs) {
+ dev = list_entry(p, struct amb_dev, entry);
if (dev == dev_id)
break;
- dev = dev->prev;
+ dev = NULL;
}
// impossible - unless we add the device to our list after both
// registering the IRQ handler for it and enabling interrupts, AND
@@ -1554,19 +1556,19 @@
/********** housekeeping **********/
static void do_housekeeping (unsigned long arg) {
- amb_dev * dev = amb_devs;
+ amb_dev * dev;
+ struct list_head *p;
// data is set to zero at module unload
(void) arg;
if (housekeeping.data) {
- while (dev) {
+ list_for_each(p, &amb_devs) {
+ dev = list_entry(p, struct amb_dev, entry);
// could collect device-specific (not driver/atm-linux) stats here
// last resort refill once every ten seconds
fill_rx_pools (dev);
-
- dev = dev->prev;
}
mod_timer(&housekeeping, jiffies + 10*HZ);
}
@@ -2225,6 +2227,7 @@
// set up known dev items straight away
dev->pci_dev = pci_dev;
+ pci_set_drvdata(pci_dev, dev);
dev->iobase = pci_resource_start (pci_dev, 1);
dev->irq = pci_dev->irq;
@@ -2284,7 +2287,7 @@
return ret;
}
-static int __init do_pci_device(struct pci_dev *pci_dev)
+static int __devinit amb_probe(struct pci_dev *pci_dev, const struct pci_device_id *pci_ent)
{
amb_dev * dev;
int err;
@@ -2292,6 +2295,12 @@
// read resources from PCI configuration space
u8 irq = pci_dev->irq;
+ if (pci_dev->device == PCI_DEVICE_ID_MADGE_AMBASSADOR_BAD) {
+ PRINTK (KERN_ERR, "skipped broken (PLX rev 2) card");
+ err = -EINVAL;
+ goto out;
+ }
+
PRINTD (DBG_INFO, "found Madge ATM adapter (amb) at"
" IO %x, IRQ %u, MEM %p", pci_resource_start(pci_dev, 1),
irq, bus_to_virt(pci_resource_start(pci_dev, 0)));
@@ -2348,8 +2357,7 @@
dev->atm_dev->ci_range.vci_bits = NUM_VCI_BITS;
// update linked list
- dev->prev = amb_devs;
- amb_devs = dev;
+ list_add_tail(&dev->entry, &amb_devs);
// enable host interrupts
interrupts_on (dev);
@@ -2370,29 +2378,26 @@
goto out;
}
-static int __init amb_probe (void) {
- struct pci_dev * pci_dev;
- int devs;
-
- PRINTD (DBG_FLOW, "amb_probe");
-
- devs = 0;
- pci_dev = NULL;
- while ((pci_dev = pci_find_device
- (PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR, pci_dev)
- )) {
- if (do_pci_device(pci_dev) == 0)
- devs++;
- }
-
- pci_dev = NULL;
- while ((pci_dev = pci_find_device
- (PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR_BAD, pci_dev)
- ))
- PRINTK (KERN_ERR, "skipped broken (PLX rev 2) card");
-
- return devs;
+static void __devexit amb_remove_one(struct pci_dev *pci_dev)
+{
+ struct amb_dev *dev;
+
+ dev = pci_get_drvdata(pci_dev);
+
+ list_del(&dev->entry);
+
+ PRINTD(DBG_INFO|DBG_INIT, "closing %p (atm_dev = %p)", dev, dev->atm_dev);
+ // the drain should not be necessary
+ drain_rx_pools(dev);
+ interrupts_off(dev);
+ amb_reset(dev, 0);
+ free_irq(dev->irq, dev);
+ pci_disable_device(pci_dev);
+ destroy_queues(dev);
+ atm_dev_deregister(dev->atm_dev);
+ kfree(dev);
+ pci_release_region(pci_dev, 1);
}
static void __init amb_check_args (void) {
@@ -2457,9 +2462,25 @@
/********** module entry **********/
-static int __init amb_module_init (void) {
- int devs;
-
+static struct pci_device_id amb_pci_tbl[] = {
+ { PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR, PCI_ANY_ID, PCI_ANY_ID,
+ 0, 0, 0 },
+ { PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR_BAD, PCI_ANY_ID, PCI_ANY_ID,
+ 0, 0, 0 },
+ { 0, }
+};
+
+static struct pci_driver amb_driver = {
+ .name = "amb",
+ .probe = amb_probe,
+ .remove = __devexit_p(amb_remove_one),
+ .id_table = amb_pci_tbl,
+};
+
+static int __init amb_module_init (void)
+{
+ int err;
+
PRINTD (DBG_FLOW|DBG_INIT, "init_module");
// sanity check - cast needed as printk does not support %Zu
@@ -2474,49 +2495,26 @@
amb_check_args();
// get the juice
- devs = amb_probe();
-
- if (devs) {
+ err = pci_module_init(&amb_driver);
+ if (err == 0)
mod_timer (&housekeeping, jiffies);
- } else {
+ else
PRINTK (KERN_INFO, "no (usable) adapters found");
- }
-
- return devs ? 0 : -ENODEV;
+
+ return err;
}
/********** module exit **********/
-static void __exit amb_module_exit (void) {
- amb_dev * dev;
-
+static void __exit amb_module_exit (void)
+{
PRINTD (DBG_FLOW|DBG_INIT, "cleanup_module");
// paranoia
housekeeping.data = 0;
del_timer_sync(&housekeeping);
- while (amb_devs) {
- struct pci_dev *pdev;
-
- dev = amb_devs;
- pdev = dev->pci_dev;
- amb_devs = dev->prev;
-
- PRINTD (DBG_INFO|DBG_INIT, "closing %p (atm_dev = %p)", dev, dev->atm_dev);
- // the drain should not be necessary
- drain_rx_pools (dev);
- interrupts_off (dev);
- amb_reset (dev, 0);
- free_irq (dev->irq, dev);
- pci_disable_device (pdev);
- destroy_queues (dev);
- atm_dev_deregister (dev->atm_dev);
- kfree (dev);
- pci_release_region (pdev, 1);
- }
-
- return;
+ return pci_unregister_driver(&amb_driver);
}
module_init(amb_module_init);
diff -Nru a/drivers/atm/ambassador.h b/drivers/atm/ambassador.h
--- a/drivers/atm/ambassador.h 2004-10-18 16:52:28 -04:00
+++ b/drivers/atm/ambassador.h 2004-10-18 16:52:28 -04:00
@@ -649,7 +649,7 @@
struct atm_dev * atm_dev;
struct pci_dev * pci_dev;
- struct amb_dev * prev;
+ struct list_head entry;
};
typedef struct amb_dev amb_dev;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH][ATM]: [ambassador] eliminate pci_find_device()
2004-10-21 11:26 [PATCH][ATM]: [ambassador] eliminate pci_find_device() chas williams (contractor)
@ 2004-10-21 11:38 ` Christoph Hellwig
2004-10-21 11:51 ` chas williams (contractor)
2004-10-22 1:29 ` chas williams (contractor)
0 siblings, 2 replies; 5+ messages in thread
From: Christoph Hellwig @ 2004-10-21 11:38 UTC (permalink / raw)
To: chas williams (contractor); +Cc: netdev, davem
> // Did one of our cards generate the interrupt?
> - while (dev) {
> + list_for_each(p, &amb_devs) {
> + dev = list_entry(p, struct amb_dev, entry);
> if (dev == dev_id)
> break;
> - dev = dev->prev;
> + dev = NULL;
> }
> // impossible - unless we add the device to our list after both
> // registering the IRQ handler for it and enabling interrupts, AND
this debug check is superflous.
>
> @@ -1554,19 +1556,19 @@
>
> /********** housekeeping **********/
> static void do_housekeeping (unsigned long arg) {
> - amb_dev * dev = amb_devs;
> + amb_dev * dev;
> + struct list_head *p;
> // data is set to zero at module unload
> (void) arg;
>
> if (housekeeping.data) {
> - while (dev) {
> + list_for_each(p, &amb_devs) {
> + dev = list_entry(p, struct amb_dev, entry);
this really should be a per-device timer - or even better using
schedule_delayed_work()
With these two changes you could get rid of amb_devs easily. If you still
want to keep it for some reason at least use list_for_each_entry.
> + list_del(&dev->entry);
> +
> + PRINTD(DBG_INFO|DBG_INIT, "closing %p (atm_dev = %p)", dev, dev->atm_dev);
> + // the drain should not be necessary
> + drain_rx_pools(dev);
> + interrupts_off(dev);
> + amb_reset(dev, 0);
> + free_irq(dev->irq, dev);
> + pci_disable_device(pci_dev);
> + destroy_queues(dev);
> + atm_dev_deregister(dev->atm_dev);
> + kfree(dev);
> + pci_release_region(pci_dev, 1);
this doesn't look correct (without knowing the atm subsystem in detail), in
general you need to unregister from the subsystem first so you don't get
new work, and then you can start tearing the hardware down.
Also disabling the device before releasing the regions doesn't make too much
sense to me, even if it may be valid.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH][ATM]: [ambassador] eliminate pci_find_device()
2004-10-21 11:38 ` Christoph Hellwig
@ 2004-10-21 11:51 ` chas williams (contractor)
2004-10-22 1:29 ` chas williams (contractor)
1 sibling, 0 replies; 5+ messages in thread
From: chas williams (contractor) @ 2004-10-21 11:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: netdev, davem
In message <20041021113841.GA3720@infradead.org>,Christoph Hellwig writes:
>> // Did one of our cards generate the interrupt?
>this debug check is superflous.
i know that. however, i didnt want to change the original more than
necessary. dropping this check doesnt bother me.
>> @@ -1554,19 +1556,19 @@
>> if (housekeeping.data) {
>> - while (dev) {
>> + list_for_each(p, &amb_devs) {
>> + dev = list_entry(p, struct amb_dev, entry);
>
>this really should be a per-device timer - or even better using
>schedule_delayed_work()
>
>With these two changes you could get rid of amb_devs easily. If you still
>want to keep it for some reason at least use list_for_each_entry.
my primary reason for keeping it is that i cant easily test the changes
to the driver. i have a programmable card that i set to the correct
vendor/device id. it doesnt do any 'real' work. i dont really know
anyone with a real ambassador (or horizon) card. this driver is essentially
"reference only" as far as i can tell. i can change to list_for_each_entry.
>> + list_del(&dev->entry);
>> +
>> + PRINTD(DBG_INFO|DBG_INIT, "closing %p (atm_dev = %p)", dev, dev->atm_dev);
>> + // the drain should not be necessary
>> + drain_rx_pools(dev);
>> + interrupts_off(dev);
>> + amb_reset(dev, 0);
>> + free_irq(dev->irq, dev);
>> + pci_disable_device(pci_dev);
>> + destroy_queues(dev);
>> + atm_dev_deregister(dev->atm_dev);
>> + kfree(dev);
>> + pci_release_region(pci_dev, 1);
>
>this doesn't look correct (without knowing the atm subsystem in detail), in
>general you need to unregister from the subsystem first so you don't get
>new work, and then you can start tearing the hardware down.
still that what was the order in the original driver. i am sure it
races with housekeeping as well if that makes you feel any better.
(yes, i know, seperate timers wouldnt race.)
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH][ATM]: [ambassador] eliminate pci_find_device()
2004-10-21 11:38 ` Christoph Hellwig
2004-10-21 11:51 ` chas williams (contractor)
@ 2004-10-22 1:29 ` chas williams (contractor)
2004-10-22 5:14 ` David S. Miller
1 sibling, 1 reply; 5+ messages in thread
From: chas williams (contractor) @ 2004-10-22 1:29 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: netdev, davem
better?
# This is a BitKeeper generated diff -Nru style patch.
#
# [ATM]: [ambassador] eliminate pci_find_device()
#
diff -Nru a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
--- a/drivers/atm/ambassador.c 2004-10-21 21:24:47 -04:00
+++ b/drivers/atm/ambassador.c 2004-10-21 21:24:47 -04:00
@@ -313,9 +313,6 @@
static void do_housekeeping (unsigned long arg);
/********** globals **********/
-static amb_dev * amb_devs = NULL;
-static struct timer_list housekeeping = TIMER_INITIALIZER(do_housekeeping, 0, 1);
-
static unsigned short debug = 0;
static unsigned int cmds = 8;
static unsigned int txs = 32;
@@ -870,7 +867,7 @@
static irqreturn_t interrupt_handler(int irq, void *dev_id,
struct pt_regs *pt_regs) {
- amb_dev * dev = amb_devs;
+ amb_dev * dev = (amb_dev *) dev_id;
(void) pt_regs;
PRINTD (DBG_IRQ|DBG_FLOW, "interrupt_handler: %p", dev_id);
@@ -879,24 +876,6 @@
PRINTD (DBG_IRQ|DBG_ERR, "irq with NULL dev_id: %d", irq);
return IRQ_NONE;
}
- // Did one of our cards generate the interrupt?
- while (dev) {
- if (dev == dev_id)
- break;
- dev = dev->prev;
- }
- // impossible - unless we add the device to our list after both
- // registering the IRQ handler for it and enabling interrupts, AND
- // the card generates an IRQ at startup - should not happen again
- if (!dev) {
- PRINTD (DBG_IRQ, "irq for unknown device: %d", irq);
- return IRQ_NONE;
- }
- // impossible - unless we have memory corruption of dev or kernel
- if (irq != dev->irq) {
- PRINTD (DBG_IRQ|DBG_ERR, "irq mismatch: %d", irq);
- return IRQ_NONE;
- }
{
u32 interrupt = rd_plain (dev, offsetof(amb_mem, interrupt));
@@ -1554,22 +1533,13 @@
/********** housekeeping **********/
static void do_housekeeping (unsigned long arg) {
- amb_dev * dev = amb_devs;
- // data is set to zero at module unload
- (void) arg;
+ amb_dev * dev = (amb_dev *) arg;
- if (housekeeping.data) {
- while (dev) {
-
- // could collect device-specific (not driver/atm-linux) stats here
+ // could collect device-specific (not driver/atm-linux) stats here
- // last resort refill once every ten seconds
- fill_rx_pools (dev);
-
- dev = dev->prev;
- }
- mod_timer(&housekeeping, jiffies + 10*HZ);
- }
+ // last resort refill once every ten seconds
+ fill_rx_pools (dev);
+ mod_timer(&dev->housekeeping, jiffies + 10*HZ);
return;
}
@@ -2225,6 +2195,7 @@
// set up known dev items straight away
dev->pci_dev = pci_dev;
+ pci_set_drvdata(pci_dev, dev);
dev->iobase = pci_resource_start (pci_dev, 1);
dev->irq = pci_dev->irq;
@@ -2284,7 +2255,7 @@
return ret;
}
-static int __init do_pci_device(struct pci_dev *pci_dev)
+static int __devinit amb_probe(struct pci_dev *pci_dev, const struct pci_device_id *pci_ent)
{
amb_dev * dev;
int err;
@@ -2292,6 +2263,12 @@
// read resources from PCI configuration space
u8 irq = pci_dev->irq;
+ if (pci_dev->device == PCI_DEVICE_ID_MADGE_AMBASSADOR_BAD) {
+ PRINTK (KERN_ERR, "skipped broken (PLX rev 2) card");
+ err = -EINVAL;
+ goto out;
+ }
+
PRINTD (DBG_INFO, "found Madge ATM adapter (amb) at"
" IO %x, IRQ %u, MEM %p", pci_resource_start(pci_dev, 1),
irq, bus_to_virt(pci_resource_start(pci_dev, 0)));
@@ -2347,9 +2324,10 @@
dev->atm_dev->ci_range.vpi_bits = NUM_VPI_BITS;
dev->atm_dev->ci_range.vci_bits = NUM_VCI_BITS;
- // update linked list
- dev->prev = amb_devs;
- amb_devs = dev;
+ init_timer(&dev->housekeeping);
+ dev->housekeeping.function = do_housekeeping;
+ dev->housekeeping.data = (unsigned long) dev;
+ mod_timer(&dev->housekeeping, jiffies);
// enable host interrupts
interrupts_on (dev);
@@ -2370,29 +2348,25 @@
goto out;
}
-static int __init amb_probe (void) {
- struct pci_dev * pci_dev;
- int devs;
-
- PRINTD (DBG_FLOW, "amb_probe");
-
- devs = 0;
- pci_dev = NULL;
- while ((pci_dev = pci_find_device
- (PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR, pci_dev)
- )) {
- if (do_pci_device(pci_dev) == 0)
- devs++;
- }
-
- pci_dev = NULL;
- while ((pci_dev = pci_find_device
- (PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR_BAD, pci_dev)
- ))
- PRINTK (KERN_ERR, "skipped broken (PLX rev 2) card");
-
- return devs;
+static void __devexit amb_remove_one(struct pci_dev *pci_dev)
+{
+ struct amb_dev *dev;
+
+ dev = pci_get_drvdata(pci_dev);
+
+ PRINTD(DBG_INFO|DBG_INIT, "closing %p (atm_dev = %p)", dev, dev->atm_dev);
+ del_timer_sync(&dev->housekeeping);
+ // the drain should not be necessary
+ drain_rx_pools(dev);
+ interrupts_off(dev);
+ amb_reset(dev, 0);
+ free_irq(dev->irq, dev);
+ pci_disable_device(pci_dev);
+ destroy_queues(dev);
+ atm_dev_deregister(dev->atm_dev);
+ kfree(dev);
+ pci_release_region(pci_dev, 1);
}
static void __init amb_check_args (void) {
@@ -2457,9 +2431,23 @@
/********** module entry **********/
-static int __init amb_module_init (void) {
- int devs;
-
+static struct pci_device_id amb_pci_tbl[] = {
+ { PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR, PCI_ANY_ID, PCI_ANY_ID,
+ 0, 0, 0 },
+ { PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR_BAD, PCI_ANY_ID, PCI_ANY_ID,
+ 0, 0, 0 },
+ { 0, }
+};
+
+static struct pci_driver amb_driver = {
+ .name = "amb",
+ .probe = amb_probe,
+ .remove = __devexit_p(amb_remove_one),
+ .id_table = amb_pci_tbl,
+};
+
+static int __init amb_module_init (void)
+{
PRINTD (DBG_FLOW|DBG_INIT, "init_module");
// sanity check - cast needed as printk does not support %Zu
@@ -2474,49 +2462,16 @@
amb_check_args();
// get the juice
- devs = amb_probe();
-
- if (devs) {
- mod_timer (&housekeeping, jiffies);
- } else {
- PRINTK (KERN_INFO, "no (usable) adapters found");
- }
-
- return devs ? 0 : -ENODEV;
+ return pci_module_init(&amb_driver);
}
/********** module exit **********/
-static void __exit amb_module_exit (void) {
- amb_dev * dev;
-
+static void __exit amb_module_exit (void)
+{
PRINTD (DBG_FLOW|DBG_INIT, "cleanup_module");
- // paranoia
- housekeeping.data = 0;
- del_timer_sync(&housekeeping);
-
- while (amb_devs) {
- struct pci_dev *pdev;
-
- dev = amb_devs;
- pdev = dev->pci_dev;
- amb_devs = dev->prev;
-
- PRINTD (DBG_INFO|DBG_INIT, "closing %p (atm_dev = %p)", dev, dev->atm_dev);
- // the drain should not be necessary
- drain_rx_pools (dev);
- interrupts_off (dev);
- amb_reset (dev, 0);
- free_irq (dev->irq, dev);
- pci_disable_device (pdev);
- destroy_queues (dev);
- atm_dev_deregister (dev->atm_dev);
- kfree (dev);
- pci_release_region (pdev, 1);
- }
-
- return;
+ return pci_unregister_driver(&amb_driver);
}
module_init(amb_module_init);
diff -Nru a/drivers/atm/ambassador.h b/drivers/atm/ambassador.h
--- a/drivers/atm/ambassador.h 2004-10-21 21:24:47 -04:00
+++ b/drivers/atm/ambassador.h 2004-10-21 21:24:47 -04:00
@@ -649,7 +649,7 @@
struct atm_dev * atm_dev;
struct pci_dev * pci_dev;
- struct amb_dev * prev;
+ struct timer_list housekeeping;
};
typedef struct amb_dev amb_dev;
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2004-10-22 5:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-10-21 11:26 [PATCH][ATM]: [ambassador] eliminate pci_find_device() chas williams (contractor)
2004-10-21 11:38 ` Christoph Hellwig
2004-10-21 11:51 ` chas williams (contractor)
2004-10-22 1:29 ` chas williams (contractor)
2004-10-22 5:14 ` David S. Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).