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

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

[Stefan Tauner]

On Wed, 02 Jan 2013 02:06:44 +0100
Carl-Daniel Hailfinger  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
>
> 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

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

[Stefan Tauner]

On Wed, 02 Jan 2013 02:06:44 +0100
Carl-Daniel Hailfinger  wrote:
> > 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.

BTW Do you understand the FIXME? I dont and I dont see where we enabled
access in the first place...!?

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

[Stefan Tauner]

On Wed, 2 Jan 2013 15:29:47 +0100
Stefan Tauner  wrote:
> BTW Do you understand the FIXME? I dont and I dont see where we enabled
> access in the first place...!?

oh. nvm. :)

[PATCH] Unify PCI init and let pcidev clean itself up.

[Stefan Tauner]

Previously the internal programmer used its own code to initialize pcilib.
This patch extracts the common code from the internal programmer and
pcidev_init() into pcidev_init_common().
This fixes the non-existent PCI cleanup of the internal programmer and adds
an additional safety by checking for an already existing PCI context.

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 
---
 atahpt.c       |   10 ----------
 drkaiser.c     |    3 +--
 gfxnvidia.c    |    6 +-----
 internal.c     |    6 ++----
 nic3com.c      |    2 +-
 nicintel.c     |    5 +----
 nicintel_spi.c |    2 +-
 nicrealtek.c   |    3 +--
 ogp_spi.c      |    2 --
 pcidev.c       |   37 ++++++++++++++++++++++++++++++++++---
 programmer.h   |    1 +
 satamv.c       |    7 +------
 satasii.c      |    1 -
 13 files changed, 44 insertions(+), 41 deletions(-)

diff --git a/atahpt.c b/atahpt.c
index 461191d..d19cb75 100644
--- a/atahpt.c
+++ b/atahpt.c
@@ -56,13 +56,6 @@ static const struct par_programmer par_programmer_atahpt = {
    	.chip_writen		= fallback_chip_writen,
 };

-static int atahpt_shutdown(void *data)
-{
-	/* Flash access is disabled automatically by PCI restore. */
-	pci_cleanup(pacc);
-	return 0;
-}
-
 int atahpt_init(void)
 {
    uint32_t reg32;
@@ -72,9 +65,6 @@ int atahpt_init(void)

io_base_addr = pcidev_init(PCI_BASE_ADDRESS_4, ata_hpt);

-	if (register_shutdown(atahpt_shutdown, NULL))
-		return 1;
-
    /* Enable flash access. */
    reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS);
    reg32 |= (1 << 24);
diff --git a/drkaiser.c b/drkaiser.c
index e461061..a6eca1c 100644
--- a/drkaiser.c
+++ b/drkaiser.c
@@ -59,8 +59,6 @@ static const struct par_programmer par_programmer_drkaiser = {
 static int drkaiser_shutdown(void *data)
 {
    physunmap(drkaiser_bar, DRKAISER_MEMMAP_SIZE);
-	/* Flash write is disabled automatically by PCI restore. */
-	pci_cleanup(pacc);
    return 0;
 }

@@ -71,6 +69,7 @@ int drkaiser_init(void)
    if (rget_io_perms())
    	return 1;

+	/* No need to check for errors, pcidev_init() will not return in case of errors. */
    addr = pcidev_init(PCI_BASE_ADDRESS_2, drkaiser_pcidev);

/* Write magic register to enable flash write. */
diff --git a/gfxnvidia.c b/gfxnvidia.c
index 624fd7c..a994d68 100644
--- a/gfxnvidia.c
+++ b/gfxnvidia.c
@@ -80,10 +80,6 @@ static const struct par_programmer par_programmer_gfxnvidia = {
 static int gfxnvidia_shutdown(void *data)
 {
    physunmap(nvidia_bar, GFXNVIDIA_MEMMAP_SIZE);
-	/* Flash interface access is disabled (and screen enabled) automatically
-	 * by PCI restore.
-	 */
-	pci_cleanup(pacc);
    return 0;
 }

@@ -94,6 +90,7 @@ int gfxnvidia_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, gfx_nvidia);

io_base_addr += 0x300000;
@@ -101,7 +98,6 @@ int gfxnvidia_init(void)

nvidia_bar = physmap("NVIDIA", io_base_addr, GFXNVIDIA_MEMMAP_SIZE);

-	/* Must be done before rpci calls. */
    if (register_shutdown(gfxnvidia_shutdown, NULL))
    	return 1;

diff --git a/internal.c b/internal.c
index eda4d59..8dd844d 100644
--- a/internal.c
+++ b/internal.c
@@ -245,10 +245,8 @@ int internal_init(void)
    internal_buses_supported = BUS_NONSPI;

/* Initialize PCI access for flash enables */
-	pacc = pci_alloc();	/* Get the pci_access structure */
-	/* Set all options you want -- here we stick with the defaults */
-	pci_init(pacc);		/* Initialize the PCI library */
-	pci_scan_bus(pacc);	/* We want to get the list of devices */
+	if (pci_init_common() != 0)
+		return 1;

if (processor_flash_enable()) {
    	msg_perr("Processor detection/init failed.\n"
diff --git a/nic3com.c b/nic3com.c
index 05eada6..4ec6193 100644
--- a/nic3com.c
+++ b/nic3com.c
@@ -81,7 +81,6 @@ static int nic3com_shutdown(void *data)
    	OUTL(internal_conf, io_base_addr + INTERNAL_CONFIG);
    }

-	pci_cleanup(pacc);
    return 0;
 }

@@ -90,6 +89,7 @@ int nic3com_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_3com);

