Message-ID: <CALs4sv0HUQjFEv_mZn0jabSDuxfuu4K6f9vFmUuzMtjZLVKc8A@mail.gmail.com>
Date: Mon, 2 Dec 2024 12:30:07 +0530
From: Pavan Chebbi <pavan.chebbi@...adcom.com>
To: Lenny Szubowicz <lszubowi@...hat.com>
Cc: mchan@...adcom.com, andrew+netdev@...n.ch, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	george.shuklin@...il.com, andrea.fois@...ntsense.it, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [patch v2] tg3: Disable tg3 PCIe AER on system reboot

On Sat, Nov 30, 2024 at 2:06 AM Lenny Szubowicz <lszubowi@...hat.com> wrote:
>
> Disable PCIe AER on the tg3 device on system reboot on a limited
> list of Dell PowerEdge systems. This prevents a fatal PCIe AER event
> on the tg3 device during the ACPI _PTS (prepare to sleep) method for
> S5 on those systems. The _PTS is invoked by acpi_enter_sleep_state_prep()
> as part of the kernel's reboot sequence as a result of commit
> 38f34dba806a ("PM: ACPI: reboot: Reinstate S5 for reboot").
>
> There was an earlier fix for this problem by commit 2ca1c94ce0b6
> ("tg3: Disable tg3 device on system reboot to avoid triggering AER").

Are you saying that if we have tg3_power_down() done then the current
new issue won't be seen?

> But it was discovered that this earlier fix caused a reboot hang
> when some Dell PowerEdge servers were booted via ipxe. To address
> this reboot hang, the earlier fix was essentially reverted by commit
> 9fc3bc764334 ("tg3: power down device only on SYSTEM_POWER_OFF").
> This re-exposed the tg3 PCIe AER on reboot problem.
>
> This fix is not an ideal solution because the root cause of the AER
> is in system firmware. Instead, it's a targeted work-around in the
> tg3 driver.
>
> Note also that the PCIe AER must be disabled on the tg3 device even
> if the system is configured to use "firmware first" error handling.

Not too sure about this. The list has some widely used latest Dell
servers. The first fix only did pci_disable_device()
But looks like this fix should be the right one for the first ever
reported issue in commit 2ca1c94ce0b6 ?

Also you may want to address the warnings generated. Also note that
netdev requires you to wait 24hours before posting a new revision of
the patch.

>
> Fixes: 9fc3bc764334 ("tg3: power down device only on SYSTEM_POWER_OFF")
> Signed-off-by: Lenny Szubowicz <lszubowi@...hat.com>
> ---
>  drivers/net/ethernet/broadcom/tg3.c | 59 +++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 9cc8db10a8d6..12ae5a976ca7 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -55,6 +55,7 @@
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/crc32poly.h>
> +#include <linux/dmi.h>
>
>  #include <net/checksum.h>
>  #include <net/gso.h>
> @@ -18192,6 +18193,51 @@ static int tg3_resume(struct device *device)
>
>  static SIMPLE_DEV_PM_OPS(tg3_pm_ops, tg3_suspend, tg3_resume);
>
> +/*
> + * Systems where ACPI _PTS (Prepare To Sleep) S5 will result in a fatal
> + * PCIe AER event on the tg3 device if the tg3 device is not, or cannot
> + * be, powered down.
> + */
> +static const struct dmi_system_id tg3_restart_aer_quirk_table[] = {
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge R440"),
> +               },
> +       },
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge R540"),
> +               },
> +       },
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge R640"),
> +               },
> +       },
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge R650"),
> +               },
> +       },
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge R740"),
> +               },
> +       },
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "PowerEdge R750"),
> +               },
> +       },
> +       {}
> +};
> +
>  static void tg3_shutdown(struct pci_dev *pdev)
>  {
>         struct net_device *dev = pci_get_drvdata(pdev);
> @@ -18208,6 +18254,19 @@ static void tg3_shutdown(struct pci_dev *pdev)
>
>         if (system_state == SYSTEM_POWER_OFF)
>                 tg3_power_down(tp);
> +       else if (system_state == SYSTEM_RESTART &&
> +                dmi_first_match(tg3_restart_aer_quirk_table) &&
> +                pdev->current_state <= PCI_D3hot) {
> +               /*
> +                * Disable PCIe AER on the tg3 to avoid a fatal
> +                * error during this system restart.
> +                */
> +               pcie_capability_clear_word(pdev, PCI_EXP_DEVCTL,
> +                                          PCI_EXP_DEVCTL_CERE |
> +                                          PCI_EXP_DEVCTL_NFERE |
> +                                          PCI_EXP_DEVCTL_FERE |
> +                                          PCI_EXP_DEVCTL_URRE);
> +       }
>
>         rtnl_unlock();
>
> --
> 2.45.2
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)

Powered by blists - more mailing lists