[flashrom] [PATCH 3/5] Automatically unmap physmap()s.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Mon Jan 7 01:46:16 CET 2013
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 <stefan.tauner at student.tuwien.ac.at>
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 <c-d.hailfinger.devel.2006 at gmx.net>
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
--
http://www.hailfinger.org/
More information about the flashrom
mailing list