| Message ID | 20210520235501.917397-1-Hi-Angel@yandex.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] PCI: don't call firmware hooks on suspend unless it's fw-controlled | expand |
On Fri, 2021-05-21 at 02:55 +0300, Konstantin Kharlamov wrote: > On Macbook 2013 resuming from s2idle resulted in external monitor no > longer being detected, and dmesg having errors like: > > pcieport 0000:06:00.0: can't change power state from D3hot to D0 (config > space inaccessible) > > and a stacktrace. The reason is that in s2idle (and in S1 as noted by > Rafael) we do not call firmware code to handle suspend, and as result > while waking up firmware also does not handle resume. > > This means, for the Thunderbolt controller that gets disabled in the > quirk by calling the firmware methods, there's no one to wake it back up > on resume. > > To quote Rafael Wysocki: > > > "Passing control to the platform firmware" means letting > > some native firmware code (like SMM code) run which happens at the end > > of S2/S3/S4 suspend transitions and it does not happen during S1 > > (standby) and s2idle suspend transitions. > > > > That's why using SXIO/SXFP/SXLF is only valid during S2/S3/S4 suspend > > transitions and it is not valid during s2idle and S1 suspend > > transitions (and yes, S1 is also affected, so s2idle is not special in > > that respect at all). > > Thus, return early from the quirk when suspend mode isn't one that calls > firmware. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212767 > Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru> > Reviewed-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/quirks.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 653660e3ba9e..f86b6388a04a 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -27,6 +27,7 @@ > #include <linux/nvme.h> > #include <linux/platform_data/x86/apple.h> > #include <linux/pm_runtime.h> > +#include <linux/suspend.h> > #include <linux/switchtec.h> > #include <asm/dma.h> /* isa_dma_bridge_buggy */ > #include "pci.h" > @@ -3646,6 +3647,15 @@ static void quirk_apple_poweroff_thunderbolt(struct > pci_dev *dev) > return; > if (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) > return; > + > + /* > + * SXIO/SXFP/SXLF turns off power to the Thunderbolt controller. We > don't > + * know how to turn it back on again, but firmware does, so we can > only use > + * SXIO/SXFP/SXLF if we're suspending via firmware. > + */ > + if (!pm_suspend_via_firmware()) > + return; > + > bridge = ACPI_HANDLE(&dev->dev); > if (!bridge) > return;
On Fri, 2021-05-28 at 10:39 +0300, Konstantin Kharlamov wrote: > On Fri, 2021-05-21 at 02:55 +0300, Konstantin Kharlamov wrote: > > On Macbook 2013 resuming from s2idle resulted in external monitor no > > longer being detected, and dmesg having errors like: > > > > pcieport 0000:06:00.0: can't change power state from D3hot to D0 (config > > space inaccessible) > > > > and a stacktrace. The reason is that in s2idle (and in S1 as noted by > > Rafael) we do not call firmware code to handle suspend, and as result > > while waking up firmware also does not handle resume. > > > > This means, for the Thunderbolt controller that gets disabled in the > > quirk by calling the firmware methods, there's no one to wake it back up > > on resume. > > > > To quote Rafael Wysocki: > > > > > "Passing control to the platform firmware" means letting > > > some native firmware code (like SMM code) run which happens at the end > > > of S2/S3/S4 suspend transitions and it does not happen during S1 > > > (standby) and s2idle suspend transitions. > > > > > > That's why using SXIO/SXFP/SXLF is only valid during S2/S3/S4 suspend > > > transitions and it is not valid during s2idle and S1 suspend > > > transitions (and yes, S1 is also affected, so s2idle is not special in > > > that respect at all). > > > > Thus, return early from the quirk when suspend mode isn't one that calls > > firmware. > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212767 > > Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru> > > Reviewed-by: Lukas Wunner <lukas@wunner.de> > > --- > > drivers/pci/quirks.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index 653660e3ba9e..f86b6388a04a 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -27,6 +27,7 @@ > > #include <linux/nvme.h> > > #include <linux/platform_data/x86/apple.h> > > #include <linux/pm_runtime.h> > > +#include <linux/suspend.h> > > #include <linux/switchtec.h> > > #include <asm/dma.h> /* isa_dma_bridge_buggy */ > > #include "pci.h" > > @@ -3646,6 +3647,15 @@ static void quirk_apple_poweroff_thunderbolt(struct > > pci_dev *dev) > > return; > > if (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) > > return; > > + > > + /* > > + * SXIO/SXFP/SXLF turns off power to the Thunderbolt controller. We > > don't > > + * know how to turn it back on again, but firmware does, so we can > > only use > > + * SXIO/SXFP/SXLF if we're suspending via firmware. > > + */ > > + if (!pm_suspend_via_firmware()) > > + return; > > + > > bridge = ACPI_HANDLE(&dev->dev); > > if (!bridge) > > return; > >
On Thu, Jun 03, 2021 at 11:36:43AM +0300, Konstantin Kharlamov wrote: > On Fri, 2021-05-28 at 10:39 +0300, Konstantin Kharlamov wrote: > > On Fri, 2021-05-21 at 02:55 +0300, Konstantin Kharlamov wrote: > > > On Macbook 2013 resuming from s2idle resulted in external monitor no > > > longer being detected, and dmesg having errors like: > > > > > > pcieport 0000:06:00.0: can't change power state from D3hot to D0 (config > > > space inaccessible) > > > > > > and a stacktrace. The reason is that in s2idle (and in S1 as noted by > > > Rafael) we do not call firmware code to handle suspend, and as result > > > while waking up firmware also does not handle resume. > > > > > > This means, for the Thunderbolt controller that gets disabled in the > > > quirk by calling the firmware methods, there's no one to wake it back up > > > on resume. > > > > > > To quote Rafael Wysocki: > > > > > > > "Passing control to the platform firmware" means letting > > > > some native firmware code (like SMM code) run which happens at the end > > > > of S2/S3/S4 suspend transitions and it does not happen during S1 > > > > (standby) and s2idle suspend transitions. > > > > > > > > That's why using SXIO/SXFP/SXLF is only valid during S2/S3/S4 suspend > > > > transitions and it is not valid during s2idle and S1 suspend > > > > transitions (and yes, S1 is also affected, so s2idle is not special in > > > > that respect at all). > > > > > > Thus, return early from the quirk when suspend mode isn't one that calls > > > firmware. > > > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212767 > > > Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru> > > > Reviewed-by: Lukas Wunner <lukas@wunner.de> > > > --- > > > drivers/pci/quirks.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > index 653660e3ba9e..f86b6388a04a 100644 > > > --- a/drivers/pci/quirks.c > > > +++ b/drivers/pci/quirks.c > > > @@ -27,6 +27,7 @@ > > > #include <linux/nvme.h> > > > #include <linux/platform_data/x86/apple.h> > > > #include <linux/pm_runtime.h> > > > +#include <linux/suspend.h> > > > #include <linux/switchtec.h> > > > #include <asm/dma.h> /* isa_dma_bridge_buggy */ > > > #include "pci.h" > > > @@ -3646,6 +3647,15 @@ static void quirk_apple_poweroff_thunderbolt(struct > > > pci_dev *dev) > > > return; > > > if (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) > > > return; > > > + > > > + /* > > > + * SXIO/SXFP/SXLF turns off power to the Thunderbolt controller. We > > > don't > > > + * know how to turn it back on again, but firmware does, so we can > > > only use > > > + * SXIO/SXFP/SXLF if we're suspending via firmware. > > > + */ > > > + if (!pm_suspend_via_firmware()) > > > + return; > > > + > > > bridge = ACPI_HANDLE(&dev->dev); > > > if (!bridge) > > > return; > > > > > > Don't worry, I haven't forgotten, but I've been busy with some other patches. If you ever want to check on the status, you can search for the patch on https://patchwork.kernel.org/project/linux-pci/list. The patchwork search is not super convenient (it's buried in the "Show patches with" link), but here's your patch: https://patchwork.kernel.org/project/linux-pci/patch/20210520235501.917397-1-Hi-Angel@yandex.ru/ It's currently "New" which means it's still in my queue. I change the state to "Accepted," "Not Applicable," "Superseded," etc., when I apply or drop patches.
Cool, I see, thank you very much! On Thu, 2021-06-03 at 12:46 -0500, Bjorn Helgaas wrote: > On Thu, Jun 03, 2021 at 11:36:43AM +0300, Konstantin Kharlamov wrote: > > On Fri, 2021-05-28 at 10:39 +0300, Konstantin Kharlamov wrote: > > > On Fri, 2021-05-21 at 02:55 +0300, Konstantin Kharlamov wrote: > > > > On Macbook 2013 resuming from s2idle resulted in external monitor no > > > > longer being detected, and dmesg having errors like: > > > > > > > > pcieport 0000:06:00.0: can't change power state from D3hot to D0 > > > > (config > > > > space inaccessible) > > > > > > > > and a stacktrace. The reason is that in s2idle (and in S1 as noted by > > > > Rafael) we do not call firmware code to handle suspend, and as result > > > > while waking up firmware also does not handle resume. > > > > > > > > This means, for the Thunderbolt controller that gets disabled in the > > > > quirk by calling the firmware methods, there's no one to wake it back up > > > > on resume. > > > > > > > > To quote Rafael Wysocki: > > > > > > > > > "Passing control to the platform firmware" means letting > > > > > some native firmware code (like SMM code) run which happens at the end > > > > > of S2/S3/S4 suspend transitions and it does not happen during S1 > > > > > (standby) and s2idle suspend transitions. > > > > > > > > > > That's why using SXIO/SXFP/SXLF is only valid during S2/S3/S4 suspend > > > > > transitions and it is not valid during s2idle and S1 suspend > > > > > transitions (and yes, S1 is also affected, so s2idle is not special in > > > > > that respect at all). > > > > > > > > Thus, return early from the quirk when suspend mode isn't one that calls > > > > firmware. > > > > > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212767 > > > > Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru> > > > > Reviewed-by: Lukas Wunner <lukas@wunner.de> > > > > --- > > > > drivers/pci/quirks.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > > index 653660e3ba9e..f86b6388a04a 100644 > > > > --- a/drivers/pci/quirks.c > > > > +++ b/drivers/pci/quirks.c > > > > @@ -27,6 +27,7 @@ > > > > #include <linux/nvme.h> > > > > #include <linux/platform_data/x86/apple.h> > > > > #include <linux/pm_runtime.h> > > > > +#include <linux/suspend.h> > > > > #include <linux/switchtec.h> > > > > #include <asm/dma.h> /* isa_dma_bridge_buggy */ > > > > #include "pci.h" > > > > @@ -3646,6 +3647,15 @@ static void > > > > quirk_apple_poweroff_thunderbolt(struct > > > > pci_dev *dev) > > > > return; > > > > if (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) > > > > return; > > > > + > > > > + /* > > > > + * SXIO/SXFP/SXLF turns off power to the Thunderbolt > > > > controller. We > > > > don't > > > > + * know how to turn it back on again, but firmware does, so we > > > > can > > > > only use > > > > + * SXIO/SXFP/SXLF if we're suspending via firmware. > > > > + */ > > > > + if (!pm_suspend_via_firmware()) > > > > + return; > > > > + > > > > bridge = ACPI_HANDLE(&dev->dev); > > > > if (!bridge) > > > > return; > > > > > > > > > > > > Don't worry, I haven't forgotten, but I've been busy with some other > patches. If you ever want to check on the status, you can search for > the patch on https://patchwork.kernel.org/project/linux-pci/list. The > patchwork search is not super convenient (it's buried in the "Show > patches with" link), but here's your patch: > > https://patchwork.kernel.org/project/linux-pci/patch/20210520235501.917397-1-Hi-Angel@yandex.ru/ > > It's currently "New" which means it's still in my queue. I change the > state to "Accepted," "Not Applicable," "Superseded," etc., when I > apply or drop patches.
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 653660e3ba9e..f86b6388a04a 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -27,6 +27,7 @@ #include <linux/nvme.h> #include <linux/platform_data/x86/apple.h> #include <linux/pm_runtime.h> +#include <linux/suspend.h> #include <linux/switchtec.h> #include <asm/dma.h> /* isa_dma_bridge_buggy */ #include "pci.h" @@ -3646,6 +3647,15 @@ static void quirk_apple_poweroff_thunderbolt(struct pci_dev *dev) return; if (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) return; + + /* + * SXIO/SXFP/SXLF turns off power to the Thunderbolt controller. We don't + * know how to turn it back on again, but firmware does, so we can only use + * SXIO/SXFP/SXLF if we're suspending via firmware. + */ + if (!pm_suspend_via_firmware()) + return; + bridge = ACPI_HANDLE(&dev->dev); if (!bridge) return;
On Macbook 2013 resuming from s2idle resulted in external monitor no longer being detected, and dmesg having errors like: pcieport 0000:06:00.0: can't change power state from D3hot to D0 (config space inaccessible) and a stacktrace. The reason is that in s2idle (and in S1 as noted by Rafael) we do not call firmware code to handle suspend, and as result while waking up firmware also does not handle resume. This means, for the Thunderbolt controller that gets disabled in the quirk by calling the firmware methods, there's no one to wake it back up on resume. To quote Rafael Wysocki: > "Passing control to the platform firmware" means letting > some native firmware code (like SMM code) run which happens at the end > of S2/S3/S4 suspend transitions and it does not happen during S1 > (standby) and s2idle suspend transitions. > > That's why using SXIO/SXFP/SXLF is only valid during S2/S3/S4 suspend > transitions and it is not valid during s2idle and S1 suspend > transitions (and yes, S1 is also affected, so s2idle is not special in > that respect at all). Thus, return early from the quirk when suspend mode isn't one that calls firmware. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212767 Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru> Reviewed-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/quirks.c | 10 ++++++++++ 1 file changed, 10 insertions(+)