[RFC] Fix misc warnings when building sandbox on 64-bits

[RFC] Fix misc warnings when building sandbox on 64-bits

[Loïc Minier]

        Hi

 I was getting a bunch of different warnings, some quite annoying ones,
 when building sandbox on x86-64, not all of them related to 64-bits.

 I'm completely new to the barebox codebase and not necessarily familiar
 with the linux kernel header usage in barebox, so sending these patches
 as RFC to see what others think of the fixes.

 Some comments on the patches:

 The first two seems pretty safe:
 - Only pass -P to cpp when generating ld scripts
 - Avoid warnings by using format(__printf__)

 This one might change some API but seems right in terms of semantics;
 sandbox builds and starts up fine:
 - fprintf() returns an int

 These two are some shots at supporting sandbox on x86-64:
 - Use %p in format for pointers
 - Use size_t for memory offsets
 essentially I've noticed that nobody is setting
 CONFIG_PHYS_ADDR_T_64BIT, yet barebox uses the resource_size_t type
 which requires this config to be set properly.  Now, because I don't
 set CONFIG_PHYS_ADDR_T_64BIT in this patch series, what you'll get is
 actually new warnings after the changes to use size_t or %p for
 pointers because they are still stored on u32 on x86-64...  Also, this
 might break some API/ABI.  I don't really know how to set
 CONFIG_PHYS_ADDR_T_64BIT automatically for sandbox, so suggestions
 welcome.

    Thanks!
-- 
Loïc Minier

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/5] Only pass -P to cpp when generating ld scripts

[Loïc Minier]

When building sandbox with ccache, one would hit warnings such as:
    warning: 'struct mmsghdr' declared inside parameter list
on random files; a way to reproduce this issue is to build a simple
file doing just:
    #include <sys/socket.h>

    int main(void) {
        return 0;
    }

    gcc -Wall -P -c -o foo foo.c

But actually the -P flag is only useful when generating non-C files,
such as linker scripts in the case of barebox.  Removing the -P flag
from all the gcc invocations, except when generating .lds files makes
the warning go away.  It turns out that this is what
linux/scripts/Makefile.build also does nowadays.

Signed-off-by: Loïc Minier <loic.minier@linaro.org>
---
 Makefile                 |    2 +-
 arch/arm/Makefile        |    2 +-
 arch/blackfin/Makefile   |    2 +-
 arch/ppc/Makefile        |    2 +-
 arch/sandbox/Makefile    |    1 -
 arch/sandbox/os/Makefile |    1 -
 arch/x86/Makefile        |    2 +-
 scripts/Makefile.build   |    2 +-
 8 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 5414e63..10d5780 100644
--- a/Makefile
+++ b/Makefile
@@ -825,7 +825,7 @@ prepare prepare-all: prepare0
 # Leave this as default for preprocessing barebox.lds.S, which is now
 # done in arch/$(ARCH)/kernel/Makefile
 
-export CPPFLAGS_barebox.lds += -P -C -U$(ARCH)
+export CPPFLAGS_barebox.lds += -C -U$(ARCH)
 
 # FIXME: The asm symlink changes when $(ARCH) changes. That's
 # hard to detect, but I suppose "make mrproper" is a good idea
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index d25412d..0c42f3d 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -121,7 +121,7 @@ endif
 
 TEXT_BASE = $(CONFIG_TEXT_BASE)
 
-CPPFLAGS += -DTEXT_BASE=$(TEXT_BASE) -P
+CPPFLAGS += -DTEXT_BASE=$(TEXT_BASE)
 
 ifndef CONFIG_MODULES
 # Add cleanup flags
diff --git a/arch/blackfin/Makefile b/arch/blackfin/Makefile
index 902268d..a0b87f7 100644
--- a/arch/blackfin/Makefile
+++ b/arch/blackfin/Makefile
@@ -7,7 +7,7 @@ cpu-$(CONFIG_BF561)		:= bf561
 
 TEXT_BASE = $(CONFIG_TEXT_BASE)
 
-CPPFLAGS += -DTEXT_BASE=$(TEXT_BASE) -P
+CPPFLAGS += -DTEXT_BASE=$(TEXT_BASE)
 CFLAGS += -D__blackfin__
 # -Ttext $(TEXT_BASE)
 KALLSYMS         += --symbol-prefix=_
