From: Arnd Bergmann <arnd@kernel.org> To: Juan Antonio Aldea-Armenteros <juant.aldea@gmail.com> Cc: Jiri Kosina <trivial@kernel.org>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, driverdevel <devel@driverdev.osuosl.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2] staging: trivial: hikey9xx: fix be32<->u32 casting warnings Date: Tue, 24 Nov 2020 15:22:40 +0100 [thread overview] Message-ID: <CAK8P3a3S+nR0tupEvk7bd0SsMYAd90YtMnL9R3JzzzV-m0pNwA@mail.gmail.com> (raw) In-Reply-To: <20201119122737.189675-1-juant.aldea@gmail.com> On Thu, Nov 19, 2020 at 1:31 PM Juan Antonio Aldea-Armenteros <juant.aldea@gmail.com> wrote: > > This patch fixes the following warnings reported by sparse, by adding > missing __force annotations. > > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > > drivers/staging/hikey9xx/hisi-spmi-controller.c:239:25: warning: cast from restricted __be32 > > Rationale for #164: > data is declared as u32, and it is read and then converted by means of > be32_to_cpu(). Said function expects a __be32 but data is u32, therefore > there's a type missmatch here. > > Rationale for #239: > Is the dual of #164. This time data going to be written so it > needs to be converted from cpu to __be32, but writel() expects u32 and the > output of cpu_to_be32 returns a __be32. Both of the casts look very suspicious, I'd leave these in unless someone can confirm what the actual desired behavior is. > SPMI_SLAVE_OFFSET * slave_id + > SPMI_APB_SPMI_RDATA0_BASE_ADDR + > i * SPMI_PER_DATAREG_BYTE); > - data = be32_to_cpu((__be32)data); > + data = be32_to_cpu((__be32 __force)data); > if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) { > memcpy(buf, &data, sizeof(data)); > buf += sizeof(data); The data comes from a readl(), which contains an endian conversion on architectures that need it, such as when running the board in big-endian arm64 mode. Having a second endian-conversion on little-endian architectures means that the data is always swapped when it gets written to the register. In the original code before Mauro's commit 8788a30c12c7 ("staging: spmi: hisi-spmi-controller: use le32 macros where needed"), the data was byteswapped, and then written into the fifo register, which produced no warning but would do a double-swap on a big-endian kernel, and change the behavior from what it was before. My guess is that Mauro inadvertently fixed this driver for big-endian mode, without noticing that it was broken to start with, and that he did not actually try it with CONFIG_CPU_BIG_ENDIAN. I think the best way would be to go back to to using swab32p() (not the open-coded version) and then use writesl() or iowrite32_rep() with count=1 to write the byteswapped FIFO register without swapping it again. Arnd
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@kernel.org> To: Juan Antonio Aldea-Armenteros <juant.aldea@gmail.com> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, driverdevel <devel@driverdev.osuosl.org>, Jiri Kosina <trivial@kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2] staging: trivial: hikey9xx: fix be32<->u32 casting warnings Date: Tue, 24 Nov 2020 15:22:40 +0100 [thread overview] Message-ID: <CAK8P3a3S+nR0tupEvk7bd0SsMYAd90YtMnL9R3JzzzV-m0pNwA@mail.gmail.com> (raw) In-Reply-To: <20201119122737.189675-1-juant.aldea@gmail.com> On Thu, Nov 19, 2020 at 1:31 PM Juan Antonio Aldea-Armenteros <juant.aldea@gmail.com> wrote: > > This patch fixes the following warnings reported by sparse, by adding > missing __force annotations. > > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > drivers/staging/hikey9xx/hisi-spmi-controller.c:164:24: warning: cast to restricted __be32 > > drivers/staging/hikey9xx/hisi-spmi-controller.c:239:25: warning: cast from restricted __be32 > > Rationale for #164: > data is declared as u32, and it is read and then converted by means of > be32_to_cpu(). Said function expects a __be32 but data is u32, therefore > there's a type missmatch here. > > Rationale for #239: > Is the dual of #164. This time data going to be written so it > needs to be converted from cpu to __be32, but writel() expects u32 and the > output of cpu_to_be32 returns a __be32. Both of the casts look very suspicious, I'd leave these in unless someone can confirm what the actual desired behavior is. > SPMI_SLAVE_OFFSET * slave_id + > SPMI_APB_SPMI_RDATA0_BASE_ADDR + > i * SPMI_PER_DATAREG_BYTE); > - data = be32_to_cpu((__be32)data); > + data = be32_to_cpu((__be32 __force)data); > if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) { > memcpy(buf, &data, sizeof(data)); > buf += sizeof(data); The data comes from a readl(), which contains an endian conversion on architectures that need it, such as when running the board in big-endian arm64 mode. Having a second endian-conversion on little-endian architectures means that the data is always swapped when it gets written to the register. In the original code before Mauro's commit 8788a30c12c7 ("staging: spmi: hisi-spmi-controller: use le32 macros where needed"), the data was byteswapped, and then written into the fifo register, which produced no warning but would do a double-swap on a big-endian kernel, and change the behavior from what it was before. My guess is that Mauro inadvertently fixed this driver for big-endian mode, without noticing that it was broken to start with, and that he did not actually try it with CONFIG_CPU_BIG_ENDIAN. I think the best way would be to go back to to using swab32p() (not the open-coded version) and then use writesl() or iowrite32_rep() with count=1 to write the byteswapped FIFO register without swapping it again. Arnd _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-11-24 14:23 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-19 11:40 [PATCH v1] staging: trivial: hikey9xx: fix be32<->u32 casting warnings Juan Antonio Aldea-Armenteros 2020-11-19 11:40 ` Juan Antonio Aldea-Armenteros 2020-11-19 12:27 ` [PATCH v2] " Juan Antonio Aldea-Armenteros 2020-11-19 12:27 ` Juan Antonio Aldea-Armenteros 2020-11-24 14:22 ` Arnd Bergmann [this message] 2020-11-24 14:22 ` Arnd Bergmann
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=CAK8P3a3S+nR0tupEvk7bd0SsMYAd90YtMnL9R3JzzzV-m0pNwA@mail.gmail.com \ --to=arnd@kernel.org \ --cc=devel@driverdev.osuosl.org \ --cc=gregkh@linuxfoundation.org \ --cc=juant.aldea@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mchehab+huawei@kernel.org \ --cc=trivial@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.