From: Vinod Koul <vkoul@kernel.org> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Guenter Roeck <linux@roeck-us.net>, 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>, Li Yang <leoyang.li@nxp.com>, Zhang Wei <zw@zh-kernel.org>, Dan Williams <dan.j.williams@intel.com>, 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() Date: Mon, 31 Aug 2020 12:09:06 +0530 [thread overview] Message-ID: <20200831063906.GD2639@vkoul-mobl> (raw) In-Reply-To: <CAHk-=wjDEiWF_DsCVFPFqNa+JCS5SkOygbqeq8_=ZNOrFt7-rg@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Vinod Koul <vkoul@kernel.org> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Herbert Xu <herbert@gondor.apana.org.au>, Joerg Roedel <joerg.roedel@amd.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Li Yang <leoyang.li@nxp.com>, Zhang Wei <zw@zh-kernel.org>, Guenter Roeck <linux@roeck-us.net>, dma <dmaengine@vger.kernel.org>, Andrew Morton <akpm@linux-foundation.org>, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, Dan Williams <dan.j.williams@intel.com>, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> Subject: Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits() Date: Mon, 31 Aug 2020 12:09:06 +0530 [thread overview] Message-ID: <20200831063906.GD2639@vkoul-mobl> (raw) In-Reply-To: <CAHk-=wjDEiWF_DsCVFPFqNa+JCS5SkOygbqeq8_=ZNOrFt7-rg@mail.gmail.com> 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
next prev parent reply other threads:[~2020-08-31 6:39 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-28 7:11 [PATCH] kernel.h: Silence sparse warning in lower_32_bits Herbert Xu 2020-08-29 10:51 ` Guenter Roeck 2020-08-29 12:45 ` [PATCH] dmaengine: fsldma: Do not pass pointers to lower_32_bits Herbert Xu 2020-08-29 15:08 ` Guenter Roeck 2020-08-29 12:45 ` [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits() Luc Van Oostenryck 2020-08-29 12:45 ` Luc Van Oostenryck 2020-08-29 17:29 ` Linus Torvalds 2020-08-29 17:29 ` Linus Torvalds 2020-08-29 20:40 ` Guenter Roeck 2020-08-29 20:40 ` Guenter Roeck 2020-08-29 21:20 ` Linus Torvalds 2020-08-29 21:20 ` Linus Torvalds 2020-08-31 1:54 ` Michael Ellerman 2020-08-31 1:54 ` Michael Ellerman 2020-08-31 6:39 ` Vinod Koul [this message] 2020-08-31 6:39 ` Vinod Koul 2020-08-31 14:25 ` Leo Li 2020-08-31 14:25 ` Leo Li 2020-08-30 12:11 ` Luc Van Oostenryck 2020-08-30 12:11 ` Luc Van Oostenryck 2020-08-29 15:05 Guenter Roeck 2020-08-29 15:05 ` Guenter Roeck 2020-08-29 15:58 ` Christophe Leroy 2020-08-29 15:58 ` Christophe Leroy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200831063906.GD2639@vkoul-mobl \ --to=vkoul@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=dan.j.williams@intel.com \ --cc=dmaengine@vger.kernel.org \ --cc=herbert@gondor.apana.org.au \ --cc=joerg.roedel@amd.com \ --cc=leoyang.li@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=luc.vanoostenryck@gmail.com \ --cc=torvalds@linux-foundation.org \ --cc=zw@zh-kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.