From: Koen Vandeputte <koen.vandeputte@ncentric.com> To: "Bjorn Helgaas" <helgaas@kernel.org>, "Krzysztof Hałasa" <khalasa@piap.pl> Cc: Rob Herring <robh@kernel.org>, Arnd Bergmann <arnd@arndb.de>, linux-pci@vger.kernel.org, Tim Harvey <tharvey@gateworks.com>, Russell King <linux@armlinux.org.uk>, stable@vger.kernel.org, Robin Leblon <robin.leblon@ncentric.com>, Olof Johansson <olof@lixom.net>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment Date: Mon, 7 Jan 2019 09:58:42 +0100 [thread overview] Message-ID: <a7531ef2-d6d0-0a48-1558-23474c97b838@ncentric.com> (raw) In-Reply-To: <20181231152918.GE159477@google.com> On 31.12.18 16:29, Bjorn Helgaas wrote: > On Mon, Dec 31, 2018 at 11:14:28AM +0100, Krzysztof Hałasa wrote: >> Bjorn Helgaas <helgaas@kernel.org> writes: >> >>> 802b7c06adc7 replaced cns3xxx_pci_write_config(), which always used >>> __raw_writel() so it only did 32-bit accesses, with >>> pci_generic_config_write(), which uses writeb/writew/writel() >>> depending on the size. >>> >>> 802b7c06adc7 also converted cns3xxx_pci_read_config() from always >>> using __raw_readl() (a 32-bit access) to using >>> pci_generic_config_read32(), which also always does a 32-bit access. >>> >>> This makes me think the cnx3xxx hardware is only capable of 32-bit >>> accesses, and this patch should change the driver to use >>> pci_generic_config_write32() instead of pci_generic_config_write() in >>> addition to the mapping fix above. >> Hasn't it already been verified that the CNS3xxx can do 8-bit accesses >> (and probably 16-bit ones as well), and that the docs don't mention any >> such limitation? > Quite possibly. I'm not familiar with the hardware or docs so can't > comment personally. > > 802b7c06adc7 reimplemented cns3xxx_pci_read_config() using > pci_generic_config_read32(), which preserved the property of only > doing 32-bit reads. It replaced cns3xxx_pci_write_config() with > pci_generic_config_write(), so it changed writes from always being 32 > bits to being the actual size. I think this was an error in the > original commit, since the changelog doesn't mention this change; in > fact, the changelog says it changes __raw_writel to writel. > > The change from 32-bit to actual size accesses would logically be a > separate commit from changing to use the generic accessors. > > Assuming the hardware does support smaller accesses, I'd propose two > patches: Bjorn, Thank you for your time to look into this. > 1) The current one that corrects the address alignment error, and Is it required to resend this specific patch? > 2) A new one that converts cns3xxx_pci_read_config() to use > pci_generic_config_read() instead of pci_generic_config_read32(). I'll first extensively test non-32b reads later today and will send a patch for it > Ideally the changelog for the second patch would refer to the > verification that the hardware is capable of 8- and 16-bit accesses. > > Bjorn Koen
WARNING: multiple messages have this Message-ID (diff)
From: Koen Vandeputte <koen.vandeputte@ncentric.com> To: "Bjorn Helgaas" <helgaas@kernel.org>, "Krzysztof Hałasa" <khalasa@piap.pl> Cc: Rob Herring <robh@kernel.org>, Arnd Bergmann <arnd@arndb.de>, linux-pci@vger.kernel.org, Tim Harvey <tharvey@gateworks.com>, Russell King <linux@armlinux.org.uk>, stable@vger.kernel.org, Robin Leblon <robin.leblon@ncentric.com>, Olof Johansson <olof@lixom.net>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment Date: Mon, 7 Jan 2019 09:58:42 +0100 [thread overview] Message-ID: <a7531ef2-d6d0-0a48-1558-23474c97b838@ncentric.com> (raw) In-Reply-To: <20181231152918.GE159477@google.com> On 31.12.18 16:29, Bjorn Helgaas wrote: > On Mon, Dec 31, 2018 at 11:14:28AM +0100, Krzysztof Hałasa wrote: >> Bjorn Helgaas <helgaas@kernel.org> writes: >> >>> 802b7c06adc7 replaced cns3xxx_pci_write_config(), which always used >>> __raw_writel() so it only did 32-bit accesses, with >>> pci_generic_config_write(), which uses writeb/writew/writel() >>> depending on the size. >>> >>> 802b7c06adc7 also converted cns3xxx_pci_read_config() from always >>> using __raw_readl() (a 32-bit access) to using >>> pci_generic_config_read32(), which also always does a 32-bit access. >>> >>> This makes me think the cnx3xxx hardware is only capable of 32-bit >>> accesses, and this patch should change the driver to use >>> pci_generic_config_write32() instead of pci_generic_config_write() in >>> addition to the mapping fix above. >> Hasn't it already been verified that the CNS3xxx can do 8-bit accesses >> (and probably 16-bit ones as well), and that the docs don't mention any >> such limitation? > Quite possibly. I'm not familiar with the hardware or docs so can't > comment personally. > > 802b7c06adc7 reimplemented cns3xxx_pci_read_config() using > pci_generic_config_read32(), which preserved the property of only > doing 32-bit reads. It replaced cns3xxx_pci_write_config() with > pci_generic_config_write(), so it changed writes from always being 32 > bits to being the actual size. I think this was an error in the > original commit, since the changelog doesn't mention this change; in > fact, the changelog says it changes __raw_writel to writel. > > The change from 32-bit to actual size accesses would logically be a > separate commit from changing to use the generic accessors. > > Assuming the hardware does support smaller accesses, I'd propose two > patches: Bjorn, Thank you for your time to look into this. > 1) The current one that corrects the address alignment error, and Is it required to resend this specific patch? > 2) A new one that converts cns3xxx_pci_read_config() to use > pci_generic_config_read() instead of pci_generic_config_read32(). I'll first extensively test non-32b reads later today and will send a patch for it > Ideally the changelog for the second patch would refer to the > verification that the hardware is capable of 8- and 16-bit accesses. > > Bjorn Koen _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-01-07 8:58 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-18 11:17 [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment Koen Vandeputte 2018-12-18 11:17 ` Koen Vandeputte 2018-12-18 16:04 ` Tim Harvey 2018-12-18 16:04 ` Tim Harvey 2018-12-20 5:34 ` Krzysztof Hałasa 2018-12-20 5:34 ` Krzysztof Hałasa 2018-12-29 14:40 ` Koen Vandeputte 2018-12-29 14:40 ` Koen Vandeputte 2018-12-30 1:06 ` Bjorn Helgaas 2018-12-30 1:06 ` Bjorn Helgaas 2018-12-31 10:14 ` Krzysztof Hałasa 2018-12-31 10:14 ` Krzysztof Hałasa 2018-12-31 15:29 ` Bjorn Helgaas 2018-12-31 15:29 ` Bjorn Helgaas 2019-01-07 8:58 ` Koen Vandeputte [this message] 2019-01-07 8:58 ` Koen Vandeputte 2019-01-07 13:24 ` Bjorn Helgaas 2019-01-07 13:24 ` Bjorn Helgaas
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=a7531ef2-d6d0-0a48-1558-23474c97b838@ncentric.com \ --to=koen.vandeputte@ncentric.com \ --cc=arnd@arndb.de \ --cc=helgaas@kernel.org \ --cc=khalasa@piap.pl \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=olof@lixom.net \ --cc=robh@kernel.org \ --cc=robin.leblon@ncentric.com \ --cc=stable@vger.kernel.org \ --cc=tharvey@gateworks.com \ /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.