[PATCH V3 0/4] PCI: Loongson-related pci quirks

[PATCH V3 0/4] PCI: Loongson-related pci quirks

[Huacai Chen]

From: Huacai Chen <chenhc@lemote.com>

This patchset resolve some Loongson-related pci problems (however, some
of them affect not only Loongon platform). The first patch has been sent
before, and nearly all reviewers' questions had been answered at that
time [1]. The second patch move some LS7A quirks to quirks.c, where can
be shared by multi architectures. The third patch improve the mrrs quirk
for LS7A chipset, and the fourth patch add a new quirk for LS7A chipset.

[1] http://patchwork.ozlabs.org/project/linux-pci/patch/1600680138-10949-1-git-send-email-chenhc@lemote.com/

V1 -> V2:
1, Rework the 4th patch;
2, Improve commit messages;
3, Remove the last patch since there is better solutions.

V2 -> V3:
1, Add more affected device ids for the 4th patch;
2, Improve commit messages to describe root causes;

Huacai Chen, Tiezhu Yang and Jianmin Lv(4):
 PCI/portdrv: Don't disable device during shutdown.
 PCI: Move loongson pci quirks to quirks.c.
 PCI: Improve the MRRS quirk for LS7A.
 PCI: Add quirk for multifunction devices of LS7A.

Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> 
---
 drivers/pci/controller/pci-loongson.c | 69 ---------------------------
 drivers/pci/pci.c                     |  5 ++
 drivers/pci/pcie/portdrv.h            |  2 +-
 drivers/pci/pcie/portdrv_core.c       |  6 ++-
 drivers/pci/pcie/portdrv_pci.c        | 15 +++++-
 drivers/pci/quirks.c                  | 89 +++++++++++++++++++++++++++++++++++
 include/linux/pci.h                   |  2 +
 include/linux/pci_ids.h               | 10 ++++
 8 files changed, 124 insertions(+), 74 deletions(-)
--
2.27.0

[PATCH V3 1/4] PCI/portdrv: Don't disable device during shutdown

[Huacai Chen]

Use separate remove()/shutdown() callback, and don't disable PCI device
during shutdown. This can avoid some poweroff/reboot failures.

The poweroff/reboot failures could easily be reproduced on Loongson
platforms. I think this is not a Loongson-specific problem, instead, is
a problem related to some specific PCI hosts. On some x86 platforms,
radeon/amdgpu devices can cause the same problem [1][2], and commit
faefba95c9e8ca3a ("drm/amdgpu: just suspend the hw on pci shutdown")
can resolve it.

As Tiezhu said, this occasionally shutdown or reboot failure is due to
clear PCI_COMMAND_MASTER on the device in do_pci_disable_device() [3].

static void do_pci_disable_device(struct pci_dev *dev)
{
        u16 pci_command;

        pci_read_config_word(dev, PCI_COMMAND, &pci_command);
        if (pci_command & PCI_COMMAND_MASTER) {
                pci_command &= ~PCI_COMMAND_MASTER;
                pci_write_config_word(dev, PCI_COMMAND, pci_command);
        }

        pcibios_disable_device(dev);
}

When remove "pci_command &= ~PCI_COMMAND_MASTER;", it can work well when
shutdown or reboot. The root cause on Loongson platform is that CPU is
still writing data to framebuffer while poweroff/reboot, and if we clear
Bus Master Bit at this time, CPU will wait ack from device, but never
return, so a hardware deadlock happens.

Radeon driver is more difficult than amdgpu due to its confusing symbol
names, and I have maintained an out-of-tree patch for a long time [4].
Recently, we found more and more devices can cause the same problem, and
it is very difficult to modify all problematic drivers as radeon/amdgpu
does (the .shutdown callback should make sure there is no DMA activity).
So, I think modify the PCIe port driver is a simple and effective way.
Because there is no poweroff/reboot problems before cc27b735ad3a75574a6a
("PCI/portdrv: Turn off PCIe services during shutdown"). And as early
discussed, kexec can still work fine after this patch [5].

[1] https://bugs.freedesktop.org/show_bug.cgi?id=97980
[2] https://bugs.freedesktop.org/show_bug.cgi?id=98638
[3] https://lore.kernel.org/patchwork/patch/1305067/
[4] https://github.com/chenhuacai/linux/commit/8da06f9b669831829416a3e9f4d1c57f217a42f0
[5] http://patchwork.ozlabs.org/project/linux-pci/patch/1600680138-10949-1-git-send-email-chenhc@lemote.com/

Cc: Sinan Kaya <okaya@kernel.org>
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 drivers/pci/pcie/portdrv.h      |  2 +-
 drivers/pci/pcie/portdrv_core.c |  6 ++++--
 drivers/pci/pcie/portdrv_pci.c  | 15 +++++++++++++--
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index 2ff5724b8f13..358d7281f6e8 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -117,7 +117,7 @@ int pcie_port_device_resume(struct device *dev);
 int pcie_port_device_runtime_suspend(struct device *dev);
 int pcie_port_device_runtime_resume(struct device *dev);
 #endif
-void pcie_port_device_remove(struct pci_dev *dev);
+void pcie_port_device_remove(struct pci_dev *dev, bool disable);
 int __must_check pcie_port_bus_register(void);
 void pcie_port_bus_unregister(void);
 
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index e1fed6649c41..98c0a99a41d6 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -484,11 +484,13 @@ EXPORT_SYMBOL_GPL(pcie_port_find_device);
  * Remove PCI Express port service devices associated with given port and
  * disable MSI-X or MSI for the port.
  */
-void pcie_port_device_remove(struct pci_dev *dev)
+void pcie_port_device_remove(struct pci_dev *dev, bool disable)
 {
 	device_for_each_child(&dev->dev, NULL, remove_iter);
 	pci_free_irq_vectors(dev);
-	pci_disable_device(dev);
+
+	if (disable)
+		pci_disable_device(dev);
 }
 
 /**
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index c7ff1eea225a..562fbf3c1ea9 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -147,7 +147,18 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
 		pm_runtime_dont_use_autosuspend(&dev->dev);
 	}
 
-	pcie_port_device_remove(dev);
+	pcie_port_device_remove(dev, true);
+}
+
+static void pcie_portdrv_shutdown(struct pci_dev *dev)
+{
+	if (pci_bridge_d3_possible(dev)) {
+		pm_runtime_forbid(&dev->dev);
+		pm_runtime_get_noresume(&dev->dev);
+		pm_runtime_dont_use_autosuspend(&dev->dev);
+	}
+
+	pcie_port_device_remove(dev, false);
 }
 
 static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev,
@@ -219,7 +230,7 @@ static struct pci_driver pcie_portdriver = {
 
 	.probe		= pcie_portdrv_probe,
 	.remove		= pcie_portdrv_remove,
-	.shutdown	= pcie_portdrv_remove,
+	.shutdown	= pcie_portdrv_shutdown,
 
 	.err_handler	= &pcie_portdrv_err_handler,
 
-- 
2.27.0

[PATCH V3 2/4] PCI: Move loongson pci quirks to quirks.c

[Huacai Chen]

Loongson PCH (LS7A chipset) will be used by both MIPS-based and
LoongArch-based Loongson processors. MIPS-based Loongson uses FDT
but LoongArch-base Loongson uses ACPI, but the driver in drivers/
pci/controller/pci-loongson.c is FDT-only. So move the quirks to
quirks.c where can be shared by all architectures.

LoongArch is a new RISC ISA, mainline support will come soon, and
documentations are here (in translation):

https://github.com/loongson/LoongArch-Documentation

Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/pci/controller/pci-loongson.c | 69 ---------------------------
 drivers/pci/quirks.c                  | 69 +++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 69 deletions(-)

diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c
index 48169b1e3817..88066e9db69e 100644
--- a/drivers/pci/controller/pci-loongson.c
+++ b/drivers/pci/controller/pci-loongson.c
@@ -12,15 +12,6 @@
 
 #include "../pci.h"
 
-/* Device IDs */
-#define DEV_PCIE_PORT_0	0x7a09
-#define DEV_PCIE_PORT_1	0x7a19
-#define DEV_PCIE_PORT_2	0x7a29
-
-#define DEV_LS2K_APB	0x7a02
-#define DEV_LS7A_CONF	0x7a10
-#define DEV_LS7A_LPC	0x7a0c
-
 #define FLAG_CFG0	BIT(0)
 #define FLAG_CFG1	BIT(1)
 #define FLAG_DEV_FIX	BIT(2)
@@ -32,66 +23,6 @@ struct loongson_pci {
 	u32 flags;
 };
 
