by cantabile

[permalink] [raw]

Subject: Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

On 15/02/18 02:45, Jakub Kicinski wrote:
> On Wed, 14 Feb 2018 13:34:38 +0200, cantabile wrote:
>> The firmware running on the device sometimes survives a reboot
>> (firmware_running returns 1). When this happens the driver never calls
>> request_firmware, which means the kernel's firmware handling code
>> doesn't know this firmware should be cached before hibernating. Upon
>> resuming from several hours of hibernation, the firmware is no longer
>> running on the device, so the driver calls request_firmware. Since the
>> firmware was never cached, it needs to be loaded from disk, and this is
>> when the system freezes, somewhere in the xfs driver. Fix this by always
>> requesting the firmware, whether it's already running on the device or not.
>>
>> Signed-off-by: John Smith <[email protected]>
>
> Thanks for tracking this down, but this seems like the wrong
> direction.
>
> What's your hard drive? Is it some complex configuration which
> prevents the block device from coming online after resume?
>
> If it's really because of some peculiarities of XFS the fix should
> go there, no driver will be able to load FW on resume...
>

My hard drive is a SATA SSD, with a regular MS-DOS partition table, with
three primary partitions: one ext4 mounted at /boot, one xfs mounted at
/, one xfs mounted at /home. No encryption, no software RAID, no logical
volumes, etc.

The kernel has a firmware caching mechanism to make sure that no driver
needs to load firmware from disk during the resume process. Because
that's unreliable. See this bit of the documentation: [1]

This is how it works: when the driver's probe callback calls
request_firmware, that function adds the firmware file's name in a
devres in your 'struct device'. When you suspend the system, the kernel
calls fw_pm_notify [2], which goes through all the 'struct device's and
looks for firmware file names previously added by request_firmware, and
loads into memory all the firmware files it finds. When you resume the
system, all calls to request_firmware ought to find the firmware files
already loaded in memory. After resuming, the kernel calls fw_pm_notify
again to release the cached firmware files (with some delay).

This caching mechanism currently doesn't always work for your driver
because your driver doesn't always call request_firmware from the probe
callback. Because the firmware running on the device sometimes survives
a reboot.

You can get some very useful messages if you compile firmware_class.c
with -DDEBUG [3]. This is how I figured out the problem.

[1]
https://www.kernel.org/doc/html/v4.13/driver-api/firmware/request_firmware.html#considerations-for-suspend-and-resume
[2]
https://github.com/torvalds/linux/blob/master/drivers/base/firmware_class.c#L1804
[3] https://www.kernel.org/doc/local/pr_debug.txt

2018-02-28 21:19:03

by cantabile

[permalink] [raw]

Subject: Re: [PATCH] mt7601u: Fix system freeze after resuming from hibernation

On 28/02/18 20:48, Luis R. Rodriguez wrote:
> On Wed, Feb 28, 2018 at 08:02:59PM +0200, cantabile wrote:
>> On 27/02/18 22:42, Luis R. Rodriguez wrote:
>>> I'd be curious if someone who can trigger the situation can test what
>>> happens if you use:
>>>
>>> diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c b/drivers/net/wireless/mediatek/mt7601u/mcu.c
>>> index 65a8004418ea..04cbffd225a1 100644
>>> --- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
>>> +++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
>>> @@ -421,7 +421,7 @@ static int mt7601u_load_firmware(struct mt7601u_dev *dev)
>>> MT_USB_DMA_CFG_TX_BULK_EN));
>>> if (firmware_running(dev))
>>> - return 0;
>>> + pr_info("Firmware already loaded but going to reload...");
>>> ret = request_firmware(&fw, MT7601U_FIRMWARE, dev->dev);
>>> if (ret)
>>>
>>>
>>> Curious, will it really fail?
>>
>> This change brings no new messages from mt7601u in dmesg (other than this
>> pr_info), and the device works fine, as far as I can tell.
>
> OK so we know that the optimization is optional, not a requirement.
> That may be worth extending in documentation on the driver.

I may have spoken too soon. mt7601u kind of exploded after resuming from
suspend to RAM. Maybe it has nothing to do with this change, but well.
The system froze after the KDE lock screen appeared, and a few minutes
later the Caps Lock LED turned on. I attached journalctl output in case
it's interesting.

>
>>> Note that I see mt7601u_stop() just calls mt7601u_mac_stop(). The big cleanup
>>> happens via mt7601u_cleanup(), but I see mt7601u_disconnect() calls it.
>>>
>>> Just curious, does that not get called on shutdown?
>>>
>>> diff --git a/drivers/net/wireless/mediatek/mt7601u/usb.c b/drivers/net/wireless/mediatek/mt7601u/usb.c
>>> index b9e4f6793138..126ef2ba77c2 100644
>>> --- a/drivers/net/wireless/mediatek/mt7601u/usb.c
>>> +++ b/drivers/net/wireless/mediatek/mt7601u/usb.c
>>> @@ -311,6 +311,7 @@ static void mt7601u_disconnect(struct usb_interface *usb_intf)
>>> {
>>> struct mt7601u_dev *dev = usb_get_intfdata(usb_intf);
>>> + pr_info("Calling mt7601u_disconnect()...");
>>> ieee80211_unregister_hw(dev->hw);
>>> mt7601u_cleanup(dev);
>>>
>>> If it does, one option is mt7601u_cleanup() can use some love to really shut down
>>> the device more... But its not clear to me what else could be done and I'm very
>>> inclined to believe its not sensible.
>>>
>> "Calling mt7601u_disconnect" does not appear in journalctl after a reboot.
>
> Oh, I didn't expect it to come up during startup, I was wondering if it did
> trigger while going down on reboot.
>

I meant that it doesn't appear among the messages from the shutdown part
of rebooting. (I can only check after rebooting.)

Attachments:

journalctl.txt (96.33 kB)