| Message ID | 1358185698.1223.13.camel@cliu38-desktop-build (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Tuesday, January 15, 2013 01:48:18 AM Chuansheng Liu wrote: > > In function rpm_suspend/resume(), when going into the for(;;), > the pre-condition judgement has been done, and the variable runtime_status > are always protected by &power.lock, so it is not necessary to judge > them again before unlock_irq &power.lock in for(;;). > > This patch clean them up. Well, I don't really think this fixes anything. Yes, we may save one check here and there, but the current code follows the wait_event() convention. Thanks, Rafael > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > --- > drivers/base/power/runtime.c | 18 +++++++++--------- > 1 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 3148b10..32d6497 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -377,14 +377,14 @@ static int rpm_suspend(struct device *dev, int rpmflags) > for (;;) { > prepare_to_wait(&dev->power.wait_queue, &wait, > TASK_UNINTERRUPTIBLE); > - if (dev->power.runtime_status != RPM_SUSPENDING) > - break; > > spin_unlock_irq(&dev->power.lock); > > schedule(); > > spin_lock_irq(&dev->power.lock); > + if (dev->power.runtime_status != RPM_SUSPENDING) > + break; > } > finish_wait(&dev->power.wait_queue, &wait); > goto repeat; > @@ -557,15 +557,15 @@ static int rpm_resume(struct device *dev, int rpmflags) > for (;;) { > prepare_to_wait(&dev->power.wait_queue, &wait, > TASK_UNINTERRUPTIBLE); > - if (dev->power.runtime_status != RPM_RESUMING > - && dev->power.runtime_status != RPM_SUSPENDING) > - break; > > spin_unlock_irq(&dev->power.lock); > > schedule(); > > spin_lock_irq(&dev->power.lock); > + if (dev->power.runtime_status != RPM_RESUMING > + && dev->power.runtime_status != RPM_SUSPENDING) > + break; > } > finish_wait(&dev->power.wait_queue, &wait); > goto repeat; > @@ -989,15 +989,15 @@ static void __pm_runtime_barrier(struct device *dev) > for (;;) { > prepare_to_wait(&dev->power.wait_queue, &wait, > TASK_UNINTERRUPTIBLE); > - if (dev->power.runtime_status != RPM_SUSPENDING > - && dev->power.runtime_status != RPM_RESUMING > - && !dev->power.idle_notification) > - break; > spin_unlock_irq(&dev->power.lock); > > schedule(); > > spin_lock_irq(&dev->power.lock); > + if (dev->power.runtime_status != RPM_SUSPENDING > + && dev->power.runtime_status != RPM_RESUMING > + && !dev->power.idle_notification) > + break; > } > finish_wait(&dev->power.wait_queue, &wait); > } >
On Mon, 14 Jan 2013, Rafael J. Wysocki wrote: > On Tuesday, January 15, 2013 01:48:18 AM Chuansheng Liu wrote: > > > > In function rpm_suspend/resume(), when going into the for(;;), > > the pre-condition judgement has been done, and the variable runtime_status > > are always protected by &power.lock, so it is not necessary to judge > > them again before unlock_irq &power.lock in for(;;). > > > > This patch clean them up. > > Well, I don't really think this fixes anything. Yes, we may save one check > here and there, but the current code follows the wait_event() convention. Indeed, this patch introduces a race. If runtime_status changes from RPM_SUSPENDING and power.wait_queue is signalled in between the test at the end of the loop and the prepare_to_wait() call, the loop will never end. Alan Stern > Thanks, > Rafael > > > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > > --- > > drivers/base/power/runtime.c | 18 +++++++++--------- > > 1 files changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index 3148b10..32d6497 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -377,14 +377,14 @@ static int rpm_suspend(struct device *dev, int rpmflags) > > for (;;) { > > prepare_to_wait(&dev->power.wait_queue, &wait, > > TASK_UNINTERRUPTIBLE); > > - if (dev->power.runtime_status != RPM_SUSPENDING) > > - break; > > > > spin_unlock_irq(&dev->power.lock); > > > > schedule(); > > > > spin_lock_irq(&dev->power.lock); > > + if (dev->power.runtime_status != RPM_SUSPENDING) > > + break; > > } -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Alan Stern [mailto:stern@rowland.harvard.edu] > Sent: Monday, January 14, 2013 11:30 PM > To: Rafael J. Wysocki > Cc: Liu, Chuansheng; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] PM / Runtime: Fix the twice judgement in > rpm_suspend/resume() > > On Mon, 14 Jan 2013, Rafael J. Wysocki wrote: > > > On Tuesday, January 15, 2013 01:48:18 AM Chuansheng Liu wrote: > > > > > > In function rpm_suspend/resume(), when going into the for(;;), > > > the pre-condition judgement has been done, and the variable > runtime_status > > > are always protected by &power.lock, so it is not necessary to judge > > > them again before unlock_irq &power.lock in for(;;). > > > > > > This patch clean them up. > > > > Well, I don't really think this fixes anything. Yes, we may save one check > > here and there, but the current code follows the wait_event() convention. OK, thanks. > > Indeed, this patch introduces a race. If runtime_status changes from > RPM_SUSPENDING and power.wait_queue is signalled in between the test at > the end of the loop and the prepare_to_wait() call, the loop will never > end. Checking the race case, it should not happen? Updating runtime_status and wake_up wait_queue are protected by power.lock. spin_lock(&power.lock) ... __update_runtime_status() ... wake_up_all(&dev->power.wait_queue) ... spin_unlock(&power.lock) > > Alan Stern > > > Thanks, > > Rafael > > > > > > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > > > --- > > > drivers/base/power/runtime.c | 18 +++++++++--------- > > > 1 files changed, 9 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > > index 3148b10..32d6497 100644 > > > --- a/drivers/base/power/runtime.c > > > +++ b/drivers/base/power/runtime.c > > > @@ -377,14 +377,14 @@ static int rpm_suspend(struct device *dev, int > rpmflags) > > > for (;;) { > > > prepare_to_wait(&dev->power.wait_queue, &wait, > > > TASK_UNINTERRUPTIBLE); > > > - if (dev->power.runtime_status != RPM_SUSPENDING) > > > - break; > > > > > > spin_unlock_irq(&dev->power.lock); > > > > > > schedule(); > > > > > > spin_lock_irq(&dev->power.lock); > > > + if (dev->power.runtime_status != RPM_SUSPENDING) > > > + break; > > > } -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 15 Jan 2013, Liu, Chuansheng wrote: > > Indeed, this patch introduces a race. If runtime_status changes from > > RPM_SUSPENDING and power.wait_queue is signalled in between the test at > > the end of the loop and the prepare_to_wait() call, the loop will never > > end. > Checking the race case, it should not happen? > Updating runtime_status and wake_up wait_queue are protected by power.lock. > spin_lock(&power.lock) > ... > __update_runtime_status() > ... > wake_up_all(&dev->power.wait_queue) > ... > spin_unlock(&power.lock) You're right, the lock prevents this race from happening. On the other hand, with your change we would end up calling finish_wait() without calling prepare_to_wait() first. And as Rafael mentioned, the current code fits the pattern that people expect to see. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 3148b10..32d6497 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -377,14 +377,14 @@ static int rpm_suspend(struct device *dev, int rpmflags) for (;;) { prepare_to_wait(&dev->power.wait_queue, &wait, TASK_UNINTERRUPTIBLE); - if (dev->power.runtime_status != RPM_SUSPENDING) - break; spin_unlock_irq(&dev->power.lock); schedule(); spin_lock_irq(&dev->power.lock); + if (dev->power.runtime_status != RPM_SUSPENDING) + break; } finish_wait(&dev->power.wait_queue, &wait); goto repeat; @@ -557,15 +557,15 @@ static int rpm_resume(struct device *dev, int rpmflags) for (;;) { prepare_to_wait(&dev->power.wait_queue, &wait, TASK_UNINTERRUPTIBLE); - if (dev->power.runtime_status != RPM_RESUMING - && dev->power.runtime_status != RPM_SUSPENDING) - break; spin_unlock_irq(&dev->power.lock); schedule(); spin_lock_irq(&dev->power.lock); + if (dev->power.runtime_status != RPM_RESUMING + && dev->power.runtime_status != RPM_SUSPENDING) + break; } finish_wait(&dev->power.wait_queue, &wait); goto repeat; @@ -989,15 +989,15 @@ static void __pm_runtime_barrier(struct device *dev) for (;;) { prepare_to_wait(&dev->power.wait_queue, &wait, TASK_UNINTERRUPTIBLE); - if (dev->power.runtime_status != RPM_SUSPENDING - && dev->power.runtime_status != RPM_RESUMING - && !dev->power.idle_notification) - break; spin_unlock_irq(&dev->power.lock); schedule(); spin_lock_irq(&dev->power.lock); + if (dev->power.runtime_status != RPM_SUSPENDING + && dev->power.runtime_status != RPM_RESUMING + && !dev->power.idle_notification) + break; } finish_wait(&dev->power.wait_queue, &wait); }
In function rpm_suspend/resume(), when going into the for(;;), the pre-condition judgement has been done, and the variable runtime_status are always protected by &power.lock, so it is not necessary to judge them again before unlock_irq &power.lock in for(;;). This patch clean them up. Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> --- drivers/base/power/runtime.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)