diff --git a/arch/ppc/Makefile b/arch/ppc/Makefile
index 46d64e5..c7bf863 100644
--- a/arch/ppc/Makefile
+++ b/arch/ppc/Makefile
@@ -14,7 +14,7 @@ cpu-$(CONFIG_ARCH_MPC5200)			:= mpc5xxx
 
 TEXT_BASE = $(CONFIG_TEXT_BASE)
 
-CPPFLAGS += -DTEXT_BASE=$(TEXT_BASE) -P
+CPPFLAGS += -DTEXT_BASE=$(TEXT_BASE)
 
 # Add cleanup flags
 ifndef CONFIG_MODULES
diff --git a/arch/sandbox/Makefile b/arch/sandbox/Makefile
index 4ca17ed..73c06db 100644
--- a/arch/sandbox/Makefile
+++ b/arch/sandbox/Makefile
@@ -10,7 +10,6 @@ lds-y   := $(BOARD)/barebox.lds
 
 TEXT_BASE = $(CONFIG_TEXT_BASE)
 
-CPPFLAGS += -P
 CFLAGS += -Dmalloc=barebox_malloc \
 		-Dfree=barebox_free -Drealloc=barebox_realloc \
 		-Dread=barebox_read -Dwrite=barebox_write \
diff --git a/arch/sandbox/os/Makefile b/arch/sandbox/os/Makefile
index 2980301..dc211d9 100644
--- a/arch/sandbox/os/Makefile
+++ b/arch/sandbox/os/Makefile
@@ -8,7 +8,6 @@ else
 CPPFLAGS = $(patsubst %,-I$(srctree)/%include,$(machdirs))
 endif
 
-CPPFLAGS += -P
 CFLAGS := -Wall
 NOSTDINC_FLAGS :=
 
diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 3b034c0..db4180b 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -5,7 +5,7 @@ machine-y := i386
 
 TEXT_BASE = $(CONFIG_TEXT_BASE)
 
-CPPFLAGS += -march=i386 -m32 -DTEXT_BASE=$(TEXT_BASE) -P
+CPPFLAGS += -march=i386 -m32 -DTEXT_BASE=$(TEXT_BASE)
 LDFLAGS += -m elf_i386
 
 ifndef CONFIG_MODULES
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index c2bab5c..f70e2b9 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -244,7 +244,7 @@ targets += $(extra-y) $(MAKECMDGOALS) $(always)
 # Linker scripts preprocessor (.lds.S -> .lds)
 # ---------------------------------------------------------------------------
 quiet_cmd_cpp_lds_S = LDS     $@
-      cmd_cpp_lds_S = $(CPP) $(cpp_flags) -D__ASSEMBLY__ -o $@ $<
+      cmd_cpp_lds_S = $(CPP) $(cpp_flags) -P -D__ASSEMBLY__ -o $@ $<
 
 %.lds: %.lds.S FORCE
 	$(call if_changed_dep,cpp_lds_S)
-- 
1.7.5.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/5] Avoid warnings by using format(__printf__)

[Loïc Minier]

When building sandbox, barebox is built with -Dprintf=barebox_printf as
to not collide with the printf provided by libc.  This would also match
the format(printf) function __attribute__.

Since gcc documents that __printf__ can be used instead of printf as a
format attribute, use this instead and avoid a lot of noisy warnings.

NB: this relates to 6b082cfe9f9b5b2bea294918ad916c739490cea7 which was
an earlier attempt at solving this, which got reverted due to other
regressions.

Signed-off-by: Loïc Minier <loic.minier@linaro.org>
---
 include/stdio.h       |   12 ++++++------
 scripts/mod/modpost.c |    2 +-
 scripts/mod/modpost.h |    2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/stdio.h b/include/stdio.h
index c824764..a0d81d3 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -9,7 +9,7 @@
  */
 
 /* serial stuff */
-void	serial_printf(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
+void	serial_printf(const char *fmt, ...) __attribute__ ((format(__printf__, 1, 2)));
 
 /* stdin */
 int	tstc(void);
@@ -30,12 +30,12 @@ static inline void putchar(char c)
 	console_putc(CONSOLE_STDOUT, c);
 }
 
