[v3] net: wwan: iosm: Fix hibernation by re-binding the driver around it

Commit Message

Maciej S. Szmigiero Jan. 8, 2025, 11:33 p.m. UTC

Currently, the driver is seriously broken with respect to the
hibernation (S4): after image restore the device is back into
IPC_MEM_EXEC_STAGE_BOOT (which AFAIK means bootloader stage) and needs
full re-launch of the rest of its firmware, but the driver restore
handler treats the device as merely sleeping and just sends it a
wake-up command.

This wake-up command times out but device nodes (/dev/wwan*) remain
accessible.
However attempting to use them causes the bootloader to crash and
enter IPC_MEM_EXEC_STAGE_CD_READY stage (which apparently means "a crash
dump is ready").

It seems that the device cannot be re-initialized from this crashed
stage without toggling some reset pin (on my test platform that's
apparently what the device _RST ACPI method does).

While it would theoretically be possible to rewrite the driver to tear
down the whole MUX / IPC layers on hibernation (so the bootloader does
not crash from improper access) and then re-launch the device on
restore this would require significant refactoring of the driver
(believe me, I've tried), since there are quite a few assumptions
hard-coded in the driver about the device never being partially
de-initialized (like channels other than devlink cannot be closed,
for example).
Probably this would also need some programming guide for this hardware.

Considering that the driver seems orphaned [1] and other people are
hitting this issue too [2] fix it by simply unbinding the PCI driver
before hibernation and re-binding it after restore, much like
USB_QUIRK_RESET_RESUME does for USB devices that exhibit a similar
problem.

Tested on XMM7360 in HP EliteBook 855 G7 both with s2idle (which uses
the existing suspend / resume handlers) and S4 (which uses the new code).

[1]: https://lore.kernel.org/all/[e-mail]/
[2]:
https://github.com/xmm7360/xmm7360-pci/issues/211#issuecomment-1804139413

Reviewed-by: Sergey Ryazanov <[e-mail]>
Signed-off-by: Maciej S. Szmigiero <[e-mail]>
---

Changes from v2:
* Move "pci_registered" near the other global variable at the top of the
file.

* Move module initialization, deinitialization and PM notifier handlers
to the end of the file.

* Add Sergey's "Reviewed-by:" tag.

* Add CC to PM experts at [e-mail].

drivers/net/wwan/iosm/iosm_ipc_pcie.c | 56 ++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

Comments

Paolo Abeni Jan. 14, 2025, 8:49 a.m. UTC | #1

On 1/9/25 12:33 AM, Maciej S. Szmigiero wrote:
 @@ -530,3 +531,56 @@ void ipc_pcie_kfree_skb(struct iosm_pcie
*ipc_pcie, struct sk_buff *skb)
>  	IPC_CB(skb)->mapping = 0;
>  	dev_kfree_skb(skb);
>  }
> +
> +static int pm_notify(struct notifier_block *nb, unsigned long mode, void *_unused)
> +{
> +	if (mode == PM_HIBERNATION_PREPARE || mode == PM_RESTORE_PREPARE) {
> +		if (pci_registered) {

Out of sheer ignorance on my side, why 'mode == PM_RESTORE_PREPARE' is
required above? Isn't the driver already unregistered by the previous
PM_HIBERNATION_PREPARE call?

Thanks,

Paolo

Maciej S. Szmigiero Jan. 14, 2025, 11:31 a.m. UTC | #2

On 14.01.2025 09:49, Paolo Abeni wrote:
> On 1/9/25 12:33 AM, Maciej S. Szmigiero wrote:
>   @@ -530,3 +531,56 @@ void ipc_pcie_kfree_skb(struct iosm_pcie
> *ipc_pcie, struct sk_buff *skb)
>>   	IPC_CB(skb)->mapping = 0;
>>   	dev_kfree_skb(skb);
>>   }
>> +
>> +static int pm_notify(struct notifier_block *nb, unsigned long mode, void *_unused)
>> +{
>> +	if (mode == PM_HIBERNATION_PREPARE || mode == PM_RESTORE_PREPARE) {
>> +		if (pci_registered) {
> 
> Out of sheer ignorance on my side, why 'mode == PM_RESTORE_PREPARE' is
> required above? Isn't the driver already unregistered by the previous
> PM_HIBERNATION_PREPARE call?

If the restore kernel had this driver loaded then it needs to be unregistered
before restoring the hibernation image so it has chance to shut the modem
firmware down rather than keep it running during the restore process.

This way when the driver from the restored image re-binds the device it finds
it in the proper non-running state.

> Thanks,
> 
> Paolo
> 

Thanks,
Maciej

Patch

diff --git a/drivers/net/wwan/iosm/iosm_ipc_pcie.c b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
index 04517bd3325a..a066977af0be 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_pcie.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_pcie.c
@@ -6,6 +6,7 @@ 
 #include <linux/acpi.h>
 #include <linux/bitfield.h>
 #include <linux/module.h>
+#include <linux/suspend.h>
 #include <net/rtnetlink.h>
 
 #include "iosm_ipc_imem.h"
@@ -18,6 +19,7 @@  MODULE_LICENSE("GPL v2");
 /* WWAN GUID */
 static guid_t wwan_acpi_guid = GUID_INIT(0xbad01b75, 0x22a8, 0x4f48, 0x87, 0x92,
 				       0xbd, 0xde, 0x94, 0x67, 0x74, 0x7d);
+static bool pci_registered;
 
 static void ipc_pcie_resources_release(struct iosm_pcie *ipc_pcie)
 {
@@ -448,7 +450,6 @@  static struct pci_driver iosm_ipc_driver = {
 	},
 	.id_table = iosm_ipc_ids,
 };
-module_pci_driver(iosm_ipc_driver);
 
 int ipc_pcie_addr_map(struct iosm_pcie *ipc_pcie, unsigned char *data,
 		      size_t size, dma_addr_t *mapping, int direction)
@@ -530,3 +531,56 @@  void ipc_pcie_kfree_skb(struct iosm_pcie *ipc_pcie, struct sk_buff *skb)
 	IPC_CB(skb)->mapping = 0;
 	dev_kfree_skb(skb);
 }
+
+static int pm_notify(struct notifier_block *nb, unsigned long mode, void *_unused)
+{
+	if (mode == PM_HIBERNATION_PREPARE || mode == PM_RESTORE_PREPARE) {
+		if (pci_registered) {
+			pci_unregister_driver(&iosm_ipc_driver);
+			pci_registered = false;
+		}
+	} else if (mode == PM_POST_HIBERNATION || mode == PM_POST_RESTORE) {
+		if (!pci_registered) {
+			int ret;
+
+			ret = pci_register_driver(&iosm_ipc_driver);
+			if (ret) {
+				pr_err(KBUILD_MODNAME ": unable to re-register PCI driver: %d\n",
+				       ret);
+			} else {
+				pci_registered = true;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static struct notifier_block pm_notifier = {
+	.notifier_call = pm_notify,
+};
+
+static int __init iosm_ipc_driver_init(void)
+{
+	int ret;
+
+	ret = pci_register_driver(&iosm_ipc_driver);
+	if (ret)
+		return ret;
+
+	pci_registered = true;
+
+	register_pm_notifier(&pm_notifier);
+
+	return 0;
+}
+module_init(iosm_ipc_driver_init);
+
+static void __exit iosm_ipc_driver_exit(void)
+{
+	unregister_pm_notifier(&pm_notifier);
+
+	if (pci_registered)
+		pci_unregister_driver(&iosm_ipc_driver);
+}
+module_exit(iosm_ipc_driver_exit);