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

[Carl-Daniel Hailfinger]

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/nicrealtek.c b/nicrealtek.c
> index 3c3b261..0929e5d 100644
> --- a/nicrealtek.c
> +++ b/nicrealtek.c
> @@ -51,22 +51,25 @@ static const struct par_programmer par_programmer_nicrealtek = {
>  		.chip_writen		= fallback_chip_writen,
>  };
>  
> +#if 0
>  static int nicrealtek_shutdown(void *data)
>  {
>  	/* FIXME: We forgot to disable software access again. */
> -	pci_cleanup(pacc);
>  	return 0;
>  }
> +#endif
>  
>  int nicrealtek_init(void)
>  {
>  	if (rget_io_perms())
>  		return 1;
>  
> +	/* No need to check for errors, pcidev_init() will not return in case of errors. */
>  	io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_realtek);
> -
> +#if 0
>  	if (register_shutdown(nicrealtek_shutdown, NULL))
>  		return 1;
> +#endif
>  
>  	/* Beware, this ignores the vendor ID! */
>  	switch (pcidev_dev->device_id) {

Please remove the #if 0 in the hunks above. It's not performance critical.


> 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)

Hm yes... I don't like the name that much, but I don't see a better
alternative, so disregard this comment.


> +{
> +	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.


> +	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.


> +		return 1;
>  	pci_scan_bus(pacc);     /* We want to get the list of devices */
>  	pci_filter_init(pacc, &filter);
>  
> @@ -244,6 +254,11 @@ struct undo_pci_write_data {
>  int undo_pci_write(void *p)
>  {
>  	struct undo_pci_write_data *data = p;
> +	if (pacc == NULL) {
> +		msg_perr("%s: Tried to undo PCI writes without a valid PCI context!\n"
> +			 "Please report a bug at flashrom at flashrom.org\n", __func__);
> +		return 1;
> +	}

Debatable, but OK. See my comment about the pacc check in pcidev_shutdown.


>  	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.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/