RE: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

RE: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Menon, Nishanth]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
> Sent: Tuesday, September 21, 2010 5:01 AM
> To: linux-omap@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-mtd@lists.infradead.org
> Subject: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw
> read/write
> 
> Following sparse warnings exists due to use of writel/w and readl/w
> functions.
> 
> This patch fixes the sparse warnings by converting readl/w functions usage
> into
> __raw_readl/__raw_readw functions.

Apologies on bringing up an old topic here -> Is'nt it better to fix readl/w or writel/w than replacing it with __raw_readl/w etc?



Regards,
Nishanth Menon

[...]

Re: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Vimal Singh]

On Tue, Sep 21, 2010 at 3:31 PM, G, Manjunath Kondaiah <manjugk@ti.com> wrote:
> Following sparse warnings exists due to use of writel/w and readl/w functions.
>
> This patch fixes the sparse warnings by converting readl/w functions usage into
> __raw_readl/__raw_readw functions.
>
[...]
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -481,7 +481,7 @@ static int omap_verify_buf(struct mtd_info *mtd, const u_char * buf, int len)
>
>        len >>= 1;
>        while (len--) {
> -               if (*p++ != cpu_to_le16(readw(info->nand.IO_ADDR_R)))
> +               if (*p++ != cpu_to_le16(__raw_readw(info->nand.IO_ADDR_R)))

There was an old comment to remove use of 'cpu_to_le16' from driver, I
just missed it. Can you rather use 'ioread16_rep' for reading data.

-- 
Regards,
Vimal Singh

Re: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Russell King - ARM Linux]

On Thu, Oct 07, 2010 at 07:17:08AM -0500, Menon, Nishanth wrote:
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
> > Sent: Tuesday, September 21, 2010 5:01 AM
> > To: linux-omap@vger.kernel.org
> > Cc: linux-arm-kernel@lists.infradead.org; linux-mtd@lists.infradead.org
> > Subject: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw
> > read/write
> > 
> > Following sparse warnings exists due to use of writel/w and readl/w
> > functions.
> > 
> > This patch fixes the sparse warnings by converting readl/w functions usage
> > into
> > __raw_readl/__raw_readw functions.
> 
> Apologies on bringing up an old topic here -> Is'nt it better to fix
> readl/w or writel/w than replacing it with __raw_readl/w etc?

No.  If you're getting sparse warnings its because _you_ are using
readl/writel wrongly.

They take a void __iomem pointer, not a u32, unsigned long, int, or
even a void pointer.

Re: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Nishanth Menon]

Russell King - ARM Linux had written, on 10/07/2010 01:56 PM, the following:
> On Thu, Oct 07, 2010 at 07:17:08AM -0500, Menon, Nishanth wrote:
>>> -----Original Message-----
>>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>>> owner@vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
>>> Sent: Tuesday, September 21, 2010 5:01 AM
>>> To: linux-omap@vger.kernel.org
>>> Cc: linux-arm-kernel@lists.infradead.org; linux-mtd@lists.infradead.org
>>> Subject: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw
>>> read/write
>>>
>>> Following sparse warnings exists due to use of writel/w and readl/w
>>> functions.
>>>
>>> This patch fixes the sparse warnings by converting readl/w functions usage
>>> into
>>> __raw_readl/__raw_readw functions.
>> Apologies on bringing up an old topic here -> Is'nt it better to fix
>> readl/w or writel/w than replacing it with __raw_readl/w etc?
> 
> No.  If you're getting sparse warnings its because _you_ are using
> readl/writel wrongly.
> 
> They take a void __iomem pointer, not a u32, unsigned long, int, or
> even a void pointer.
void __iomem *p;
...

readl(p);

