Re: [RFC PATCH v2 3/6] usb: add runtime pm support for usb port device

[Lan Tianyu <lantianyu1986-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

于 2012/11/14 20:45, Lan Tianyu 写道:
> 于 2012/11/14 17:49, Rafael J. Wysocki 写道:
>> On Wednesday, November 14, 2012 02:34:37 PM Lan Tianyu wrote:
>>> On 2012年11月14日 08:08, Rafael J. Wysocki wrote:
>>>> On Tuesday, November 13, 2012 04:00:02 PM Lan Tianyu wrote:
>>>>> This patch is to add runtime pm callback for usb port device.
>>>>> Set/clear PORT_POWER feature in the resume/suspend callbak.
>>>>> Add portnum for struct usb_port to record port number.
>>>>>
>>>>> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>
>>>> This one looks reasonable to me.  From the PM side
>>>>
>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> Hi Rafael and Alan:
>>>     This patch has a collaboration problem with pm qos. Since pm core would
>>> pm_runtime_get_sync/put(port_dev) if pm qos flags was changed and port's
>>> suspend call_back() clear PORT_POWER feature without any check. This
>>> will cause PORT_POWER feather being cleared every time after pm qos
>>> flags being changed.
>>>
>>>     I have an idea that add check in the port's runtime idle callback.
>>> Check NO_POWER_OFF flag firstly. If set return. Second, for port without
>>> device, suspend port directly and for port with device, increase child
>>> device's runtime usage without resume and do barrier to ensure all
>>> suspend process finish, at last check the child runtime status. If it
>>> was suspended, suspend port and if do nothing.
>>>
>>> static int usb_port_runtime_idle(struct device *dev)
>>> {
>>>     struct usb_port *port_dev = to_usb_port(dev);
>>>     int retval = 0;
>>>
>>>     if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
>>>             == PM_QOS_FLAGS_ALL)
>>>         return 0;
>>>
>>>     if (!port_dev->child) {
>>>         retval = pm_runtime_suspend(&port_dev->dev);
>>>         if (!retval)
>>>             port_dev->power_on =false;
>>>     }
>>>     else {
>>
>> This usually is written as "} else {" in the kernel code.
>>
>>>         pm_runtime_get_noresume(&port_dev->child->dev);
>>>         pm_runtime_barrier(&port_dev->child->dev);
>>>         if (port_dev->child->dev.power.runtime_status
>>>                 == RPM_SUSPENDED) {
>>>             retval = pm_runtime_suspend(&port_dev->dev);
>>>             if (!retval)
>>>                 port_dev->power_on =false;
>>>         }
>>>         pm_runtime_put_noidle(&port_dev->child->dev);
>>>     }
>>
>> Hmm.  If child->dev is not suspended, then our usage_count should be
>> at least 1, so pm_runtime_suspend(&port_dev->dev) shouldn't actually
>> suspend us.  Isn't that the case?
> No, because the child device is not under port device and so even if
> child->dev is not suspended, port device's usage still can be 0 and
> power off the port.
>>
Please ignore this reply. I may not understand your reply correctly.
You are right if the child->dev is not suspended, the usage count should be at
last 1. But how about if the child->dev is suspended.

Assume that usb device was suspended and power off, so port's usage count must be 0
since it has been suspended. If pm qos NO_POWER_OFF was set at this time, pm core
would get port resume and suspend it again. the usage change 0 - 1 - 0. So port is
power off with NO_POWER_OFF flag setting, Does this make sense?

>> Rafael
>>
>>
>

-- 
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html