Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu isright for sector_t

From: Andrew Morton
Date: Fri Oct 08 2004 - 14:25:25 EST

Next message: Neil Horman: "Re: character device interface to existing socket interface."
Previous message: michael_soulier: "wait_event and preemption in 2.6"
In reply to: blaisorblade_spam: "[patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t"
Next in thread: Anton Altaparmakov: "Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu isright for sector_t"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

(cc'ed Neil)

blaisorblade_spam@xxxxxxxx wrote:
>
>
> The Device Manager code barfs when sector_t is 64bit wide (i.e. an u64);
> this can happen with CONFIG_LBD on i386, too, but sector_t is usually a long.
> And region_t, chunk_t are typedefs for sector_t.
>
> The problem is this code in drivers/md/dm.h (wouldn't we better fix the
> FIXME?):
>
> /*
> * FIXME: I think this should be with the definition of sector_t
> * in types.h.
> */
> #ifdef CONFIG_LBD
> #define SECTOR_FORMAT "%Lu"
> #else
> #define SECTOR_FORMAT "%lu"
> #endif
>
> Btw, x86_64 does not need to define sector_t on its own.
> If there is any _good_ reason for that, the fix becomes adding a
> #define SECTOR_FORMAT "%Lu"
> in fact, gcc tries to be smart for warnings to ensure portability; so, even
> when sizeof(long) == sizeof(long long), "%ld" and "%Ld" are different, for the
> warnings).
>
> Sample warnings (from both 2.6.8.1 and 2.6.9-rc2):
> drivers/md/dm-raid1.c: In function `get_mirror':
> drivers/md/dm-raid1.c:930: warning: long unsigned int format, sector_t arg (arg 3)
> drivers/md/dm-raid1.c: In function `mirror_status':
> drivers/md/dm-raid1.c:1200: warning: long unsigned int format, region_t arg (arg 4)
> drivers/md/dm-raid1.c:1200: warning: long unsigned int format, region_t arg (arg 5)
> drivers/md/dm-raid1.c:1206: warning: long unsigned int format, sector_t arg (arg 5)
> drivers/md/dm-raid1.c:1212: warning: long unsigned int format, sector_t arg (arg 5)
>

I would prefer that SECTOR_FORMAT be removed altogether.

The industry-standard way of printing a sector_t is:

printk("%llu", (unsigned long long)sector);

Or %Ld or %llo or whatever. The key point is that the format string should
specify long long and the argument should be typecast to long long.

The sector_t should not be typecast to u64, btw. We must never typecast
printk arguments to u64 because u64 is long long on some architectures and
long on others.

