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 <[email protected]>
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 [email protected]\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/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