id = pcidev_dev->device_id;
diff --git a/nicintel.c b/nicintel.c
index d210d78..8481915 100644
--- a/nicintel.c
+++ b/nicintel.c
@@ -63,7 +63,6 @@ static int nicintel_shutdown(void *data)
 {
    physunmap(nicintel_control_bar, NICINTEL_CONTROL_MEMMAP_SIZE);
    physunmap(nicintel_bar, NICINTEL_MEMMAP_SIZE);
-	pci_cleanup(pacc);
    return 0;
 }

@@ -77,8 +76,7 @@ int nicintel_init(void)
    if (rget_io_perms())
    	return 1;

-	/* No need to check for errors, pcidev_init() will not return in case
-	 * of errors.
+	/* No need to check for errors, pcidev_init() will not return in case of errors.
     * FIXME: BAR2 is not available if the device uses the CardBus function.
     */
    addr = pcidev_init(PCI_BASE_ADDRESS_2, nics_intel);
@@ -117,7 +115,6 @@ int nicintel_init(void)
 error_out_unmap:
    physunmap(nicintel_bar, NICINTEL_MEMMAP_SIZE);
 error_out:
-	pci_cleanup(pacc);
    return 1;
 }

diff --git a/nicintel_spi.c b/nicintel_spi.c
index 325e61c..f61c2b1 100644
--- a/nicintel_spi.c
+++ b/nicintel_spi.c
@@ -160,7 +160,6 @@ static int nicintel_spi_shutdown(void *data)
    pci_mmio_writel(tmp, nicintel_spibar + EECD);

physunmap(nicintel_spibar, MEMMAP_SIZE);
-	pci_cleanup(pacc);

return 0;
 }
@@ -172,6 +171,7 @@ int nicintel_spi_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_intel_spi);

nicintel_spibar = physmap("Intel Gigabit NIC w/ SPI flash",
diff --git a/nicrealtek.c b/nicrealtek.c
index 3c3b261..8349b42 100644
--- a/nicrealtek.c
+++ b/nicrealtek.c
@@ -54,7 +54,6 @@ static const struct par_programmer par_programmer_nicrealtek = {
 static int nicrealtek_shutdown(void *data)
 {
    /* FIXME: We forgot to disable software access again. */
-	pci_cleanup(pacc);
    return 0;
 }

@@ -63,8 +62,8 @@ 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 (register_shutdown(nicrealtek_shutdown, NULL))
    	return 1;

diff --git a/ogp_spi.c b/ogp_spi.c
index d1bb12f..6fb1a77 100644
--- a/ogp_spi.c
+++ b/ogp_spi.c
@@ -100,8 +100,6 @@ static const struct bitbang_spi_master bitbang_spi_master_ogp = {
 static int ogp_spi_shutdown(void *data)
 {
    physunmap(ogp_spibar, 4096);
-	pci_cleanup(pacc);
-
    return 0;
 }

diff --git a/pcidev.c b/pcidev.c
index 1a26e99..f2c8827 100644
--- a/pcidev.c
+++ b/pcidev.c
@@ -154,6 +154,33 @@ uintptr_t pcidev_readbar(struct pci_dev *dev, int bar)
    return (uintptr_t)addr;
 }

+static int pcidev_shutdown(void *data)
+{
+	pcidev_dev = NULL;
+	if (pacc == NULL) {
+		msg_perr("%s: Tried to cleanup an invalid PCI context!\n"
+			 "Please report a bug at [e-mail]\n", __func__);
+		return 1;
+	}
+	pci_cleanup(pacc);
+	return 0;
+}
+
+int pci_init_common(void)
+{
+	if (pacc != NULL) {
+		msg_perr("%s: Tried to allocate a new PCI context, but there is still an old one!\n"
+			 "Please report a bug at [e-mail]\n", __func__);
+		return 1;
+	}
+	pacc = pci_alloc();     /* Get the pci_access structure */
+	pci_init(pacc);         /* Initialize the PCI library */
+	if (register_shutdown(pcidev_shutdown, NULL))
+		return 1;
+	pci_scan_bus(pacc);     /* We want to get the list of devices */
+	return 0;
+}
+
 uintptr_t pcidev_init(int bar, const struct dev_entry *devs)
 {
    struct pci_dev *dev;
@@ -164,9 +191,8 @@ uintptr_t pcidev_init(int bar, const struct dev_entry *devs)
    int i;
    uintptr_t addr = 0, curaddr = 0;

-	pacc = pci_alloc();     /* Get the pci_access structure */
-	pci_init(pacc);         /* Initialize the PCI library */
-	pci_scan_bus(pacc);     /* We want to get the list of devices */
+	if(pci_init_common() != 0)
+		return 1;
    pci_filter_init(pacc, &filter);

/* Filter by bb:dd.f (if supplied by the user). */
@@ -244,6 +270,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;
+	}
    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) {
diff --git a/programmer.h b/programmer.h
index 8de42bc..4302809 100644
--- a/programmer.h
+++ b/programmer.h
@@ -238,6 +238,7 @@ void internal_delay(int usecs);
 extern uint32_t io_base_addr;
 extern struct pci_access *pacc;
 extern struct pci_dev *pcidev_dev;
+int pci_init_common(void);
 uintptr_t pcidev_readbar(struct pci_dev *dev, int bar);
 uintptr_t pcidev_init(int bar, const struct dev_entry *devs);
 /* rpci_write_* are reversible writes. The original PCI config space register
diff --git a/satamv.c b/satamv.c
index c0c1ffa..46a0e2d 100644
--- a/satamv.c
+++ b/satamv.c
@@ -60,7 +60,6 @@ static const struct par_programmer par_programmer_satamv = {
 static int satamv_shutdown(void *data)
 {
    physunmap(mv_bar, 0x20000);
-	pci_cleanup(pacc);
    return 0;
 }

@@ -96,7 +95,7 @@ int satamv_init(void)

mv_bar = physmap("Marvell 88SX7042 registers", addr, 0x20000);
    if (mv_bar == ERROR_PTR)
-		goto error_out;
+		return 1;

if (register_shutdown(satamv_shutdown, NULL))
    	return 1;
@@ -159,10 +158,6 @@ int satamv_init(void)
    register_par_programmer(&par_programmer_satamv, BUS_PARALLEL);

return 0;
-
-error_out:
-	pci_cleanup(pacc);
-	return 1;
 }

/* BAR2 (MEM) can map NVRAM and flash. We set it to flash in the init function.
diff --git a/satasii.c b/satasii.c
index 7b94203..730c420 100644
--- a/satasii.c
+++ b/satasii.c
@@ -57,7 +57,6 @@ static const struct par_programmer par_programmer_satasii = {
 static int satasii_shutdown(void *data)
 {
    physunmap(sii_bar, SATASII_MEMMAP_SIZE);
-	pci_cleanup(pacc);
    return 0;
 }