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

[Lan Tianyu <lantianyu1986@gmail.com>]

于 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@intel.com>
>>>>
>>>> This one looks reasonable to me.  From the PM side
>>>>
>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> 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.

Maybe I should add pm_runtime_get_noresume(&port_dev->dev) before enable
port's runtime pm. Just like following. Then it will work like you said.

@@ -72,6 +109,8 @@ int usb_hub_create_port_device(struct device *intfdev,
  	if (retval)
  		goto error_register;

+	pm_runtime_set_active(&port_dev->dev);
+	pm_runtime_get_noresume(&port_dev->dev); /* new add */
+	pm_runtime_enable(&port_dev->dev);
>>
>> Rafael
>>
>>
>

-- 
Best regards
Tianyu Lan