> diff -puN drivers/md/dm.h~fix-dm-warnings drivers/md/dm.h
> --- linux-2.6.9-current/drivers/md/dm.h~fix-dm-warnings 2004-10-08 16:39:05.888125592 +0200
> +++ linux-2.6.9-current-paolo/drivers/md/dm.h 2004-10-08 16:39:05.942117384 +0200
> @@ -22,16 +22,6 @@
> #define DMEMIT(x...) sz += ((sz >= maxlen) ? \
> 0 : scnprintf(result + sz, maxlen - sz, x))
>
> -/*
> - * FIXME: I think this should be with the definition of sector_t
> - * in types.h.
> - */
> -#ifdef CONFIG_LBD
> -#define SECTOR_FORMAT "%Lu"
> -#else
> -#define SECTOR_FORMAT "%lu"
> -#endif
> -
> #define SECTOR_SHIFT 9
>
> /*
> diff -puN include/asm-h8300/types.h~fix-dm-warnings include/asm-h8300/types.h
> --- linux-2.6.9-current/include/asm-h8300/types.h~fix-dm-warnings 2004-10-08 16:39:05.902123464 +0200
> +++ linux-2.6.9-current-paolo/include/asm-h8300/types.h 2004-10-08 16:39:05.959114800 +0200
> @@ -56,6 +56,7 @@ typedef unsigned long long u64;
> typedef u32 dma_addr_t;
>
> #define HAVE_SECTOR_T
> +#define SECTOR_FORMAT "%Lu"
> typedef u64 sector_t;
>
> typedef unsigned int kmem_bufctl_t;
> diff -puN include/asm-i386/types.h~fix-dm-warnings include/asm-i386/types.h
> --- linux-2.6.9-current/include/asm-i386/types.h~fix-dm-warnings 2004-10-08 16:39:05.914121640 +0200
> +++ linux-2.6.9-current-paolo/include/asm-i386/types.h 2004-10-08 16:39:05.959114800 +0200
> @@ -60,6 +60,7 @@ typedef u64 dma64_addr_t;
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-mips/types.h~fix-dm-warnings include/asm-mips/types.h
> --- linux-2.6.9-current/include/asm-mips/types.h~fix-dm-warnings 2004-10-08 16:39:05.915121488 +0200
> +++ linux-2.6.9-current-paolo/include/asm-mips/types.h 2004-10-08 16:39:05.980111608 +0200
> @@ -96,6 +96,7 @@ typedef unsigned long phys_t;
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-ppc/types.h~fix-dm-warnings include/asm-ppc/types.h
> --- linux-2.6.9-current/include/asm-ppc/types.h~fix-dm-warnings 2004-10-08 16:39:05.916121336 +0200
> +++ linux-2.6.9-current-paolo/include/asm-ppc/types.h 2004-10-08 16:39:05.980111608 +0200
> @@ -59,6 +59,7 @@ typedef u64 dma64_addr_t;
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-s390/types.h~fix-dm-warnings include/asm-s390/types.h
> --- linux-2.6.9-current/include/asm-s390/types.h~fix-dm-warnings 2004-10-08 16:39:05.917121184 +0200
> +++ linux-2.6.9-current-paolo/include/asm-s390/types.h 2004-10-08 16:39:05.981111456 +0200
> @@ -92,6 +92,7 @@ typedef union {
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-sh/types.h~fix-dm-warnings include/asm-sh/types.h
> --- linux-2.6.9-current/include/asm-sh/types.h~fix-dm-warnings 2004-10-08 16:39:05.920120728 +0200
> +++ linux-2.6.9-current-paolo/include/asm-sh/types.h 2004-10-08 16:39:05.981111456 +0200
> @@ -55,6 +55,7 @@ typedef u32 dma_addr_t;
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-x86_64/types.h~fix-dm-warnings include/asm-x86_64/types.h
> --- linux-2.6.9-current/include/asm-x86_64/types.h~fix-dm-warnings 2004-10-08 16:39:05.921120576 +0200
> +++ linux-2.6.9-current-paolo/include/asm-x86_64/types.h 2004-10-08 16:39:05.981111456 +0200
> @@ -48,9 +48,6 @@ typedef unsigned long long u64;
> typedef u64 dma64_addr_t;
> typedef u64 dma_addr_t;
>
> -typedef u64 sector_t;
> -#define HAVE_SECTOR_T
> -
> typedef unsigned short kmem_bufctl_t;
>
> #endif /* __ASSEMBLY__ */
> diff -puN include/linux/types.h~fix-dm-warnings include/linux/types.h
> --- linux-2.6.9-current/include/linux/types.h~fix-dm-warnings 2004-10-08 16:39:05.939117840 +0200
> +++ linux-2.6.9-current-paolo/include/linux/types.h 2004-10-08 16:39:05.959114800 +0200
> @@ -130,6 +130,7 @@ typedef __s64 int64_t;
> */
> #ifndef HAVE_SECTOR_T
> typedef unsigned long sector_t;
> +#define SECTOR_FORMAT "%lu"
> #endif
>
> /*
> _
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Neil Horman: "Re: character device interface to existing socket interface."
Previous message: michael_soulier: "wait_event and preemption in 2.6"
In reply to: blaisorblade_spam: "[patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t"
Next in thread: Anton Altaparmakov: "Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu isright for sector_t"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]