Re: [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

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 [e-mail]\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