From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Lan Tianyu <tianyu.lan@intel.com>
Cc: lenb@kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ACPI/Power: Check physical device's runtime pm status before requesting to resume it
Date: Tue, 15 Oct 2013 23:22:28 +0200
Message-ID: <16284735.7LFgSiThhv@vostro.rjw.lan>
User-Agent: KMail/4.10.5 (Linux/3.11.0+; KDE/4.10.5; x86_64; ; )
In-Reply-To: <525D03B4.8050709@intel.com>
References: <1381479385-1614-1-git-send-email-tianyu.lan@intel.com> <1761658.DWcBvZSdOl@vostro.rjw.lan> <525D03B4.8050709@intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8BIT
Content-Type: text/plain; charset="utf-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3762
Lines: 90

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@intel.com wrote:
> >> From: Lan Tianyu <tianyu.lan@intel.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@intel.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.

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

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?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/