[flashrom] [PATCH 06/10] uintptr_t-ify physmap functions.

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Mon Jul 15 22:49:29 CEST 2013


On Mon, 15 Jul 2013 10:20:29 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 10.07.2013 21:17 schrieb Stefan Tauner:
> > unsigned long is not the right type for manipulating pointer values.
> > Since C99 there are suitable unsigned and signed types available, namely
> > uintptr_t and intptr_t respectively.
> >
> > Use them in physmap.c and its callers where applicable.
> >
> > This patch also changes the display width of all address values in
> > physmap.c to 16/8 hex characters depending on the actual size.
> >
> > Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> 
> Why don't you use PRIxPTR_WIDTH everywhere including dummyflasher.c?

That's a bug AFAICS, thanks.

> Put
> another way, is there a reason to use PRIxPTR_WIDTH at all?
> Do we care that C99 doesn't guarantee a 0x prefix for printed addresses
> with PRIxPTR? Will that cause confusion?

The main point of it is to get nicely aligned/readable addresses that
are padded to the right(tm) size, and yes I guess the guaranteed 0x
prefix too (I can't remember TBH :).

> I think PRIxPTR is a good idea, but I'm not so sure about the side effects.
> 
> There's also the problem of programmer_map_flash_region. It should not
> take an uintptr_t. The address parameter is a horrible mess: A physical
> address from the CPU point of view for the internal programmer, a
> top-of-4-GB aligned fake address for some other programmers, and a
> bottom-aligned address for the rest of our programmers. If we want to do
> this the right way, we need to have a wrapper for physmap in the
> "internal" programmer case instead of setting map_flash_region =
> physmap. And then I need to update my patch which abstracts the flash
> bus address vs programmer address conflict into something that won't
> cause madness.

Hm, I need to look into this again. Using a union for the type and a
wrapper does not sound so bad to me (without looking at the code :)
This could at least be made platform-independent without jumping
through hoops.

> Overall, while I'd like to have this patch in, it conceptually breaks as
> much stuff as it fixes. I don't see this getting fixed within the
> release timeframe, so I'd like to postpone.

Fine with me, but this means no win64 support in this stable.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner




More information about the flashrom mailing list