-int	printf(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
+int	printf(const char *fmt, ...) __attribute__ ((format(__printf__, 1, 2)));
 int	vprintf(const char *fmt, va_list args);
-int	sprintf(char *buf, const char *fmt, ...) __attribute__ ((format(printf, 2, 3)));
-int	snprintf(char *buf, size_t size, const char *fmt, ...) __attribute__ ((format(printf, 3, 4)));
+int	sprintf(char *buf, const char *fmt, ...) __attribute__ ((format(__printf__, 2, 3)));
+int	snprintf(char *buf, size_t size, const char *fmt, ...) __attribute__ ((format(__printf__, 3, 4)));
 int	vsprintf(char *buf, const char *fmt, va_list args);
-char	*asprintf(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
+char	*asprintf(const char *fmt, ...) __attribute__ ((format(__printf__, 1, 2)));
 char	*vasprintf(const char *fmt, va_list ap);
 int	vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
 int	vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
@@ -54,7 +54,7 @@ int	vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
 #define stderr		2
 #define MAX_FILES	128
 
-void	fprintf(int file, const char *fmt, ...) __attribute__ ((format(printf, 2, 3)));
+void	fprintf(int file, const char *fmt, ...) __attribute__ ((format(__printf__, 2, 3)));
 int	fputs(int file, const char *s);
 int	fputc(int file, const char c);
 int	ftstc(int file);
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 08b75b6..e994486 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1300,7 +1300,7 @@ static void read_symbols(char *modname)
  * following helper, then compare to the file on disk and
  * only update the later if anything changed */
 
-void __attribute__((format(printf, 2, 3))) buf_printf(struct buffer *buf,
+void __attribute__((format(__printf__, 2, 3))) buf_printf(struct buffer *buf,
 						      const char *fmt, ...)
 {
 	char tmp[SZ];
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 4156dd3..0c23259 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -94,7 +94,7 @@ struct buffer {
 	int size;
 };
 
-void __attribute__((format(printf, 2, 3)))
+void __attribute__((format(__printf__, 2, 3)))
 buf_printf(struct buffer *buf, const char *fmt, ...);
 
 void
-- 
1.7.5.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/5] Avoid warnings by using format(__printf__)

[Sascha Hauer]

Hi Loïc,

On Fri, Oct 14, 2011 at 12:06:38AM +0200, Loïc Minier wrote:
> When building sandbox, barebox is built with -Dprintf=barebox_printf as
> to not collide with the printf provided by libc.  This would also match
> the format(printf) function __attribute__.
> 
> Since gcc documents that __printf__ can be used instead of printf as a
> format attribute, use this instead and avoid a lot of noisy warnings.
> 
> NB: this relates to 6b082cfe9f9b5b2bea294918ad916c739490cea7 which was
> an earlier attempt at solving this, which got reverted due to other
> regressions.

\o/

Thank you for finding a solution for this. That was really an annoying
issue.

Sascha

> 
> Signed-off-by: Loïc Minier <loic.minier@linaro.org>
> ---
>  include/stdio.h       |   12 ++++++------
>  scripts/mod/modpost.c |    2 +-
>  scripts/mod/modpost.h |    2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/stdio.h b/include/stdio.h
> index c824764..a0d81d3 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -9,7 +9,7 @@
>   */
>  
>  /* serial stuff */
> -void	serial_printf(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
> +void	serial_printf(const char *fmt, ...) __attribute__ ((format(__printf__, 1, 2)));
>  
>  /* stdin */
>  int	tstc(void);
> @@ -30,12 +30,12 @@ static inline void putchar(char c)
>  	console_putc(CONSOLE_STDOUT, c);
>  }
>  
> -int	printf(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
> +int	printf(const char *fmt, ...) __attribute__ ((format(__printf__, 1, 2)));
>  int	vprintf(const char *fmt, va_list args);
> -int	sprintf(char *buf, const char *fmt, ...) __attribute__ ((format(printf, 2, 3)));
> -int	snprintf(char *buf, size_t size, const char *fmt, ...) __attribute__ ((format(printf, 3, 4)));
> +int	sprintf(char *buf, const char *fmt, ...) __attribute__ ((format(__printf__, 2, 3)));
> +int	snprintf(char *buf, size_t size, const char *fmt, ...) __attribute__ ((format(__printf__, 3, 4)));
>  int	vsprintf(char *buf, const char *fmt, va_list args);
> -char	*asprintf(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
> +char	*asprintf(const char *fmt, ...) __attribute__ ((format(__printf__, 1, 2)));
>  char	*vasprintf(const char *fmt, va_list ap);
>  int	vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
>  int	vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
> @@ -54,7 +54,7 @@ int	vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
>  #define stderr		2
>  #define MAX_FILES	128
>  
> -void	fprintf(int file, const char *fmt, ...) __attribute__ ((format(printf, 2, 3)));
> +void	fprintf(int file, const char *fmt, ...) __attribute__ ((format(__printf__, 2, 3)));
>  int	fputs(int file, const char *s);
>  int	fputc(int file, const char c);
>  int	ftstc(int file);
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 08b75b6..e994486 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1300,7 +1300,7 @@ static void read_symbols(char *modname)
>   * following helper, then compare to the file on disk and
>   * only update the later if anything changed */
>  
> -void __attribute__((format(printf, 2, 3))) buf_printf(struct buffer *buf,
> +void __attribute__((format(__printf__, 2, 3))) buf_printf(struct buffer *buf,
>  						      const char *fmt, ...)
>  {
>  	char tmp[SZ];
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 4156dd3..0c23259 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -94,7 +94,7 @@ struct buffer {
>  	int size;
>  };
>  
> -void __attribute__((format(printf, 2, 3)))
> +void __attribute__((format(__printf__, 2, 3)))
>  buf_printf(struct buffer *buf, const char *fmt, ...);
>  
>  void
> -- 
> 1.7.5.4
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/5] fprintf() returns an int

[Loïc Minier]

For consistency, let fprintf return an int just like it's regular libc
implementation and all the other printf variations barebox has.  This
also fixes a warning on variable i being never read in the function.

Signed-off-by: Loïc Minier <loic.minier@linaro.org>
---
 common/console.c |    4 +++-
 include/stdio.h  |    2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/common/console.c b/common/console.c
index 06e9c29..0d2a33b 100644
--- a/common/console.c
+++ b/common/console.c
@@ -327,7 +327,7 @@ void console_flush(void)
 }
 EXPORT_SYMBOL(console_flush);
 
-void fprintf (int file, const char *fmt, ...)
+int fprintf (int file, const char *fmt, ...)
 {
 	va_list args;
 	uint i;
@@ -343,6 +343,8 @@ void fprintf (int file, const char *fmt, ...)
 
 	/* Print the string */
 	fputs (file, printbuffer);
+
+	return i;
 }
 EXPORT_SYMBOL(fprintf);
 
diff --git a/include/stdio.h b/include/stdio.h
index a0d81d3..0c68fa8 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -54,7 +54,7 @@ int	vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
 #define stderr		2
 #define MAX_FILES	128
 
-void	fprintf(int file, const char *fmt, ...) __attribute__ ((format(__printf__, 2, 3)));
+int	fprintf(int file, const char *fmt, ...) __attribute__ ((format(__printf__, 2, 3)));
 int	fputs(int file, const char *s);
 int	fputc(int file, const char c);
 int	ftstc(int file);
-- 
1.7.5.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/5] fprintf() returns an int

[Sascha Hauer]

On Fri, Oct 14, 2011 at 12:06:39AM +0200, Loïc Minier wrote:
> For consistency, let fprintf return an int just like it's regular libc
> implementation and all the other printf variations barebox has.  This
> also fixes a warning on variable i being never read in the function.
> 
> Signed-off-by: Loïc Minier <loic.minier@linaro.org>
> ---
>  common/console.c |    4 +++-
>  include/stdio.h  |    2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/common/console.c b/common/console.c
> index 06e9c29..0d2a33b 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -327,7 +327,7 @@ void console_flush(void)
>  }
>  EXPORT_SYMBOL(console_flush);
>  
> -void fprintf (int file, const char *fmt, ...)
> +int fprintf (int file, const char *fmt, ...)
>  {
>  	va_list args;
>  	uint i;
> @@ -343,6 +343,8 @@ void fprintf (int file, const char *fmt, ...)
>  
>  	/* Print the string */
>  	fputs (file, printbuffer);
> +
> +	return i;
>  }
>  EXPORT_SYMBOL(fprintf);

fprintf is supposed to return the number of characters printed, that is
the return value of fputs. I know that fputs currently returns a wrong
value, but maybe we can fix this with the following:



From 51a7a01e90cc50c6b41bae60dcf70a50f0ff3486 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Fri, 14 Oct 2011 08:56:22 +0200
Subject: [PATCH] console: fix return values of puts functions

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/console.c |   14 ++++++++++----
 include/stdio.h  |    6 +++---
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/common/console.c b/common/console.c
index 0d2a33b..6216e88 100644
--- a/common/console.c
+++ b/common/console.c
@@ -292,24 +292,30 @@ int fputc(int fd, char c)
 }
 EXPORT_SYMBOL(fputc);
 
-void console_puts(unsigned int ch, const char *str)
+int console_puts(unsigned int ch, const char *str)
 {
 	const char *s = str;
+	int n = 0;
+
 	while (*s) {
-		if (*s == '\n')
+		if (*s == '\n') {
 			console_putc(ch, '\r');
+			n++;
+		}
 		console_putc(ch, *s);
+		n++;
 		s++;
 	}
+	return n;
 }
 EXPORT_SYMBOL(console_puts);
 
 int fputs(int fd, const char *s)
 {
 	if (fd == 1)
-		puts(s);
+		return puts(s);
 	else if (fd == 2)
-		eputs(s);
+		return eputs(s);
 	else
 		return write(fd, s, strlen(s));
 	return 0;
diff --git a/include/stdio.h b/include/stdio.h
index 0c68fa8..4901bc7 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -17,12 +17,12 @@ int	tstc(void);
 /* stdout */
 void	console_putc(unsigned int ch, const char c);
 int	getc(void);
-void	console_puts(unsigned int ch, const char *s);
+int	console_puts(unsigned int ch, const char *s);
 void	console_flush(void);
 
-static inline void puts(const char *s)
+static inline int puts(const char *s)
 {
-	console_puts(CONSOLE_STDOUT, s);
+	return console_puts(CONSOLE_STDOUT, s);
 }
 
 static inline void putchar(char c)
-- 
1.7.7

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/5] Use %p in format for pointers

[Loïc Minier]

res->start and res->size are resource_type_t which is a phys_addr_t
which itself could someday be a 64-bits pointer (for instance when
building sandbox on a 64-bits host).  Instead of hardcoding %08x as a
format for pointers, use the %p format.

Signed-off-by: Loïc Minier <loic.minier@linaro.org>
---
 drivers/base/driver.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 0b80103..515ff8d 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -349,8 +349,8 @@ static int do_devinfo(struct command *cmdtp, int argc, char *argv[])
 			printf("num   : %d\n", i);
 			if (res->name)
 				printf("name  : %s\n", res->name);
-			printf("start : 0x%08x\nsize  : 0x%08x\n",
-			       res->start, res->size);
+			printf("start : 0x%p\nsize  : 0x%p\n",
+			       (void *)res->start, (void *)res->size);
 		}
 
 		printf("driver: %s\n\n", dev->driver ?
-- 
1.7.5.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] Use %p in format for pointers

[Sascha Hauer]

On Fri, Oct 14, 2011 at 12:06:40AM +0200, Loïc Minier wrote:
> res->start and res->size are resource_type_t which is a phys_addr_t
> which itself could someday be a 64-bits pointer (for instance when
> building sandbox on a 64-bits host).  Instead of hardcoding %08x as a
> format for pointers, use the %p format.
> 
> Signed-off-by: Loïc Minier <loic.minier@linaro.org>
> ---
>  drivers/base/driver.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 0b80103..515ff8d 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -349,8 +349,8 @@ static int do_devinfo(struct command *cmdtp, int argc, char *argv[])
>  			printf("num   : %d\n", i);
>  			if (res->name)
>  				printf("name  : %s\n", res->name);
> -			printf("start : 0x%08x\nsize  : 0x%08x\n",
> -			       res->start, res->size);
> +			printf("start : 0x%p\nsize  : 0x%p\n",
> +			       (void *)res->start, (void *)res->size);

With this patch I get the following warning on a 64bit host:

drivers/base/driver.c: In function ‘do_devinfo’:
drivers/base/driver.c:353:11: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
drivers/base/driver.c:353:31: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

We should probably set CONFIG_PHYS_ADDR_T_64BIT correctly on a 64bit
host.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] Use %p in format for pointers

