Re: [PATCH][ATM]: [ambassador] eliminate pci_find_device()
From: Christoph Hellwig <hch@infradead.org>
Date: 2004-10-21 11:38:41
// 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, ANDthis debug check is superflous.
quoted hunk ↗ jump to hunk
@@ -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.