diff mbox series

[fixes,v4] pci: loongson: Workaround MIPS firmware MRRS settings

Message ID 20231101114957.309902-1-jiaxun.yang@flygoat.com
State New
Headers
Series [fixes,v4] pci: loongson: Workaround MIPS firmware MRRS settings |

Commit Message

Jiaxun Yang Nov. 1, 2023, 11:49 a.m. UTC 
  This is a partial revert of commit 8b3517f88ff2 ("PCI:
loongson: Prevent LS7A MRRS increases") for MIPS based Loongson.

There are many MIPS based Loongson systems in wild that
shipped with firmware which does not set maximum MRRS properly.

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.

Cc: stable@vger.kernel.org
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@flygoat.com>
---
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(+)

Comments

Bjorn Helgaas Nov. 1, 2023, 10:02 p.m. UTC | #1 
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@vger.kernel.org
> 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@flygoat.com>

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
>
Jiaxun Yang Nov. 2, 2023, 11:26 a.m. UTC | #2 
在2023年11月1日十一月 下午10:02，Bjorn Helgaas写道：
> 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.

Yep, it happens that we can't trust MRRS left by firmware on MIPS
Loongson.
>
>> 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.

I should make myself clear that MRRS reset actually happens when the
PCIe port (i.e. the PCI to PCI bridge) lose it's PCI_COMMAND_MASTER.

Since pci_enable_bridge is called by pci_enable_device it is still the
best place for such quirk.

I'll amend the commit message to make it clear.

>
>> Cc: stable@vger.kernel.org
>> 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@flygoat.com>
>
> 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?

I'm dumb to ACPI enabled Loongson so I think Huacai must be the maintainer
but I'd like to take care of this driver as well, perhaps having two M entry
or give me an R entry?

Huacai, what's your opinion?

>
> 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?

I think no_inc_mrrs still needs to be set for those devices so I left
the "new" quirk enabled for MIPS as well here.

But we can also do it in "old" quirk with an extra match, will do in next
version.

[...]
Huacai Chen Nov. 4, 2023, 3:12 a.m. UTC | #3 
On Wed, Nov 1, 2023 at 11:27 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
> This is a partial revert of commit 8b3517f88ff2 ("PCI:
> loongson: Prevent LS7A MRRS increases") for MIPS based Loongson.
>
> There are many MIPS based Loongson systems in wild that
> shipped with firmware which does not set maximum MRRS properly.
>
> 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.
>
> Cc: stable@vger.kernel.org
> 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@flygoat.com>
> ---
> 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) },
> +               { 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
Rename it to loongson_set_min_mrrs() and move it before
loongson_mrrs_quirk() may be better.

Huacai

> +
>  static void loongson_pci_pin_quirk(struct pci_dev *pdev)
>  {
>         pdev->pin = 1 + (PCI_FUNC(pdev->devfn) & 3);
> --
> 2.34.1
>
diff mbox series

Patch

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) },
+		{ 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);