-/* Fixup wrong class code in PCIe bridges */
-static void bridge_class_quirk(struct pci_dev *dev)
-{
-	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
-}
-DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
-			DEV_PCIE_PORT_0, bridge_class_quirk);
-DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
-			DEV_PCIE_PORT_1, bridge_class_quirk);
-DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
-			DEV_PCIE_PORT_2, bridge_class_quirk);
-
-static void system_bus_quirk(struct pci_dev *pdev)
-{
-	/*
-	 * The address space consumed by these devices is outside the
-	 * resources of the host bridge.
-	 */
-	pdev->mmio_always_on = 1;
-	pdev->non_compliant_bars = 1;
-}
-DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
-			DEV_LS2K_APB, system_bus_quirk);
-DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
-			DEV_LS7A_CONF, system_bus_quirk);
-DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
-			DEV_LS7A_LPC, system_bus_quirk);
-
-static void loongson_mrrs_quirk(struct pci_dev *dev)
-{
-	struct pci_bus *bus = dev->bus;
-	struct pci_dev *bridge;
-	static const struct pci_device_id bridge_devids[] = {
-		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
-		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
-		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
-		{ 0, },
-	};
-
-	/* look for the matching bridge */
-	while (!pci_is_root_bus(bus)) {
-		bridge = bus->self;
-		bus = bus->parent;
-		/*
-		 * Some Loongson PCIe ports have a h/w limitation of
-		 * 256 bytes maximum read request size. They can't handle
-		 * anything larger than this. So force this limit on
-		 * any devices attached under these ports.
-		 */
-		if (pci_match_id(bridge_devids, bridge)) {
-			if (pcie_get_readrq(dev) > 256) {
-				pci_info(dev, "limiting MRRS to 256\n");
-				pcie_set_readrq(dev, 256);
-			}
-			break;
-		}
-	}
-}
-DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk);
-
 static void __iomem *cfg1_map(struct loongson_pci *priv, int bus,
 				unsigned int devfn, int where)
 {
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 22b2bb1109c9..dee4798a49fc 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -205,6 +205,75 @@ static void quirk_mmio_always_on(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
 				PCI_CLASS_BRIDGE_HOST, 8, quirk_mmio_always_on);
 
+/* Loongson-related quirks */
+#define DEV_PCIE_PORT_0	0x7a09
+#define DEV_PCIE_PORT_1	0x7a19
+#define DEV_PCIE_PORT_2	0x7a29
+
+#define DEV_LS2K_APB	0x7a02
+#define DEV_LS7A_CONF	0x7a10
+#define DEV_LS7A_LPC	0x7a0c
+
+/* Fixup wrong class code in PCIe bridges */
+static void loongson_bridge_class_quirk(struct pci_dev *dev)
+{
+	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
+			DEV_PCIE_PORT_0, loongson_bridge_class_quirk);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
+			DEV_PCIE_PORT_1, loongson_bridge_class_quirk);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
+			DEV_PCIE_PORT_2, loongson_bridge_class_quirk);
+
+static void loongson_system_bus_quirk(struct pci_dev *pdev)
+{
+	/*
+	 * The address space consumed by these devices is outside the
+	 * resources of the host bridge.
+	 */
+	pdev->mmio_always_on = 1;
+	pdev->non_compliant_bars = 1;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
+			DEV_LS2K_APB, loongson_system_bus_quirk);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
+			DEV_LS7A_CONF, loongson_system_bus_quirk);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
+			DEV_LS7A_LPC, loongson_system_bus_quirk);
+
+static void loongson_mrrs_quirk(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	struct pci_dev *bridge;
+	static const struct pci_device_id bridge_devids[] = {
+		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
+		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
+		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
+		{ 0, },
+	};
+
+	/* look for the matching bridge */
+	while (!pci_is_root_bus(bus)) {
+		bridge = bus->self;
+		bus = bus->parent;
+		/*
+		 * Some Loongson PCIe ports have a h/w limitation of
+		 * 256 bytes maximum read request size. They can't handle
+		 * anything larger than this. So force this limit on
+		 * any devices attached under these ports.
+		 */
+		if (pci_match_id(bridge_devids, bridge)) {
+			if (pcie_get_readrq(dev) > 256) {
+				pci_info(dev, "limiting MRRS to 256\n");
+				pcie_set_readrq(dev, 256);
+			}
+			break;
+		}
+	}
+}
+DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk);
+
 /*
  * The Mellanox Tavor device gives false positive parity errors.  Disable
  * parity error reporting.
-- 
2.27.0

[PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Huacai Chen]

In new revision of LS7A, some PCIe ports support larger value than 256,
but their maximum supported MRRS values are not detectable. Moreover,
the current loongson_mrrs_quirk() cannot avoid devices increasing its
MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
will actually set a big value in its driver. So the only possible way is
configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.

However, according to PCIe Spec, it is legal for an OS to program any
value for MRRS, and it is also legal for an endpoint to generate a Read
Request with any size up to its MRRS. As the hardware engineers says,
the root cause here is LS7A doesn't break up large read requests (Yes,
that is a problem in the LS7A design).

Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/pci/pci.c    | 5 +++++
 drivers/pci/quirks.c | 8 +++++++-
 include/linux/pci.h  | 2 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b717680377a9..6f0d2f5b6f30 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5802,6 +5802,11 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
 
 	v = (ffs(rq) - 8) << 12;
 
+	if (dev->dev_flags & PCI_DEV_FLAGS_NO_INCREASE_MRRS) {
+		if (rq > pcie_get_readrq(dev))
+			return -EINVAL;
+	}
+
 	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
 						  PCI_EXP_DEVCTL_READRQ, v);
 
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index dee4798a49fc..8284480dc7e4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -263,7 +263,13 @@ static void loongson_mrrs_quirk(struct pci_dev *dev)
 		 * anything larger than this. So force this limit on
 		 * any devices attached under these ports.
 		 */
-		if (pci_match_id(bridge_devids, bridge)) {
+		if (bridge && pci_match_id(bridge_devids, bridge)) {
+			dev->dev_flags |= PCI_DEV_FLAGS_NO_INCREASE_MRRS;
+
+			if (pcie_bus_config == PCIE_BUS_DEFAULT ||
+			    pcie_bus_config == PCIE_BUS_TUNE_OFF)
+				break;
+
 			if (pcie_get_readrq(dev) > 256) {
 				pci_info(dev, "limiting MRRS to 256\n");
 				pcie_set_readrq(dev, 256);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 24306504226a..5e0ec3e4318b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -227,6 +227,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
 	/* Don't use Relaxed Ordering for TLPs directed at this device */
 	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
+	/* Don't increase BIOS's MRRS configuration */
+	PCI_DEV_FLAGS_NO_INCREASE_MRRS = (__force pci_dev_flags_t) (1 << 12),
 };
 
 enum pci_irq_reroute_variant {
-- 
2.27.0

[PATCH V3 4/4] PCI: Add quirk for multifunction devices of LS7A

[Huacai Chen]

From: Jianmin Lv <lvjianmin@loongson.cn>

In LS7A, multifunction device use same PCI PIN (because the PIN register
report the same INTx value to each function) but we need different IRQ
for different functions, so add a quirk to fix it for standard PCI PIN
usage.

This patch only affect ACPI based systems (and only needed by ACPI based
systems, too). For DT based systems, the irq mappings is defined in .dts
files and be handled by of_irq_parse_pci().

Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/pci/quirks.c    | 14 ++++++++++++++
 include/linux/pci_ids.h | 10 ++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 8284480dc7e4..bf3002cff64c 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -242,6 +242,20 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
 			DEV_LS7A_LPC, loongson_system_bus_quirk);
 
+static void loongson_pci_pin_quirk(struct pci_dev *dev)
+{
+	dev->pin = 1 + (PCI_FUNC(dev->devfn) & 3);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_PCIE_PORT_0, loongson_pci_pin_quirk);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_PCIE_PORT_1, loongson_pci_pin_quirk);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_PCIE_PORT_2, loongson_pci_pin_quirk);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_AHCI, loongson_pci_pin_quirk);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_EHCI, loongson_pci_pin_quirk);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_OHCI, loongson_pci_pin_quirk);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_DC, loongson_pci_pin_quirk);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_GPU, loongson_pci_pin_quirk);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_GMAC, loongson_pci_pin_quirk);
+
 static void loongson_mrrs_quirk(struct pci_dev *dev)
 {
 	struct pci_bus *bus = dev->bus;
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 4c3fa5293d76..dc024ab21d91 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -151,6 +151,16 @@
 /* Vendors and devices.  Sort key: vendor first, device next. */
 
 #define PCI_VENDOR_ID_LOONGSON		0x0014
+#define PCI_DEVICE_ID_LOONGSON_APB      0x7a02
+#define PCI_DEVICE_ID_LOONGSON_GMAC     0x7a03
+#define PCI_DEVICE_ID_LOONGSON_DC       0x7a06
+#define PCI_DEVICE_ID_LOONGSON_HDA      0x7a07
+#define PCI_DEVICE_ID_LOONGSON_GPU      0x7a15
+#define PCI_DEVICE_ID_LOONGSON_AHCI     0x7a08
+#define PCI_DEVICE_ID_LOONGSON_EHCI     0x7a14
+#define PCI_DEVICE_ID_LOONGSON_OHCI     0x7a24
+#define PCI_DEVICE_ID_LOONGSON_LPC      0x7a0c
+#define PCI_DEVICE_ID_LOONGSON_DMA      0x7a0f
 
 #define PCI_VENDOR_ID_TTTECH		0x0357
 #define PCI_DEVICE_ID_TTTECH_MC322	0x000a
-- 
2.27.0

Re: [PATCH V3 1/4] PCI/portdrv: Don't disable device during shutdown

[Bjorn Helgaas]

On Fri, Jun 25, 2021 at 05:30:27PM +0800, Huacai Chen wrote:
> Use separate remove()/shutdown() callback, and don't disable PCI device
> during shutdown. This can avoid some poweroff/reboot failures.
> 
> The poweroff/reboot failures could easily be reproduced on Loongson
> platforms. I think this is not a Loongson-specific problem, instead, is
> a problem related to some specific PCI hosts. On some x86 platforms,
> radeon/amdgpu devices can cause the same problem [1][2], and commit
> faefba95c9e8ca3a ("drm/amdgpu: just suspend the hw on pci shutdown")
> can resolve it.
> 
> As Tiezhu said, this occasionally shutdown or reboot failure is due to
> clear PCI_COMMAND_MASTER on the device in do_pci_disable_device() [3].
> 
> static void do_pci_disable_device(struct pci_dev *dev)
> {
>         u16 pci_command;
> 
>         pci_read_config_word(dev, PCI_COMMAND, &pci_command);
>         if (pci_command & PCI_COMMAND_MASTER) {
>                 pci_command &= ~PCI_COMMAND_MASTER;
>                 pci_write_config_word(dev, PCI_COMMAND, pci_command);
>         }
> 
>         pcibios_disable_device(dev);
> }
> 
> When remove "pci_command &= ~PCI_COMMAND_MASTER;", it can work well when
> shutdown or reboot. The root cause on Loongson platform is that CPU is
> still writing data to framebuffer while poweroff/reboot, and if we clear
> Bus Master Bit at this time, CPU will wait ack from device, but never
> return, so a hardware deadlock happens.

Doesn't make sense yet.  Bus Master enables the *device* to do DMA.  A
CPU can do MMIO to a device, e.g., to write data to a framebuffer,
regardless of the state of Bus Master Enable.  Also, those MMIO writes
done by a CPU are Memory Write transactions on PCIe, which are
"Posted" Requests, which means they do not receive acks.  So this
cannot be the root cause.

Bjorn

Re: [PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Bjorn Helgaas]

On Fri, Jun 25, 2021 at 05:30:29PM +0800, Huacai Chen wrote:
> In new revision of LS7A, some PCIe ports support larger value than 256,
> but their maximum supported MRRS values are not detectable. Moreover,
> the current loongson_mrrs_quirk() cannot avoid devices increasing its
> MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> will actually set a big value in its driver. So the only possible way is
> configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
> PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.
> 
> However, according to PCIe Spec, it is legal for an OS to program any
> value for MRRS, and it is also legal for an endpoint to generate a Read
> Request with any size up to its MRRS. As the hardware engineers says,
> the root cause here is LS7A doesn't break up large read requests (Yes,
> that is a problem in the LS7A design).

