Message-ID: <5df4a525-dc5d-405a-be07-5b33e94f5a4f@maciej.szmigiero.name>
Date: Wed, 8 Jan 2025 21:03:54 +0100
From: "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: M Chetan Kumar <m.chetan.kumar@...el.com>,
 Sergey Ryazanov <ryazanov.s.a@...il.com>,
 Loic Poulain <loic.poulain@...aro.org>,
 Johannes Berg <johannes@...solutions.net>,
 Bjorn Helgaas <bhelgaas@...gle.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 netdev@...r.kernel.org, linux-pci@...r.kernel.org,
 linux-kernel@...r.kernel.org, "Rafael J. Wysocki" <rjw@...ysocki.net>,
 linux-pm@...r.kernel.org
Subject: Re: [PATCH v2] net: wwan: iosm: Fix hibernation by re-binding the
 driver around it

On 8.01.2025 20:51, Bjorn Helgaas wrote:
> [+cc Rafael, linux-pm because they *are* PM experts :)]
> 
> On Wed, Jan 08, 2025 at 02:15:28AM +0200, Sergey Ryazanov wrote:
>> On 08.01.2025 01:45, Bjorn Helgaas wrote:
>>> On Wed, Jan 08, 2025 at 01:13:41AM +0200, Sergey Ryazanov wrote:
>>>> On 05.01.2025 19:39, Maciej S. Szmigiero wrote:
>>>>> 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/c248f0b4-2114-4c61-905f-466a786bdebb@leemhuis.info/
>>>>> [2]:
>>>>> https://github.com/xmm7360/xmm7360-pci/issues/211#issuecomment-1804139413
>>>>>
>>>>> Signed-off-by: Maciej S. Szmigiero <mail@...iej.szmigiero.name>
>>>>
>>>> Generally looks good to me. Lets wait for approval from PCI
>>>> maintainers to be sure that there no unexpected side effects.
>>>
>>> I have nothing useful to contribute here.  Seems like kind of a
>>> mess.  But Intel claims to maintain this, so it would be nice if
>>> they would step up and make this work nicely.
>>
>> Suddenly, Intel lost their interest in the modems market and, as
>> Maciej mentioned, the driver was abandon for a quite time now. The
>> author no more works for Intel. You will see the bounce.
> 
> Well, that's unfortunate :)  Maybe step 0 is to remove the Intel
> entry from MAINTAINERS for this driver.
> 
>> Bjorn, could you suggest how to deal easily with the device that is
>> incapable to seamlessly recover from hibernation? I am totally
>> hopeless regarding the PM topic. Or is the deep driver rework the
>> only option?
> 
> I'm pretty PM-illiterate myself.  Based on
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/pm/sleep-states.rst?id=v6.12#n109,
> I assume that when we resume after hibernate, devices are in the same
> state as after a fresh boot, i.e., the state driver .probe() methods
> see.
> 
> So I assume that some combination of dev_pm_ops methods must be able
> to do basically the same as .probe() to get the device usable again
> after it was completely powered off and back on.

You are right that it should be theoretically possible to fix this issue
by re-initializing the driver in the hibernation restore/thaw callbacks
and I even have tried to do so in the beginning.

But as I wrote in this patch description, doing so would need significant
refactoring of the driver as it is not currently capable of being
de-initialized and re-initialized partially.

Hence this patch approach of simply re-binding the driver which also
seemed safer in the absence of any real programming docs for this hardware.

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/pm/devices.rst?id=v6.12#n506
> mentions .freeze(), .thaw(), .restore(), etc, but the fact that few
> drivers set those pointers and all the nice macros for setting pm ops
> (SYSTEM_SLEEP_PM_OPS, NOIRQ_SYSTEM_SLEEP_PM_OPS, etc) only take
> suspend and resume functions makes me think most drivers must handle
> hibernation in the same .suspend() and .resume() functions they use
> for non-hibernate transitions.
> 
> Since all drivers have to cope with devices needing to be
> reinitialized after hibernate, I would look around to see how other
> drivers do it and see if you can do it similarly.
>
>
> Sorry this is still really a non-answer.
> 
> Bjorn

Thanks,
Maciej

Powered by blists - more mailing lists