Message-ID: <525E3C6D.90803@intel.com>
Date:	Wed, 16 Oct 2013 15:12:45 +0800
From:	Lan Tianyu <tianyu.lan@...el.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
CC:	lenb@...nel.org, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ACPI/Power: Check physical device's runtime pm status
 before requesting to resume it

On 10/16/2013 05:22 AM, Rafael J. Wysocki wrote:
> On Tuesday, October 15, 2013 04:58:28 PM Lan Tianyu wrote:
>> On 2013年10月11日 19:19, Rafael J. Wysocki wrote:
>>> On Friday, October 11, 2013 04:16:25 PM tianyu.lan@...el.com 
>>> wrote:
>>>> From: Lan Tianyu <tianyu.lan@...el.com>
>>>> 
>>>> Currently, when one power resource is turned on, devices owning
>>>> it will be requested to resume regardless of their runtime pm
>>>> status. ACPI power resource maybe turn on in some devices'
>>>> runtime pm resume callback(E.G, usb port) while turning on the
>>>> power resource will trigger one new resume request of the
>>>> device. It causes infinite loop between resume and suspend.
>>>> This has happened on clearing usb port's PM Qos NO_POWER_OFF
>>>> flag twice. This patch is to add check of physical device's
>>>> runtime pm status and request resume if the device is 
>>>> suspended.
>>>> 
>>>> Signed-off-by: Lan Tianyu <tianyu.lan@...el.com> --- 
>>>> drivers/acpi/power.c | 6 ++++-- 1 file changed, 4 
>>>> insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c index
>>>>  0dbe5cd..228c138 100644 --- a/drivers/acpi/power.c +++ 
>>>> b/drivers/acpi/power.c @@ -250,8 +250,10 @@ static void 
>>>> acpi_power_resume_dependent(struct work_struct *work)
>>>> 
>>>> mutex_lock(&adev->physical_node_lock);
>>>> 
>>>> -	list_for_each_entry(pn, &adev->physical_node_list, node) - 
>>>> pm_request_resume(pn->dev); +	list_for_each_entry(pn, 
>>>> &adev->physical_node_list, node) { +		if 
>>>> (pm_runtime_suspended(pn->dev)) + pm_request_resume(pn->dev);
>>>> + }
>>> 
>>> This is racy, because the status may change right after you check
>>> it and before you call pm_request_resume().
>> 
>> Yes, the runtime status may be changed just after the check.
>> 
>>> 
>>> Besides, pm_request_resume() checks the status of the device and
>>>  won't try to resume it if it is not suspended.
>>> 
>> 
>> For this issue, usb port is in the RPM_SUSPENDING state when 
>> pm_request_resume() is called.
> 
> Why exactly does that happen?
> 
>> The deferred_resume will be set to true during this procedure and 
>> trigger resume after finishing suspend. USB port runtime resume 
>> callback will turn on its power resource again and the work of 
>> acpi_power_resume_dependent() is scheduled. Because the usb port's
>>  usage count remains zero, it's to be suspended soon. When 
>> pm_request_resume() of acpi_power_resume_dependent() is called, the
>> usb port is always in the PRM_SUSPENDING. Fall in the loop of 
>> suspend and resume.
>> 
>> How about running acpi_power_dependent when turning on power 
>> resource rather than scheduling a work to run it?
> 
> Is this actually going to help?  Even if 
> acpi_power_resume_dependent() is run synchronously from within the 
> resume callback, it will still see RPM_SUSPENDING as the device's 
> status, won't it?
> 
>> After this, pm_request_resume() can check device's right status 
>> just after turning on power resource.
> 
> The status doesn't change until the .runtime_suspend() callback 
> returns and running pm_request_resume() syncrhonously from that 
> callback for the device being suspended just plain doesn't make 
> sense.
> 
> 
> Can you please explain to me how this is possible that the USB port's
> power resource is turned "on" while the port is RPM_SUSPENDING?
> 

Hi Rafael:

No, I mean the acpi_power_resume_dependent() is running while the port's
status is RPM_SUSPENDING. It runs from a work item and turning on power
resource adds the work to workqueue. There is a timeslot between running
acpi_power_resume_dependent() and turning on power resource. In the
timeslot, the device runtime pm status maybe change.

In this case, changing PM Qos flag will do pm_runtime_get_sync and
pm_runtime_put usb port. The usb port is without device attached and so
it will be resumed and suspended soon during changing PM Qos flag.

Resume callback turns on power resource if NO_POWER_OFF flag has been
cleared and add the work of acpi_power_resume_dependent() to
workqueue. After resume, the usb port will be suspended while the
acpi_power_resume_dependent() is running. pm_request_resume() gets
RPM_SUSPENDING as the usb port's runtime status and set the
deferred_resume of usb port.

After suspend, usb port will be resumed again and turn on power
resource. The work of acpi_power_resume_dependent() is also added to
workqueue. Because the usb port's usage count remains 0 during this
procedure. it will be suspended soon after resume. While suspending,
acpi_power_resume_dependent() is running and pm_request_resume()
sets deferred_resume again. The infinite look starts.

I think the main problem is the timeslot between turning on power
resource and running acpi_power_resume_dependent(). if
acpi_power_resume_dependent() runs synchronously, pm_request_resume()
can check the device's status at the point of turning on power resource.
For this case, the usb port status at that point is RPM_RESUMING and
pm_request_resume() will not trigger a resume.

Attach the ftrace result to show the issue.

>From my opinion, if the timeslot is bigger, the usb port maybe be
already suspended and pm_request_resume() will add a resume work to
pm_wq directly. The timeslot is random and depends on the system status
at that point.

>> Furthermore, pm_request_resume() is async resume and this change 
>> will not consume much time.
> 
> acpi_power_resume_dependent() is run from a work item to avoid 
> locking problems.

Yes, I notice the dead lock issue when I try to make
acpi_power_resume_dependent() run synchronously. I think the
resource_lock mutexes too many things. Especially, mutex to execute
power resource's _ON, _OFF and _STA. We can introduce a new lock to do
it and this can resolve the deadlock issue.











View attachment "trace" of type "text/plain" (132928 bytes)

Powered by blists - more mailing lists