[flashrom] [PATCH 1/5] Let pcidev clean itself up.

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Wed Jan 2 02:39:58 CET 2013


On Wed, 02 Jan 2013 02:06:44 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at 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 at 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


-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner




More information about the flashrom mailing list