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@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);#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