| Message ID | 20200829124538.7475-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | fsldma: fsl_ioread64*() do not need lower_32_bits() | expand |
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
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
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
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
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
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.
> -----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
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));
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(-)