"LS7A doesn't break up large read requests" claims to be a root cause,
but you haven't yet said what the actual *problem* is.

Is the problem that an endpoint reports a malformed TLP because it
received a completion bigger than it can handle?  Is it that the LS7A
root port reports some kind of error if it receives a Memory Read
request with a size that's "too big"?  Maybe the LS7A doesn't know
what to do when it receives a Memory Read request with MRRS > MPS?
What exactly happens when the problem occurs?

MRRS applies only to the read request.  It is not directly related to
the size of the completions that carry the data back to the device
(except that obviously you shouldn't get a completion larger than the
read you requested).

The setting that directly controls the size of completions is MPS
(Max_Payload_Size).  One reason to break up read requests is because
the endpoint's buffers can't accommodate big TLPs.  One way to deal
with that is to set MPS in the hierarchy to a smaller value.  Then the
root port must ensure that no TLP exceeds the MPS size, regardless of
what the MRRS in the read request was.

For example, if the endpoint's MRRS=4096 and the hierarchy's MPS=128,
it's up to the root port to break up completions into 128-byte chunks.

It's also possible to set the endpoint's MRRS=128, which means reads
to main memory will never receive completions larger than 128 bytes.
But it does NOT guarantee that a peer-to-peer DMA from another device
will be limited to 128 bytes.  The other device is allowed to generate
Memory Write TLPs with payloads up to its MPS size, and MRRS is not
involved at all.

It's not clear yet whether the LS7A problem is with MRRS, with MPS, or
with some combination.  It's important to understand exactly what is
broken here so the quirk doesn't get in the way of future changes to
the generic MRRS and MPS configuration.

Here's a good overview:

  https://www.xilinx.com/support/documentation/white_papers/wp350.pdf

> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
>  drivers/pci/pci.c    | 5 +++++
>  drivers/pci/quirks.c | 8 +++++++-
>  include/linux/pci.h  | 2 ++
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b717680377a9..6f0d2f5b6f30 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5802,6 +5802,11 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>  
>  	v = (ffs(rq) - 8) << 12;
>  
> +	if (dev->dev_flags & PCI_DEV_FLAGS_NO_INCREASE_MRRS) {
> +		if (rq > pcie_get_readrq(dev))
> +			return -EINVAL;
> +	}
> +
>  	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
>  						  PCI_EXP_DEVCTL_READRQ, v);
>  
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index dee4798a49fc..8284480dc7e4 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -263,7 +263,13 @@ static void loongson_mrrs_quirk(struct pci_dev *dev)
>  		 * anything larger than this. So force this limit on
>  		 * any devices attached under these ports.
>  		 */
> -		if (pci_match_id(bridge_devids, bridge)) {
> +		if (bridge && pci_match_id(bridge_devids, bridge)) {
> +			dev->dev_flags |= PCI_DEV_FLAGS_NO_INCREASE_MRRS;
> +
> +			if (pcie_bus_config == PCIE_BUS_DEFAULT ||
> +			    pcie_bus_config == PCIE_BUS_TUNE_OFF)
> +				break;
> +
>  			if (pcie_get_readrq(dev) > 256) {
>  				pci_info(dev, "limiting MRRS to 256\n");
>  				pcie_set_readrq(dev, 256);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 24306504226a..5e0ec3e4318b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -227,6 +227,8 @@ enum pci_dev_flags {
>  	PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
>  	/* Don't use Relaxed Ordering for TLPs directed at this device */
>  	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
> +	/* Don't increase BIOS's MRRS configuration */
> +	PCI_DEV_FLAGS_NO_INCREASE_MRRS = (__force pci_dev_flags_t) (1 << 12),
>  };
>  
>  enum pci_irq_reroute_variant {
> -- 
> 2.27.0
> 

Re: [PATCH V3 2/4] PCI: Move loongson pci quirks to quirks.c

[Bjorn Helgaas]

On Fri, Jun 25, 2021 at 05:30:28PM +0800, Huacai Chen wrote:
> Loongson PCH (LS7A chipset) will be used by both MIPS-based and
> LoongArch-based Loongson processors. MIPS-based Loongson uses FDT
> but LoongArch-base Loongson uses ACPI, but the driver in drivers/
> pci/controller/pci-loongson.c is FDT-only. So move the quirks to
> quirks.c where can be shared by all architectures.
> 
> LoongArch is a new RISC ISA, mainline support will come soon, and
> documentations are here (in translation):
> 
> https://github.com/loongson/LoongArch-Documentation
> 
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
>  drivers/pci/controller/pci-loongson.c | 69 ---------------------------
>  drivers/pci/quirks.c                  | 69 +++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c
> index 48169b1e3817..88066e9db69e 100644
> --- a/drivers/pci/controller/pci-loongson.c
> +++ b/drivers/pci/controller/pci-loongson.c
> @@ -12,15 +12,6 @@
>  
>  #include "../pci.h"
>  
> -/* Device IDs */
> -#define DEV_PCIE_PORT_0	0x7a09
> -#define DEV_PCIE_PORT_1	0x7a19
> -#define DEV_PCIE_PORT_2	0x7a29
> -
> -#define DEV_LS2K_APB	0x7a02
> -#define DEV_LS7A_CONF	0x7a10
> -#define DEV_LS7A_LPC	0x7a0c
> -
>  #define FLAG_CFG0	BIT(0)
>  #define FLAG_CFG1	BIT(1)
>  #define FLAG_DEV_FIX	BIT(2)
> @@ -32,66 +23,6 @@ struct loongson_pci {
>  	u32 flags;
>  };
>  
> -/* Fixup wrong class code in PCIe bridges */
> -static void bridge_class_quirk(struct pci_dev *dev)
> -{
> -	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> -}
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> -			DEV_PCIE_PORT_0, bridge_class_quirk);
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> -			DEV_PCIE_PORT_1, bridge_class_quirk);
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> -			DEV_PCIE_PORT_2, bridge_class_quirk);
> -
> -static void system_bus_quirk(struct pci_dev *pdev)
> -{
> -	/*
> -	 * The address space consumed by these devices is outside the
> -	 * resources of the host bridge.
> -	 */
> -	pdev->mmio_always_on = 1;
> -	pdev->non_compliant_bars = 1;
> -}
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> -			DEV_LS2K_APB, system_bus_quirk);
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> -			DEV_LS7A_CONF, system_bus_quirk);
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> -			DEV_LS7A_LPC, system_bus_quirk);
> -
> -static void loongson_mrrs_quirk(struct pci_dev *dev)
> -{
> -	struct pci_bus *bus = dev->bus;
> -	struct pci_dev *bridge;
> -	static const struct pci_device_id bridge_devids[] = {
> -		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
> -		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
> -		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
> -		{ 0, },
> -	};
> -
> -	/* look for the matching bridge */
> -	while (!pci_is_root_bus(bus)) {
> -		bridge = bus->self;
> -		bus = bus->parent;
> -		/*
> -		 * Some Loongson PCIe ports have a h/w limitation of
> -		 * 256 bytes maximum read request size. They can't handle
> -		 * anything larger than this. So force this limit on
> -		 * any devices attached under these ports.
> -		 */
> -		if (pci_match_id(bridge_devids, bridge)) {
> -			if (pcie_get_readrq(dev) > 256) {
> -				pci_info(dev, "limiting MRRS to 256\n");
> -				pcie_set_readrq(dev, 256);
> -			}
> -			break;
> -		}
> -	}
> -}
> -DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk);
> -
>  static void __iomem *cfg1_map(struct loongson_pci *priv, int bus,
>  				unsigned int devfn, int where)
>  {
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 22b2bb1109c9..dee4798a49fc 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -205,6 +205,75 @@ static void quirk_mmio_always_on(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
>  				PCI_CLASS_BRIDGE_HOST, 8, quirk_mmio_always_on);
>  
> +/* Loongson-related quirks */
> +#define DEV_PCIE_PORT_0	0x7a09
> +#define DEV_PCIE_PORT_1	0x7a19
> +#define DEV_PCIE_PORT_2	0x7a29
> +
> +#define DEV_LS2K_APB	0x7a02
> +#define DEV_LS7A_CONF	0x7a10
> +#define DEV_LS7A_LPC	0x7a0c
> +
> +/* Fixup wrong class code in PCIe bridges */
> +static void loongson_bridge_class_quirk(struct pci_dev *dev)
> +{
> +	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> +			DEV_PCIE_PORT_0, loongson_bridge_class_quirk);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> +			DEV_PCIE_PORT_1, loongson_bridge_class_quirk);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> +			DEV_PCIE_PORT_2, loongson_bridge_class_quirk);
> +
> +static void loongson_system_bus_quirk(struct pci_dev *pdev)
> +{
> +	/*
> +	 * The address space consumed by these devices is outside the
> +	 * resources of the host bridge.
> +	 */
> +	pdev->mmio_always_on = 1;
> +	pdev->non_compliant_bars = 1;
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> +			DEV_LS2K_APB, loongson_system_bus_quirk);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> +			DEV_LS7A_CONF, loongson_system_bus_quirk);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> +			DEV_LS7A_LPC, loongson_system_bus_quirk);
> +
> +static void loongson_mrrs_quirk(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus = dev->bus;
> +	struct pci_dev *bridge;
> +	static const struct pci_device_id bridge_devids[] = {
> +		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
> +		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
> +		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
> +		{ 0, },
> +	};
> +
> +	/* look for the matching bridge */
> +	while (!pci_is_root_bus(bus)) {
> +		bridge = bus->self;
> +		bus = bus->parent;
> +		/*
> +		 * Some Loongson PCIe ports have a h/w limitation of
> +		 * 256 bytes maximum read request size. They can't handle
> +		 * anything larger than this. So force this limit on
> +		 * any devices attached under these ports.
> +		 */
> +		if (pci_match_id(bridge_devids, bridge)) {
> +			if (pcie_get_readrq(dev) > 256) {
> +				pci_info(dev, "limiting MRRS to 256\n");
> +				pcie_set_readrq(dev, 256);
> +			}
> +			break;
> +		}
> +	}
> +}
> +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk);

I don't want to run this quirk on *every* PCI device on *every* system
in the world when the defect is not even in these devices at all.  The
defect is apparently in some Loongson hardware, so the quirk should
somehow be limited to that.

