Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Guenter Roeck]

On Sat, Aug 29, 2020 at 02:45:38PM +0200, Luc Van Oostenryck wrote:
> For ppc32, the functions fsl_ioread64() & fsl_ioread64be()
> use lower_32_bits() as a fancy way to cast the pointer to u32
> in order to do non-atomic 64-bit IO.
> 
> But the pointer is already 32-bit, so simply cast the pointer to u32.
> 
> This fixes a compile error introduced by
>    ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")
> 
> Fixes: ef91bb196b0db1013ef8705367bc2d7944ef696b

checkpatch complains about this and prefers 

Fixes: ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")

Otherwise

Tested-by: Guenter Roeck <linux@roeck-us.net>

> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Cc: Li Yang <leoyang.li@nxp.com>
> Cc: Zhang Wei <zw@zh-kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: dmaengine@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  drivers/dma/fsldma.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
> index 56f18ae99233..6f6fa7641fa2 100644
> --- a/drivers/dma/fsldma.h
> +++ b/drivers/dma/fsldma.h
> @@ -205,7 +205,7 @@ struct fsldma_chan {
>  #else
>  static u64 fsl_ioread64(const u64 __iomem *addr)
>  {
> -	u32 fsl_addr = lower_32_bits(addr);
> +	u32 fsl_addr = (u32) addr;
>  	u64 fsl_addr_hi = (u64)in_le32((u32 *)(fsl_addr + 1)) << 32;
>  
>  	return fsl_addr_hi | in_le32((u32 *)fsl_addr);
> @@ -219,7 +219,7 @@ static void fsl_iowrite64(u64 val, u64 __iomem *addr)
>  
>  static u64 fsl_ioread64be(const u64 __iomem *addr)
>  {
> -	u32 fsl_addr = lower_32_bits(addr);
> +	u32 fsl_addr = (u32) addr;
>  	u64 fsl_addr_hi = (u64)in_be32((u32 *)fsl_addr) << 32;
>  
>  	return fsl_addr_hi | in_be32((u32 *)(fsl_addr + 1));
> -- 
> 2.28.0
> 

Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Christophe Leroy]

Le 29/08/2020 à 17:05, Guenter Roeck a écrit :
> On Sat, Aug 29, 2020 at 02:45:38PM +0200, Luc Van Oostenryck wrote:
>> For ppc32, the functions fsl_ioread64() & fsl_ioread64be()
>> use lower_32_bits() as a fancy way to cast the pointer to u32
>> in order to do non-atomic 64-bit IO.
>>
>> But the pointer is already 32-bit, so simply cast the pointer to u32.
>>
>> This fixes a compile error introduced by
>>     ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")
>>
>> Fixes: ef91bb196b0db1013ef8705367bc2d7944ef696b
> 
> checkpatch complains about this and prefers
> 
> Fixes: ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")

Checkpatch also complains about spacing:

CHECK:SPACING: No space is necessary after a cast
#39: FILE: drivers/dma/fsldma.h:208:
+	u32 fsl_addr = (u32) addr;

CHECK:SPACING: No space is necessary after a cast
#48: FILE: drivers/dma/fsldma.h:222:
+	u32 fsl_addr = (u32) addr;

total: 0 errors, 0 warnings, 2 checks, 16 lines checked

Christophe

> 
> Otherwise
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Cc: Li Yang <leoyang.li@nxp.com>
>> Cc: Zhang Wei <zw@zh-kernel.org>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Vinod Koul <vkoul@kernel.org>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: dmaengine@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
>> ---
>>   drivers/dma/fsldma.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
>> index 56f18ae99233..6f6fa7641fa2 100644
>> --- a/drivers/dma/fsldma.h
>> +++ b/drivers/dma/fsldma.h
>> @@ -205,7 +205,7 @@ struct fsldma_chan {
>>   #else
>>   static u64 fsl_ioread64(const u64 __iomem *addr)
>>   {
>> -	u32 fsl_addr = lower_32_bits(addr);
>> +	u32 fsl_addr = (u32) addr;
>>   	u64 fsl_addr_hi = (u64)in_le32((u32 *)(fsl_addr + 1)) << 32;
>>   
>>   	return fsl_addr_hi | in_le32((u32 *)fsl_addr);
>> @@ -219,7 +219,7 @@ static void fsl_iowrite64(u64 val, u64 __iomem *addr)
>>   
>>   static u64 fsl_ioread64be(const u64 __iomem *addr)
>>   {
>> -	u32 fsl_addr = lower_32_bits(addr);
>> +	u32 fsl_addr = (u32) addr;
>>   	u64 fsl_addr_hi = (u64)in_be32((u32 *)fsl_addr) << 32;
>>   
>>   	return fsl_addr_hi | in_be32((u32 *)(fsl_addr + 1));
>> -- 
>> 2.28.0
>>

Re: [PATCH] kernel.h: Silence sparse warning in lower_32_bits

[Guenter Roeck]

On Fri, Aug 28, 2020 at 05:11:25PM +1000, Herbert Xu wrote:
> I keep getting sparse warnings in crypto such as:
> 

This patch results in the following compile error when compiling 
ppc:mpc85xx_defconfig.

Error log:
In file included from ./include/linux/list.h:9,
                 from ./include/linux/module.h:12,
                 from drivers/dma/fsldma.c:23:
drivers/dma/fsldma.h: In function 'fsl_ioread64':
./include/linux/kernel.h:189:37: error: invalid operands to binary & (have 'const u64 *' {aka 'const long long unsigned int *'} and 'unsigned int')
  189 | #define lower_32_bits(n) ((u32)((n) & 0xffffffff))
      |                                     ^
drivers/dma/fsldma.h:208:17: note: in expansion of macro 'lower_32_bits'
  208 |  u32 fsl_addr = lower_32_bits(addr);
      |                 ^~~~~~~~~~~~~
drivers/dma/fsldma.h: In function 'fsl_ioread64be':
./include/linux/kernel.h:189:37: error: invalid operands to binary & (have 'const u64 *' {aka 'const long long unsigned int *'} and 'unsigned int')
  189 | #define lower_32_bits(n) ((u32)((n) & 0xffffffff))
      |                                     ^
drivers/dma/fsldma.h:222:17: note: in expansion of macro 'lower_32_bits'
  222 |  u32 fsl_addr = lower_32_bits(addr);
      |                 ^~~~~~~~~~~~~
make[2]: *** [drivers/dma/fsldma.o] Error 1

Bisct log attached.

Guenter

---
# bad: [4d41ead6ead97c3730bbd186a601a64828668f01] Merge tag 'block-5.9-2020-08-28' of git://git.kernel.dk/linux-block
# good: [15bc20c6af4ceee97a1f90b43c0e386643c071b4] Merge tag 'tty-5.9-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty
git bisect start 'HEAD' '15bc20c6af4c'
# good: [5ec06b5c0d259a8c7c4376b121b2f62dfbfe57ef] Merge tag 'drm-fixes-2020-08-28' of git://anongit.freedesktop.org/drm/drm
git bisect good 5ec06b5c0d259a8c7c4376b121b2f62dfbfe57ef
# bad: [326e311b849426a95cac0149406efb2bbd13fa65] Merge tag 'pm-5.9-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect bad 326e311b849426a95cac0149406efb2bbd13fa65
# good: [e30942859030199dab5ad73f95faac226133c639] Merge tag 'writeback_for_v5.9-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs
git bisect good e30942859030199dab5ad73f95faac226133c639
# bad: [96d454cd2c1668010406ea4c28ab915bcbb747f4] Merge tag 'arm64-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux
git bisect bad 96d454cd2c1668010406ea4c28ab915bcbb747f4
# good: [e9ee186bb735bfc17fa81dbc9aebf268aee5b41e] KVM: arm64: Add kvm_extable for vaxorcism code
git bisect good e9ee186bb735bfc17fa81dbc9aebf268aee5b41e
# good: [71a7f8cb1ca4ca7214a700b1243626759b6c11d4] KVM: arm64: Set HCR_EL2.PTW to prevent AT taking synchronous exception
git bisect good 71a7f8cb1ca4ca7214a700b1243626759b6c11d4
# bad: [ef91bb196b0db1013ef8705367bc2d7944ef696b] kernel.h: Silence sparse warning in lower_32_bits
git bisect bad ef91bb196b0db1013ef8705367bc2d7944ef696b
# first bad commit: [ef91bb196b0db1013ef8705367bc2d7944ef696b] kernel.h: Silence sparse warning in lower_32_bits

[PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Luc Van Oostenryck]

For ppc32, the functions fsl_ioread64() & fsl_ioread64be()
use lower_32_bits() as a fancy way to cast the pointer to u32
in order to do non-atomic 64-bit IO.

But the pointer is already 32-bit, so simply cast the pointer to u32.

This fixes a compile error introduced by
   ef91bb196b0d ("kernel.h: Silence sparse warning in lower_32_bits")

Fixes: ef91bb196b0db1013ef8705367bc2d7944ef696b
Reported-by: Guenter Roeck <linux@roeck-us.net>
Cc: Li Yang <leoyang.li@nxp.com>
Cc: Zhang Wei <zw@zh-kernel.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: dmaengine@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 drivers/dma/fsldma.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index 56f18ae99233..6f6fa7641fa2 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -205,7 +205,7 @@ struct fsldma_chan {
 #else
 static u64 fsl_ioread64(const u64 __iomem *addr)
 {
-	u32 fsl_addr = lower_32_bits(addr);
+	u32 fsl_addr = (u32) addr;
 	u64 fsl_addr_hi = (u64)in_le32((u32 *)(fsl_addr + 1)) << 32;
 
 	return fsl_addr_hi | in_le32((u32 *)fsl_addr);
@@ -219,7 +219,7 @@ static void fsl_iowrite64(u64 val, u64 __iomem *addr)
 
 static u64 fsl_ioread64be(const u64 __iomem *addr)
 {
-	u32 fsl_addr = lower_32_bits(addr);
+	u32 fsl_addr = (u32) addr;
 	u64 fsl_addr_hi = (u64)in_be32((u32 *)fsl_addr) << 32;
 
 	return fsl_addr_hi | in_be32((u32 *)(fsl_addr + 1));
-- 
2.28.0

Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 2203 bytes --]

On Sat, Aug 29, 2020 at 5:46 AM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> But the pointer is already 32-bit, so simply cast the pointer to u32.

Yeah, that code was completely pointless. If the pointer had actually
been 64-bit, the old code would have warned too.

The odd thing is that the fsl_iowrite64() functions make sense. It's
only the fsl_ioread64() functions that seem to be written by somebody
who is really confused.

That said, this patch only humors the confusion. The cast to 'u32' is
completely pointless. In fact, it seems to be actively wrong, because
it means that the later "fsl_addr + 1" is done entirely incorrectly -
it now literally adds "1" to an integer value, while the iowrite()
functions will add one to a "u32 __iomem *" pointer (so will do
pointer arithmetic, and add 4).

So this code has never ever worked correctly to begin with, but the
patches to fix the warning miss the point. The problem isn't the
warning, the problem is that the code is broken and completely wrong
to begin with.

And the "lower_32_bits()" thing has always been pure and utter
confusion and complete garbage.

I *think* the right patch is the one attached, but since this code is
clearly utterly broken, I'd want somebody to test it.

It has probably never ever worked on 32-bit powerpc, or did so purely
by mistake (perhaps because nobody really cares - the only 64-bit use
is this:

    static dma_addr_t get_cdar(struct fsldma_chan *chan)
    {
        return FSL_DMA_IN(chan, &chan->regs->cdar, 64) & ~FSL_DMA_SNEN;
    }

and there are two users of that: one which ignores the return value,
and one that looks like it might end up half-way working even if the
value read was garbage (it's used only to compare against a "current
descriptor" value).

Anyway, the fix is definitely not to just shut up the warning. The
warning is only a sign of utter confusion in that driver.

Can somebody with the hardware test this on 32-bit ppc?

And if not (judging by just how broken those functions are, maybe it
never did work), can somebody with a ppc32 setup at least compile-test
this patch and look at whether it makes sense, in ways the old code
did not.

                Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 1167 bytes --]

 drivers/dma/fsldma.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index 56f18ae99233..c574d223d52e 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -205,10 +205,10 @@ struct fsldma_chan {
 #else
 static u64 fsl_ioread64(const u64 __iomem *addr)
 {
-	u32 fsl_addr = lower_32_bits(addr);
-	u64 fsl_addr_hi = (u64)in_le32((u32 *)(fsl_addr + 1)) << 32;
+	u32 val_lo = in_le32((u32 __iomem *)addr);
+	u32 val_hi = in_le32((u32 __iomem *)addr + 1);
 
-	return fsl_addr_hi | in_le32((u32 *)fsl_addr);
+	return ((u64)val_hi << 32) + val_lo;
 }
 
 static void fsl_iowrite64(u64 val, u64 __iomem *addr)
@@ -219,10 +219,10 @@ static void fsl_iowrite64(u64 val, u64 __iomem *addr)
 
 static u64 fsl_ioread64be(const u64 __iomem *addr)
 {
-	u32 fsl_addr = lower_32_bits(addr);
-	u64 fsl_addr_hi = (u64)in_be32((u32 *)fsl_addr) << 32;
+	u32 val_hi = in_be32((u32 __iomem *)addr);
+	u32 val_lo = in_be32((u32 __iomem *)addr+1);
 
-	return fsl_addr_hi | in_be32((u32 *)(fsl_addr + 1));
+	return ((u64)val_hi << 32) + val_lo;
 }
 
 static void fsl_iowrite64be(u64 val, u64 __iomem *addr)

Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Guenter Roeck]

On 8/29/20 10:29 AM, Linus Torvalds wrote:
> On Sat, Aug 29, 2020 at 5:46 AM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>>
>> But the pointer is already 32-bit, so simply cast the pointer to u32.
> 
> Yeah, that code was completely pointless. If the pointer had actually
> been 64-bit, the old code would have warned too.
> 
> The odd thing is that the fsl_iowrite64() functions make sense. It's
> only the fsl_ioread64() functions that seem to be written by somebody
> who is really confused.
> 
> That said, this patch only humors the confusion. The cast to 'u32' is
> completely pointless. In fact, it seems to be actively wrong, because
> it means that the later "fsl_addr + 1" is done entirely incorrectly -
> it now literally adds "1" to an integer value, while the iowrite()
> functions will add one to a "u32 __iomem *" pointer (so will do
> pointer arithmetic, and add 4).
> 

Outch.

> So this code has never ever worked correctly to begin with, but the
> patches to fix the warning miss the point. The problem isn't the
> warning, the problem is that the code is broken and completely wrong
> to begin with.
> 
> And the "lower_32_bits()" thing has always been pure and utter
> confusion and complete garbage.
> 
> I *think* the right patch is the one attached, but since this code is
> clearly utterly broken, I'd want somebody to test it.
> 
> It has probably never ever worked on 32-bit powerpc, or did so purely
> by mistake (perhaps because nobody really cares - the only 64-bit use
> is this:
> 
>     static dma_addr_t get_cdar(struct fsldma_chan *chan)
>     {
>         return FSL_DMA_IN(chan, &chan->regs->cdar, 64) & ~FSL_DMA_SNEN;
>     }
> 
> and there are two users of that: one which ignores the return value,
> and one that looks like it might end up half-way working even if the
> value read was garbage (it's used only to compare against a "current
> descriptor" value).
> 
> Anyway, the fix is definitely not to just shut up the warning. The
> warning is only a sign of utter confusion in that driver.
> 
> Can somebody with the hardware test this on 32-bit ppc?
> 
> And if not (judging by just how broken those functions are, maybe it
> never did work), can somebody with a ppc32 setup at least compile-test
> this patch and look at whether it makes sense, in ways the old code
> did not.
> 

A bit more careful this time. For the attached patch:

Compile-tested-by: Guenter Roeck <linux@roeck-us.net>

Except for

CHECK: spaces preferred around that '+' (ctx:VxV)
#29: FILE: drivers/dma/fsldma.h:223:
+	u32 val_lo = in_be32((u32 __iomem *)addr+1);

I don't see anything wrong with it either, so

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Since I didn't see the real problem with the original code,
I'd take that with a grain of salt, though.

Guenter

Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Linus Torvalds]

On Sat, Aug 29, 2020 at 1:40 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Except for
>
> CHECK: spaces preferred around that '+' (ctx:VxV)
> #29: FILE: drivers/dma/fsldma.h:223:
> +       u32 val_lo = in_be32((u32 __iomem *)addr+1);

Added spaces.

> I don't see anything wrong with it either, so
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
> Since I didn't see the real problem with the original code,
> I'd take that with a grain of salt, though.

Well, honestly, the old code was so confused that just making it build
is clearly already an improvement even if everything else were to be
wrong.

So I committed my "fix". If it turns out there's more wrong in there
and somebody tests it, we can fix it again. But now it hopefully
compiles, at least.

My bet is that if that driver ever worked on ppc32, it will continue
to work whatever we do to that function.

I _think_ the old code happened to - completely by mistake - get the
value right for the case of "little endian access, with dma_addr_t
being 32-bit". Because then it would still read the upper bits wrong,
but the cast to dma_addr_t would then throw those bits away. And the
lower bits would be right.

But for big-endian accesses or for ARCH_DMA_ADDR_T_64BIT it really
looks like it always returned a completely incorrect value.

And again - the driver may have worked even with that completely
incorrect value, since the use of it seems to be very incidental.

In either case ("it didn't work before" or "it worked because the
value doesn't really matter"), I don't think I could possibly have
made things worse.

Famous last words.

                Linus

Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Michael Ellerman]

Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Sat, Aug 29, 2020 at 1:40 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Except for
>>
>> CHECK: spaces preferred around that '+' (ctx:VxV)
>> #29: FILE: drivers/dma/fsldma.h:223:
>> +       u32 val_lo = in_be32((u32 __iomem *)addr+1);
>
> Added spaces.
>
>> I don't see anything wrong with it either, so
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>> Since I didn't see the real problem with the original code,
>> I'd take that with a grain of salt, though.
>
> Well, honestly, the old code was so confused that just making it build
> is clearly already an improvement even if everything else were to be
> wrong.

The old code is not that old, only ~18 months:

a1ff82a9c165 ("dmaengine: fsldma: Adding macro FSL_DMA_IN/OUT implement for ARM platform") (Jan 2019)

So I think it's possible it's never been tested on 32-bit ppc at all.

I did have a 32-bit FSL machine but it lost its network card in a power
outage and now it won't boot (and I can't get to it physically).

> So I committed my "fix". If it turns out there's more wrong in there
> and somebody tests it, we can fix it again. But now it hopefully
> compiles, at least.
>
> My bet is that if that driver ever worked on ppc32, it will continue
> to work whatever we do to that function.
>
> I _think_ the old code happened to - completely by mistake - get the
> value right for the case of "little endian access, with dma_addr_t
> being 32-bit". Because then it would still read the upper bits wrong,
> but the cast to dma_addr_t would then throw those bits away. And the
> lower bits would be right.
>
> But for big-endian accesses or for ARCH_DMA_ADDR_T_64BIT it really
> looks like it always returned a completely incorrect value.
>
> And again - the driver may have worked even with that completely
> incorrect value, since the use of it seems to be very incidental.
>
> In either case ("it didn't work before" or "it worked because the
> value doesn't really matter"), I don't think I could possibly have
> made things worse.

Agreed.

Hopefully someone from NXP can test it.

cheers

Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Vinod Koul]

Hi Linus,

On 29-08-20, 14:20, Linus Torvalds wrote:
> On Sat, Aug 29, 2020 at 1:40 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > Except for
> >
> > CHECK: spaces preferred around that '+' (ctx:VxV)
> > #29: FILE: drivers/dma/fsldma.h:223:
> > +       u32 val_lo = in_be32((u32 __iomem *)addr+1);
> 
> Added spaces.
> 
> > I don't see anything wrong with it either, so
> >
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> >
> > Since I didn't see the real problem with the original code,
> > I'd take that with a grain of salt, though.
> 
> Well, honestly, the old code was so confused that just making it build
> is clearly already an improvement even if everything else were to be
> wrong.
> 
> So I committed my "fix". If it turns out there's more wrong in there
> and somebody tests it, we can fix it again. But now it hopefully
> compiles, at least.
> 
> My bet is that if that driver ever worked on ppc32, it will continue
> to work whatever we do to that function.
> 
> I _think_ the old code happened to - completely by mistake - get the
> value right for the case of "little endian access, with dma_addr_t
> being 32-bit". Because then it would still read the upper bits wrong,
> but the cast to dma_addr_t would then throw those bits away. And the
> lower bits would be right.
> 
> But for big-endian accesses or for ARCH_DMA_ADDR_T_64BIT it really
> looks like it always returned a completely incorrect value.
> 
> And again - the driver may have worked even with that completely
> incorrect value, since the use of it seems to be very incidental.

Thank you for the fix.

Acked-By: Vinod Koul <vkoul@kernel.org>

> 
> In either case ("it didn't work before" or "it worked because the
> value doesn't really matter"), I don't think I could possibly have
> made things worse.
> 
> Famous last words.

I guess no one tested this on 32bits seems to have caused this.

-- 
~Vinod

RE: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Leo Li]

> -----Original Message-----
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Sent: Saturday, August 29, 2020 4:20 PM
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>; Herbert Xu
> <herbert@gondor.apana.org.au>; Andrew Morton <akpm@linux-
> foundation.org>; Joerg Roedel <joerg.roedel@amd.com>; Leo Li
> <leoyang.li@nxp.com>; Zhang Wei <zw@zh-kernel.org>; Dan Williams
> <dan.j.williams@intel.com>; Vinod Koul <vkoul@kernel.org>; linuxppc-dev
> <linuxppc-dev@lists.ozlabs.org>; dma <dmaengine@vger.kernel.org>; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
> 
> On Sat, Aug 29, 2020 at 1:40 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > Except for
> >
> > CHECK: spaces preferred around that '+' (ctx:VxV)
> > #29: FILE: drivers/dma/fsldma.h:223:
> > +       u32 val_lo = in_be32((u32 __iomem *)addr+1);
> 
> Added spaces.
> 
> > I don't see anything wrong with it either, so
> >
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> >
> > Since I didn't see the real problem with the original code, I'd take
> > that with a grain of salt, though.
> 
> Well, honestly, the old code was so confused that just making it build is
> clearly already an improvement even if everything else were to be wrong.
> 
> So I committed my "fix". If it turns out there's more wrong in there and
> somebody tests it, we can fix it again. But now it hopefully compiles, at least.
> 
> My bet is that if that driver ever worked on ppc32, it will continue to work
> whatever we do to that function.
> 
> I _think_ the old code happened to - completely by mistake - get the value
> right for the case of "little endian access, with dma_addr_t being 32-bit".
> Because then it would still read the upper bits wrong, but the cast to
> dma_addr_t would then throw those bits away. And the lower bits would be
> right.
> 
> But for big-endian accesses or for ARCH_DMA_ADDR_T_64BIT it really looks
> like it always returned a completely incorrect value.
> 
> And again - the driver may have worked even with that completely incorrect
> value, since the use of it seems to be very incidental.
> 
> In either case ("it didn't work before" or "it worked because the value
> doesn't really matter"), I don't think I could possibly have made things worse.
> 
> Famous last words.

Thanks for the patch.  

Acked-by: Li Yang <leoyang.li@nxp.com>

We are having periodical auto regression tests covering ppc32 platforms.  But looks like it missed this issue.  I will ask the test team to investigate on why the test cases are not sufficient.

Regards,
Leo

Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

[Luc Van Oostenryck]

On Sat, Aug 29, 2020 at 10:29:55AM -0700, Linus Torvalds wrote:
> On Sat, Aug 29, 2020 at 5:46 AM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > But the pointer is already 32-bit, so simply cast the pointer to u32.
> 
> Yeah, that code was completely pointless. If the pointer had actually
> been 64-bit, the old code would have warned too.
> 
> The odd thing is that the fsl_iowrite64() functions make sense. It's
> only the fsl_ioread64() functions that seem to be written by somebody
> who is really confused.
> 
> That said, this patch only humors the confusion. The cast to 'u32' is
> completely pointless. In fact, it seems to be actively wrong, because
> it means that the later "fsl_addr + 1" is done entirely incorrectly -
> it now literally adds "1" to an integer value, while the iowrite()
> functions will add one to a "u32 __iomem *" pointer (so will do
> pointer arithmetic, and add 4).
> 
My bad. I had noticed the '+ 1' and so automatically assumed
'OK, pointer arithmetic now' without noticing that the cast was
done only after the addition. Grrr.

FWIW, the version you committed looks much better to me.

-- Luc