LinuxLists.cc - [PATCH] Fix pointer cast for 32 bits arch

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

On Sun, Apr 12, 2015 at 11:26 AM, Peter Senna Tschudin
<[email protected]> wrote:
> Sparse compalins about casting void * to u64 on i386.
> Change the cast to resource_size_t.
>
> Signed-off-by: Peter Senna Tschudin <[email protected]>
> ---
>
> 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 -- [email protected]

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

2015-04-12 13:48:54

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

On Sun, Apr 12, 2015 at 3:05 PM, Geert Uytterhoeven
<[email protected]> wrote:
> On Sun, Apr 12, 2015 at 11:26 AM, Peter Senna Tschudin
> <[email protected]> wrote:
>> Sparse compalins about casting void * to u64 on i386.
>> Change the cast to resource_size_t.
>>
>> Signed-off-by: Peter Senna Tschudin <[email protected]>
>> ---
>>
>> 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 -- [email protected]
>
> 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

2015-04-12 15:38:26

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

On Sun, Apr 12, 2015 at 3:48 PM, Peter Senna Tschudin
<[email protected]> wrote:
> On Sun, Apr 12, 2015 at 3:05 PM, Geert Uytterhoeven
> <[email protected]> wrote:
>> On Sun, Apr 12, 2015 at 11:26 AM, Peter Senna Tschudin
>> <[email protected]> wrote:
>>> Sparse compalins about casting void * to u64 on i386.
>>> Change the cast to resource_size_t.
>>>
>>> Signed-off-by: Peter Senna Tschudin <[email protected]>
>>> ---
>>>
>>> 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 <[email protected]>
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 -- [email protected]

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

2015-04-12 18:14:51

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

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.

2015-04-12 19:06:45

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

On Sun, Apr 12, 2015 at 8:14 PM, Joe Perches <[email protected]> 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 -- [email protected]

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

2015-04-13 09:10:40

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

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 <[email protected]>
> ---
>
> 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
}

2015-04-13 09:16:58

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

On Mon, Apr 13, 2015 at 11:10 AM, Dan Carpenter
<[email protected]> 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 <[email protected]>
>> ---
>>
>> 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

2015-04-13 09:22:17

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

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
> <[email protected]> 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 <[email protected]>
> >> ---
> >>
> >> 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

2015-04-13 11:14:27

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

On Sun, Apr 12, 2015 at 5:38 PM, Geert Uytterhoeven
<[email protected]> wrote:
> On Sun, Apr 12, 2015 at 3:48 PM, Peter Senna Tschudin
> <[email protected]> wrote:
>> On Sun, Apr 12, 2015 at 3:05 PM, Geert Uytterhoeven
>> <[email protected]> wrote:
>>> On Sun, Apr 12, 2015 at 11:26 AM, Peter Senna Tschudin
>>> <[email protected]> wrote:
>>>> Sparse compalins about casting void * to u64 on i386.
>>>> Change the cast to resource_size_t.
>>>>
>>>> Signed-off-by: Peter Senna Tschudin <[email protected]>
>>>> ---
>>>>
>>>> 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 <[email protected]>
> 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 -- [email protected]
>
> 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

2015-04-13 11:21:45

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

Hi Peter,

On Mon, Apr 13, 2015 at 1:14 PM, Peter Senna Tschudin
<[email protected]> 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 -- [email protected]

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

2015-04-13 11:25:22

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

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
<[email protected]> wrote:
> Hi Peter,
>
> On Mon, Apr 13, 2015 at 1:14 PM, Peter Senna Tschudin
> <[email protected]> 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 -- [email protected]
>
> 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

2015-04-13 12:31:39

[permalink] [raw]

Subject: Re: [PATCH] Fix pointer cast for 32 bits arch

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

2015-04-16 13:39:25

[permalink] [raw]

Subject: [PATCH V4] Fix pointer cast for 32 bits arch

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 <[email protected]>
---
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

2015-04-16 16:15:04

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

On Thu, Apr 16, 2015 at 3:39 PM, Peter Senna Tschudin
<[email protected]> 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 -- [email protected]

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

2015-04-16 17:02:19

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

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
> <[email protected]> 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

2015-04-16 17:05:39

by Alan

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

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
> > <[email protected]> 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

2015-04-16 18:36:58

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

On Thu, Apr 16, 2015 at 7:05 PM, Alan Cox <[email protected]> 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
>> > <[email protected]> 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

2015-04-17 08:12:21

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

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

2015-04-17 08:20:41

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

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

2015-04-17 13:31:59

by Alan

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

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

2015-04-17 14:00:02

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

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

2015-04-17 14:10:33

by Alan

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

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

2015-04-18 13:34:24

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

On Fri, Apr 17, 2015 at 4:10 PM, Alan Cox <[email protected]> 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

2015-04-18 14:00:30

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

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 <[email protected]> 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

2015-05-03 19:02:30

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH V4] Fix pointer cast for 32 bits arch

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 <[email protected]> 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
> >> > <[email protected]> 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

2015-05-04 13:12:34

[permalink] [raw]

Subject: [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits

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 <[email protected]>
Signed-off-by: Peter Senna Tschudin <[email protected]>
---
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

2015-05-04 13:35:40

[permalink] [raw]

Subject: Re: [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits

Looks good.

regards,
dan carpenter

2015-05-09 16:27:55

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH V5] staging: goldfish: Fix pointer cast for 32 bits

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 <[email protected]>
> Signed-off-by: Peter Senna Tschudin <[email protected]>
> ---
> 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

2015-05-19 09:45:07