[Sascha Hauer]

On Fri, Oct 14, 2011 at 09:18:50AM +0200, Sascha Hauer wrote:
> On Fri, Oct 14, 2011 at 12:06:40AM +0200, Loïc Minier wrote:
> > res->start and res->size are resource_type_t which is a phys_addr_t
> > which itself could someday be a 64-bits pointer (for instance when
> > building sandbox on a 64-bits host).  Instead of hardcoding %08x as a
> > format for pointers, use the %p format.
> > 
> > Signed-off-by: Loïc Minier <loic.minier@linaro.org>
> > ---
> >  drivers/base/driver.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> > index 0b80103..515ff8d 100644
> > --- a/drivers/base/driver.c
> > +++ b/drivers/base/driver.c
> > @@ -349,8 +349,8 @@ static int do_devinfo(struct command *cmdtp, int argc, char *argv[])
> >  			printf("num   : %d\n", i);
> >  			if (res->name)
> >  				printf("name  : %s\n", res->name);
> > -			printf("start : 0x%08x\nsize  : 0x%08x\n",
> > -			       res->start, res->size);
> > +			printf("start : 0x%p\nsize  : 0x%p\n",
> > +			       (void *)res->start, (void *)res->size);
> 
> With this patch I get the following warning on a 64bit host:
> 
> drivers/base/driver.c: In function ‘do_devinfo’:
> drivers/base/driver.c:353:11: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> drivers/base/driver.c:353:31: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 
> We should probably set CONFIG_PHYS_ADDR_T_64BIT correctly on a 64bit
> host.

