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

[Stefan Tauner]

On Mon, 07 Jan 2013 01:46:16 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> 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.

and then it was forgotten more or less, sorry ;)
I had to rebase it (and solve small conflicts).

> > 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.

right.
I solved this by casting ERROR_PTR to chipaddr too, but I am not
entirely sure that is safe/what we want.

> > +#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).

Talk to me about MSVC again when they support C11. ;)

> >  #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.

How often do you read these arguments? Exactly. :P
Reverted anyway and added PHYSMAP_NOCLEANUP and PHYSMAP_CLEANUP
respectively.

> > +struct physmap_stutdown_data {
> 
> Maybe rename to undo_physmap_data for consistency with undo_pci_write_data.

done
 
> > +	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?

done

> > +	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?

done

> > +{
> > +	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.

No. This is mandatory to be supported since C99 and helps keeping scopes
small and uninitialized variables to a minimum which reduces bugs and
readability for anybody not mentally fixed to C89 syntax.

> > +	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.

... and would fail in cases when we do not need to malloc at all and we
would have to split the if... I don't think this makes sense for this
kind of error (which hopefully will never be triggered).

But there is a real bug... we have to check mayfail and act accordingly.
I have added this to the code at unmap_out.

New patch set will follow.
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner