On Wed, 02 Jan 2013 02:06:44 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 01.01.2013 18:29 schrieb Stefan Tauner:
We got a nice shutdown function registration infrastructure, but did not use it very wisely. Instead we added shutdown functions to a myriad of programmers unnecessarily. In this patch we get rid of those that do only call pci_cleanup(pacc) by adding a shutdown function the pcidev.c itself that gets registered by pcidev_init().
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Thanks for your patch, I really like it. A few comments:
diff --git a/pcidev.c b/pcidev.c index 1a26e99..37bcc22 100644 --- a/pcidev.c +++ b/pcidev.c @@ -154,6 +154,14 @@ uintptr_t pcidev_readbar(struct pci_dev *dev, int bar) return (uintptr_t)addr; }
+static int pcidev_shutdown(void *data) +{
- if (pacc != NULL)
pci_cleanup(pacc);undo_pci_write() spits out an error message for pacc==NULL, but here you don't... that is inconsistent. If anything, it's really an error here. Or am I missing something.
You are not really missing anything I guess. The message in undo_pci_write() was put in there before I came up with the autoshutdown idea. I left it in because it does not hurt. And I did not add it here because... - the method is static which *should* mean we have complete control about its execution (which is not true if we leak a reference to it like we do) - if pacc is already gone, then this is fine and nothing we need to worry at this point (apart from the small detail that his should never happen and obviously pacc-man (© idwer) was set free...). hm... we should maybe check for pacc == NULL in pcidev_init? If shutting down twice is an error then initializing twice should even more so (especially since this would probably wreck havoc way more than cleaning it up twice?).
I'll add on here too.
- pcidev_dev = NULL;
- return 0;
+}
uintptr_t pcidev_init(int bar, const struct dev_entry *devs) { struct pci_dev *dev; @@ -166,6 +174,8 @@ uintptr_t pcidev_init(int bar, const struct dev_entry *devs)
pacc = pci_alloc(); /* Get the pci_access structure */ pci_init(pacc); /* Initialize the PCI library */
- if (register_shutdown(pcidev_shutdown, NULL))
Hm. My local tree has register_shutdown(pci_cleanup_wrapper, pacc); instead. I think your variant is better because it doesn't keep a possibly deleted pointer around. Then again, deleting pacc should never ever happen outside pcidev_shutdown.
hm yes, keeping the old reference around does only make sense if we need to free exactly that reference and not another newer one... which is not gonna happen anytime soon i presume (no idea how pcilib would explode, but i am sure there wouldnt be any survivors :)
msg_pdbg("Restoring PCI config space for %02x:%02x:%01x reg 0x%02x\n", data->dev.bus, data->dev.dev, data->dev.func, data->reg); switch (data->type) {
Everything else is fine.
yay progress