Should have read your introductory mail more closely :-/

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/5] Use size_t for memory offsets

[Loïc Minier]

mem_read() and mem_write() did arithmetic with a local ulong size
variable, a size_t count and an ulong offset to return a ssize_t result.
Change them to use size_t for size and offset and stop casting count to
ulong.

This fixes a warning when building for 64-bits.

This change might have impact on the exported symbols.

Signed-off-by: Loïc Minier <loic.minier@linaro.org>
---
 fs/fs.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index 7d65ec8..0cb226d 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -1038,31 +1038,31 @@ static void memcpy_sz(void *_dst, const void *_src, ulong count, ulong rwsize)
 	}
 }
 
-ssize_t mem_read(struct cdev *cdev, void *buf, size_t count, ulong offset, ulong flags)
+ssize_t mem_read(struct cdev *cdev, void *buf, size_t count, size_t offset, ulong flags)
 {
-	ulong size;
+	size_t size;
 	struct device_d *dev;
 
 	if (!cdev->dev || cdev->dev->num_resources < 1)
 		return -1;
 	dev = cdev->dev;
 
-	size = min((ulong)count, dev->resource[0].size - offset);
+	size = min(count, (size_t)dev->resource[0].size - offset);
 	memcpy_sz(buf, dev_get_mem_region(dev, 0) + offset, size, flags & O_RWSIZE_MASK);
 	return size;
 }
 EXPORT_SYMBOL(mem_read);
 
