On 8/29/20 10:29 AM, Linus Torvalds wrote: > On Sat, Aug 29, 2020 at 5:46 AM Luc Van Oostenryck > <luc.vanoostenryck@xxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> Since I didn't see the real problem with the original code, I'd take that with a grain of salt, though. Guenter