lkml.org 
[lkml]   [2022]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] USB:Fix ehci infinite suspend-resume loop issue in zhaoxin
From
On 2022/3/16 10:18, WeitaoWang-oc@zhaoxin.com wrote:
> On 2022/3/15 11:18, Alan Stern wrote:
>> On Tue, Mar 15, 2022 at 08:39:09PM +0800, WeitaoWang-oc@zhaoxin.com wrote:
>>> On 2022/3/14 10:24, Alan Stern wrote:
>>>>> +       t1 = ehci_readl(ehci, &ehci->regs->status);
>>>>> +       ehci_writel(ehci, t1 & STS_PCD, &ehci->regs->status);
>>>>> +       ehci_readl(ehci, &ehci->regs->status);
>>>>
>>>> You should not clear the STS_PCD bit.  What if some other port had a
>>>> status change at the same time?  Then because you cleared the
>>>> port-change-detect bit, the system would not realize that the other port
>>>> needed to be handled.
>>>
>>> I really didn't think about this case.
>>>
>>>> Leaving the STS_PCD bit turned on will cause the driver to do a little
>>>> extra work, but it shouldn't cause any harm.
>>>>
>>> I have encountered the following situation if EHCI runtime suspend is
>>> enabled by default.
>>>
>>>
>>>
>>> 1.Wake from system to disk and boot OS.
>>
>> You're talking about resuming after hibernation, right?
>
> You're right.
>>> 2.EHCI will entry runtime suspend after enumerated by driver during boot
>>> phase of suspend to disk
>>
>> I'm not sure what you mean by "boot phase of suspend to disk".  This is
>> while the restore kernel is starting up at the beginning of resume from
>> hibernation, right?
>>
> You understood exactly what I was saying.
>
>
>>> 3.EHCI will be placed to freeze state and ehci_resume is called after image
>>> is loaded.
>>
>> ehci_resume is called to leave runtime suspend.  Going into the freeze
>> state doesn't require any changes.
>>
>>> 4.If PCD flag is set(caused by patch), then HCD_FLAG_RH_RUNNING will be set.
>>>
>>> 5.Pci_pm_freeze_noirq is called to check ehci root hub state and return
>>> value is -EBUSY. which will cause
>>>   quiesce phase of suspend to disk fail.
>>
>> You're talking about check_root_hub_suspended() in hcd-pci.c, right?
>>
> It's right.
>> You know, I'm not at all certain that the callbacks for freeze and
>> freeze_noirq should ever return anything other than 0.  It's okay for
>> them to call check_root_hub_suspended(), but they should ignore its
>> return value.
>>
>> Can you check if the patch below helps?
>>
>> Alan Stern
>>
>>
>> Index: usb-devel/drivers/usb/core/hcd-pci.c
>> ===================================================================
>> --- usb-devel.orig/drivers/usb/core/hcd-pci.c
>> +++ usb-devel/drivers/usb/core/hcd-pci.c
>> @@ -575,6 +575,12 @@ static int hcd_pci_resume(struct device
>>       return resume_common(dev, PM_EVENT_RESUME);
>>   }
>> +static int hcd_pci_freeze_check(struct device *dev)
>> +{
>> +    (void) check_root_hub_suspended(dev);
>> +    return 0;
>> +}
>> +
>>   static int hcd_pci_restore(struct device *dev)
>>   {
>>       return resume_common(dev, PM_EVENT_RESTORE);
>> @@ -586,6 +592,7 @@ static int hcd_pci_restore(struct device
>>   #define hcd_pci_suspend_noirq    NULL
>>   #define hcd_pci_resume_noirq    NULL
>>   #define hcd_pci_resume        NULL
>> +#define hcd_pci_freeze_check    NULL
>>   #define hcd_pci_restore        NULL
>>   #endif    /* CONFIG_PM_SLEEP */
>> @@ -616,8 +623,8 @@ const struct dev_pm_ops usb_hcd_pci_pm_o
>>       .suspend_noirq    = hcd_pci_suspend_noirq,
>>       .resume_noirq    = hcd_pci_resume_noirq,
>>       .resume        = hcd_pci_resume,
>> -    .freeze        = check_root_hub_suspended,
>> -    .freeze_noirq    = check_root_hub_suspended,
>> +    .freeze        = hcd_pci_freeze_check,
>> +    .freeze_noirq    = hcd_pci_freeze_check,
>
> This patch can fix pci driver's fail check in freeze_noirq phase.
>
> Restoring system state from a hibernation image can continue and success.
>
> Weitao Wang

This patch seems work for our platform, but I don't know if there will
be side-effects for others. So I want to take your previous suggestion
and not to clear PCD, As ehci runtime suspend enabled at OS startup is
not a real requirement. I'll resubmit revised patch without the whitespace
damage.
Thanks for your great help.

Best Wishes
Weitao Wang
>>       .thaw_noirq    = NULL,
>>       .thaw        = NULL,
>>       .poweroff    = hcd_pci_suspend,
>> .
>>

\
 
 \ /
  Last update: 2022-03-22 14:52    [W:0.073 / U:0.576 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site