* [PATCH] Fix pointer cast for 32 bits arch @ 2015-04-12 9:26 Peter Senna Tschudin 2015-04-12 13:05 ` Geert Uytterhoeven 2015-04-13 9:10 ` [PATCH] " Dan Carpenter 0 siblings, 2 replies; 30+ messages in thread From: Peter Senna Tschudin @ 2015-04-12 9:26 UTC (permalink / raw) To: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, kristina.martsenko, Nick Kralevich, devel, linux-kernel Cc: Peter Senna Tschudin Sparse compalins about casting void * to u64 on i386. Change the cast to resource_size_t. Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> --- Tested by compilation only. Tested for x86 and x86_64. drivers/staging/goldfish/goldfish_nand.c | 3 ++- include/linux/goldfish.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c index d68f216..738fdc4 100644 --- a/drivers/staging/goldfish/goldfish_nand.c +++ b/drivers/staging/goldfish/goldfish_nand.c @@ -87,7 +87,8 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); writel((u32)addr, base + NAND_ADDR_LOW); writel(len, base + NAND_TRANSFER_SIZE); - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); + gf_write64((resource_size_t)ptr, base + NAND_DATA, + base + NAND_DATA_HIGH); writel(cmd, base + NAND_COMMAND); rv = readl(base + NAND_RESULT); } diff --git a/include/linux/goldfish.h b/include/linux/goldfish.h index 569236e..494e943 100644 --- a/include/linux/goldfish.h +++ b/include/linux/goldfish.h @@ -3,7 +3,7 @@ /* Helpers for Goldfish virtual platform */ -static inline void gf_write64(unsigned long data, +static inline void gf_write64(resource_size_t data, void __iomem *portl, void __iomem *porth) { writel((u32)data, portl); -- 2.1.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-12 9:26 [PATCH] Fix pointer cast for 32 bits arch Peter Senna Tschudin @ 2015-04-12 13:05 ` Geert Uytterhoeven 2015-04-12 13:48 ` Peter Senna Tschudin 2015-04-13 9:10 ` [PATCH] " Dan Carpenter 1 sibling, 1 reply; 30+ messages in thread From: Geert Uytterhoeven @ 2015-04-12 13:05 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, kristina.martsenko, Nick Kralevich, driverdevel, linux-kernel On Sun, Apr 12, 2015 at 11:26 AM, Peter Senna Tschudin <peter.senna@gmail.com> wrote: > Sparse compalins about casting void * to u64 on i386. > Change the cast to resource_size_t. > > Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> > --- > > Tested by compilation only. Tested for x86 and x86_64. > > drivers/staging/goldfish/goldfish_nand.c | 3 ++- > include/linux/goldfish.h | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c > index d68f216..738fdc4 100644 > --- a/drivers/staging/goldfish/goldfish_nand.c > +++ b/drivers/staging/goldfish/goldfish_nand.c > @@ -87,7 +87,8 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, > writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); > writel((u32)addr, base + NAND_ADDR_LOW); > writel(len, base + NAND_TRANSFER_SIZE); > - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); > + gf_write64((resource_size_t)ptr, base + NAND_DATA, > + base + NAND_DATA_HIGH); I guess sparse still complains if CONFIG_X86_PAE=y, which makes resource_size_t u64? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-12 13:05 ` Geert Uytterhoeven @ 2015-04-12 13:48 ` Peter Senna Tschudin 2015-04-12 15:38 ` Geert Uytterhoeven 0 siblings, 1 reply; 30+ messages in thread From: Peter Senna Tschudin @ 2015-04-12 13:48 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, Kristina Martšenko, Nick Kralevich, driverdevel, linux-kernel On Sun, Apr 12, 2015 at 3:05 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Sun, Apr 12, 2015 at 11:26 AM, Peter Senna Tschudin > <peter.senna@gmail.com> wrote: >> Sparse compalins about casting void * to u64 on i386. >> Change the cast to resource_size_t. >> >> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> >> --- >> >> Tested by compilation only. Tested for x86 and x86_64. >> >> drivers/staging/goldfish/goldfish_nand.c | 3 ++- >> include/linux/goldfish.h | 2 +- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c >> index d68f216..738fdc4 100644 >> --- a/drivers/staging/goldfish/goldfish_nand.c >> +++ b/drivers/staging/goldfish/goldfish_nand.c >> @@ -87,7 +87,8 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, >> writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); >> writel((u32)addr, base + NAND_ADDR_LOW); >> writel(len, base + NAND_TRANSFER_SIZE); >> - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); >> + gf_write64((resource_size_t)ptr, base + NAND_DATA, >> + base + NAND_DATA_HIGH); > > I guess sparse still complains if CONFIG_X86_PAE=y, which makes > resource_size_t u64? Yes, when CONFIG_X86_PAE=y the patch doesn't fix the warning(gcc warning). What is the correct/portable way of fixing this? > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds -- Peter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-12 13:48 ` Peter Senna Tschudin @ 2015-04-12 15:38 ` Geert Uytterhoeven 2015-04-12 18:14 ` Joe Perches 2015-04-13 11:14 ` Peter Senna Tschudin 0 siblings, 2 replies; 30+ messages in thread From: Geert Uytterhoeven @ 2015-04-12 15:38 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, Kristina Martšenko, Nick Kralevich, driverdevel, linux-kernel On Sun, Apr 12, 2015 at 3:48 PM, Peter Senna Tschudin <peter.senna@gmail.com> wrote: > On Sun, Apr 12, 2015 at 3:05 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Sun, Apr 12, 2015 at 11:26 AM, Peter Senna Tschudin >> <peter.senna@gmail.com> wrote: >>> Sparse compalins about casting void * to u64 on i386. >>> Change the cast to resource_size_t. >>> >>> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> >>> --- >>> >>> Tested by compilation only. Tested for x86 and x86_64. >>> >>> drivers/staging/goldfish/goldfish_nand.c | 3 ++- >>> include/linux/goldfish.h | 2 +- >>> 2 files changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c >>> index d68f216..738fdc4 100644 >>> --- a/drivers/staging/goldfish/goldfish_nand.c >>> +++ b/drivers/staging/goldfish/goldfish_nand.c >>> @@ -87,7 +87,8 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, >>> writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); >>> writel((u32)addr, base + NAND_ADDR_LOW); >>> writel(len, base + NAND_TRANSFER_SIZE); >>> - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); >>> + gf_write64((resource_size_t)ptr, base + NAND_DATA, >>> + base + NAND_DATA_HIGH); >> >> I guess sparse still complains if CONFIG_X86_PAE=y, which makes >> resource_size_t u64? > > Yes, when CONFIG_X86_PAE=y the patch doesn't fix the warning(gcc > warning). What is the correct/portable way of fixing this? As gf_write64() takes "unsigned long data" for its first argument, why not simply cast ptr to "unsigned long"? "void *" and "unsigned long" have the same size on all Linux platforms. Seems like there a several silly casting tricks being done in the goldfish drivers: $ git grep -w gf_write64drivers/platform/goldfish/goldfish_pipe.c: gf_write64((u64)(unsigned long)p drivers/platform/goldfish/goldfish_pipe.c: gf_write64((u64)(unsigned long)p drivers/platform/goldfish/goldfish_pipe.c: gf_write64((u64) drivers/platform/goldfish/goldfish_pipe.c: gf_write64(addre drivers/staging/goldfish/goldfish_audio.c: (gf_write64((u64)(x), data->reg_ drivers/staging/goldfish/goldfish_nand.c: gf_write64((u64)ptr, bas drivers/tty/goldfish.c: gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, drivers/tty/goldfish.c: gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, include/linux/goldfish.h:static inline void gf_write64(unsigned long data, with interesting history showing one wrong cast being "fixed" using another wrong cast: commit f4e131dc38d34469b9fee1840ad237324831bce3 Author: Octavian Purdila <octavian.purdila@intel.com> Date: Fri May 16 08:24:59 2014 +0300 goldfish: pipe: fix warnings for 32bit builds - gf_write64((u64)pipe, dev->base + PIPE_REG_CHANNEL, + gf_write64((u64)(unsigned long)pipe, dev->base + PIPE_REG_CHANNEL, As in general casts don't belong in drivers[*], what about creating an inline helper gf_set_pipe() that converts the pointer to unsigned long and calls gf_write64()? Perhaps checkpatch should complain about casts outside header files? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-12 15:38 ` Geert Uytterhoeven @ 2015-04-12 18:14 ` Joe Perches 2015-04-12 19:06 ` Geert Uytterhoeven 2015-04-13 11:14 ` Peter Senna Tschudin 1 sibling, 1 reply; 30+ messages in thread From: Joe Perches @ 2015-04-12 18:14 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Peter Senna Tschudin, Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, Kristina Martšenko, Nick Kralevich, driverdevel, linux-kernel On Sun, 2015-04-12 at 17:38 +0200, Geert Uytterhoeven wrote: > Perhaps checkpatch should complain about casts outside header files? I think that's not feasible/desirable. type casting is pretty common and necessary. There are 50k+ casts in drivers/ alone. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-12 18:14 ` Joe Perches @ 2015-04-12 19:06 ` Geert Uytterhoeven 0 siblings, 0 replies; 30+ messages in thread From: Geert Uytterhoeven @ 2015-04-12 19:06 UTC (permalink / raw) To: Joe Perches Cc: Peter Senna Tschudin, Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, Kristina Martšenko, Nick Kralevich, driverdevel, linux-kernel On Sun, Apr 12, 2015 at 8:14 PM, Joe Perches <joe@perches.com> wrote: > On Sun, 2015-04-12 at 17:38 +0200, Geert Uytterhoeven wrote: >> Perhaps checkpatch should complain about casts outside header files? > > I think that's not feasible/desirable. > > type casting is pretty common and necessary. > There are 50k+ casts in drivers/ alone. What a pity.... How many of those are wrong? ;-) If there's one thing I like about C++, it's the new-style casts, and how easy they are to grep for, unlike their C counterparts. It would help if gcc offered -Wcast, so we could at least identify all newly introduced casts by comparing build logs, and audit them manually (like with other warnings). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-12 15:38 ` Geert Uytterhoeven 2015-04-12 18:14 ` Joe Perches @ 2015-04-13 11:14 ` Peter Senna Tschudin 2015-04-13 11:21 ` Geert Uytterhoeven 1 sibling, 1 reply; 30+ messages in thread From: Peter Senna Tschudin @ 2015-04-13 11:14 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, Kristina Martšenko, Nick Kralevich, driverdevel, linux-kernel On Sun, Apr 12, 2015 at 5:38 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Sun, Apr 12, 2015 at 3:48 PM, Peter Senna Tschudin > <peter.senna@gmail.com> wrote: >> On Sun, Apr 12, 2015 at 3:05 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Sun, Apr 12, 2015 at 11:26 AM, Peter Senna Tschudin >>> <peter.senna@gmail.com> wrote: >>>> Sparse compalins about casting void * to u64 on i386. >>>> Change the cast to resource_size_t. >>>> >>>> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> >>>> --- >>>> >>>> Tested by compilation only. Tested for x86 and x86_64. >>>> >>>> drivers/staging/goldfish/goldfish_nand.c | 3 ++- >>>> include/linux/goldfish.h | 2 +- >>>> 2 files changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c >>>> index d68f216..738fdc4 100644 >>>> --- a/drivers/staging/goldfish/goldfish_nand.c >>>> +++ b/drivers/staging/goldfish/goldfish_nand.c >>>> @@ -87,7 +87,8 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, >>>> writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); >>>> writel((u32)addr, base + NAND_ADDR_LOW); >>>> writel(len, base + NAND_TRANSFER_SIZE); >>>> - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); >>>> + gf_write64((resource_size_t)ptr, base + NAND_DATA, >>>> + base + NAND_DATA_HIGH); >>> >>> I guess sparse still complains if CONFIG_X86_PAE=y, which makes >>> resource_size_t u64? >> >> Yes, when CONFIG_X86_PAE=y the patch doesn't fix the warning(gcc >> warning). What is the correct/portable way of fixing this? > > As gf_write64() takes "unsigned long data" for its first argument, why not > simply cast ptr to "unsigned long"? "void *" and "unsigned long" have the same > size on all Linux platforms. > > Seems like there a several silly casting tricks being done in the goldfish > drivers: > > $ git grep -w gf_write64drivers/platform/goldfish/goldfish_pipe.c: > gf_write64((u64)(unsigned long)p > drivers/platform/goldfish/goldfish_pipe.c: gf_write64((u64)(unsigned long)p > drivers/platform/goldfish/goldfish_pipe.c: gf_write64((u64) > drivers/platform/goldfish/goldfish_pipe.c: gf_write64(addre > drivers/staging/goldfish/goldfish_audio.c: (gf_write64((u64)(x), data->reg_ > drivers/staging/goldfish/goldfish_nand.c: gf_write64((u64)ptr, bas > drivers/tty/goldfish.c: gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, > drivers/tty/goldfish.c: gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, > include/linux/goldfish.h:static inline void gf_write64(unsigned long data, > > with interesting history showing one wrong cast being "fixed" using another > wrong cast: > > commit f4e131dc38d34469b9fee1840ad237324831bce3 > Author: Octavian Purdila <octavian.purdila@intel.com> > Date: Fri May 16 08:24:59 2014 +0300 > > goldfish: pipe: fix warnings for 32bit builds > > - gf_write64((u64)pipe, dev->base + PIPE_REG_CHANNEL, > + gf_write64((u64)(unsigned long)pipe, dev->base + PIPE_REG_CHANNEL, > > As in general casts don't belong in drivers[*], what about creating an inline > helper gf_set_pipe() that converts the pointer to unsigned long and calls > gf_write64()? > > Perhaps checkpatch should complain about casts outside header files? Cocinelle found 664 double casts: http://pastebin.com/2bi9Dg7k and 9 triple casts: http://pastebin.com/RkJhPTTV Those are 'not' patches, just the output of Coccinelle for analysis. The .cocci for triple cast: @@ expression e1, e2; type t1, t2, t3; @@ e1 = - (t1)(t2)(t3) e2 So if there are cast patterns known to be wrong, it is easy to find and probably fix them with Coccinelle. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds -- Peter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-13 11:14 ` Peter Senna Tschudin @ 2015-04-13 11:21 ` Geert Uytterhoeven 2015-04-13 11:25 ` Peter Senna Tschudin 0 siblings, 1 reply; 30+ messages in thread From: Geert Uytterhoeven @ 2015-04-13 11:21 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, Kristina Martšenko, Nick Kralevich, driverdevel, linux-kernel Hi Peter, On Mon, Apr 13, 2015 at 1:14 PM, Peter Senna Tschudin <peter.senna@gmail.com> wrote: >> Perhaps checkpatch should complain about casts outside header files? > Cocinelle found 664 double casts: > http://pastebin.com/2bi9Dg7k > > and 9 triple casts: > http://pastebin.com/RkJhPTTV I think double casts are only needed when casting between integers and pointers of different sizes: 1. One cast to make the sizes match, 2. One cast to convert between pointer and integer. I think triple casts can always be simplified. Or am I missing some use cases? > Those are 'not' patches, just the output of Coccinelle for analysis. > The .cocci for triple cast: > @@ > expression e1, e2; > type t1, t2, t3; > @@ > e1 = > - (t1)(t2)(t3) > e2 > > So if there are cast patterns known to be wrong, it is easy to find > and probably fix them with Coccinelle. It depends on the original type and on the destination type. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-13 11:21 ` Geert Uytterhoeven @ 2015-04-13 11:25 ` Peter Senna Tschudin 2015-04-13 12:31 ` Dan Carpenter 0 siblings, 1 reply; 30+ messages in thread From: Peter Senna Tschudin @ 2015-04-13 11:25 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, Kristina Martšenko, Nick Kralevich, driverdevel, linux-kernel Line 4677 of http://pastebin.com/2bi9Dg7k looks like a bug diff -u -p a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c - regs->nip = (unsigned long) (unsigned long)ksig->ka.sa.sa_handler; On Mon, Apr 13, 2015 at 1:21 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Peter, > > On Mon, Apr 13, 2015 at 1:14 PM, Peter Senna Tschudin > <peter.senna@gmail.com> wrote: >>> Perhaps checkpatch should complain about casts outside header files? >> Cocinelle found 664 double casts: >> http://pastebin.com/2bi9Dg7k >> >> and 9 triple casts: >> http://pastebin.com/RkJhPTTV > > I think double casts are only needed when casting between integers > and pointers of different sizes: > 1. One cast to make the sizes match, > 2. One cast to convert between pointer and integer. > > I think triple casts can always be simplified. > Or am I missing some use cases? > >> Those are 'not' patches, just the output of Coccinelle for analysis. >> The .cocci for triple cast: >> @@ >> expression e1, e2; >> type t1, t2, t3; >> @@ >> e1 = >> - (t1)(t2)(t3) >> e2 >> >> So if there are cast patterns known to be wrong, it is easy to find >> and probably fix them with Coccinelle. > > It depends on the original type and on the destination type. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds -- Peter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-13 11:25 ` Peter Senna Tschudin @ 2015-04-13 12:31 ` Dan Carpenter 2015-04-16 13:39 ` [PATCH V4] " Peter Senna Tschudin 0 siblings, 1 reply; 30+ messages in thread From: Dan Carpenter @ 2015-04-13 12:31 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Geert Uytterhoeven, driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Garret Kelly, Octavian Purdila, Nick Kralevich, Alan On Mon, Apr 13, 2015 at 01:25:15PM +0200, Peter Senna Tschudin wrote: > Line 4677 of http://pastebin.com/2bi9Dg7k looks like a bug > > diff -u -p a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c > - regs->nip = (unsigned long) (unsigned long)ksig->ka.sa.sa_handler; I don't know about "bug" but it does look uninentional, yes. Double casting is probably a good thing to print a static checker warning about. Other static checkers complain about: 1) ~(char)foo; (char)foo is type promoted to an int before the bitwise NOT. 2) (long *)&some_int 3) some_function((struct foo *)ptr); Casting function arguments is almost always wrong. I check this with Smatch. I don't remember the details. 4) (long long)(x + y) Probably it should be (long long)x + y regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-13 12:31 ` Dan Carpenter @ 2015-04-16 13:39 ` Peter Senna Tschudin 2015-04-16 16:14 ` Geert Uytterhoeven 0 siblings, 1 reply; 30+ messages in thread From: Peter Senna Tschudin @ 2015-04-16 13:39 UTC (permalink / raw) To: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, kristina.martsenko, Nick Kralevich, Andreas, devel, linux-kernel, dan.carpenter Cc: Peter Senna Tschudin As the first argument of gf_write64() was of type unsigned long, and as some calls to gf_write64() were casting the first argument from void * to u64 the compiler and/or sparse were printing warnings for casts of wrong sizes when compiling for i386. This patch changes the type of the first argument of gf_write64() to void *, and update all calls to the function. This change fixed the warings and allowed to remove casts from 3 calls to gf_write64(). In addition gf_write64() was renamed to gf_write_ptr() as the name was misleading because it only writes 32 bits on 32 bit systems. Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> --- Tested by compilation only for x86, x86 with CONFIG_X86_PAE=y, and for x86_64. Changes from V3: - Changed type of first argument of gf_write64 - Renamed from gf_write64 to gf_write_ptr - Updated all calls to gf_write_ptr Changes from V2: - Fixed spelling of complains - Updated commit message Changes from V1: - Updated commit message drivers/platform/goldfish/goldfish_pipe.c | 18 +++++++++--------- drivers/staging/goldfish/goldfish_audio.c | 2 +- drivers/staging/goldfish/goldfish_nand.c | 2 +- drivers/tty/goldfish.c | 8 ++++---- include/linux/goldfish.h | 8 ++++---- 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c index d9a09d9..aad16bc 100644 --- a/drivers/platform/goldfish/goldfish_pipe.c +++ b/drivers/platform/goldfish/goldfish_pipe.c @@ -158,8 +158,8 @@ static u32 goldfish_cmd_status(struct goldfish_pipe *pipe, u32 cmd) struct goldfish_pipe_dev *dev = pipe->dev; spin_lock_irqsave(&dev->lock, flags); - gf_write64((u64)(unsigned long)pipe, dev->base + PIPE_REG_CHANNEL, - dev->base + PIPE_REG_CHANNEL_HIGH); + gf_write_ptr(pipe, dev->base + PIPE_REG_CHANNEL, + dev->base + PIPE_REG_CHANNEL_HIGH); writel(cmd, dev->base + PIPE_REG_COMMAND); status = readl(dev->base + PIPE_REG_STATUS); spin_unlock_irqrestore(&dev->lock, flags); @@ -172,8 +172,8 @@ static void goldfish_cmd(struct goldfish_pipe *pipe, u32 cmd) struct goldfish_pipe_dev *dev = pipe->dev; spin_lock_irqsave(&dev->lock, flags); - gf_write64((u64)(unsigned long)pipe, dev->base + PIPE_REG_CHANNEL, - dev->base + PIPE_REG_CHANNEL_HIGH); + gf_write_ptr(pipe, dev->base + PIPE_REG_CHANNEL, + dev->base + PIPE_REG_CHANNEL_HIGH); writel(cmd, dev->base + PIPE_REG_COMMAND); spin_unlock_irqrestore(&dev->lock, flags); } @@ -327,12 +327,12 @@ static ssize_t goldfish_pipe_read_write(struct file *filp, char __user *buffer, spin_lock_irqsave(&dev->lock, irq_flags); if (access_with_param(dev, CMD_WRITE_BUFFER + cmd_offset, address, avail, pipe, &status)) { - gf_write64((u64)(unsigned long)pipe, - dev->base + PIPE_REG_CHANNEL, - dev->base + PIPE_REG_CHANNEL_HIGH); + gf_write_ptr(pipe, dev->base + PIPE_REG_CHANNEL, + dev->base + PIPE_REG_CHANNEL_HIGH); writel(avail, dev->base + PIPE_REG_SIZE); - gf_write64(address, dev->base + PIPE_REG_ADDRESS, - dev->base + PIPE_REG_ADDRESS_HIGH); + gf_write_ptr((void *)address, + dev->base + PIPE_REG_ADDRESS, + dev->base + PIPE_REG_ADDRESS_HIGH); writel(CMD_WRITE_BUFFER + cmd_offset, dev->base + PIPE_REG_COMMAND); status = readl(dev->base + PIPE_REG_STATUS); diff --git a/drivers/staging/goldfish/goldfish_audio.c b/drivers/staging/goldfish/goldfish_audio.c index 702ae04..ffb3f44 100644 --- a/drivers/staging/goldfish/goldfish_audio.c +++ b/drivers/staging/goldfish/goldfish_audio.c @@ -63,7 +63,7 @@ struct goldfish_audio { #define AUDIO_READ(data, addr) (readl(data->reg_base + addr)) #define AUDIO_WRITE(data, addr, x) (writel(x, data->reg_base + addr)) #define AUDIO_WRITE64(data, addr, addr2, x) \ - (gf_write64((u64)(x), data->reg_base + addr, data->reg_base+addr2)) + (gf_write_ptr((void *)(x), data->reg_base + addr, data->reg_base+addr2)) /* * temporary variable used between goldfish_audio_probe() and diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c index 213877a..66ae48f 100644 --- a/drivers/staging/goldfish/goldfish_nand.c +++ b/drivers/staging/goldfish/goldfish_nand.c @@ -87,7 +87,7 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); writel((u32)addr, base + NAND_ADDR_LOW); writel(len, base + NAND_TRANSFER_SIZE); - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); + gf_write_ptr(ptr, base + NAND_DATA, base + NAND_DATA_HIGH); writel(cmd, base + NAND_COMMAND); rv = readl(base + NAND_RESULT); } diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c index 0655fec..8cbfa02 100644 --- a/drivers/tty/goldfish.c +++ b/drivers/tty/goldfish.c @@ -59,8 +59,8 @@ static void goldfish_tty_do_write(int line, const char *buf, unsigned count) struct goldfish_tty *qtty = &goldfish_ttys[line]; void __iomem *base = qtty->base; spin_lock_irqsave(&qtty->lock, irq_flags); - gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, - base + GOLDFISH_TTY_DATA_PTR_HIGH); + gf_write_ptr((void *)buf, base + GOLDFISH_TTY_DATA_PTR, + base + GOLDFISH_TTY_DATA_PTR_HIGH); writel(count, base + GOLDFISH_TTY_DATA_LEN); writel(GOLDFISH_TTY_CMD_WRITE_BUFFER, base + GOLDFISH_TTY_CMD); spin_unlock_irqrestore(&qtty->lock, irq_flags); @@ -81,8 +81,8 @@ static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id) count = tty_prepare_flip_string(&qtty->port, &buf, count); spin_lock_irqsave(&qtty->lock, irq_flags); - gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, - base + GOLDFISH_TTY_DATA_PTR_HIGH); + gf_write_ptr((void *)buf, base + GOLDFISH_TTY_DATA_PTR, + base + GOLDFISH_TTY_DATA_PTR_HIGH); writel(count, base + GOLDFISH_TTY_DATA_LEN); writel(GOLDFISH_TTY_CMD_READ_BUFFER, base + GOLDFISH_TTY_CMD); spin_unlock_irqrestore(&qtty->lock, irq_flags); diff --git a/include/linux/goldfish.h b/include/linux/goldfish.h index 569236e..8621042 100644 --- a/include/linux/goldfish.h +++ b/include/linux/goldfish.h @@ -3,12 +3,12 @@ /* Helpers for Goldfish virtual platform */ -static inline void gf_write64(unsigned long data, - void __iomem *portl, void __iomem *porth) +static inline void gf_write_ptr(void *data, void __iomem *portl, + void __iomem *porth) { - writel((u32)data, portl); + writel((u32)(unsigned long)data, portl); #ifdef CONFIG_64BIT - writel(data>>32, porth); + writel((unsigned long)data >> 32, porth); #endif } -- 2.1.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-16 13:39 ` [PATCH V4] " Peter Senna Tschudin @ 2015-04-16 16:14 ` Geert Uytterhoeven 2015-04-16 17:01 ` Dan Carpenter 0 siblings, 1 reply; 30+ messages in thread From: Geert Uytterhoeven @ 2015-04-16 16:14 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, Kristina Martšenko, Nick Kralevich, Andreas, driverdevel, linux-kernel, Dan Carpenter On Thu, Apr 16, 2015 at 3:39 PM, Peter Senna Tschudin <peter.senna@gmail.com> wrote: > --- a/drivers/staging/goldfish/goldfish_audio.c > +++ b/drivers/staging/goldfish/goldfish_audio.c > @@ -63,7 +63,7 @@ struct goldfish_audio { > #define AUDIO_READ(data, addr) (readl(data->reg_base + addr)) > #define AUDIO_WRITE(data, addr, x) (writel(x, data->reg_base + addr)) > #define AUDIO_WRITE64(data, addr, addr2, x) \ > - (gf_write64((u64)(x), data->reg_base + addr, data->reg_base+addr2)) > + (gf_write_ptr((void *)(x), data->reg_base + addr, data->reg_base+addr2)) This one should not be converted, as all callers pass a dma_addr_t, which may be 64-bit on 32-bit systems, i.e. larger than void *. > diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c > index 0655fec..8cbfa02 100644 > --- a/drivers/tty/goldfish.c > +++ b/drivers/tty/goldfish.c > @@ -59,8 +59,8 @@ static void goldfish_tty_do_write(int line, const char *buf, unsigned count) > struct goldfish_tty *qtty = &goldfish_ttys[line]; > void __iomem *base = qtty->base; > spin_lock_irqsave(&qtty->lock, irq_flags); > - gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, > - base + GOLDFISH_TTY_DATA_PTR_HIGH); > + gf_write_ptr((void *)buf, base + GOLDFISH_TTY_DATA_PTR, Please drop the unneeded "void *" cast, as it will hide warnings if a type is changed some future time. > + base + GOLDFISH_TTY_DATA_PTR_HIGH); > writel(count, base + GOLDFISH_TTY_DATA_LEN); > writel(GOLDFISH_TTY_CMD_WRITE_BUFFER, base + GOLDFISH_TTY_CMD); > spin_unlock_irqrestore(&qtty->lock, irq_flags); > @@ -81,8 +81,8 @@ static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id) > > count = tty_prepare_flip_string(&qtty->port, &buf, count); > spin_lock_irqsave(&qtty->lock, irq_flags); > - gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, > - base + GOLDFISH_TTY_DATA_PTR_HIGH); > + gf_write_ptr((void *)buf, base + GOLDFISH_TTY_DATA_PTR, Idem. > + base + GOLDFISH_TTY_DATA_PTR_HIGH); > writel(count, base + GOLDFISH_TTY_DATA_LEN); > writel(GOLDFISH_TTY_CMD_READ_BUFFER, base + GOLDFISH_TTY_CMD); > spin_unlock_irqrestore(&qtty->lock, irq_flags); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-16 16:14 ` Geert Uytterhoeven @ 2015-04-16 17:01 ` Dan Carpenter 2015-04-16 17:05 ` Alan Cox 0 siblings, 1 reply; 30+ messages in thread From: Dan Carpenter @ 2015-04-16 17:01 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Peter Senna Tschudin, driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Andreas, Garret Kelly, Octavian Purdila, Nick Kralevich, Alan On Thu, Apr 16, 2015 at 06:14:55PM +0200, Geert Uytterhoeven wrote: > On Thu, Apr 16, 2015 at 3:39 PM, Peter Senna Tschudin > <peter.senna@gmail.com> wrote: > > --- a/drivers/staging/goldfish/goldfish_audio.c > > +++ b/drivers/staging/goldfish/goldfish_audio.c > > @@ -63,7 +63,7 @@ struct goldfish_audio { > > #define AUDIO_READ(data, addr) (readl(data->reg_base + addr)) > > #define AUDIO_WRITE(data, addr, x) (writel(x, data->reg_base + addr)) > > #define AUDIO_WRITE64(data, addr, addr2, x) \ > > - (gf_write64((u64)(x), data->reg_base + addr, data->reg_base+addr2)) > > + (gf_write_ptr((void *)(x), data->reg_base + addr, data->reg_base+addr2)) > > This one should not be converted, as all callers pass a dma_addr_t, which may > be 64-bit on 32-bit systems, i.e. larger than void *. Ugh... You're right. I've been avoiding asking this but I can't any longer. What is gf_write64() actually doing? We are writing dma addresses, user space pointers and kernel space pointers to this hardware? This stuff doesn't seem to make any kind of sense and I can easily imagine a situation where it wrote a 64 bit pointer. Then we partially write over it with a 32 bit userspace pointer. Then it writes somewhere totally unintended. This thing doesn't make any sort of sense to me. regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-16 17:01 ` Dan Carpenter @ 2015-04-16 17:05 ` Alan Cox 2015-04-16 18:36 ` Peter Senna Tschudin 2015-04-17 8:11 ` [PATCH V4] Fix pointer cast for 32 bits arch Dan Carpenter 0 siblings, 2 replies; 30+ messages in thread From: Alan Cox @ 2015-04-16 17:05 UTC (permalink / raw) To: Dan Carpenter Cc: Geert Uytterhoeven, Peter Senna Tschudin, driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Andreas, Garret Kelly, Octavian Purdila, Nick Kralevich On Thu, 2015-04-16 at 20:01 +0300, Dan Carpenter wrote: > On Thu, Apr 16, 2015 at 06:14:55PM +0200, Geert Uytterhoeven wrote: > > On Thu, Apr 16, 2015 at 3:39 PM, Peter Senna Tschudin > > <peter.senna@gmail.com> wrote: > > > --- a/drivers/staging/goldfish/goldfish_audio.c > > > +++ b/drivers/staging/goldfish/goldfish_audio.c > > > @@ -63,7 +63,7 @@ struct goldfish_audio { > > > #define AUDIO_READ(data, addr) (readl(data->reg_base + addr)) > > > #define AUDIO_WRITE(data, addr, x) (writel(x, data->reg_base + addr)) > > > #define AUDIO_WRITE64(data, addr, addr2, x) \ > > > - (gf_write64((u64)(x), data->reg_base + addr, data->reg_base+addr2)) > > > + (gf_write_ptr((void *)(x), data->reg_base + addr, data->reg_base+addr2)) > > > > This one should not be converted, as all callers pass a dma_addr_t, which may > > be 64-bit on 32-bit systems, i.e. larger than void *. > > Ugh... You're right. > > I've been avoiding asking this but I can't any longer. What is > gf_write64() actually doing? We are writing dma addresses, user space > pointers and kernel space pointers to this hardware? > > This stuff doesn't seem to make any kind of sense and I can easily > imagine a situation where it wrote a 64 bit pointer. Then we partially > write over it with a 32 bit userspace pointer. Then it writes somewhere > totally unintended. > > This thing doesn't make any sort of sense to me. Its a 64 on 64 or 32 on 32 virtual machine. Goldfish is used for Android emulation for all the system level phone emulation tools. On the emulation side it provides an interface for the emulated OS but makes no effort to emulate it as if it was a real hardware. If you think of it as a funky emulator interface all is good. If you think about it as "hardware" you've got the wrong model and chunks of Goldfish make less sense. Alan ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-16 17:05 ` Alan Cox @ 2015-04-16 18:36 ` Peter Senna Tschudin 2015-05-03 18:32 ` Greg Kroah-Hartman 2015-04-17 8:11 ` [PATCH V4] Fix pointer cast for 32 bits arch Dan Carpenter 1 sibling, 1 reply; 30+ messages in thread From: Peter Senna Tschudin @ 2015-04-16 18:36 UTC (permalink / raw) To: Alan Cox Cc: Dan Carpenter, Geert Uytterhoeven, driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Andreas, Garret Kelly, Octavian Purdila, Nick Kralevich On Thu, Apr 16, 2015 at 7:05 PM, Alan Cox <alan@linux.intel.com> wrote: > On Thu, 2015-04-16 at 20:01 +0300, Dan Carpenter wrote: >> On Thu, Apr 16, 2015 at 06:14:55PM +0200, Geert Uytterhoeven wrote: >> > On Thu, Apr 16, 2015 at 3:39 PM, Peter Senna Tschudin >> > <peter.senna@gmail.com> wrote: >> > > --- a/drivers/staging/goldfish/goldfish_audio.c >> > > +++ b/drivers/staging/goldfish/goldfish_audio.c >> > > @@ -63,7 +63,7 @@ struct goldfish_audio { >> > > #define AUDIO_READ(data, addr) (readl(data->reg_base + addr)) >> > > #define AUDIO_WRITE(data, addr, x) (writel(x, data->reg_base + addr)) >> > > #define AUDIO_WRITE64(data, addr, addr2, x) \ >> > > - (gf_write64((u64)(x), data->reg_base + addr, data->reg_base+addr2)) >> > > + (gf_write_ptr((void *)(x), data->reg_base + addr, data->reg_base+addr2)) >> > >> > This one should not be converted, as all callers pass a dma_addr_t, which may >> > be 64-bit on 32-bit systems, i.e. larger than void *. >> >> Ugh... You're right. >> >> I've been avoiding asking this but I can't any longer. What is >> gf_write64() actually doing? We are writing dma addresses, user space >> pointers and kernel space pointers to this hardware? >> >> This stuff doesn't seem to make any kind of sense and I can easily >> imagine a situation where it wrote a 64 bit pointer. Then we partially >> write over it with a 32 bit userspace pointer. Then it writes somewhere >> totally unintended. >> >> This thing doesn't make any sort of sense to me. > > Its a 64 on 64 or 32 on 32 virtual machine. Goldfish is used for Android > emulation for all the system level phone emulation tools. On the > emulation side it provides an interface for the emulated OS but makes no > effort to emulate it as if it was a real hardware. If you think of it as > a funky emulator interface all is good. If you think about it as > "hardware" you've got the wrong model and chunks of Goldfish make less > sense. Is is better to leave the code as is, and ignore the compiler / sparse warnings for i386? Or is the proposal welcome if done correctly? And if so what would be correctly? -- Peter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-16 18:36 ` Peter Senna Tschudin @ 2015-05-03 18:32 ` Greg Kroah-Hartman 2015-05-04 13:11 ` [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits Peter Senna Tschudin 0 siblings, 1 reply; 30+ messages in thread From: Greg Kroah-Hartman @ 2015-05-03 18:32 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Alan Cox, Dan Carpenter, Geert Uytterhoeven, driverdevel, Jun Tian, linux-kernel, Andreas, Garret Kelly, Octavian Purdila, Nick Kralevich On Thu, Apr 16, 2015 at 08:36:51PM +0200, Peter Senna Tschudin wrote: > On Thu, Apr 16, 2015 at 7:05 PM, Alan Cox <alan@linux.intel.com> wrote: > > On Thu, 2015-04-16 at 20:01 +0300, Dan Carpenter wrote: > >> On Thu, Apr 16, 2015 at 06:14:55PM +0200, Geert Uytterhoeven wrote: > >> > On Thu, Apr 16, 2015 at 3:39 PM, Peter Senna Tschudin > >> > <peter.senna@gmail.com> wrote: > >> > > --- a/drivers/staging/goldfish/goldfish_audio.c > >> > > +++ b/drivers/staging/goldfish/goldfish_audio.c > >> > > @@ -63,7 +63,7 @@ struct goldfish_audio { > >> > > #define AUDIO_READ(data, addr) (readl(data->reg_base + addr)) > >> > > #define AUDIO_WRITE(data, addr, x) (writel(x, data->reg_base + addr)) > >> > > #define AUDIO_WRITE64(data, addr, addr2, x) \ > >> > > - (gf_write64((u64)(x), data->reg_base + addr, data->reg_base+addr2)) > >> > > + (gf_write_ptr((void *)(x), data->reg_base + addr, data->reg_base+addr2)) > >> > > >> > This one should not be converted, as all callers pass a dma_addr_t, which may > >> > be 64-bit on 32-bit systems, i.e. larger than void *. > >> > >> Ugh... You're right. > >> > >> I've been avoiding asking this but I can't any longer. What is > >> gf_write64() actually doing? We are writing dma addresses, user space > >> pointers and kernel space pointers to this hardware? > >> > >> This stuff doesn't seem to make any kind of sense and I can easily > >> imagine a situation where it wrote a 64 bit pointer. Then we partially > >> write over it with a 32 bit userspace pointer. Then it writes somewhere > >> totally unintended. > >> > >> This thing doesn't make any sort of sense to me. > > > > Its a 64 on 64 or 32 on 32 virtual machine. Goldfish is used for Android > > emulation for all the system level phone emulation tools. On the > > emulation side it provides an interface for the emulated OS but makes no > > effort to emulate it as if it was a real hardware. If you think of it as > > a funky emulator interface all is good. If you think about it as > > "hardware" you've got the wrong model and chunks of Goldfish make less > > sense. > > Is is better to leave the code as is, and ignore the compiler / sparse > warnings for i386? Or is the proposal welcome if done correctly? And > if so what would be correctly? What's the status with this mess? Oh, and please, put a "staging: goldfish:" at the front of your subject line in the future so I know to look at it, otherwise it gets lost at times... thanks, greg k-h ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits 2015-05-03 18:32 ` Greg Kroah-Hartman @ 2015-05-04 13:11 ` Peter Senna Tschudin 2015-05-04 13:34 ` Dan Carpenter 2015-05-09 16:24 ` Greg KH 0 siblings, 2 replies; 30+ messages in thread From: Peter Senna Tschudin @ 2015-05-04 13:11 UTC (permalink / raw) To: gregkh, alan, dan.carpenter, geert, devel, jun.j.tian, linux-kernel, schwab, garret.kelly, octavian.purdila, nnk Cc: Peter Senna Tschudin As the first argument of gf_write64() was of type unsigned long, and as some calls to gf_write64() were casting the first argument from void * to u64 the compiler and/or sparse were printing warnings for casts of wrong sizes when compiling for i386. This patch changes the type of the first argument of gf_write64() to void *, and update calls to the function. This change fixed the warnings and allowed to remove casts from 6 calls to gf_write64(). In addition gf_write64() was renamed to gf_write_ptr() as the name was misleading because it only writes 32 bits on 32 bit systems. gf_write_dma_addr() was added to handle dma_addr_t values which is used at drivers/staging/goldfish/goldfish_audio.c. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> --- Tested by compilation only for x86 and for x86_64. Changes from V4: - Added the function gf_write_dma_addr() - Changed the subject line Changes from V3: - Changed type of first argument of gf_write64 - Renamed from gf_write64 to gf_write_ptr - Updated all calls to gf_write_ptr Changes from V2: - Fixed spelling of complains - Updated commit message Changes from V1: - Updated commit message drivers/platform/goldfish/goldfish_pipe.c | 18 +++++++++--------- drivers/staging/goldfish/goldfish_audio.c | 2 +- drivers/staging/goldfish/goldfish_nand.c | 2 +- drivers/tty/goldfish.c | 4 ++-- include/linux/goldfish.h | 18 ++++++++++++++---- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c index d9a09d9..aad16bc 100644 --- a/drivers/platform/goldfish/goldfish_pipe.c +++ b/drivers/platform/goldfish/goldfish_pipe.c @@ -158,8 +158,8 @@ static u32 goldfish_cmd_status(struct goldfish_pipe *pipe, u32 cmd) struct goldfish_pipe_dev *dev = pipe->dev; spin_lock_irqsave(&dev->lock, flags); - gf_write64((u64)(unsigned long)pipe, dev->base + PIPE_REG_CHANNEL, - dev->base + PIPE_REG_CHANNEL_HIGH); + gf_write_ptr(pipe, dev->base + PIPE_REG_CHANNEL, + dev->base + PIPE_REG_CHANNEL_HIGH); writel(cmd, dev->base + PIPE_REG_COMMAND); status = readl(dev->base + PIPE_REG_STATUS); spin_unlock_irqrestore(&dev->lock, flags); @@ -172,8 +172,8 @@ static void goldfish_cmd(struct goldfish_pipe *pipe, u32 cmd) struct goldfish_pipe_dev *dev = pipe->dev; spin_lock_irqsave(&dev->lock, flags); - gf_write64((u64)(unsigned long)pipe, dev->base + PIPE_REG_CHANNEL, - dev->base + PIPE_REG_CHANNEL_HIGH); + gf_write_ptr(pipe, dev->base + PIPE_REG_CHANNEL, + dev->base + PIPE_REG_CHANNEL_HIGH); writel(cmd, dev->base + PIPE_REG_COMMAND); spin_unlock_irqrestore(&dev->lock, flags); } @@ -327,12 +327,12 @@ static ssize_t goldfish_pipe_read_write(struct file *filp, char __user *buffer, spin_lock_irqsave(&dev->lock, irq_flags); if (access_with_param(dev, CMD_WRITE_BUFFER + cmd_offset, address, avail, pipe, &status)) { - gf_write64((u64)(unsigned long)pipe, - dev->base + PIPE_REG_CHANNEL, - dev->base + PIPE_REG_CHANNEL_HIGH); + gf_write_ptr(pipe, dev->base + PIPE_REG_CHANNEL, + dev->base + PIPE_REG_CHANNEL_HIGH); writel(avail, dev->base + PIPE_REG_SIZE); - gf_write64(address, dev->base + PIPE_REG_ADDRESS, - dev->base + PIPE_REG_ADDRESS_HIGH); + gf_write_ptr((void *)address, + dev->base + PIPE_REG_ADDRESS, + dev->base + PIPE_REG_ADDRESS_HIGH); writel(CMD_WRITE_BUFFER + cmd_offset, dev->base + PIPE_REG_COMMAND); status = readl(dev->base + PIPE_REG_STATUS); diff --git a/drivers/staging/goldfish/goldfish_audio.c b/drivers/staging/goldfish/goldfish_audio.c index 702ae04..b0927e4 100644 --- a/drivers/staging/goldfish/goldfish_audio.c +++ b/drivers/staging/goldfish/goldfish_audio.c @@ -63,7 +63,7 @@ struct goldfish_audio { #define AUDIO_READ(data, addr) (readl(data->reg_base + addr)) #define AUDIO_WRITE(data, addr, x) (writel(x, data->reg_base + addr)) #define AUDIO_WRITE64(data, addr, addr2, x) \ - (gf_write64((u64)(x), data->reg_base + addr, data->reg_base+addr2)) + (gf_write_dma_addr((x), data->reg_base + addr, data->reg_base+addr2)) /* * temporary variable used between goldfish_audio_probe() and diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c index 213877a..66ae48f 100644 --- a/drivers/staging/goldfish/goldfish_nand.c +++ b/drivers/staging/goldfish/goldfish_nand.c @@ -87,7 +87,7 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); writel((u32)addr, base + NAND_ADDR_LOW); writel(len, base + NAND_TRANSFER_SIZE); - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); + gf_write_ptr(ptr, base + NAND_DATA, base + NAND_DATA_HIGH); writel(cmd, base + NAND_COMMAND); rv = readl(base + NAND_RESULT); } diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c index 0655fec..0f82c0b 100644 --- a/drivers/tty/goldfish.c +++ b/drivers/tty/goldfish.c @@ -59,7 +59,7 @@ static void goldfish_tty_do_write(int line, const char *buf, unsigned count) struct goldfish_tty *qtty = &goldfish_ttys[line]; void __iomem *base = qtty->base; spin_lock_irqsave(&qtty->lock, irq_flags); - gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, + gf_write_ptr(buf, base + GOLDFISH_TTY_DATA_PTR, base + GOLDFISH_TTY_DATA_PTR_HIGH); writel(count, base + GOLDFISH_TTY_DATA_LEN); writel(GOLDFISH_TTY_CMD_WRITE_BUFFER, base + GOLDFISH_TTY_CMD); @@ -81,7 +81,7 @@ static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id) count = tty_prepare_flip_string(&qtty->port, &buf, count); spin_lock_irqsave(&qtty->lock, irq_flags); - gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, + gf_write_ptr(buf, base + GOLDFISH_TTY_DATA_PTR, base + GOLDFISH_TTY_DATA_PTR_HIGH); writel(count, base + GOLDFISH_TTY_DATA_LEN); writel(GOLDFISH_TTY_CMD_READ_BUFFER, base + GOLDFISH_TTY_CMD); diff --git a/include/linux/goldfish.h b/include/linux/goldfish.h index 569236e..37a5e2f 100644 --- a/include/linux/goldfish.h +++ b/include/linux/goldfish.h @@ -3,13 +3,23 @@ /* Helpers for Goldfish virtual platform */ -static inline void gf_write64(unsigned long data, - void __iomem *portl, void __iomem *porth) +static inline void gf_write_ptr(void *ptr, void __iomem *portl, + void __iomem *porth) { - writel((u32)data, portl); + writel((u32)(unsigned long)ptr, portl); #ifdef CONFIG_64BIT - writel(data>>32, porth); + writel((unsigned long)ptr >> 32, porth); #endif } +static inline void gf_write_dma_addr(dma_addr_t addr, void __iomem *portl, + void __iomem *porth) +{ + writel((u32)addr, portl); +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT + writel(addr >> 32, porth); +#endif +} + + #endif /* __LINUX_GOLDFISH_H */ -- 2.1.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits 2015-05-04 13:11 ` [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits Peter Senna Tschudin @ 2015-05-04 13:34 ` Dan Carpenter 2015-05-09 16:24 ` Greg KH 1 sibling, 0 replies; 30+ messages in thread From: Dan Carpenter @ 2015-05-04 13:34 UTC (permalink / raw) To: Peter Senna Tschudin Cc: gregkh, alan, geert, devel, jun.j.tian, linux-kernel, schwab, garret.kelly, octavian.purdila, nnk Looks good. regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits 2015-05-04 13:11 ` [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits Peter Senna Tschudin 2015-05-04 13:34 ` Dan Carpenter @ 2015-05-09 16:24 ` Greg KH 2015-05-19 9:44 ` [PATCH V6] " Peter Senna Tschudin 1 sibling, 1 reply; 30+ messages in thread From: Greg KH @ 2015-05-09 16:24 UTC (permalink / raw) To: Peter Senna Tschudin Cc: alan, dan.carpenter, geert, devel, jun.j.tian, linux-kernel, schwab, garret.kelly, octavian.purdila, nnk On Mon, May 04, 2015 at 03:11:18PM +0200, Peter Senna Tschudin wrote: > As the first argument of gf_write64() was of type unsigned long, and as > some calls to gf_write64() were casting the first argument from void * > to u64 the compiler and/or sparse were printing warnings for casts of > wrong sizes when compiling for i386. > > This patch changes the type of the first argument of gf_write64() to > void *, and update calls to the function. This change fixed the > warnings and allowed to remove casts from 6 calls to gf_write64(). > > In addition gf_write64() was renamed to gf_write_ptr() as the name was > misleading because it only writes 32 bits on 32 bit systems. > > gf_write_dma_addr() was added to handle dma_addr_t values which is > used at drivers/staging/goldfish/goldfish_audio.c. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> > --- > Tested by compilation only for x86 and for x86_64. This adds two build warnings: drivers/tty/goldfish.c: In function ‘goldfish_tty_do_write’: drivers/tty/goldfish.c:62:15: warning: passing argument 1 of ‘gf_write_ptr’ discards ‘const’ qualifier from pointer target type gf_write_ptr(buf, base + GOLDFISH_TTY_DATA_PTR, ^ In file included from drivers/tty/goldfish.c:24:0: include/linux/goldfish.h:6:20: note: expected ‘void *’ but argument is of type ‘const char *’ static inline void gf_write_ptr(void *ptr, void __iomem *portl, ^ So I can't take this. Please fix up and resend, and always test build your patches... thanks, greg k-h ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH V6] staging: goldfish: Fix pointer cast for 32 bits 2015-05-09 16:24 ` Greg KH @ 2015-05-19 9:44 ` Peter Senna Tschudin 0 siblings, 0 replies; 30+ messages in thread From: Peter Senna Tschudin @ 2015-05-19 9:44 UTC (permalink / raw) To: gregkh, alan, dan.carpenter, geert, devel, jun.j.tian, linux-kernel, schwab, garret.kelly, octavian.purdila, nnk Cc: Peter Senna Tschudin As the first argument of gf_write64() was of type unsigned long, and as some calls to gf_write64() were casting the first argument from void * to u64 the compiler and/or sparse were printing warnings for casts of wrong sizes when compiling for i386. This patch changes the type of the first argument of gf_write64() to const void *, and update calls to the function. This change fixed the warnings and allowed to remove casts from 3 calls to gf_write64(). In addition gf_write64() was renamed to gf_write_ptr() as the name was misleading because it only writes 32 bits on 32 bit systems. gf_write_dma_addr() was added to handle dma_addr_t values which is used at drivers/staging/goldfish/goldfish_audio.c. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> --- When submiting V5, I havent tested the patch properly. It was my fault. Tested by compilation only for i386 and for x86_64. Changes from v5: - Added const to first argument of both gf_write_ptr() and gf_write_dma_addr() Changes from V4: - Added the function gf_write_dma_addr() - Changed the subject line Changes from V3: - Changed type of first argument of gf_write64 - Renamed from gf_write64 to gf_write_ptr - Updated all calls to gf_write_ptr Changes from V2: - Fixed spelling of complains - Updated commit message Changes from V1: - Updated commit message drivers/platform/goldfish/goldfish_pipe.c | 18 +++++++++--------- drivers/staging/goldfish/goldfish_audio.c | 2 +- drivers/staging/goldfish/goldfish_nand.c | 2 +- drivers/tty/goldfish.c | 4 ++-- include/linux/goldfish.h | 19 +++++++++++++++---- 5 files changed, 28 insertions(+), 17 deletions(-) diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c index d9a09d9..aad16bc 100644 --- a/drivers/platform/goldfish/goldfish_pipe.c +++ b/drivers/platform/goldfish/goldfish_pipe.c @@ -158,8 +158,8 @@ static u32 goldfish_cmd_status(struct goldfish_pipe *pipe, u32 cmd) struct goldfish_pipe_dev *dev = pipe->dev; spin_lock_irqsave(&dev->lock, flags); - gf_write64((u64)(unsigned long)pipe, dev->base + PIPE_REG_CHANNEL, - dev->base + PIPE_REG_CHANNEL_HIGH); + gf_write_ptr(pipe, dev->base + PIPE_REG_CHANNEL, + dev->base + PIPE_REG_CHANNEL_HIGH); writel(cmd, dev->base + PIPE_REG_COMMAND); status = readl(dev->base + PIPE_REG_STATUS); spin_unlock_irqrestore(&dev->lock, flags); @@ -172,8 +172,8 @@ static void goldfish_cmd(struct goldfish_pipe *pipe, u32 cmd) struct goldfish_pipe_dev *dev = pipe->dev; spin_lock_irqsave(&dev->lock, flags); - gf_write64((u64)(unsigned long)pipe, dev->base + PIPE_REG_CHANNEL, - dev->base + PIPE_REG_CHANNEL_HIGH); + gf_write_ptr(pipe, dev->base + PIPE_REG_CHANNEL, + dev->base + PIPE_REG_CHANNEL_HIGH); writel(cmd, dev->base + PIPE_REG_COMMAND); spin_unlock_irqrestore(&dev->lock, flags); } @@ -327,12 +327,12 @@ static ssize_t goldfish_pipe_read_write(struct file *filp, char __user *buffer, spin_lock_irqsave(&dev->lock, irq_flags); if (access_with_param(dev, CMD_WRITE_BUFFER + cmd_offset, address, avail, pipe, &status)) { - gf_write64((u64)(unsigned long)pipe, - dev->base + PIPE_REG_CHANNEL, - dev->base + PIPE_REG_CHANNEL_HIGH); + gf_write_ptr(pipe, dev->base + PIPE_REG_CHANNEL, + dev->base + PIPE_REG_CHANNEL_HIGH); writel(avail, dev->base + PIPE_REG_SIZE); - gf_write64(address, dev->base + PIPE_REG_ADDRESS, - dev->base + PIPE_REG_ADDRESS_HIGH); + gf_write_ptr((void *)address, + dev->base + PIPE_REG_ADDRESS, + dev->base + PIPE_REG_ADDRESS_HIGH); writel(CMD_WRITE_BUFFER + cmd_offset, dev->base + PIPE_REG_COMMAND); status = readl(dev->base + PIPE_REG_STATUS); diff --git a/drivers/staging/goldfish/goldfish_audio.c b/drivers/staging/goldfish/goldfish_audio.c index 702ae04..b0927e4 100644 --- a/drivers/staging/goldfish/goldfish_audio.c +++ b/drivers/staging/goldfish/goldfish_audio.c @@ -63,7 +63,7 @@ struct goldfish_audio { #define AUDIO_READ(data, addr) (readl(data->reg_base + addr)) #define AUDIO_WRITE(data, addr, x) (writel(x, data->reg_base + addr)) #define AUDIO_WRITE64(data, addr, addr2, x) \ - (gf_write64((u64)(x), data->reg_base + addr, data->reg_base+addr2)) + (gf_write_dma_addr((x), data->reg_base + addr, data->reg_base+addr2)) /* * temporary variable used between goldfish_audio_probe() and diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c index 213877a..66ae48f 100644 --- a/drivers/staging/goldfish/goldfish_nand.c +++ b/drivers/staging/goldfish/goldfish_nand.c @@ -87,7 +87,7 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); writel((u32)addr, base + NAND_ADDR_LOW); writel(len, base + NAND_TRANSFER_SIZE); - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); + gf_write_ptr(ptr, base + NAND_DATA, base + NAND_DATA_HIGH); writel(cmd, base + NAND_COMMAND); rv = readl(base + NAND_RESULT); } diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c index 0655fec..0f82c0b 100644 --- a/drivers/tty/goldfish.c +++ b/drivers/tty/goldfish.c @@ -59,7 +59,7 @@ static void goldfish_tty_do_write(int line, const char *buf, unsigned count) struct goldfish_tty *qtty = &goldfish_ttys[line]; void __iomem *base = qtty->base; spin_lock_irqsave(&qtty->lock, irq_flags); - gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, + gf_write_ptr(buf, base + GOLDFISH_TTY_DATA_PTR, base + GOLDFISH_TTY_DATA_PTR_HIGH); writel(count, base + GOLDFISH_TTY_DATA_LEN); writel(GOLDFISH_TTY_CMD_WRITE_BUFFER, base + GOLDFISH_TTY_CMD); @@ -81,7 +81,7 @@ static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id) count = tty_prepare_flip_string(&qtty->port, &buf, count); spin_lock_irqsave(&qtty->lock, irq_flags); - gf_write64((u64)buf, base + GOLDFISH_TTY_DATA_PTR, + gf_write_ptr(buf, base + GOLDFISH_TTY_DATA_PTR, base + GOLDFISH_TTY_DATA_PTR_HIGH); writel(count, base + GOLDFISH_TTY_DATA_LEN); writel(GOLDFISH_TTY_CMD_READ_BUFFER, base + GOLDFISH_TTY_CMD); diff --git a/include/linux/goldfish.h b/include/linux/goldfish.h index 569236e..93e080b 100644 --- a/include/linux/goldfish.h +++ b/include/linux/goldfish.h @@ -3,13 +3,24 @@ /* Helpers for Goldfish virtual platform */ -static inline void gf_write64(unsigned long data, - void __iomem *portl, void __iomem *porth) +static inline void gf_write_ptr(const void *ptr, void __iomem *portl, + void __iomem *porth) { - writel((u32)data, portl); + writel((u32)(unsigned long)ptr, portl); #ifdef CONFIG_64BIT - writel(data>>32, porth); + writel((unsigned long)ptr >> 32, porth); #endif } +static inline void gf_write_dma_addr(const dma_addr_t addr, + void __iomem *portl, + void __iomem *porth) +{ + writel((u32)addr, portl); +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT + writel(addr >> 32, porth); +#endif +} + + #endif /* __LINUX_GOLDFISH_H */ -- 2.1.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-16 17:05 ` Alan Cox 2015-04-16 18:36 ` Peter Senna Tschudin @ 2015-04-17 8:11 ` Dan Carpenter 2015-04-17 8:20 ` Dan Carpenter 1 sibling, 1 reply; 30+ messages in thread From: Dan Carpenter @ 2015-04-17 8:11 UTC (permalink / raw) To: Alan Cox Cc: Geert Uytterhoeven, Peter Senna Tschudin, driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Andreas, Garret Kelly, Octavian Purdila, Nick Kralevich On Thu, Apr 16, 2015 at 06:05:27PM +0100, Alan Cox wrote: > Its a 64 on 64 or 32 on 32 virtual machine. Goldfish is used for Android > emulation for all the system level phone emulation tools. On the > emulation side it provides an interface for the emulated OS but makes no > effort to emulate it as if it was a real hardware. If you think of it as > a funky emulator interface all is good. If you think about it as > "hardware" you've got the wrong model and chunks of Goldfish make less > sense. > Ah. Ok. That makes sense. Peter maybe the fix is to make a different function: static inline void gf_write_dma_addr(dma_addr_t addr, void __iomem *portl, void __iomem *porth) { writel((u32)data, portl); #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT writel(data >> 32, porth); #endif } Something like that. The gf_write_ptr() function patch you wrote preserves the current behavior, but the current behavior is buggy, (I think) so we need both gf_write_dma_addr() and gf_write_ptr(). regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-17 8:11 ` [PATCH V4] Fix pointer cast for 32 bits arch Dan Carpenter @ 2015-04-17 8:20 ` Dan Carpenter 2015-04-17 13:31 ` Alan Cox 0 siblings, 1 reply; 30+ messages in thread From: Dan Carpenter @ 2015-04-17 8:20 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Alan Cox, driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Geert Uytterhoeven, Garret Kelly, Octavian Purdila, Andreas, Nick Kralevich Actually, my patch seems like a good idea to me but it's one of those things that someone should probably test. Unless someone can test goldfish on a 32 bit system with 64 bit dma addresses then maybe the thing to is to write the new function but preserve the current behavior. static inline void gf_write_dma_addr(dma_addr_t addr, void __iomem *portl, void __iomem *porth) { writel((u32)data, portl); /* * This should be CONFIG_ARCH_DMA_ADDR_T_64BIT but someone needs to * test it. */ #ifdef CONFIG_64BIT writel(data >> 32, porth); #endif } regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-17 8:20 ` Dan Carpenter @ 2015-04-17 13:31 ` Alan Cox 2015-04-17 13:59 ` Dan Carpenter 0 siblings, 1 reply; 30+ messages in thread From: Alan Cox @ 2015-04-17 13:31 UTC (permalink / raw) To: Dan Carpenter Cc: Peter Senna Tschudin, driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Geert Uytterhoeven, Garret Kelly, Octavian Purdila, Andreas, Nick Kralevich On Fri, 2015-04-17 at 11:20 +0300, Dan Carpenter wrote: > Actually, my patch seems like a good idea to me but it's one of those > things that someone should probably test. Unless someone can test > goldfish on a 32 bit system with 64 bit dma addresses No such "system" exists. Alan ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-17 13:31 ` Alan Cox @ 2015-04-17 13:59 ` Dan Carpenter 2015-04-17 14:10 ` Alan Cox 0 siblings, 1 reply; 30+ messages in thread From: Dan Carpenter @ 2015-04-17 13:59 UTC (permalink / raw) To: Alan Cox Cc: driverdevel, Greg Kroah-Hartman, Jun Tian, Peter Senna Tschudin, linux-kernel, Geert Uytterhoeven, Garret Kelly, Octavian Purdila, Andreas, Nick Kralevich On Fri, Apr 17, 2015 at 02:31:49PM +0100, Alan Cox wrote: > On Fri, 2015-04-17 at 11:20 +0300, Dan Carpenter wrote: > > Actually, my patch seems like a good idea to me but it's one of those > > things that someone should probably test. Unless someone can test > > goldfish on a 32 bit system with 64 bit dma addresses > > No such "system" exists. I don't understand. We definitely can have 64bit dma addresses on x86_32. regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-17 13:59 ` Dan Carpenter @ 2015-04-17 14:10 ` Alan Cox 2015-04-18 13:34 ` Peter Senna Tschudin 0 siblings, 1 reply; 30+ messages in thread From: Alan Cox @ 2015-04-17 14:10 UTC (permalink / raw) To: Dan Carpenter Cc: driverdevel, Greg Kroah-Hartman, Jun Tian, Peter Senna Tschudin, linux-kernel, Geert Uytterhoeven, Garret Kelly, Octavian Purdila, Andreas, Nick Kralevich On Fri, 2015-04-17 at 16:59 +0300, Dan Carpenter wrote: > On Fri, Apr 17, 2015 at 02:31:49PM +0100, Alan Cox wrote: > > On Fri, 2015-04-17 at 11:20 +0300, Dan Carpenter wrote: > > > Actually, my patch seems like a good idea to me but it's one of those > > > things that someone should probably test. Unless someone can test > > > goldfish on a 32 bit system with 64 bit dma addresses > > > > No such "system" exists. > > I don't understand. We definitely can have 64bit dma addresses on > x86_32. Yes but no actual Goldfish environment is built that way ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-17 14:10 ` Alan Cox @ 2015-04-18 13:34 ` Peter Senna Tschudin 2015-04-18 13:59 ` Dan Carpenter 0 siblings, 1 reply; 30+ messages in thread From: Peter Senna Tschudin @ 2015-04-18 13:34 UTC (permalink / raw) To: Alan Cox Cc: Dan Carpenter, driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Geert Uytterhoeven, Garret Kelly, Octavian Purdila, Andreas, Nick Kralevich On Fri, Apr 17, 2015 at 4:10 PM, Alan Cox <alan@linux.intel.com> wrote: > On Fri, 2015-04-17 at 16:59 +0300, Dan Carpenter wrote: >> On Fri, Apr 17, 2015 at 02:31:49PM +0100, Alan Cox wrote: >> > On Fri, 2015-04-17 at 11:20 +0300, Dan Carpenter wrote: >> > > Actually, my patch seems like a good idea to me but it's one of those >> > > things that someone should probably test. Unless someone can test >> > > goldfish on a 32 bit system with 64 bit dma addresses >> > >> > No such "system" exists. >> >> I don't understand. We definitely can have 64bit dma addresses on >> x86_32. > > > Yes but no actual Goldfish environment is built that way Isn't this a simpler fix? diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c index 213877a..053ac11 100644 --- a/drivers/staging/goldfish/goldfish_nand.c +++ b/drivers/staging/goldfish/goldfish_nand.c @@ -87,7 +87,7 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); writel((u32)addr, base + NAND_ADDR_LOW); writel(len, base + NAND_TRANSFER_SIZE); - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); + gf_write64((unsigned long)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); writel(cmd, base + NAND_COMMAND); rv = readl(base + NAND_RESULT); } -- Peter ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH V4] Fix pointer cast for 32 bits arch 2015-04-18 13:34 ` Peter Senna Tschudin @ 2015-04-18 13:59 ` Dan Carpenter 0 siblings, 0 replies; 30+ messages in thread From: Dan Carpenter @ 2015-04-18 13:59 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Alan Cox, driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Geert Uytterhoeven, Garret Kelly, Octavian Purdila, Andreas, Nick Kralevich On Sat, Apr 18, 2015 at 03:34:08PM +0200, Peter Senna Tschudin wrote: > > > On Fri, Apr 17, 2015 at 4:10 PM, Alan Cox <alan@linux.intel.com> wrote: > > On Fri, 2015-04-17 at 16:59 +0300, Dan Carpenter wrote: > >> On Fri, Apr 17, 2015 at 02:31:49PM +0100, Alan Cox wrote: > >> > On Fri, 2015-04-17 at 11:20 +0300, Dan Carpenter wrote: > >> > > Actually, my patch seems like a good idea to me but it's one of those > >> > > things that someone should probably test. Unless someone can test > >> > > goldfish on a 32 bit system with 64 bit dma addresses > >> > > >> > No such "system" exists. > >> > >> I don't understand. We definitely can have 64bit dma addresses on > >> x86_32. > > > > > > Yes but no actual Goldfish environment is built that way > Isn't this a simpler fix? I still think my fix is cleanest even though dma_addr_t and size_t are always the same. It just means that we can commit it without worrying about testing. regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-12 9:26 [PATCH] Fix pointer cast for 32 bits arch Peter Senna Tschudin 2015-04-12 13:05 ` Geert Uytterhoeven @ 2015-04-13 9:10 ` Dan Carpenter 2015-04-13 9:16 ` Peter Senna Tschudin 1 sibling, 1 reply; 30+ messages in thread From: Dan Carpenter @ 2015-04-13 9:10 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, kristina.martsenko, Nick Kralevich, devel, linux-kernel On Sun, Apr 12, 2015 at 11:26:43AM +0200, Peter Senna Tschudin wrote: > Sparse compalins about casting void * to u64 on i386. > Change the cast to resource_size_t. > > Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> > --- > > Tested by compilation only. Tested for x86 and x86_64. > > drivers/staging/goldfish/goldfish_nand.c | 3 ++- > include/linux/goldfish.h | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c > index d68f216..738fdc4 100644 > --- a/drivers/staging/goldfish/goldfish_nand.c > +++ b/drivers/staging/goldfish/goldfish_nand.c > @@ -87,7 +87,8 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, > writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); > writel((u32)addr, base + NAND_ADDR_LOW); > writel(len, base + NAND_TRANSFER_SIZE); > - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); > + gf_write64((resource_size_t)ptr, base + NAND_DATA, > + base + NAND_DATA_HIGH); write64 is a misleading name because it only writes 32 bits on 32 bit systems. Are you sure it's a resource_size_t? If that's really true then this code needs a lot more work. The ifdef in gf_write64() should be CONFIG_PHYS_ADDR_T_64BIT and ops->datbuf needs to updated. It would be a lot of changes... I *think* but I'm not positive that gf_write64 should just take a void pointer like this: diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c index 213877a..92d6479 100644 --- a/drivers/staging/goldfish/goldfish_nand.c +++ b/drivers/staging/goldfish/goldfish_nand.c @@ -87,7 +87,7 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); writel((u32)addr, base + NAND_ADDR_LOW); writel(len, base + NAND_TRANSFER_SIZE); - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); + gf_write64(ptr, base + NAND_DATA, base + NAND_DATA_HIGH); writel(cmd, base + NAND_COMMAND); rv = readl(base + NAND_RESULT); } diff --git a/include/linux/goldfish.h b/include/linux/goldfish.h index 569236e..4cdb913 100644 --- a/include/linux/goldfish.h +++ b/include/linux/goldfish.h @@ -3,12 +3,12 @@ /* Helpers for Goldfish virtual platform */ -static inline void gf_write64(unsigned long data, +static inline void gf_write64(void *data, void __iomem *portl, void __iomem *porth) { - writel((u32)data, portl); + writel((u32)(unsigned long)data, portl); #ifdef CONFIG_64BIT - writel(data>>32, porth); + writel((unsigned long)data >> 32, porth); #endif } ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-13 9:10 ` [PATCH] " Dan Carpenter @ 2015-04-13 9:16 ` Peter Senna Tschudin 2015-04-13 9:21 ` Dan Carpenter 0 siblings, 1 reply; 30+ messages in thread From: Peter Senna Tschudin @ 2015-04-13 9:16 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, Loic Pefferkorn, Alan, Jun Tian, Octavian Purdila, Garret Kelly, Kristina Martšenko, Nick Kralevich, driverdevel, linux-kernel On Mon, Apr 13, 2015 at 11:10 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Sun, Apr 12, 2015 at 11:26:43AM +0200, Peter Senna Tschudin wrote: >> Sparse compalins about casting void * to u64 on i386. >> Change the cast to resource_size_t. >> >> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> >> --- >> >> Tested by compilation only. Tested for x86 and x86_64. >> >> drivers/staging/goldfish/goldfish_nand.c | 3 ++- >> include/linux/goldfish.h | 2 +- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c >> index d68f216..738fdc4 100644 >> --- a/drivers/staging/goldfish/goldfish_nand.c >> +++ b/drivers/staging/goldfish/goldfish_nand.c >> @@ -87,7 +87,8 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, >> writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); >> writel((u32)addr, base + NAND_ADDR_LOW); >> writel(len, base + NAND_TRANSFER_SIZE); >> - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); >> + gf_write64((resource_size_t)ptr, base + NAND_DATA, >> + base + NAND_DATA_HIGH); > > write64 is a misleading name because it only writes 32 bits on 32 bit > systems. > > Are you sure it's a resource_size_t? If that's really true then this > code needs a lot more work. The ifdef in gf_write64() should be > CONFIG_PHYS_ADDR_T_64BIT and ops->datbuf needs to updated. It would be > a lot of changes... > > I *think* but I'm not positive that gf_write64 should just take a void > pointer like this: > > diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c > index 213877a..92d6479 100644 > --- a/drivers/staging/goldfish/goldfish_nand.c > +++ b/drivers/staging/goldfish/goldfish_nand.c > @@ -87,7 +87,7 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, > writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); > writel((u32)addr, base + NAND_ADDR_LOW); > writel(len, base + NAND_TRANSFER_SIZE); > - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); > + gf_write64(ptr, base + NAND_DATA, base + NAND_DATA_HIGH); > writel(cmd, base + NAND_COMMAND); > rv = readl(base + NAND_RESULT); > } > diff --git a/include/linux/goldfish.h b/include/linux/goldfish.h > index 569236e..4cdb913 100644 > --- a/include/linux/goldfish.h > +++ b/include/linux/goldfish.h > @@ -3,12 +3,12 @@ > > /* Helpers for Goldfish virtual platform */ > > -static inline void gf_write64(unsigned long data, > +static inline void gf_write64(void *data, > void __iomem *portl, void __iomem *porth) > { > - writel((u32)data, portl); > + writel((u32)(unsigned long)data, portl); > #ifdef CONFIG_64BIT > - writel(data>>32, porth); > + writel((unsigned long)data >> 32, porth); > #endif > } > I like this solution. Should I send a single patch changing gf_write64() and removing all casts when calling the gf_write64(), or if to split, how to split the patch? -- Peter ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] Fix pointer cast for 32 bits arch 2015-04-13 9:16 ` Peter Senna Tschudin @ 2015-04-13 9:21 ` Dan Carpenter 0 siblings, 0 replies; 30+ messages in thread From: Dan Carpenter @ 2015-04-13 9:21 UTC (permalink / raw) To: Peter Senna Tschudin Cc: driverdevel, Greg Kroah-Hartman, Jun Tian, linux-kernel, Garret Kelly, Octavian Purdila, Nick Kralevich, Alan On Mon, Apr 13, 2015 at 11:16:49AM +0200, Peter Senna Tschudin wrote: > On Mon, Apr 13, 2015 at 11:10 AM, Dan Carpenter > <dan.carpenter@oracle.com> wrote: > > On Sun, Apr 12, 2015 at 11:26:43AM +0200, Peter Senna Tschudin wrote: > >> Sparse compalins about casting void * to u64 on i386. > >> Change the cast to resource_size_t. > >> > >> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com> > >> --- > >> > >> Tested by compilation only. Tested for x86 and x86_64. > >> > >> drivers/staging/goldfish/goldfish_nand.c | 3 ++- > >> include/linux/goldfish.h | 2 +- > >> 2 files changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c > >> index d68f216..738fdc4 100644 > >> --- a/drivers/staging/goldfish/goldfish_nand.c > >> +++ b/drivers/staging/goldfish/goldfish_nand.c > >> @@ -87,7 +87,8 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, > >> writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); > >> writel((u32)addr, base + NAND_ADDR_LOW); > >> writel(len, base + NAND_TRANSFER_SIZE); > >> - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); > >> + gf_write64((resource_size_t)ptr, base + NAND_DATA, > >> + base + NAND_DATA_HIGH); > > > > write64 is a misleading name because it only writes 32 bits on 32 bit > > systems. > > > > Are you sure it's a resource_size_t? If that's really true then this > > code needs a lot more work. The ifdef in gf_write64() should be > > CONFIG_PHYS_ADDR_T_64BIT and ops->datbuf needs to updated. It would be > > a lot of changes... > > > > I *think* but I'm not positive that gf_write64 should just take a void > > pointer like this: > > > > diff --git a/drivers/staging/goldfish/goldfish_nand.c b/drivers/staging/goldfish/goldfish_nand.c > > index 213877a..92d6479 100644 > > --- a/drivers/staging/goldfish/goldfish_nand.c > > +++ b/drivers/staging/goldfish/goldfish_nand.c > > @@ -87,7 +87,7 @@ static u32 goldfish_nand_cmd(struct mtd_info *mtd, enum nand_cmd cmd, > > writel((u32)(addr >> 32), base + NAND_ADDR_HIGH); > > writel((u32)addr, base + NAND_ADDR_LOW); > > writel(len, base + NAND_TRANSFER_SIZE); > > - gf_write64((u64)ptr, base + NAND_DATA, base + NAND_DATA_HIGH); > > + gf_write64(ptr, base + NAND_DATA, base + NAND_DATA_HIGH); > > writel(cmd, base + NAND_COMMAND); > > rv = readl(base + NAND_RESULT); > > } > > diff --git a/include/linux/goldfish.h b/include/linux/goldfish.h > > index 569236e..4cdb913 100644 > > --- a/include/linux/goldfish.h > > +++ b/include/linux/goldfish.h > > @@ -3,12 +3,12 @@ > > > > /* Helpers for Goldfish virtual platform */ > > > > -static inline void gf_write64(unsigned long data, > > +static inline void gf_write64(void *data, > > void __iomem *portl, void __iomem *porth) > > { > > - writel((u32)data, portl); > > + writel((u32)(unsigned long)data, portl); > > #ifdef CONFIG_64BIT > > - writel(data>>32, porth); > > + writel((unsigned long)data >> 32, porth); > > #endif > > } > > > > I like this solution. Should I send a single patch changing > gf_write64() and removing all casts when calling the gf_write64(), or > if to split, how to split the patch? Single patch, please. Maybe rename the function to gf_write_ptr() or something similar. regards, dan carpenter ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2015-05-19 9:45 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-12 9:26 [PATCH] Fix pointer cast for 32 bits arch Peter Senna Tschudin 2015-04-12 13:05 ` Geert Uytterhoeven 2015-04-12 13:48 ` Peter Senna Tschudin 2015-04-12 15:38 ` Geert Uytterhoeven 2015-04-12 18:14 ` Joe Perches 2015-04-12 19:06 ` Geert Uytterhoeven 2015-04-13 11:14 ` Peter Senna Tschudin 2015-04-13 11:21 ` Geert Uytterhoeven 2015-04-13 11:25 ` Peter Senna Tschudin 2015-04-13 12:31 ` Dan Carpenter 2015-04-16 13:39 ` [PATCH V4] " Peter Senna Tschudin 2015-04-16 16:14 ` Geert Uytterhoeven 2015-04-16 17:01 ` Dan Carpenter 2015-04-16 17:05 ` Alan Cox 2015-04-16 18:36 ` Peter Senna Tschudin 2015-05-03 18:32 ` Greg Kroah-Hartman 2015-05-04 13:11 ` [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits Peter Senna Tschudin 2015-05-04 13:34 ` Dan Carpenter 2015-05-09 16:24 ` Greg KH 2015-05-19 9:44 ` [PATCH V6] " Peter Senna Tschudin 2015-04-17 8:11 ` [PATCH V4] Fix pointer cast for 32 bits arch Dan Carpenter 2015-04-17 8:20 ` Dan Carpenter 2015-04-17 13:31 ` Alan Cox 2015-04-17 13:59 ` Dan Carpenter 2015-04-17 14:10 ` Alan Cox 2015-04-18 13:34 ` Peter Senna Tschudin 2015-04-18 13:59 ` Dan Carpenter 2015-04-13 9:10 ` [PATCH] " Dan Carpenter 2015-04-13 9:16 ` Peter Senna Tschudin 2015-04-13 9:21 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.