On Thu, Jan 24, 2019 at 04:23:05PM +0100, Koen Vandeputte wrote:
>
> On 24.01.19 12:56, Lorenzo Pieralisi wrote:
> >On Mon, Jan 07, 2019 at 02:45:09PM +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")
> >>Acked-by: Krzysztof Halasa <khalasa@xxxxxxx>
> >>Acked-by: Tim Harvey <tharvey@xxxxxxxxxxxxx>
> >>Signed-off-by: Koen Vandeputte <koen.vandeputte@xxxxxxxxxxxx>
> >>CC: Arnd Bergmann <arnd@xxxxxxxx>
> >>CC: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >>CC: Olof Johansson <olof@xxxxxxxxx>
> >>CC: Robin Leblon <robin.leblon@xxxxxxxxxxxx>
> >>CC: Rob Herring <robh@xxxxxxxxxx>
> >>CC: Russell King <linux@xxxxxxxxxxxxxxx>
> >>CC: stable@xxxxxxxxxxxxxxx # v4.0+
> >>---
> >> arch/arm/mach-cns3xxx/pcie.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >I have applied both patches to pci/arm-cns3xxx for v5.1, however I had
> >to reformat and rewrite both logs (commit line wrappings,
> >capitalization, etc.) so have a look.
> >
> >Thanks,
> >Lorenzo
>
> Hi Lorenzo,
>
> Thank you for taking care of the wrappings etc.?? it seems my auto-wrap was
> disabled in my git tooling ..
> Your adaptions look more than fine.
>
>
> Purely for my information:
>
> Testing on a lot of devices here shows a huge improvement towards stability.
> Is it possible to get it merged sooner?
> Does "queued for 5.1" also mean that backporting to stables only will happen
> at 5.1_rc1 release?
Yes, I will ask Bjorn if we can send them for one of the upcoming -rc*
(so effectively you will get them in v5.0 and propagated to stable
earlier), I do not think it is that urgent either though, let me handle
that.
Thanks,
Lorenzo
