Re: [flashrom] [PATCH 3/5] Automatically unmap physmap()s.

[Carl-Daniel Hailfinger]

Am 01.01.2013 18:29 schrieb Stefan Tauner:
> Similarly to the previous PCI self-clean up patch this one allows to get rid
> of a huge number of programmer shutdown functions and makes introducing
> bugs harder. It adds a new function physmap_autocleanup() that takes
> care of unmapping at shutdown. Callers are changed where it makes sense.
>
> Signed-off-by: Stefan Tauner

Good work! The individual driver conversions were checked thoroughly and
seem to be OK.

With the comments addressed, this is
Acked-by: Carl-Daniel Hailfinger

Can you please change the name of physmap_autocleanup to rphysmap? That
would be consistent with rmmio_write* and rpci_write* and be shorter as
well (fixing some 112 columns limit issues).
> diff --git a/it85spi.c b/it85spi.c
> index 0b074eb..b778436 100644
> --- a/it85spi.c
> +++ b/it85spi.c
> @@ -262,6 +262,9 @@ static int it85xx_spi_common_init(struct superio s)
>     * Major TODO here, and it will be a lot of work.
>     */
>    base = (chipaddr)physmap("it85 communication", 0xFFFFF000, 0x1000);
> if (base == ERROR_PTR)

I somehow doubt that this really compiles. AFAICS you might not have
noticed the type mismatch problem because of #if guards.
> return 1;
> msg_pdbg("%s():%d base=0x%08x\n", __func__, __LINE__,
>           (unsigned int)base);
>  ce_high = (unsigned char *)(base + 0xE00);  /* 0xFFFFFE00 */
>
> diff --git a/nicintel.c b/nicintel.c
> index 8481915..5463af2 100644
> --- a/nicintel.c
> +++ b/nicintel.c
> @@ -81,19 +74,15 @@ int nicintel_init(void)
>     */
>    addr = pcidev_init(PCI_BASE_ADDRESS_2, nics_intel);
> nicintel_bar = physmap("Intel NIC flash", addr, NICINTEL_MEMMAP_SIZE);
> nicintel_bar = physmap_autocleanup("Intel NIC flash", addr, NICINTEL_MEMMAP_SIZE);
> if (nicintel_bar == ERROR_PTR)
> goto error_out_unmap;
> return 1;
>
> /* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */
> addr = pcidev_readbar(pcidev_dev, PCI_BASE_ADDRESS_0);
> /* FIXME: This is not an aligned mapping. Use 4k? */
> nicintel_control_bar = physmap("Intel NIC control/status reg",
>                               addr, NICINTEL_CONTROL_MEMMAP_SIZE);
> nicintel_control_bar = physmap_autocleanup("Intel NIC control/status reg", addr, NICINTEL_CONTROL_MEMMAP_SIZE);

112 column limit... not a problem with rphysmap.
> if (nicintel_control_bar == ERROR_PTR)
> goto error_out;
>
> if (register_shutdown(nicintel_shutdown, NULL))
> return 1;
>
> /* FIXME: This register is pretty undocumented in all publicly available
>
> diff --git a/physmap.c b/physmap.c
> index 5aa9874..a9445d5 100644
> --- a/physmap.c
> +++ b/physmap.c
> @@ -21,6 +21,7 @@
>   */
>
> #include <unistd.h>
> +#include <stdbool.h>

Note: stdbool.h is only available in C99 compilers, which means this
won't ever compile with MSVC. I don't really have a strong preference
either way (and we use mingw/cygwin gcc for win* targets anyway).
> #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -214,13 +215,25 @@ void physunmap(void *virt_addr, size_t len)
>  }
>  #endif
>
> -#define PHYSMAP_NOFAIL	0
> -#define PHYSMAP_MAYFAIL	1
> -#define PHYSMAP_RW	0
> -#define PHYSMAP_RO	1

IMHO those #defines really helped readability. You can replace 0 and 1
by false/true if you want, but please keep the #defines.
> +struct physmap_stutdown_data {

Maybe rename to undo_physmap_data for consistency with undo_pci_write_data.
> void *addr;

Use virt_addr to make it obvious that this is not a physical address,
but the address where this was mapped into the address space of the
flashrom process?
> size_t len;
>
> +};
>
> -static void *physmap_common(const char *descr, unsigned long phys_addr,
> 	    size_t len, int mayfail, int readonly)
>
> +static int physmap_shutdown(void *data)

Rename to undo_physmap for consistency with undo_pci_write?
> +{
> if (data == NULL) {
> msg_perr("%s: tried to shutdown without valid data!\n", __func__);
> return 1;
> }
> struct physmap_stutdown_data *d = data;

Not all compilers support defining a new variable after code. Please
move the line above to the head of the function.
> physunmap(d->addr, d->len);
> free(data);
> return 0;
>
> +}
>
> +static void *physmap_common(const char *descr, unsigned long phys_addr, size_t len, bool mayfail,
> 	    bool readonly, bool autocleanup)
>
> {
>    void *virt_addr;
>
> @@ -268,19 +281,40 @@ static void *physmap_common(const char *descr, unsigned long phys_addr,
>    		exit(3);
>    }
> if (autocleanup) {
> struct physmap_stutdown_data *d = malloc(sizeof(struct physmap_stutdown_data));

I wonder if that malloc should be moved to the start of physmap_common.
It would detect OOM earlier, before we do any mapping.
> if (d == NULL) {
> 	msg_perr("%s: Out of memory!\n", __func__);
> 	goto unmap_out;
> }
> d->addr = virt_addr;
> d->len = len;
> if (register_shutdown(physmap_shutdown, d) != 0) {
> 	msg_perr("%s: Could not register shutdown function!\n", __func__);
> 	goto unmap_out;
> }
> }
> return virt_addr;
>
> +unmap_out:
> physunmap(virt_addr, len);
> return ERROR_PTR;
>
> }
>
> void *physmap(const char *descr, unsigned long phys_addr, size_t len)
>  {
> return physmap_common(descr, phys_addr, len, PHYSMAP_NOFAIL,
> 	      PHYSMAP_RW);
> return physmap_common(descr, phys_addr, len, false, false, false);

I really liked the #defines here because they increased readability.
> +}
>
> +void *physmap_autocleanup(const char *descr, unsigned long phys_addr, size_t len)
> +{
> return physmap_common(descr, phys_addr, len, false, false, true);
>
> }
>
> void *physmap_try_ro(const char *descr, unsigned long phys_addr, size_t len)
>  {
> return physmap_common(descr, phys_addr, len, PHYSMAP_MAYFAIL,
> 	      PHYSMAP_RO);
> return physmap_common(descr, phys_addr, len, true, true, false);
>
> }
>
> #if defined(__i386__) || defined(__x86_64__)

Regards,
Carl-Daniel