-ssize_t mem_write(struct cdev *cdev, const void *buf, size_t count, ulong offset, ulong flags)
+ssize_t mem_write(struct cdev *cdev, const void *buf, size_t count, size_t offset, ulong flags)
 {
-	ulong size;
+	size_t size;
 	struct device_d *dev;
 
 	if (!cdev->dev || cdev->dev->num_resources < 1)
 		return -1;
 	dev = cdev->dev;
 
-	size = min((ulong)count, dev->resource[0].size - offset);
+	size = min(count, (size_t)dev->resource[0].size - offset);
 	memcpy_sz(dev_get_mem_region(dev, 0) + offset, buf, size, flags & O_RWSIZE_MASK);
 	return size;
 }
-- 
1.7.5.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/5] Use size_t for memory offsets

[Sascha Hauer]

On Fri, Oct 14, 2011 at 12:06:41AM +0200, Loïc Minier wrote:
> mem_read() and mem_write() did arithmetic with a local ulong size
> variable, a size_t count and an ulong offset to return a ssize_t result.
> Change them to use size_t for size and offset and stop casting count to
> ulong.

You should change the prototypes in include/driver.h aswell.

Sascha

> 
> This fixes a warning when building for 64-bits.
> 
> This change might have impact on the exported symbols.
> 
> Signed-off-by: Loïc Minier <loic.minier@linaro.org>
> ---
>  fs/fs.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/fs.c b/fs/fs.c
> index 7d65ec8..0cb226d 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -1038,31 +1038,31 @@ static void memcpy_sz(void *_dst, const void *_src, ulong count, ulong rwsize)
>  	}
>  }
>  
> -ssize_t mem_read(struct cdev *cdev, void *buf, size_t count, ulong offset, ulong flags)
> +ssize_t mem_read(struct cdev *cdev, void *buf, size_t count, size_t offset, ulong flags)
>  {
> -	ulong size;
> +	size_t size;
>  	struct device_d *dev;
>  
>  	if (!cdev->dev || cdev->dev->num_resources < 1)
>  		return -1;
>  	dev = cdev->dev;
>  
> -	size = min((ulong)count, dev->resource[0].size - offset);
> +	size = min(count, (size_t)dev->resource[0].size - offset);
>  	memcpy_sz(buf, dev_get_mem_region(dev, 0) + offset, size, flags & O_RWSIZE_MASK);
>  	return size;
>  }
>  EXPORT_SYMBOL(mem_read);
>  
> -ssize_t mem_write(struct cdev *cdev, const void *buf, size_t count, ulong offset, ulong flags)
> +ssize_t mem_write(struct cdev *cdev, const void *buf, size_t count, size_t offset, ulong flags)
>  {
> -	ulong size;
> +	size_t size;
>  	struct device_d *dev;
>  
>  	if (!cdev->dev || cdev->dev->num_resources < 1)
>  		return -1;
>  	dev = cdev->dev;
>  
> -	size = min((ulong)count, dev->resource[0].size - offset);
> +	size = min(count, (size_t)dev->resource[0].size - offset);
>  	memcpy_sz(dev_get_mem_region(dev, 0) + offset, buf, size, flags & O_RWSIZE_MASK);
>  	return size;
>  }
> -- 
> 1.7.5.4
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/5] Use size_t for memory offsets

