On Wed, Nov 01, 2023 at 11:49:56AM +0000, Jiaxun Yang wrote:
> This is a partial revert of commit 8b3517f88ff2 ("PCI:
> loongson: Prevent LS7A MRRS increases") for MIPS based Loongson.
Thanks for this patch. We're in the v6.7 merge window, so we won't
start merging v6.8 content until v6.7-rc1 (probably Nov 12).
> There are many MIPS based Loongson systems in wild that
> shipped with firmware which does not set maximum MRRS properly.
As far as I know, there's no requirement for firmware to set MRRS at
all *except* for the "no_inc_mrrs" hack added by 8b3517f88ff2. That
hack treats the current MRRS value as a limit to work around the
Loongson bug that read requests larger than the limit cause a
Completer Abort instead of multiple completions.
> Limiting MRRS to 256 for all as MIPS Loongson comes with higher
> MRRS support is considered rare.
>
> It must be done at device enablement stage because hardware will
> reset MRRS to inavlid value if a device got disabled.
s/inavlid/invalid/
This part isn't clear to me, though. What exactly does "device got
disabled" mean? The device got reset? Power cycled?
PCI_COMMAND_MASTER was cleared?
PCI_FIXUP_ENABLE quirks are run during pci_enable_device(), which
basically just turns on PCI_COMMAND_MEMORY and/or PCI_COMMAND_IO.
If MRRS gets reset when PCI_COMMAND_MASTER is set or cleared, we don't
have a quirk phase that runs during pci_set_master(), which is where
PCI_COMMAND_MASTER gets set, so it's not clear that
pci_enable_device() is the right place.
> Cc: stable@xxxxxxxxxxxxxxx
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217680
> Fixes: 8b3517f88ff2 ("PCI: loongson: Prevent LS7A MRRS increases")
> Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
We'll look for an ack from the maintainer. Maybe that's you, since
you added the driver in the first place? Or maybe it's Huacai?
MAINTAINERS currently doesn't list anybody for
drivers/pci/controller/pci-loongson.c, and it should. That should be
a separate patch.
> ---
> v4: Improve commit message
>
> This is a partial revert of the origin quirk so there shouldn't
> be any drama.
> ---
> drivers/pci/controller/pci-loongson.c | 38 +++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c
> index d45e7b8dc530..d184d7b97e54 100644
> --- a/drivers/pci/controller/pci-loongson.c
> +++ b/drivers/pci/controller/pci-loongson.c
> @@ -108,6 +108,44 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> DEV_LS7A_PCIE_PORT6, loongson_mrrs_quirk);
>
> +#ifdef CONFIG_MIPS
> +static void loongson_old_mrrs_quirk(struct pci_dev *pdev)
> +{
> + struct pci_bus *bus = pdev->bus;
> + struct pci_dev *bridge;
> + static const struct pci_device_id bridge_devids[] = {
> + { PCI_VDEVICE(LOONGSON, DEV_LS2K_PCIE_PORT0) },
> + { PCI_VDEVICE(LOONGSON, DEV_LS7A_PCIE_PORT0) },
> + { PCI_VDEVICE(LOONGSON, DEV_LS7A_PCIE_PORT1) },
> + { PCI_VDEVICE(LOONGSON, DEV_LS7A_PCIE_PORT2) },
> + { PCI_VDEVICE(LOONGSON, DEV_LS7A_PCIE_PORT3) },
> + { PCI_VDEVICE(LOONGSON, DEV_LS7A_PCIE_PORT4) },
> + { PCI_VDEVICE(LOONGSON, DEV_LS7A_PCIE_PORT5) },
> + { PCI_VDEVICE(LOONGSON, DEV_LS7A_PCIE_PORT6) },
This looks like the same list of devices as for loongson_mrrs_quirk().
So I guess the idea is that we need loongson_mrrs_quirk() for
Loongarch-based systems, and this loongson_old_mrrs_quirk() for
MIPS-based systems?
If so, maybe they could be #ifdef'd to show that, e.g., so that only
one or the other is compiled?
> + { 0, },
> + };
> +
> + /* look for the matching bridge */
> + while (!pci_is_root_bus(bus)) {
> + bridge = bus->self;
> + bus = bus->parent;
> + /*
> + * There are still some wild MIPS Loongson firmware won't
> + * set MRRS properly. Limiting MRRS to 256 as MIPS Loongson
> + * comes with higher MRRS support is considered rare.
> + */
> + if (pci_match_id(bridge_devids, bridge)) {
> + if (pcie_get_readrq(pdev) > 256) {
> + pci_info(pdev, "limiting MRRS to 256\n");
> + pcie_set_readrq(pdev, 256);
> + }
> + break;
> + }
> + }
> +}
> +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_old_mrrs_quirk);
> +#endif
> +
> static void loongson_pci_pin_quirk(struct pci_dev *pdev)
> {
> pdev->pin = 1 + (PCI_FUNC(pdev->devfn) & 3);
> --
> 2.34.1
>
