[flashrom] [PATCH 06/10] uintptr_t-ify physmap functions.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Mon Jul 15 10:20:29 CEST 2013
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? 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?
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.
> ---
> dummyflasher.c | 4 ++--
> flash.h | 3 +--
> flashrom.c | 3 +--
> physmap.c | 36 +++++++++++++++++++-----------------
> programmer.c | 2 +-
> programmer.h | 10 +++++-----
> 6 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/dummyflasher.c b/dummyflasher.c
> index e2ec036..8e861ee 100644
> --- a/dummyflasher.c
> +++ b/dummyflasher.c
> @@ -412,9 +412,9 @@ dummy_init_out:
> return 0;
> }
>
> -void *dummy_map(const char *descr, unsigned long phys_addr, size_t len)
> +void *dummy_map(const char *descr, uintptr_t phys_addr, size_t len)
> {
> - msg_pspew("%s: Mapping %s, 0x%lx bytes at 0x%08lx\n",
> + msg_pspew("%s: Mapping %s, 0x%lx bytes at 0x%08" PRIxPTR "\n",
> __func__, descr, (unsigned long)len, phys_addr);
> return (void *)phys_addr;
> }
> diff --git a/flash.h b/flash.h
> index 9e787c0..a752d6b 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -45,8 +45,7 @@
> typedef uintptr_t chipaddr;
>
> int register_shutdown(int (*function) (void *data), void *data);
> -void *programmer_map_flash_region(const char *descr, unsigned long phys_addr,
> - size_t len);
> +void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len);
> void programmer_unmap_flash_region(void *virt_addr, size_t len);
> void programmer_delay(int usecs);
>
> diff --git a/flashrom.c b/flashrom.c
> index c298748..43b7b11 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -417,8 +417,7 @@ int programmer_shutdown(void)
> return ret;
> }
>
> -void *programmer_map_flash_region(const char *descr, unsigned long phys_addr,
> - size_t len)
> +void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len)
> {
> return programmer_table[programmer].map_flash_region(descr,
> phys_addr, len);
> diff --git a/physmap.c b/physmap.c
> index 644ee35..8be4f06 100644
> --- a/physmap.c
> +++ b/physmap.c
> @@ -20,6 +20,7 @@
> * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> */
>
> +#include <inttypes.h>
> #include <unistd.h>
> #include <stdio.h>
> #include <stdlib.h>
> @@ -34,6 +35,8 @@
> #include <fcntl.h>
> #endif
>
> +#define PRIxPTR_WIDTH ((int)(sizeof(uintptr_t)*2))
> +
> #ifdef __DJGPP__
> #include <dpmi.h>
> #include <sys/nearptr.h>
> @@ -42,7 +45,7 @@
>
> static void *realmem_map;
>
> -static void *map_first_meg(unsigned long phys_addr, size_t len)
> +static void *map_first_meg(uintptr_t phys_addr, size_t len)
> {
> if (realmem_map)
> return realmem_map + phys_addr;
> @@ -61,7 +64,7 @@ static void *map_first_meg(unsigned long phys_addr, size_t len)
> return realmem_map + phys_addr;
> }
>
> -static void *sys_physmap(unsigned long phys_addr, size_t len)
> +static void *sys_physmap(uintptr_t phys_addr, size_t len)
> {
> int ret;
> __dpmi_meminfo mi;
> @@ -109,7 +112,7 @@ void physunmap(void *virt_addr, size_t len)
>
> #define MEM_DEV ""
>
> -void *sys_physmap(unsigned long phys_addr, size_t len)
> +void *sys_physmap(uintptr_t phys_addr, size_t len)
> {
> return (void *)phys_to_virt(phys_addr);
> }
> @@ -124,7 +127,7 @@ void physunmap(void *virt_addr, size_t len)
>
> #define MEM_DEV "DirectHW"
>
> -static void *sys_physmap(unsigned long phys_addr, size_t len)
> +static void *sys_physmap(uintptr_t phys_addr, size_t len)
> {
> /* The short form of ?: is a GNU extension.
> * FIXME: map_physical returns NULL both for errors and for success
> @@ -156,7 +159,7 @@ static int fd_mem = -1;
> static int fd_mem_cached = -1;
>
> /* For MMIO access. Must be uncached, doesn't make sense to restrict to ro. */
> -static void *sys_physmap_rw_uncached(unsigned long phys_addr, size_t len)
> +static void *sys_physmap_rw_uncached(uintptr_t phys_addr, size_t len)
> {
> void *virt_addr;
>
> @@ -176,7 +179,7 @@ static void *sys_physmap_rw_uncached(unsigned long phys_addr, size_t len)
> /* For reading DMI/coreboot/whatever tables. We should never write, and we
> * do not care about caching.
> */
> -static void *sys_physmap_ro_cached(unsigned long phys_addr, size_t len)
> +static void *sys_physmap_ro_cached(uintptr_t phys_addr, size_t len)
> {
> void *virt_addr;
>
> @@ -209,25 +212,24 @@ void physunmap(void *virt_addr, size_t len)
> #define PHYSMAP_RW 0
> #define PHYSMAP_RO 1
>
> -static void *physmap_common(const char *descr, unsigned long phys_addr,
> +static void *physmap_common(const char *descr, uintptr_t phys_addr,
> size_t len, int mayfail, int readonly)
> {
> void *virt_addr;
>
> if (len == 0) {
> - msg_pspew("Not mapping %s, zero size at 0x%08lx.\n",
> - descr, phys_addr);
> + msg_pspew("Not mapping %s, zero size at 0x%0*" PRIxPTR ".\n", descr, PRIxPTR_WIDTH, phys_addr);
> return ERROR_PTR;
> }
>
> if ((getpagesize() - 1) & len) {
> - msg_perr("Mapping %s at 0x%08lx, unaligned size 0x%lx.\n",
> - descr, phys_addr, (unsigned long)len);
> + msg_perr("Mapping %s at 0x%0*" PRIxPTR ", unaligned size 0x%lx.\n",
> + descr, PRIxPTR_WIDTH, phys_addr, (unsigned long)len);
> }
>
> if ((getpagesize() - 1) & phys_addr) {
> - msg_perr("Mapping %s, 0x%lx bytes at unaligned 0x%08lx.\n",
> - descr, (unsigned long)len, phys_addr);
> + msg_perr("Mapping %s, 0x%lx bytes at unaligned 0x%0*" PRIxPTR ".\n",
> + descr, (unsigned long)len, PRIxPTR_WIDTH, phys_addr);
> }
>
> if (readonly)
> @@ -238,8 +240,8 @@ static void *physmap_common(const char *descr, unsigned long phys_addr,
> if (ERROR_PTR == virt_addr) {
> if (NULL == descr)
> descr = "memory";
> - msg_perr("Error accessing %s, 0x%lx bytes at 0x%08lx\n", descr,
> - (unsigned long)len, phys_addr);
> + msg_perr("Error accessing %s, 0x%0zx bytes at 0x%0*" PRIxPTR "\n",
> + descr, len, PRIxPTR_WIDTH, phys_addr);
> msg_perr(MEM_DEV " mmap failed: %s\n", strerror(errno));
> #ifdef __linux__
> if (EINVAL == errno) {
> @@ -261,13 +263,13 @@ static void *physmap_common(const char *descr, unsigned long phys_addr,
> return virt_addr;
> }
>
> -void *physmap(const char *descr, unsigned long phys_addr, size_t len)
> +void *physmap(const char *descr, uintptr_t phys_addr, size_t len)
> {
> return physmap_common(descr, phys_addr, len, PHYSMAP_NOFAIL,
> PHYSMAP_RW);
> }
>
> -void *physmap_try_ro(const char *descr, unsigned long phys_addr, size_t len)
> +void *physmap_try_ro(const char *descr, uintptr_t phys_addr, size_t len)
> {
> return physmap_common(descr, phys_addr, len, PHYSMAP_MAYFAIL,
> PHYSMAP_RO);
> diff --git a/programmer.c b/programmer.c
> index 3b4def0..bf7dca1 100644
> --- a/programmer.c
> +++ b/programmer.c
> @@ -28,7 +28,7 @@ int noop_shutdown(void)
> }
>
> /* Fallback map() for programmers which don't need special handling */
> -void *fallback_map(const char *descr, unsigned long phys_addr, size_t len)
> +void *fallback_map(const char *descr, uintptr_t phys_addr, size_t len)
> {
> /* FIXME: Should return phys_addr. */
> return NULL;
> diff --git a/programmer.h b/programmer.h
> index 7de8f2c..4ed7c90 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -117,7 +117,7 @@ struct programmer_entry {
>
> int (*init) (void);
>
> - void *(*map_flash_region) (const char *descr, unsigned long phys_addr, size_t len);
> + void *(*map_flash_region) (const char *descr, uintptr_t phys_addr, size_t len);
> void (*unmap_flash_region) (void *virt_addr, size_t len);
>
> void (*delay) (int usecs);
> @@ -275,8 +275,8 @@ int processor_flash_enable(void);
> #endif
>
> /* physmap.c */
> -void *physmap(const char *descr, unsigned long phys_addr, size_t len);
> -void *physmap_try_ro(const char *descr, unsigned long phys_addr, size_t len);
> +void *physmap(const char *descr, uintptr_t phys_addr, size_t len);
> +void *physmap_try_ro(const char *descr, uintptr_t phys_addr, size_t len);
> void physunmap(void *virt_addr, size_t len);
> #if CONFIG_INTERNAL == 1
> int setup_cpu_msr(int cpu);
> @@ -359,7 +359,7 @@ void rmmio_vall(void *addr);
> /* dummyflasher.c */
> #if CONFIG_DUMMY == 1
> int dummy_init(void);
> -void *dummy_map(const char *descr, unsigned long phys_addr, size_t len);
> +void *dummy_map(const char *descr, uintptr_t phys_addr, size_t len);
> void dummy_unmap(void *virt_addr, size_t len);
> #endif
>
> @@ -609,7 +609,7 @@ int register_opaque_programmer(const struct opaque_programmer *pgm);
>
> /* programmer.c */
> int noop_shutdown(void);
> -void *fallback_map(const char *descr, unsigned long phys_addr, size_t len);
> +void *fallback_map(const char *descr, uintptr_t phys_addr, size_t len);
> void fallback_unmap(void *virt_addr, size_t len);
> void noop_chip_writeb(const struct flashctx *flash, uint8_t val, chipaddr addr);
> void fallback_chip_writew(const struct flashctx *flash, uint16_t val, chipaddr addr);
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.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list