* Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment [not found] <20181218111743.25566-1-koen.vandeputte@ncentric.com> @ 2018-12-30 1:06 ` Bjorn Helgaas 2018-12-31 10:14 ` Krzysztof Hałasa 0 siblings, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2018-12-30 1:06 UTC (permalink / raw) To: Koen Vandeputte Cc: linux-arm-kernel, Rob Herring, Arnd Bergmann, Tim Harvey, Russell King, stable, Robin Leblon, Olof Johansson, Krzysztof Halasa, linux-pci [+cc linux-pci] On Tue, Dec 18, 2018 at 12:17:43PM +0100, Koen Vandeputte wrote: > Originally, cns3xxx used it's own functions for mapping, reading and writing registers. > > Commit 802b7c06adc7 ("ARM: cns3xxx: Convert PCI to use generic config accessors") > removed the internal PCI config write function in favor of the generic one: > > cns3xxx_pci_write_config() --> pci_generic_config_write() > > cns3xxx_pci_write_config() expected aligned addresses, being produced by cns3xxx_pci_map_bus() > while the generic one pci_generic_config_write() actually expects the real address > as both the function and hardware are capable of byte-aligned writes. > > This currently leads to pci_generic_config_write() writing > to the wrong registers on some ocasions. > > First issue seen due to this: > > - driver ath9k gets loaded > - The driver wants to write value 0xA8 to register PCI_LATENCY_TIMER, located at 0x0D > - cns3xxx_pci_map_bus() aligns the address to 0x0C > - pci_generic_config_write() effectively writes 0xA8 into register 0x0C (CACHE_LINE_SIZE) > > This seems to cause some slight instability when certain PCI devices are used. > > Another issue example caused by this this is the PCI bus numbering, > where the primary bus is higher than the secondary, which is impossible. > > Before: > > 00:00.0 PCI bridge: Cavium, Inc. Device 3400 (rev 01) (prog-if 00 [Normal decode]) > Flags: bus master, fast devsel, latency 0, IRQ 255 > Bus: primary=02, secondary=01, subordinate=ff, sec-latency=0 > > After fix: > > 00:00.0 PCI bridge: Cavium, Inc. Device 3400 (rev 01) (prog-if 00 [Normal decode]) > Flags: bus master, fast devsel, latency 0, IRQ 255 > Bus: primary=00, secondary=01, subordinate=02, sec-latency=0 > > And very likely some more .. > > Fix all by omitting the alignment being done in the mapping function. > > Fixes: 802b7c06adc7 ("ARM: cns3xxx: Convert PCI to use generic config accessors") > Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com> > CC: Arnd Bergmann <arnd@arndb.de> > CC: Bjorn Helgaas <bhelgaas@google.com> > CC: Krzysztof Halasa <khalasa@piap.pl> > CC: Olof Johansson <olof@lixom.net> > CC: Robin Leblon <robin.leblon@ncentric.com> > CC: Rob Herring <robh@kernel.org> > CC: Russell King <linux@armlinux.org.uk> > CC: Tim Harvey <tharvey@gateworks.com> > CC: stable@vger.kernel.org # v4.0+ > --- > arch/arm/mach-cns3xxx/pcie.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/mach-cns3xxx/pcie.c b/arch/arm/mach-cns3xxx/pcie.c > index 318394ed5c7a..5e11ad3164e0 100644 > --- a/arch/arm/mach-cns3xxx/pcie.c > +++ b/arch/arm/mach-cns3xxx/pcie.c > @@ -83,7 +83,7 @@ static void __iomem *cns3xxx_pci_map_bus(struct pci_bus *bus, > } else /* remote PCI bus */ > base = cnspci->cfg1_regs + ((busno & 0xf) << 20); > > - return base + (where & 0xffc) + (devfn << 12); > + return base + where + (devfn << 12); 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. What do you think? I don't think hardware that only supports 32-bit PCI writes can be quite spec-compliant (see the comments in pci_generic_config_write32()). So (1) you may see an additional dmesg warning when you convert to use it, and (2) it's possible that there may still be instability related to the corruption caused by using a 32-bit write when an 8-bit write was intended. > } > > static int cns3xxx_pci_read_config(struct pci_bus *bus, unsigned int devfn, > -- > 2.17.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment 2018-12-30 1:06 ` [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment Bjorn Helgaas @ 2018-12-31 10:14 ` Krzysztof Hałasa 2018-12-31 15:29 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: Krzysztof Hałasa @ 2018-12-31 10:14 UTC (permalink / raw) To: Bjorn Helgaas Cc: Koen Vandeputte, linux-arm-kernel, Rob Herring, Arnd Bergmann, Tim Harvey, Russell King, stable, Robin Leblon, Olof Johansson, linux-pci Hi, 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? > I don't think hardware that only supports 32-bit PCI writes can be > quite spec-compliant (see the comments in > pci_generic_config_write32()). So (1) you may see an additional > dmesg warning when you convert to use it, and (2) it's possible that > there may still be instability related to the corruption caused by > using a 32-bit write when an 8-bit write was intended. Right. I think IDE/SATA controllers are affected, for example. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment 2018-12-31 10:14 ` Krzysztof Hałasa @ 2018-12-31 15:29 ` Bjorn Helgaas 2019-01-07 8:58 ` Koen Vandeputte 0 siblings, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2018-12-31 15:29 UTC (permalink / raw) To: Krzysztof Hałasa Cc: Rob Herring, Arnd Bergmann, linux-pci, Tim Harvey, Russell King, stable, Robin Leblon, Koen Vandeputte, Olof Johansson, linux-arm-kernel 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: 1) The current one that corrects the address alignment error, and 2) A new one that converts cns3xxx_pci_read_config() to use pci_generic_config_read() instead of pci_generic_config_read32(). Ideally the changelog for the second patch would refer to the verification that the hardware is capable of 8- and 16-bit accesses. Bjorn ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment 2018-12-31 15:29 ` Bjorn Helgaas @ 2019-01-07 8:58 ` Koen Vandeputte 2019-01-07 13:24 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: Koen Vandeputte @ 2019-01-07 8:58 UTC (permalink / raw) To: Bjorn Helgaas, Krzysztof Hałasa Cc: Rob Herring, Arnd Bergmann, linux-pci, Tim Harvey, Russell King, stable, Robin Leblon, Olof Johansson, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment 2019-01-07 8:58 ` Koen Vandeputte @ 2019-01-07 13:24 ` Bjorn Helgaas 0 siblings, 0 replies; 5+ messages in thread From: Bjorn Helgaas @ 2019-01-07 13:24 UTC (permalink / raw) To: Koen Vandeputte Cc: Krzysztof Hałasa, Rob Herring, Arnd Bergmann, linux-pci, Tim Harvey, Russell King, stable, Robin Leblon, Olof Johansson, linux-arm-kernel On Mon, Jan 07, 2019 at 09:58:42AM +0100, Koen Vandeputte wrote: > On 31.12.18 16:29, Bjorn Helgaas wrote: > > 1) The current one that corrects the address alignment error, and > Is it required to resend this specific patch? It's easier if you resend this patch along with the other. When I suggest changes to a patch, I mark it (and the whole series if it's part of a series) as "changes requested" in the patchwork patch tracker. That means it falls off my to-do list until the resend happens. > > 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 Perfect, thanks! Bjorn ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-07 13:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20181218111743.25566-1-koen.vandeputte@ncentric.com> 2018-12-30 1:06 ` [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment Bjorn Helgaas 2018-12-31 10:14 ` Krzysztof Hałasa 2018-12-31 15:29 ` Bjorn Helgaas 2019-01-07 8:58 ` Koen Vandeputte 2019-01-07 13:24 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).