>  /*
>   * The Mellanox Tavor device gives false positive parity errors.  Disable
>   * parity error reporting.
> -- 
> 2.27.0
> 

Re: [PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Bjorn Helgaas]

On Fri, Jun 25, 2021 at 05:22:04PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 25, 2021 at 05:30:29PM +0800, Huacai Chen wrote:
> > In new revision of LS7A, some PCIe ports support larger value than 256,
> > but their maximum supported MRRS values are not detectable. Moreover,
> > the current loongson_mrrs_quirk() cannot avoid devices increasing its
> > MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> > will actually set a big value in its driver. So the only possible way is
> > configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
> > PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.
> > 
> > However, according to PCIe Spec, it is legal for an OS to program any
> > value for MRRS, and it is also legal for an endpoint to generate a Read
> > Request with any size up to its MRRS. As the hardware engineers says,
> > the root cause here is LS7A doesn't break up large read requests (Yes,
> > that is a problem in the LS7A design).
> 
> "LS7A doesn't break up large read requests" claims to be a root cause,
> but you haven't yet said what the actual *problem* is.
> 
> Is the problem that an endpoint reports a malformed TLP because it
> received a completion bigger than it can handle?  Is it that the LS7A
> root port reports some kind of error if it receives a Memory Read
> request with a size that's "too big"?  Maybe the LS7A doesn't know
> what to do when it receives a Memory Read request with MRRS > MPS?
> What exactly happens when the problem occurs?
> 
> MRRS applies only to the read request.  It is not directly related to
> the size of the completions that carry the data back to the device
> (except that obviously you shouldn't get a completion larger than the
> read you requested).
> 
> The setting that directly controls the size of completions is MPS
> (Max_Payload_Size).  One reason to break up read requests is because
> the endpoint's buffers can't accommodate big TLPs.  One way to deal
> with that is to set MPS in the hierarchy to a smaller value.  Then the
> root port must ensure that no TLP exceeds the MPS size, regardless of
> what the MRRS in the read request was.
> 
> For example, if the endpoint's MRRS=4096 and the hierarchy's MPS=128,
> it's up to the root port to break up completions into 128-byte chunks.
> 
> It's also possible to set the endpoint's MRRS=128, which means reads
> to main memory will never receive completions larger than 128 bytes.
> But it does NOT guarantee that a peer-to-peer DMA from another device
> will be limited to 128 bytes.  The other device is allowed to generate
> Memory Write TLPs with payloads up to its MPS size, and MRRS is not
> involved at all.
> 
> It's not clear yet whether the LS7A problem is with MRRS, with MPS, or
> with some combination.  It's important to understand exactly what is
> broken here so the quirk doesn't get in the way of future changes to
> the generic MRRS and MPS configuration.
> 
> Here's a good overview:
> 
>   https://www.xilinx.com/support/documentation/white_papers/wp350.pdf
> 
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > ---
> >  drivers/pci/pci.c    | 5 +++++
> >  drivers/pci/quirks.c | 8 +++++++-
> >  include/linux/pci.h  | 2 ++
> >  3 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b717680377a9..6f0d2f5b6f30 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5802,6 +5802,11 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> >  
> >  	v = (ffs(rq) - 8) << 12;
> >  
> > +	if (dev->dev_flags & PCI_DEV_FLAGS_NO_INCREASE_MRRS) {
> > +		if (rq > pcie_get_readrq(dev))
> > +			return -EINVAL;
> > +	}
> > +
> >  	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
> >  						  PCI_EXP_DEVCTL_READRQ, v);
> >  
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index dee4798a49fc..8284480dc7e4 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -263,7 +263,13 @@ static void loongson_mrrs_quirk(struct pci_dev *dev)
> >  		 * anything larger than this. So force this limit on
> >  		 * any devices attached under these ports.
> >  		 */
> > -		if (pci_match_id(bridge_devids, bridge)) {
> > +		if (bridge && pci_match_id(bridge_devids, bridge)) {
> > +			dev->dev_flags |= PCI_DEV_FLAGS_NO_INCREASE_MRRS;
> > +
> > +			if (pcie_bus_config == PCIE_BUS_DEFAULT ||
> > +			    pcie_bus_config == PCIE_BUS_TUNE_OFF)
> > +				break;

Another approach might be to make a quirk that prevents Linux from
touching MPS and MRRS at all under any circumstances.

You'd have to do this without reference to pcie_bus_config so future
MPS/MRRS algorithm changes wouldn't be affected.  And the quirk bit
would have to be in struct pci_host_bridge, similar to no_ext_tags.

> >  			if (pcie_get_readrq(dev) > 256) {
> >  				pci_info(dev, "limiting MRRS to 256\n");
> >  				pcie_set_readrq(dev, 256);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 24306504226a..5e0ec3e4318b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -227,6 +227,8 @@ enum pci_dev_flags {
> >  	PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
> >  	/* Don't use Relaxed Ordering for TLPs directed at this device */
> >  	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
> > +	/* Don't increase BIOS's MRRS configuration */
> > +	PCI_DEV_FLAGS_NO_INCREASE_MRRS = (__force pci_dev_flags_t) (1 << 12),
> >  };
> >  
> >  enum pci_irq_reroute_variant {
> > -- 
> > 2.27.0
> > 

Re: [PATCH V3 2/4] PCI: Move loongson pci quirks to quirks.c

[Huacai Chen]

Hi, Bjorn,

On Sat, Jun 26, 2021 at 11:39 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jun 25, 2021 at 05:30:28PM +0800, Huacai Chen wrote:
> > Loongson PCH (LS7A chipset) will be used by both MIPS-based and
> > LoongArch-based Loongson processors. MIPS-based Loongson uses FDT
> > but LoongArch-base Loongson uses ACPI, but the driver in drivers/
> > pci/controller/pci-loongson.c is FDT-only. So move the quirks to
> > quirks.c where can be shared by all architectures.
> >
> > LoongArch is a new RISC ISA, mainline support will come soon, and
> > documentations are here (in translation):
> >
> > https://github.com/loongson/LoongArch-Documentation
> >
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > ---
> >  drivers/pci/controller/pci-loongson.c | 69 ---------------------------
> >  drivers/pci/quirks.c                  | 69 +++++++++++++++++++++++++++
> >  2 files changed, 69 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c
> > index 48169b1e3817..88066e9db69e 100644
> > --- a/drivers/pci/controller/pci-loongson.c
> > +++ b/drivers/pci/controller/pci-loongson.c
> > @@ -12,15 +12,6 @@
> >
> >  #include "../pci.h"
> >
> > -/* Device IDs */
> > -#define DEV_PCIE_PORT_0      0x7a09
> > -#define DEV_PCIE_PORT_1      0x7a19
> > -#define DEV_PCIE_PORT_2      0x7a29
> > -
> > -#define DEV_LS2K_APB 0x7a02
> > -#define DEV_LS7A_CONF        0x7a10
> > -#define DEV_LS7A_LPC 0x7a0c
> > -
> >  #define FLAG_CFG0    BIT(0)
> >  #define FLAG_CFG1    BIT(1)
> >  #define FLAG_DEV_FIX BIT(2)
> > @@ -32,66 +23,6 @@ struct loongson_pci {
> >       u32 flags;
> >  };
> >
> > -/* Fixup wrong class code in PCIe bridges */
> > -static void bridge_class_quirk(struct pci_dev *dev)
> > -{
> > -     dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> > -}
> > -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > -                     DEV_PCIE_PORT_0, bridge_class_quirk);
> > -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > -                     DEV_PCIE_PORT_1, bridge_class_quirk);
> > -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > -                     DEV_PCIE_PORT_2, bridge_class_quirk);
> > -
> > -static void system_bus_quirk(struct pci_dev *pdev)
> > -{
> > -     /*
> > -      * The address space consumed by these devices is outside the
> > -      * resources of the host bridge.
> > -      */
> > -     pdev->mmio_always_on = 1;
> > -     pdev->non_compliant_bars = 1;
> > -}
> > -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > -                     DEV_LS2K_APB, system_bus_quirk);
> > -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > -                     DEV_LS7A_CONF, system_bus_quirk);
> > -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > -                     DEV_LS7A_LPC, system_bus_quirk);
> > -
> > -static void loongson_mrrs_quirk(struct pci_dev *dev)
> > -{
> > -     struct pci_bus *bus = dev->bus;
> > -     struct pci_dev *bridge;
> > -     static const struct pci_device_id bridge_devids[] = {
> > -             { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
> > -             { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
> > -             { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
> > -             { 0, },
> > -     };
> > -
> > -     /* look for the matching bridge */
> > -     while (!pci_is_root_bus(bus)) {
> > -             bridge = bus->self;
> > -             bus = bus->parent;
> > -             /*
> > -              * Some Loongson PCIe ports have a h/w limitation of
> > -              * 256 bytes maximum read request size. They can't handle
> > -              * anything larger than this. So force this limit on
> > -              * any devices attached under these ports.
> > -              */
> > -             if (pci_match_id(bridge_devids, bridge)) {
> > -                     if (pcie_get_readrq(dev) > 256) {
> > -                             pci_info(dev, "limiting MRRS to 256\n");
> > -                             pcie_set_readrq(dev, 256);
> > -                     }
> > -                     break;
> > -             }
> > -     }
> > -}
> > -DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk);
> > -
> >  static void __iomem *cfg1_map(struct loongson_pci *priv, int bus,
> >                               unsigned int devfn, int where)
> >  {
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 22b2bb1109c9..dee4798a49fc 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -205,6 +205,75 @@ static void quirk_mmio_always_on(struct pci_dev *dev)
> >  DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
> >                               PCI_CLASS_BRIDGE_HOST, 8, quirk_mmio_always_on);
> >
> > +/* Loongson-related quirks */
> > +#define DEV_PCIE_PORT_0      0x7a09
> > +#define DEV_PCIE_PORT_1      0x7a19
> > +#define DEV_PCIE_PORT_2      0x7a29
> > +
> > +#define DEV_LS2K_APB 0x7a02
> > +#define DEV_LS7A_CONF        0x7a10
> > +#define DEV_LS7A_LPC 0x7a0c
> > +
> > +/* Fixup wrong class code in PCIe bridges */
> > +static void loongson_bridge_class_quirk(struct pci_dev *dev)
> > +{
> > +     dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> > +}
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > +                     DEV_PCIE_PORT_0, loongson_bridge_class_quirk);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > +                     DEV_PCIE_PORT_1, loongson_bridge_class_quirk);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > +                     DEV_PCIE_PORT_2, loongson_bridge_class_quirk);
> > +
> > +static void loongson_system_bus_quirk(struct pci_dev *pdev)
> > +{
> > +     /*
> > +      * The address space consumed by these devices is outside the
> > +      * resources of the host bridge.
> > +      */
> > +     pdev->mmio_always_on = 1;
> > +     pdev->non_compliant_bars = 1;
> > +}
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > +                     DEV_LS2K_APB, loongson_system_bus_quirk);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > +                     DEV_LS7A_CONF, loongson_system_bus_quirk);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > +                     DEV_LS7A_LPC, loongson_system_bus_quirk);
> > +
> > +static void loongson_mrrs_quirk(struct pci_dev *dev)
> > +{
> > +     struct pci_bus *bus = dev->bus;
> > +     struct pci_dev *bridge;
> > +     static const struct pci_device_id bridge_devids[] = {
> > +             { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
> > +             { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
> > +             { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
> > +             { 0, },
> > +     };
> > +
> > +     /* look for the matching bridge */
> > +     while (!pci_is_root_bus(bus)) {
> > +             bridge = bus->self;
> > +             bus = bus->parent;
> > +             /*
> > +              * Some Loongson PCIe ports have a h/w limitation of
> > +              * 256 bytes maximum read request size. They can't handle
> > +              * anything larger than this. So force this limit on
> > +              * any devices attached under these ports.
> > +              */
> > +             if (pci_match_id(bridge_devids, bridge)) {
> > +                     if (pcie_get_readrq(dev) > 256) {
> > +                             pci_info(dev, "limiting MRRS to 256\n");
> > +                             pcie_set_readrq(dev, 256);
> > +                     }
> > +                     break;
> > +             }
> > +     }
> > +}
> > +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk);
>
> I don't want to run this quirk on *every* PCI device on *every* system
> in the world when the defect is not even in these devices at all.  The
> defect is apparently in some Loongson hardware, so the quirk should
> somehow be limited to that.
I'm very sorry that you have told me this problem twice but I forgot
it later, I will improve in the next version.

Huacai
>
> >  /*
> >   * The Mellanox Tavor device gives false positive parity errors.  Disable
> >   * parity error reporting.
> > --
> > 2.27.0
> >

Re: [PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Huacai Chen]

Hi, Bjorn,

On Sat, Jun 26, 2021 at 6:22 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jun 25, 2021 at 05:30:29PM +0800, Huacai Chen wrote:
> > In new revision of LS7A, some PCIe ports support larger value than 256,
> > but their maximum supported MRRS values are not detectable. Moreover,
> > the current loongson_mrrs_quirk() cannot avoid devices increasing its
> > MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> > will actually set a big value in its driver. So the only possible way is
> > configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
> > PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.
> >
> > However, according to PCIe Spec, it is legal for an OS to program any
> > value for MRRS, and it is also legal for an endpoint to generate a Read
> > Request with any size up to its MRRS. As the hardware engineers says,
> > the root cause here is LS7A doesn't break up large read requests (Yes,
> > that is a problem in the LS7A design).
>
> "LS7A doesn't break up large read requests" claims to be a root cause,
> but you haven't yet said what the actual *problem* is.
>
> Is the problem that an endpoint reports a malformed TLP because it
> received a completion bigger than it can handle?  Is it that the LS7A
> root port reports some kind of error if it receives a Memory Read
> request with a size that's "too big"?  Maybe the LS7A doesn't know
> what to do when it receives a Memory Read request with MRRS > MPS?
> What exactly happens when the problem occurs?
The hardware engineer said that the problem is: LS7A PCIe port reports
CA (Completer Abort) if it receives a Memory Read
request with a size that's "too big".

Huacai
>
> MRRS applies only to the read request.  It is not directly related to
> the size of the completions that carry the data back to the device
> (except that obviously you shouldn't get a completion larger than the
> read you requested).
>
> The setting that directly controls the size of completions is MPS
> (Max_Payload_Size).  One reason to break up read requests is because
> the endpoint's buffers can't accommodate big TLPs.  One way to deal
> with that is to set MPS in the hierarchy to a smaller value.  Then the
> root port must ensure that no TLP exceeds the MPS size, regardless of
> what the MRRS in the read request was.
>
> For example, if the endpoint's MRRS=4096 and the hierarchy's MPS=128,
> it's up to the root port to break up completions into 128-byte chunks.
>
> It's also possible to set the endpoint's MRRS=128, which means reads
> to main memory will never receive completions larger than 128 bytes.
> But it does NOT guarantee that a peer-to-peer DMA from another device
> will be limited to 128 bytes.  The other device is allowed to generate
> Memory Write TLPs with payloads up to its MPS size, and MRRS is not
> involved at all.
>
> It's not clear yet whether the LS7A problem is with MRRS, with MPS, or
> with some combination.  It's important to understand exactly what is
> broken here so the quirk doesn't get in the way of future changes to
> the generic MRRS and MPS configuration.
>
> Here's a good overview:
>
>   https://www.xilinx.com/support/documentation/white_papers/wp350.pdf
>
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > ---
> >  drivers/pci/pci.c    | 5 +++++
> >  drivers/pci/quirks.c | 8 +++++++-
> >  include/linux/pci.h  | 2 ++
> >  3 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b717680377a9..6f0d2f5b6f30 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5802,6 +5802,11 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> >
> >       v = (ffs(rq) - 8) << 12;
> >
> > +     if (dev->dev_flags & PCI_DEV_FLAGS_NO_INCREASE_MRRS) {
> > +             if (rq > pcie_get_readrq(dev))
> > +                     return -EINVAL;
> > +     }
> > +
> >       ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
> >                                                 PCI_EXP_DEVCTL_READRQ, v);
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index dee4798a49fc..8284480dc7e4 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -263,7 +263,13 @@ static void loongson_mrrs_quirk(struct pci_dev *dev)
> >                * anything larger than this. So force this limit on
> >                * any devices attached under these ports.
> >                */
> > -             if (pci_match_id(bridge_devids, bridge)) {
> > +             if (bridge && pci_match_id(bridge_devids, bridge)) {
> > +                     dev->dev_flags |= PCI_DEV_FLAGS_NO_INCREASE_MRRS;
> > +
> > +                     if (pcie_bus_config == PCIE_BUS_DEFAULT ||
> > +                         pcie_bus_config == PCIE_BUS_TUNE_OFF)
> > +                             break;
> > +
> >                       if (pcie_get_readrq(dev) > 256) {
> >                               pci_info(dev, "limiting MRRS to 256\n");
> >                               pcie_set_readrq(dev, 256);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 24306504226a..5e0ec3e4318b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -227,6 +227,8 @@ enum pci_dev_flags {
> >       PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
> >       /* Don't use Relaxed Ordering for TLPs directed at this device */
> >       PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
> > +     /* Don't increase BIOS's MRRS configuration */
> > +     PCI_DEV_FLAGS_NO_INCREASE_MRRS = (__force pci_dev_flags_t) (1 << 12),
> >  };
> >
> >  enum pci_irq_reroute_variant {
> > --
> > 2.27.0
> >

Re: [PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Huacai Chen]

Hi, Bjorn,

On Sat, Jun 26, 2021 at 11:48 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jun 25, 2021 at 05:22:04PM -0500, Bjorn Helgaas wrote:
> > On Fri, Jun 25, 2021 at 05:30:29PM +0800, Huacai Chen wrote:
> > > In new revision of LS7A, some PCIe ports support larger value than 256,
> > > but their maximum supported MRRS values are not detectable. Moreover,
> > > the current loongson_mrrs_quirk() cannot avoid devices increasing its
> > > MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> > > will actually set a big value in its driver. So the only possible way is
> > > configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
> > > PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.
> > >
> > > However, according to PCIe Spec, it is legal for an OS to program any
> > > value for MRRS, and it is also legal for an endpoint to generate a Read
> > > Request with any size up to its MRRS. As the hardware engineers says,
> > > the root cause here is LS7A doesn't break up large read requests (Yes,
> > > that is a problem in the LS7A design).
> >
> > "LS7A doesn't break up large read requests" claims to be a root cause,
> > but you haven't yet said what the actual *problem* is.
> >
> > Is the problem that an endpoint reports a malformed TLP because it
> > received a completion bigger than it can handle?  Is it that the LS7A
> > root port reports some kind of error if it receives a Memory Read
> > request with a size that's "too big"?  Maybe the LS7A doesn't know
> > what to do when it receives a Memory Read request with MRRS > MPS?
> > What exactly happens when the problem occurs?
> >
> > MRRS applies only to the read request.  It is not directly related to
> > the size of the completions that carry the data back to the device
> > (except that obviously you shouldn't get a completion larger than the
> > read you requested).
> >
> > The setting that directly controls the size of completions is MPS
> > (Max_Payload_Size).  One reason to break up read requests is because
> > the endpoint's buffers can't accommodate big TLPs.  One way to deal
> > with that is to set MPS in the hierarchy to a smaller value.  Then the
> > root port must ensure that no TLP exceeds the MPS size, regardless of
> > what the MRRS in the read request was.
> >
> > For example, if the endpoint's MRRS=4096 and the hierarchy's MPS=128,
> > it's up to the root port to break up completions into 128-byte chunks.
> >
> > It's also possible to set the endpoint's MRRS=128, which means reads
> > to main memory will never receive completions larger than 128 bytes.
> > But it does NOT guarantee that a peer-to-peer DMA from another device
> > will be limited to 128 bytes.  The other device is allowed to generate
> > Memory Write TLPs with payloads up to its MPS size, and MRRS is not
> > involved at all.
> >
> > It's not clear yet whether the LS7A problem is with MRRS, with MPS, or
> > with some combination.  It's important to understand exactly what is
> > broken here so the quirk doesn't get in the way of future changes to
> > the generic MRRS and MPS configuration.
> >
> > Here's a good overview:
> >
> >   https://www.xilinx.com/support/documentation/white_papers/wp350.pdf
> >
> > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > > ---
> > >  drivers/pci/pci.c    | 5 +++++
> > >  drivers/pci/quirks.c | 8 +++++++-
> > >  include/linux/pci.h  | 2 ++
> > >  3 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index b717680377a9..6f0d2f5b6f30 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5802,6 +5802,11 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> > >
> > >     v = (ffs(rq) - 8) << 12;
> > >
> > > +   if (dev->dev_flags & PCI_DEV_FLAGS_NO_INCREASE_MRRS) {
> > > +           if (rq > pcie_get_readrq(dev))
> > > +                   return -EINVAL;
> > > +   }
> > > +
> > >     ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
> > >                                               PCI_EXP_DEVCTL_READRQ, v);
> > >
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index dee4798a49fc..8284480dc7e4 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -263,7 +263,13 @@ static void loongson_mrrs_quirk(struct pci_dev *dev)
> > >              * anything larger than this. So force this limit on
> > >              * any devices attached under these ports.
> > >              */
> > > -           if (pci_match_id(bridge_devids, bridge)) {
> > > +           if (bridge && pci_match_id(bridge_devids, bridge)) {
> > > +                   dev->dev_flags |= PCI_DEV_FLAGS_NO_INCREASE_MRRS;
> > > +
> > > +                   if (pcie_bus_config == PCIE_BUS_DEFAULT ||
> > > +                       pcie_bus_config == PCIE_BUS_TUNE_OFF)
> > > +                           break;
>
> Another approach might be to make a quirk that prevents Linux from
> touching MPS and MRRS at all under any circumstances.
>
> You'd have to do this without reference to pcie_bus_config so future
> MPS/MRRS algorithm changes wouldn't be affected.  And the quirk bit
> would have to be in struct pci_host_bridge, similar to no_ext_tags.
I will change the quirk bit to be a bus flag, but still allow to decrease MRRS.

Huacai
>
> > >                     if (pcie_get_readrq(dev) > 256) {
> > >                             pci_info(dev, "limiting MRRS to 256\n");
> > >                             pcie_set_readrq(dev, 256);
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 24306504226a..5e0ec3e4318b 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -227,6 +227,8 @@ enum pci_dev_flags {
> > >     PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
> > >     /* Don't use Relaxed Ordering for TLPs directed at this device */
> > >     PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
> > > +   /* Don't increase BIOS's MRRS configuration */
> > > +   PCI_DEV_FLAGS_NO_INCREASE_MRRS = (__force pci_dev_flags_t) (1 << 12),
> > >  };
> > >
> > >  enum pci_irq_reroute_variant {
> > > --
> > > 2.27.0
> > >

Re: [PATCH V3 1/4] PCI/portdrv: Don't disable device during shutdown

[Huacai Chen]

Hi, Bjorn,

On Sat, Jun 26, 2021 at 4:45 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jun 25, 2021 at 05:30:27PM +0800, Huacai Chen wrote:
> > Use separate remove()/shutdown() callback, and don't disable PCI device
> > during shutdown. This can avoid some poweroff/reboot failures.
> >
> > The poweroff/reboot failures could easily be reproduced on Loongson
> > platforms. I think this is not a Loongson-specific problem, instead, is
> > a problem related to some specific PCI hosts. On some x86 platforms,
> > radeon/amdgpu devices can cause the same problem [1][2], and commit
> > faefba95c9e8ca3a ("drm/amdgpu: just suspend the hw on pci shutdown")
> > can resolve it.
> >
> > As Tiezhu said, this occasionally shutdown or reboot failure is due to
> > clear PCI_COMMAND_MASTER on the device in do_pci_disable_device() [3].
> >
> > static void do_pci_disable_device(struct pci_dev *dev)
> > {
> >         u16 pci_command;
> >
> >         pci_read_config_word(dev, PCI_COMMAND, &pci_command);
> >         if (pci_command & PCI_COMMAND_MASTER) {
> >                 pci_command &= ~PCI_COMMAND_MASTER;
> >                 pci_write_config_word(dev, PCI_COMMAND, pci_command);
> >         }
> >
> >         pcibios_disable_device(dev);
> > }
> >
> > When remove "pci_command &= ~PCI_COMMAND_MASTER;", it can work well when
> > shutdown or reboot. The root cause on Loongson platform is that CPU is
> > still writing data to framebuffer while poweroff/reboot, and if we clear
> > Bus Master Bit at this time, CPU will wait ack from device, but never
> > return, so a hardware deadlock happens.
>
> Doesn't make sense yet.  Bus Master enables the *device* to do DMA.  A
> CPU can do MMIO to a device, e.g., to write data to a framebuffer,
> regardless of the state of Bus Master Enable.  Also, those MMIO writes
> done by a CPU are Memory Write transactions on PCIe, which are
> "Posted" Requests, which means they do not receive acks.  So this
> cannot be the root cause.
For LS7A, if we disable Bus Master bit when CPU is still accessing
PCIe devices, the PCIe controller doesn't forward requests to
downstream devices, and also doesn't send TIMEOUT to CPU, which causes
CPU wait forever (hardware lockup). This behavior is a PCIe protocol
violation, and will be fixed in new revision of hardware (add timeout
mechanism for CPU read request).

Huacai
>
> Bjorn

Re: [PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Bjorn Helgaas]

On Sun, Jun 27, 2021 at 06:25:04PM +0800, Huacai Chen wrote:
> On Sat, Jun 26, 2021 at 6:22 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Jun 25, 2021 at 05:30:29PM +0800, Huacai Chen wrote:
> > > In new revision of LS7A, some PCIe ports support larger value than 256,
> > > but their maximum supported MRRS values are not detectable. Moreover,
> > > the current loongson_mrrs_quirk() cannot avoid devices increasing its
> > > MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> > > will actually set a big value in its driver. So the only possible way is
> > > configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
> > > PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.
> > >
> > > However, according to PCIe Spec, it is legal for an OS to program any
> > > value for MRRS, and it is also legal for an endpoint to generate a Read
> > > Request with any size up to its MRRS. As the hardware engineers says,
> > > the root cause here is LS7A doesn't break up large read requests (Yes,
> > > that is a problem in the LS7A design).
> >
> > "LS7A doesn't break up large read requests" claims to be a root cause,
> > but you haven't yet said what the actual *problem* is.
> >
> > Is the problem that an endpoint reports a malformed TLP because it
> > received a completion bigger than it can handle?  Is it that the LS7A
> > root port reports some kind of error if it receives a Memory Read
> > request with a size that's "too big"?  Maybe the LS7A doesn't know
> > what to do when it receives a Memory Read request with MRRS > MPS?
> > What exactly happens when the problem occurs?
>
> The hardware engineer said that the problem is: LS7A PCIe port reports
> CA (Completer Abort) if it receives a Memory Read
> request with a size that's "too big".

What is "too big"?

I'm trying to figure out how to make this work with hot-added devices.
Per spec (PCIe r5.0, sec 7.5.3.4), devices should power up with
MRRS=010b (512 bytes).

If Linux does not touch MRRS at all in hierarchices under LS7A, will a
hot-added device with MRRS=010b work?  Or does Linux need to actively
write MRRS to 000b (128 bytes) or 001b (256 bytes)?

Bjorn

Re: [PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Huacai Chen]

Hi, Bjorn,

On Tue, Jun 29, 2021 at 4:51 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Sun, Jun 27, 2021 at 06:25:04PM +0800, Huacai Chen wrote:
> > On Sat, Jun 26, 2021 at 6:22 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Jun 25, 2021 at 05:30:29PM +0800, Huacai Chen wrote:
> > > > In new revision of LS7A, some PCIe ports support larger value than 256,
> > > > but their maximum supported MRRS values are not detectable. Moreover,
> > > > the current loongson_mrrs_quirk() cannot avoid devices increasing its
> > > > MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> > > > will actually set a big value in its driver. So the only possible way is
> > > > configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
> > > > PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.
> > > >
> > > > However, according to PCIe Spec, it is legal for an OS to program any
> > > > value for MRRS, and it is also legal for an endpoint to generate a Read
> > > > Request with any size up to its MRRS. As the hardware engineers says,
> > > > the root cause here is LS7A doesn't break up large read requests (Yes,
> > > > that is a problem in the LS7A design).
> > >
> > > "LS7A doesn't break up large read requests" claims to be a root cause,
> > > but you haven't yet said what the actual *problem* is.
> > >
> > > Is the problem that an endpoint reports a malformed TLP because it
> > > received a completion bigger than it can handle?  Is it that the LS7A
> > > root port reports some kind of error if it receives a Memory Read
> > > request with a size that's "too big"?  Maybe the LS7A doesn't know
> > > what to do when it receives a Memory Read request with MRRS > MPS?
> > > What exactly happens when the problem occurs?
> >
> > The hardware engineer said that the problem is: LS7A PCIe port reports
> > CA (Completer Abort) if it receives a Memory Read
> > request with a size that's "too big".
>
> What is "too big"?
>
"Too big" means bigger than the port can handle, PCIe SPEC allows any
MRRS value, but, but, LS7A surely violates the protocol here.

Huacai

> I'm trying to figure out how to make this work with hot-added devices.
> Per spec (PCIe r5.0, sec 7.5.3.4), devices should power up with
> MRRS=010b (512 bytes).
>
> If Linux does not touch MRRS at all in hierarchices under LS7A, will a
> hot-added device with MRRS=010b work?  Or does Linux need to actively
> write MRRS to 000b (128 bytes) or 001b (256 bytes)?
>
> Bjorn

Re: [PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Bjorn Helgaas]

On Tue, Jun 29, 2021 at 10:00:20AM +0800, Huacai Chen wrote:
> On Tue, Jun 29, 2021 at 4:51 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sun, Jun 27, 2021 at 06:25:04PM +0800, Huacai Chen wrote:
> > > On Sat, Jun 26, 2021 at 6:22 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, Jun 25, 2021 at 05:30:29PM +0800, Huacai Chen wrote:
> > > > > In new revision of LS7A, some PCIe ports support larger value than 256,
> > > > > but their maximum supported MRRS values are not detectable. Moreover,
> > > > > the current loongson_mrrs_quirk() cannot avoid devices increasing its
> > > > > MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> > > > > will actually set a big value in its driver. So the only possible way is
> > > > > configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
> > > > > PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.
> > > > >
> > > > > However, according to PCIe Spec, it is legal for an OS to program any
> > > > > value for MRRS, and it is also legal for an endpoint to generate a Read
> > > > > Request with any size up to its MRRS. As the hardware engineers says,
> > > > > the root cause here is LS7A doesn't break up large read requests (Yes,
> > > > > that is a problem in the LS7A design).
> > > >
> > > > "LS7A doesn't break up large read requests" claims to be a root cause,
> > > > but you haven't yet said what the actual *problem* is.
> > > >
> > > > Is the problem that an endpoint reports a malformed TLP because it
> > > > received a completion bigger than it can handle?  Is it that the LS7A
> > > > root port reports some kind of error if it receives a Memory Read
> > > > request with a size that's "too big"?  Maybe the LS7A doesn't know
> > > > what to do when it receives a Memory Read request with MRRS > MPS?
> > > > What exactly happens when the problem occurs?
> > >
> > > The hardware engineer said that the problem is: LS7A PCIe port reports
> > > CA (Completer Abort) if it receives a Memory Read
> > > request with a size that's "too big".
> >
> > What is "too big"?
> >
> "Too big" means bigger than the port can handle, PCIe SPEC allows any
> MRRS value, but, but, LS7A surely violates the protocol here.

Right, I just wanted to know what the number is.  That is, what values
we can write to MRRS safely.

But ISTR you saying that it's not actually fixed, and that's why you
wanted to rely on what firmware put there.

This is important to know for the question about hot-added devices
below, because a hot-added device should power up with MRRS=512 bytes,
and if that's too big for LS7A, then we have a problem and the quirk
needs to be more extensive.

> > I'm trying to figure out how to make this work with hot-added devices.
> > Per spec (PCIe r5.0, sec 7.5.3.4), devices should power up with
> > MRRS=010b (512 bytes).
> >
> > If Linux does not touch MRRS at all in hierarchices under LS7A, will a
> > hot-added device with MRRS=010b work?  Or does Linux need to actively
> > write MRRS to 000b (128 bytes) or 001b (256 bytes)?
> >
> > Bjorn

Re: [PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Huacai Chen]

Hi, Bjorn,

On Tue, Jun 29, 2021 at 10:12 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Jun 29, 2021 at 10:00:20AM +0800, Huacai Chen wrote:
> > On Tue, Jun 29, 2021 at 4:51 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Sun, Jun 27, 2021 at 06:25:04PM +0800, Huacai Chen wrote:
> > > > On Sat, Jun 26, 2021 at 6:22 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Fri, Jun 25, 2021 at 05:30:29PM +0800, Huacai Chen wrote:
> > > > > > In new revision of LS7A, some PCIe ports support larger value than 256,
> > > > > > but their maximum supported MRRS values are not detectable. Moreover,
> > > > > > the current loongson_mrrs_quirk() cannot avoid devices increasing its
> > > > > > MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> > > > > > will actually set a big value in its driver. So the only possible way is
> > > > > > configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
> > > > > > PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.
> > > > > >
> > > > > > However, according to PCIe Spec, it is legal for an OS to program any
> > > > > > value for MRRS, and it is also legal for an endpoint to generate a Read
> > > > > > Request with any size up to its MRRS. As the hardware engineers says,
> > > > > > the root cause here is LS7A doesn't break up large read requests (Yes,
> > > > > > that is a problem in the LS7A design).
> > > > >
> > > > > "LS7A doesn't break up large read requests" claims to be a root cause,
> > > > > but you haven't yet said what the actual *problem* is.
> > > > >
> > > > > Is the problem that an endpoint reports a malformed TLP because it
> > > > > received a completion bigger than it can handle?  Is it that the LS7A
> > > > > root port reports some kind of error if it receives a Memory Read
> > > > > request with a size that's "too big"?  Maybe the LS7A doesn't know
> > > > > what to do when it receives a Memory Read request with MRRS > MPS?
> > > > > What exactly happens when the problem occurs?
> > > >
> > > > The hardware engineer said that the problem is: LS7A PCIe port reports
> > > > CA (Completer Abort) if it receives a Memory Read
> > > > request with a size that's "too big".
> > >
> > > What is "too big"?
> > >
> > "Too big" means bigger than the port can handle, PCIe SPEC allows any
> > MRRS value, but, but, LS7A surely violates the protocol here.
>
> Right, I just wanted to know what the number is.  That is, what values
> we can write to MRRS safely.
>
> But ISTR you saying that it's not actually fixed, and that's why you
> wanted to rely on what firmware put there.
Yes, it's not fixed (256 on some ports and 4096 on other ports), so we
should heavily depend on firmware.

Huacai
>
> This is important to know for the question about hot-added devices
> below, because a hot-added device should power up with MRRS=512 bytes,
> and if that's too big for LS7A, then we have a problem and the quirk
> needs to be more extensive.
>
> > > I'm trying to figure out how to make this work with hot-added devices.
> > > Per spec (PCIe r5.0, sec 7.5.3.4), devices should power up with
> > > MRRS=010b (512 bytes).

> > >
> > > If Linux does not touch MRRS at all in hierarchices under LS7A, will a
> > > hot-added device with MRRS=010b work?  Or does Linux need to actively
> > > write MRRS to 000b (128 bytes) or 001b (256 bytes)?
Emm, hot-plug is a problem, maybe we can only disable hot-plug in
board design...

Huacai
> > >
> > > Bjorn

Re: [PATCH V3 3/4] PCI: Improve the MRRS quirk for LS7A

[Jiaxun Yang]

在 2021/6/29 上午11:32, Huacai Chen 写道:
> Hi, Bjorn,
>
> On Tue, Jun 29, 2021 at 10:12 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>> On Tue, Jun 29, 2021 at 10:00:20AM +0800, Huacai Chen wrote:
>>> On Tue, Jun 29, 2021 at 4:51 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>> On Sun, Jun 27, 2021 at 06:25:04PM +0800, Huacai Chen wrote:
>>>>> On Sat, Jun 26, 2021 at 6:22 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>> On Fri, Jun 25, 2021 at 05:30:29PM +0800, Huacai Chen wrote:
>>>>>>> In new revision of LS7A, some PCIe ports support larger value than 256,
>>>>>>> but their maximum supported MRRS values are not detectable. Moreover,
>>>>>>> the current loongson_mrrs_quirk() cannot avoid devices increasing its
>>>>>>> MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
>>>>>>> will actually set a big value in its driver. So the only possible way is
>>>>>>> configure MRRS of all devices in BIOS, and add a PCI device flag (i.e.,
>>>>>>> PCI_DEV_FLAGS_NO_INCREASE_MRRS) to stop the increasing MRRS operations.
>>>>>>>
>>>>>>> However, according to PCIe Spec, it is legal for an OS to program any
>>>>>>> value for MRRS, and it is also legal for an endpoint to generate a Read
>>>>>>> Request with any size up to its MRRS. As the hardware engineers says,
>>>>>>> the root cause here is LS7A doesn't break up large read requests (Yes,
>>>>>>> that is a problem in the LS7A design).
>>>>>> "LS7A doesn't break up large read requests" claims to be a root cause,
>>>>>> but you haven't yet said what the actual *problem* is.
>>>>>>
>>>>>> Is the problem that an endpoint reports a malformed TLP because it
>>>>>> received a completion bigger than it can handle?  Is it that the LS7A
>>>>>> root port reports some kind of error if it receives a Memory Read
>>>>>> request with a size that's "too big"?  Maybe the LS7A doesn't know
>>>>>> what to do when it receives a Memory Read request with MRRS > MPS?
>>>>>> What exactly happens when the problem occurs?
>>>>> The hardware engineer said that the problem is: LS7A PCIe port reports
>>>>> CA (Completer Abort) if it receives a Memory Read
>>>>> request with a size that's "too big".
>>>> What is "too big"?
>>>>
>>> "Too big" means bigger than the port can handle, PCIe SPEC allows any
>>> MRRS value, but, but, LS7A surely violates the protocol here.
>> Right, I just wanted to know what the number is.  That is, what values
>> we can write to MRRS safely.
>>
>> But ISTR you saying that it's not actually fixed, and that's why you
>> wanted to rely on what firmware put there.
> Yes, it's not fixed (256 on some ports and 4096 on other ports), so we
> should heavily depend on firmware.
>
> Huacai
>> This is important to know for the question about hot-added devices
>> below, because a hot-added device should power up with MRRS=512 bytes,
>> and if that's too big for LS7A, then we have a problem and the quirk
>> needs to be more extensive.
>>
>>>> I'm trying to figure out how to make this work with hot-added devices.
>>>> Per spec (PCIe r5.0, sec 7.5.3.4), devices should power up with
>>>> MRRS=010b (512 bytes).
>>>> If Linux does not touch MRRS at all in hierarchices under LS7A, will a
>>>> hot-added device with MRRS=010b work?  Or does Linux need to actively
>>>> write MRRS to 000b (128 bytes) or 001b (256 bytes)?
> Emm, hot-plug is a problem, maybe we can only disable hot-plug in
> board design...
in ACPI?

Thanks.

- Jiaxun
>
> Huacai
>>>> Bjorn