[PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[Koen Vandeputte]

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);
 }
 
 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

Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[Tim Harvey]

On Tue, Dec 18, 2018 at 3:19 AM Koen Vandeputte
<koen.vandeputte@ncentric.com> 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);
>  }
>
>  static int cns3xxx_pci_read_config(struct pci_bus *bus, unsigned int devfn,
> --
> 2.17.1
>

Acked-by: Tim Harvey <tharvey@gateworks.com>

Thanks Koen!

Tim

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[Krzysztof Hałasa]

Koen Vandeputte <koen.vandeputte@ncentric.com> writes:

> --- 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);
>  }
>  
>  static int cns3xxx_pci_read_config(struct pci_bus *bus, unsigned int devfn,

Acked-by: Krzysztof Halasa <khalasa@piap.pl>
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[Koen Vandeputte]

Friendly ping :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[Bjorn Helgaas]

[+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[Krzysztof Hałasa]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[Bjorn Helgaas]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[Koen Vandeputte]

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

Re: [PATCH] arm: cns3xxx: fix writing to wrong PCI registers after alignment

[Bjorn Helgaas]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel