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