[Loïc Minier]

On Fri, Oct 14, 2011, Sascha Hauer wrote:
> You should change the prototypes in include/driver.h aswell.

 Ah thanks, now it strikes me that the very same constructs are present
 in many file_operations implementations; e.g. imx_iim_cdev_read and
 imx_iim_cdev_write also use an ulong offset, as well as
 ubi_volume_cdev_read/ubi_volume_cdev_write (unsigned long), lp_read,
 miidev_read/_write etc.

 I had a look at file_operations in linux now, and it uses
 size_t/ssize_t and a loff_t type for regular read/write:
struct file_operations {
        loff_t (*llseek) (struct file *, loff_t, int);
        ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
        ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);

 however for aio:
        ssize_t (*aio_read) (struct kiocb *, const struct iovec *, unsigned long, loff_t);
        ssize_t (*aio_write) (struct kiocb *, const struct iovec *, unsigned long, loff_t);

 loff_t is defined as long long on 32-bits and 64-bits arches, which I
 believe are both 8 bytes.

 So perhaps it's better to switch from ulong to unsigned long long for
 offsets?  This isn't important for mem_read/mem_write, but it would be
 for e.g. MMC accesses as it's of course valid to seek after the first
 4 G of a MMC on a 32-bits system.

-- 
Loïc Minier

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/5] Use size_t for memory offsets

[Sascha Hauer]

On Fri, Oct 14, 2011 at 10:59:03AM +0200, Loïc Minier wrote:
> On Fri, Oct 14, 2011, Sascha Hauer wrote:
> > You should change the prototypes in include/driver.h aswell.
> 
>  Ah thanks, now it strikes me that the very same constructs are present
>  in many file_operations implementations; e.g. imx_iim_cdev_read and
>  imx_iim_cdev_write also use an ulong offset, as well as
>  ubi_volume_cdev_read/ubi_volume_cdev_write (unsigned long), lp_read,
>  miidev_read/_write etc.
> 
>  I had a look at file_operations in linux now, and it uses
>  size_t/ssize_t and a loff_t type for regular read/write:
> struct file_operations {
>         loff_t (*llseek) (struct file *, loff_t, int);
>         ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
>         ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
> 
>  however for aio:
>         ssize_t (*aio_read) (struct kiocb *, const struct iovec *, unsigned long, loff_t);
>         ssize_t (*aio_write) (struct kiocb *, const struct iovec *, unsigned long, loff_t);
> 
>  loff_t is defined as long long on 32-bits and 64-bits arches, which I
>  believe are both 8 bytes.
> 
>  So perhaps it's better to switch from ulong to unsigned long long for
>  offsets?  This isn't important for mem_read/mem_write, but it would be
>  for e.g. MMC accesses as it's of course valid to seek after the first
>  4 G of a MMC on a 32-bits system.

Yes, an 8 byte type would be definitely better for file offsets. It
would be a first step to support SD cards > 4G. I have looked into
this several times before and ended up with huge patches everytime
I tried.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox