On 4/29/2019 21:43, Doug Anderson wrote:
> Hi,
>
> On Mon, Apr 29, 2019 at 5:01 AM Artur Petrosyan
> <Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On 4/27/2019 01:01, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
>>> <Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
>>>>
>>>> Added a new flow of entering and exiting hibernation when PC is
>>>> hibernated or suspended.
>>>>
>>>> Signed-off-by: Artur Petrosyan <arturp@xxxxxxxxxxxx>
>>>> ---
>>>> drivers/usb/dwc2/hcd.c | 128 +++++++++++++++++++++++++++++++------------------
>>>> 1 file changed, 81 insertions(+), 47 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>> index 45d4a3e1ebd2..f1e92a287cb1 100644
>>>> --- a/drivers/usb/dwc2/hcd.c
>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>> @@ -4510,35 +4510,54 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>>>> if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
>>>> goto unlock;
>>>>
>>>> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
>>>> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE ||
>>>> hsotg->flags.b.port_connect_status == 0)
>>>> goto skip_power_saving;
>>>>
>>>> - /*
>>>> - * Drive USB suspend and disable port Power
>>>> - * if usb bus is not suspended.
>>>> - */
>>>> - if (!hsotg->bus_suspended) {
>>>> - hprt0 = dwc2_read_hprt0(hsotg);
>>>> - hprt0 |= HPRT0_SUSP;
>>>> - hprt0 &= ~HPRT0_PWR;
>>>> - dwc2_writel(hsotg, hprt0, HPRT0);
>>>> - spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> - dwc2_vbus_supply_exit(hsotg);
>>>> - spin_lock_irqsave(&hsotg->lock, flags);
>>>> - }
>>>> + switch (hsotg->params.power_down) {
>>>> + case DWC2_POWER_DOWN_PARAM_PARTIAL:
>>>> + /*
>>>> + * Drive USB suspend and disable port Power
>>>> + * if usb bus is not suspended.
>>>> + */
>>>> + if (!hsotg->bus_suspended) {
>>>> + hprt0 = dwc2_read_hprt0(hsotg);
>>>> + hprt0 |= HPRT0_SUSP;
>>>> + hprt0 &= ~HPRT0_PWR;
>>>> + dwc2_writel(hsotg, hprt0, HPRT0);
>>>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> + dwc2_vbus_supply_exit(hsotg);
>>>> + spin_lock_irqsave(&hsotg->lock, flags);
>>>> + }
>>>>
>>>> - /* Enter partial_power_down */
>>>> - ret = dwc2_enter_partial_power_down(hsotg);
>>>> - if (ret) {
>>>> - if (ret != -ENOTSUPP)
>>>> - dev_err(hsotg->dev,
>>>> - "enter partial_power_down failed\n");
>>>> + /* Enter partial_power_down */
>>>> + ret = dwc2_enter_partial_power_down(hsotg);
>>>> + if (ret) {
>>>> + if (ret != -ENOTSUPP)
>>>> + dev_err(hsotg->dev,
>>>> + "enter partial_power_down failed\n");
>>>> + goto skip_power_saving;
>>>> + }
>>>> + hsotg->bus_suspended = true;
>>>> + break;
>>>> + case DWC2_POWER_DOWN_PARAM_HIBERNATION:
>>>> + if (!hsotg->bus_suspended) {
>>>
>>> Do you have any idea why for DWC2_POWER_DOWN_PARAM_PARTIAL we still
>>> call dwc2_enter_partial_power_down() even if bus_suspended is true,
>>> but for hibernate you don't call dwc2_enter_hibernation()?
>> For Hibernation I do call dwc2_enter_hibernation().
>
> Maybe you didn't understand the question. I'll be clearer.
>
> Imagine _dwc2_hcd_suspend() is called but "bus_suspended" is already
> true at the start of the function.
>
> If we're in DWC2_POWER_DOWN_PARAM_PARTIAL, _dwc2_hcd_suspend() _will_
> call dwc2_enter_partial_power_down()
>
> If we're in DWC2_POWER_DOWN_PARAM_HIBERNATION, _dwc2_hcd_suspend()
> _will NOT_ call dwc2_enter_partial_power_down()
So what? why should dwc2_enter_partial_power_down() be called when we
have power_down == DWC2_POWER_DOWN_PARAM_HIBERNATION ?
So what the switch statement basically does is that it checks if core
supports partial power down then it calls dwc2_enter_partial_power_down()
if core supports hibernation it calls
dwc2_enter_hibernation();
>
>
> This is all part of the whole asymmetry between PARTIAL and
> HIBERNATION that makes it hard to understand.
See partial power down and hibernation are actually two separate
implementations.
1. When core supports partial power down we enter to partial power down.
2. When core supports Hibernation we enter to Hibernation
3. If core doesn't support none of them we just skip the power saving.
So the switch statement checks the availability of one of the power
saving modes and then calls either dwc2_enter_hibernation() or
dwc2_enter_partial_power_down().
>
>
>>>> + /* Enter hibernation */
>>>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> + ret = dwc2_enter_hibernation(hsotg, 1);
>>>> + spin_lock_irqsave(&hsotg->lock, flags);
>>>> + if (ret && ret != -ENOTSUPP)
>>>> + dev_err(hsotg->dev,
>>>> + "%s: enter hibernation failed\n",
>>>> + __func__);
>>>
>>> nit: no __func__ in dev_xxx() error messages. The device plus the
>>> message should be enough. Only resort to __func__ if you're forced to
>>> do an error message without a "struct device *".
>> This code comes form previous implementations I have not touched it not
>> to back anything.
>
> Please fix. Even if you had internal code that did this it still
> needs to be fixed when going upstream. It is highly unlikely you'll
> break something when removing something like this.
>
>
>
>>> nit: as per my comments in an earlier patch, remove special case for -ENOTSUPP
>>>
>>> Also, presumably you want to match the error handling in
>>> DWC2_POWER_DOWN_PARAM_PARTIAL and do a "goto skip_power_saving" when
>>> you see an error?
>> When there is an error power_saving should be skipped.
>
> OK, so you agree?
Agree that if there is an error en entering to hibernation then power
saving should be skipped.
>
>
>>>> + } else {
>>>> + goto skip_power_saving;
>>>> + }
>>>> + break;
>>>> + default:
>>>> goto skip_power_saving;
>>>> }
>>>>
>>>> - hsotg->bus_suspended = true;
>>>> -
>>>
>>> It's a bit weird to remove this, but I guess it just got moved to the
>>> partial power down case? ...and in the hibernate case you're relying
>>> on the hibernate function to set this? Yet another frustratingly
>>> asymmetric code structure...
>> Enter hibernation implements setting bus_suspend so I don't touch this.
>> Actually this patch set fixes issues it doesn't clean up everything
>> related to hibernation or partial power down.
>
> Yet more asymmetry.
>
>
>>>> /* Ask phy to be suspended */
>>>> if (!IS_ERR_OR_NULL(hsotg->uphy)) {
>>>> spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> @@ -4564,17 +4583,17 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>>>> int ret = 0;
>>>> u32 hprt0;
>>>>
>>>> - hprt0 = dwc2_read_hprt0(hsotg);
>>>> -
>>>> spin_lock_irqsave(&hsotg->lock, flags);
>>>>
>>>> - if (dwc2_is_device_mode(hsotg))
>>>> + if (!hsotg->bus_suspended)
>>>
>>> As per my comments above I don't have a good grasp on what
>>> "bus_suspended" is for. ...that being said, if your change here is
>>> actually correct then you probably (?) want to remove the "if
>>> (hsotg->bus_suspended)" check later in this same function.
>>>
>>> Said another way, you've now got code that looks like:
>>>
>>> if (!hsotg->bus_suspended)
>>> goto exit;
>>>
>>> /* code that I think doesn't touch bus_suspended */
>>>
>>> if (hsotg->bus_suspended) {
>>> /* do something */
>>> } else {
>>> /* do something else */
>>> }
>>>
>>> Presumably the "do something" is now dead code?
>>>
>> That part is not dad because if hsotg->bus_suspended is true
>> resuming/exiting from suspend/partial power down/hibernation should be
>> performed.
>> On the other hand if hsotg->bus_suspended is false there is no need to
>> resume.
>>
>> So of course if core is not suspended the code responsible for resuming
>> should not be called. In that sense the code can be called dead.
>
> I think I didn't explain it well. Does this patch help you?
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2B_4e84efdbeb74bcb8b24e2b1fea24153981acc185-255E-2521_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=nZWkYoZKaqSi_evQfEH0uCUKIXb5UZydmgRSXsVwSOs&s=h-ZqBu_CidHWrCH6sAbTi1ddgSfHkYjwSfCOGjpHbRM&e=
No it doesn't.
This is diff from your suggestion.
hsotg->lx_state = DWC2_L0;
+ hsotg->flags.b.port_suspend_change = 1;
Why are you setting this ?
spin_unlock_irqrestore(&hsotg->lock, flags);
-
- if (hsotg->bus_suspended) {
- spin_lock_irqsave(&hsotg->lock, flags);
- hsotg->flags.b.port_suspend_change = 1;
- spin_unlock_irqrestore(&hsotg->lock, flags);
- dwc2_port_resume(hsotg);
You have removed this but it should be called when we exit from partial
power down.
Have you performed testes on this? I am not sure that this is a working
code.
- } else {
- dwc2_vbus_supply_init(hsotg);
-
- /* Wait for controller to correctly update D+/D- level */
- usleep_range(3000, 5000);
-
- /*
- * Clear Port Enable and Port Status changes.
- * Enable Port Power.
- */
- dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET |
- HPRT0_ENACHG, HPRT0);
- /* Wait for controller to detect Port Connect */
- usleep_range(5000, 7000);
- }
+ dwc2_port_resume(hsotg);
break;
>
>
>>>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> +
>>>
>>> I'm pretty curious if you tested DWC2_POWER_DOWN_PARAM_PARTIAL after
>>> applying your patch series. As far as I can tell your switch
>>> statement for DWC2_POWER_DOWN_PARAM_PARTIAL will "break" with the
>>> spinlock already unlocked. ...so you'll run spin_unlock_irqrestore
>>> twice. Is that really legit?
>> I have tested the patches on HAPS-DX and Linaro HiKey 960 boards.
>
> How is it possible that you don't end up spin unlocking more than
> once? This seems like a pretty serious problem.
>
> My guess is that whatever tests you ran didn't actually exercise this
> function. Personally I could only get it to exercise by doing
> suspend/resume.
>
> ...and when I did, sure enough I saw:
>
> BUG: spinlock already unlocked on CPU#2, kworker/u8:32/5812
I will debug it once more.
>
>
>>> ...with that a quick test seems to show that partial power down is
>>> sorta working on rk3288 now. I _think_ I saw one case where hotplug
>>> failed but I've seen several where it works. ...unfortunately it
>>> seems to break when I do hotplug on the port where I have
>>> "snps,reset-phy-on-wake" set.
>> You can provide debug logs for that scenario I will try to help you fix
>> issues with that.
>
> All you need to do is make sure you run the function
> _dwc2_hcd_suspend() with power down mode set to
> "DWC2_POWER_DOWN_PARAM_PARTIAL". ...or walk through the code and see
> that spin_unlock_irqrestore() will certainly be called twice.
I hibernate the PC it enters to partial power down then I turn PC on
everything works fine. But I will check this one more time.
>
>
>
> -Doug
>
--
Regards,
Artur
