Re: [net-next v2] net: wwan: t7xx: reset device if suspend fails

[Jinjian Song <jinjian.song@fibocom.com>]

From: Sergey Ryazanov <ryazanov.s.a@gmail.com>

>Hi Jinjian,
>
>On 29.10.2024 05:46, Jinjian Song wrote:
>> From: Sergey Ryazanov <ryazanov.s.a@gmail.com>
>>> On 22.10.2024 11:43, Jinjian Song wrote:
>>>> If driver fails to set the device to suspend, it means that the
>>>> device is abnormal. In this case, reset the device to recover
>>>> when PCIe device is offline.
>>>
>>> Is it a reproducible or a speculative issue? Does the fix recover 
>>> modem from a problematic state?
>>>
>>> Anyway we need someone more familiar with this hardware (Intel or 
>>> MediaTek engineer) to Ack the change to make sure we are not going to 
>>> put a system in a more complicated state.
>> 
>> Hi Sergey,
>> 
>> This is a very difficult issue to replicate onece occured and fixed.
>> 
>> The issue occured when driver and device lost the connection. I have
>> encountered this problem twice so far:
>> 1. During suspend/resume stress test, there was a probabilistic D3L2
>> time sequence issue with the BIOS, result in PCIe link down, driver
>> read and write the register of device invalid, so suspend failed.
>> This issue was eventually fixed in the BIOS and I was able to restore
>> it through the reset module after reproducing the problem.
>> 
>> 2. During idle test, the modem probabilistic hang up, result in PCIe
>> link down, driver read and write the register of device invalid, so
>> suspend failed. This issue was eventually fiex in device modem firmware
>> by adjust a certain power supply voltage, and reset modem as a workround
>> to restore when the MBIM port command timeout in userspace applycations.
>> 
>> Hardware reset modem to recover was discussed with MTK, and they said
>> that if we don't want to keep the on-site problem location in case of
>> suspend failure, we can use the recover solution.
>> Both the ocurred issues result in the PCIe link issue, driver can't read 
>> and writer the register of WWAN device, so I want to add this path
>> to restore, hardware reset modem can recover modem, but using the 
>> pci_channle_offline() as the judgment is my inference.
>
>Thank you for the clarification. Let me summarize what I've understood 
>from the explanation:
>a) there were hardware (firmware) issues,
>b) issues already were solved,
>c) issues were not directly related to the device suspension procedure,
>d) you want to implement a backup plan to make the modem support robust.
>
>If got it right, then I would like to recommend to implement a generic 
>error handling solution for the PCIe interface. You can check this 
>document: Documentation/PCI/pci-error-recovery.rst
Hi Sergey,

Yes, got it right.
I want to identify the scenario and then recover by reset device,
otherwise suspend failure will aways prevent the system from suspending
if it occurs.

>Suddenly, I am not an expert in the PCIe link recovery procedure, so 
>I've CCed this message to PCI subsystem maintainers. I hope they can 
>suggest a conceptually correct way to handle these cases.

Thanks.

>>>> Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
>>>> ---
>>>> V2:
>>>>   * Add judgment, reset when device is offline
>>>> ---
>>>>   drivers/net/wwan/t7xx/t7xx_pci.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/ 
>>>> t7xx/t7xx_pci.c
>>>> index e556e5bd49ab..4f89a353588b 100644
>>>> --- a/drivers/net/wwan/t7xx/t7xx_pci.c
>>>> +++ b/drivers/net/wwan/t7xx/t7xx_pci.c
>>>> @@ -427,6 +427,10 @@ static int __t7xx_pci_pm_suspend(struct pci_dev 
>>>> *pdev)
>>>>       iowrite32(T7XX_L1_BIT(0), IREG_BASE(t7xx_dev) + 
>>>> ENABLE_ASPM_LOWPWR);
>>>>       atomic_set(&t7xx_dev->md_pm_state, MTK_PM_RESUMED);
>>>>       t7xx_pcie_mac_set_int(t7xx_dev, SAP_RGU_INT);
>>>> +    if (pci_channel_offline(pdev)) {
>>>> +        dev_err(&pdev->dev, "Device offline, reset to recover\n");
>>>> +        t7xx_reset_device(t7xx_dev, PLDR);
>>>> +    }
>>>>       return ret;
>>>>   }
>
>--
>Sergey
>

Best Regards,
Jinjian