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 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.
>
> 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