unrolls to:
({ u32 __v = ({ u32 __v = (( __u32)(__le32)(( __le32) ((void)0, 
*(volatile unsigned int *)((p))))); __v; }); __asm__ __volatile__ ("mcr 
p15,
, %0, c7, c10, 5" : : "r" (0) : "memory"); __v; });

({ u32 __v = ({ u32 __v

seems to be the obvious cause of sparse warnings such as that attempted 
to be fixed in [1]

  warning: symbol '__v' shadows an earlier one

my comment being that by moving {read,write}[wlb] to __raw versions dont 
solve the real issue of namespace here. fix should be in 
arch/arm/include/asm/io.h IMHO. so that there are no overlaps as this.

[1]http://marc.info/?l=linux-omap&m=128506333803725&w=2

-- 
Regards,
Nishanth Menon

Re: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[David Woodhouse]

On Thu, 2010-10-07 at 14:50 -0500, Nishanth Menon wrote:
> my comment being that by moving {read,write}[wlb] to __raw versions dont 
> solve the real issue of namespace here. fix should be in 
> arch/arm/include/asm/io.h IMHO. so that there are no overlaps as this. 

Indeed. This patch is complete nonsense.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[G, Manjunath Kondaiah]

David,

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org] 
> Sent: Monday, October 25, 2010 5:32 AM
> To: Menon, Nishanth
> Cc: Russell King - ARM Linux; G, Manjunath Kondaiah; 
> linux-omap@vger.kernel.org; linux-mtd@lists.infradead.org; 
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2 09/10] OMAP2/3: Convert write/read 
> functions to raw read/write
> 
> On Thu, 2010-10-07 at 14:50 -0500, Nishanth Menon wrote:
> > my comment being that by moving {read,write}[wlb] to __raw versions 
> > dont solve the real issue of namespace here. fix should be in 
> > arch/arm/include/asm/io.h IMHO. so that there are no 
> overlaps as this.
> 
> Indeed. This patch is complete nonsense.

nonsense? There are two types of fixes proposed to fix these sparse warnings.
1. with this patch
2. fixing in io.h
https://patchwork.kernel.org/patch/250171/

1st option is already merged with 2.6.37 merge window.

If 2 get accepted, all __raw_read/write will get replaced with readl/writel.

Apart from those two, do you have any other alternate idea to fix these sparse warnings?

-Manjunath

Re: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Artem Bityutskiy]

On Mon, 2010-10-25 at 01:01 +0100, David Woodhouse wrote:
> On Thu, 2010-10-07 at 14:50 -0500, Nishanth Menon wrote:
> > my comment being that by moving {read,write}[wlb] to __raw versions dont 
> > solve the real issue of namespace here. fix should be in 
> > arch/arm/include/asm/io.h IMHO. so that there are no overlaps as this. 
> 
> Indeed. This patch is complete nonsense.

Removing from my l2 tree.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

RE: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[David Woodhouse]

On Mon, 2010-10-25 at 11:04 +0530, G, Manjunath Kondaiah wrote:
> David,
> 
> > -----Original Message-----
> > From: David Woodhouse [mailto:dwmw2@infradead.org] 
> > Sent: Monday, October 25, 2010 5:32 AM
> > To: Menon, Nishanth
> > Cc: Russell King - ARM Linux; G, Manjunath Kondaiah; 
> > linux-omap@vger.kernel.org; linux-mtd@lists.infradead.org; 
> > linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH v2 09/10] OMAP2/3: Convert write/read 
> > functions to raw read/write
> > 
> > On Thu, 2010-10-07 at 14:50 -0500, Nishanth Menon wrote:
> > > my comment being that by moving {read,write}[wlb] to __raw versions 
> > > dont solve the real issue of namespace here. fix should be in 
> > > arch/arm/include/asm/io.h IMHO. so that there are no 
> > overlaps as this.
> > 
> > Indeed. This patch is complete nonsense.
> 
> nonsense? There are two types of fixes proposed to fix these sparse warnings.
> 1. with this patch
> 2. fixing in io.h
> https://patchwork.kernel.org/patch/250171/
> 
> 1st option is already merged with 2.6.37 merge window.
> 
> If 2 get accepted, all __raw_read/write will get replaced with readl/writel.
> 
> Apart from those two, do you have any other alternate idea to fix these sparse warnings?

The latter is obviously the better approach. The problem that sparse is
complaining about is the fact that you have two nested C blocks, both of
which contain a local variable called '__v'. By changing the variable
names so that they're actually unique, we fix the real problem.

The answer is not that *all* drivers which call cpu_to_le16(readw())
must be changed, which is what you seem to have assumed. And you
*certainly* can't change them to use __raw_readw(), which has
*different* semantics (and endianness in some cases), without a clear
comment in the changelog entry saying *why* that change was OK.

-- 
dwmw2