diff mbox series

[v1] PM: runtime: Unify error handling during suspend and resume

Message ID 1922654.tdWV9SEqCh@rjwysocki.net
State Superseded
Headers show
Series [v1] PM: runtime: Unify error handling during suspend and resume | expand

Commit Message

Rafael J. Wysocki Feb. 20, 2025, 8:18 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There is a confusing difference in error handling between rpm_suspend()
and rpm_resume() related to the special way in which the -EAGAIN and
-EBUSY error values are treated by the former.  Also, converting
-EACCES coming from the callback to an I/O error, which it quite likely
is not, may confuse runtime PM users a bit.

To address the above, modify rpm_callback() to convert -EACCES coming
from the driver to -EAGAIN and to set power.runtime_error only if the
return value is not -EAGAIN or -EBUSY.

This will cause the error handling in rpm_resume() and rpm_suspend() to
work consistently, so drop the no longer needed -EAGAIN or -EBUSY
special case from the latter and make it retry autosuspend if
power.runtime_error is unset.

Link: https://lore.kernel.org/linux-pm/20220620144231.GA23345@axis.com/
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/runtime.c |   34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

Comments

Raag Jadav Feb. 24, 2025, 12:56 p.m. UTC | #1 
On Mon, Feb 24, 2025 at 01:39:14PM +0100, Rafael J. Wysocki wrote:
> On Sun, Feb 23, 2025 at 4:42 PM Raag Jadav <raag.jadav@intel.com> wrote:
> >
> > On Sun, Feb 23, 2025 at 01:56:07PM +0100, Rafael J. Wysocki wrote:
> > > On Sun, Feb 23, 2025 at 8:33 AM Raag Jadav <raag.jadav@intel.com> wrote:
> > > >
> > > > On Thu, Feb 20, 2025 at 09:18:23PM +0100, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > There is a confusing difference in error handling between rpm_suspend()
> > > > > and rpm_resume() related to the special way in which the -EAGAIN and
> > > > > -EBUSY error values are treated by the former.  Also, converting
> > > > > -EACCES coming from the callback to an I/O error, which it quite likely
> > > > > is not, may confuse runtime PM users a bit.
> > > > >
> > > > > To address the above, modify rpm_callback() to convert -EACCES coming
> > > > > from the driver to -EAGAIN and to set power.runtime_error only if the
> > > > > return value is not -EAGAIN or -EBUSY.
> > > > >
> > > > > This will cause the error handling in rpm_resume() and rpm_suspend() to
> > > > > work consistently, so drop the no longer needed -EAGAIN or -EBUSY
> > > > > special case from the latter and make it retry autosuspend if
> > > > > power.runtime_error is unset.
> > > > >
> > > > > Link: https://lore.kernel.org/linux-pm/20220620144231.GA23345@axis.com/
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > ---
> > > > >  drivers/base/power/runtime.c |   34 ++++++++++++++++++----------------
> > > > >  1 file changed, 18 insertions(+), 16 deletions(-)
> > > > >
> > > > > --- a/drivers/base/power/runtime.c
> > > > > +++ b/drivers/base/power/runtime.c
> > > > > @@ -448,8 +448,13 @@
> > > > >               retval = __rpm_callback(cb, dev);
> > > > >       }
> > > > >
> > > > > -     dev->power.runtime_error = retval;
> > > > > -     return retval != -EACCES ? retval : -EIO;
> > > > > +     if (retval == -EACCES)
> > > > > +             retval = -EAGAIN;
> > > >
> > > > While this is one way to address the problem, are we opening the door
> > > > to changing error codes when convenient? This might lead to different
> > > > kind of confusion from user standpoint.
> > >
> > > Are you saying that if a mistake was made sufficiently long ago, it
> > > can't be fixed any more because someone may be confused?
> >
> > Nothing against the fix but "sufficiently long ago" is why we might
> > have users that rely on it. As long as we don't break anything I don't
> > see a problem.
> >
> > Messing with error codes is usually received with mixed feelings and
> > coming across such a code raises more questions than answers. Perhaps a
> > small explanation might do the trick?
> 
> Do you mean an explanation why -EACCES needs to be converted to something else?
> 
> That's because -EACCES has a special meaning in runtime PM: it means
> that runtime PM is disabled for the given device.

I meant a small comment above for those who may not see it as an obvious
thing, but whatever you think is best.

Raag
diff mbox series

Patch

--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -448,8 +448,13 @@ 
 		retval = __rpm_callback(cb, dev);
 	}
 
-	dev->power.runtime_error = retval;
-	return retval != -EACCES ? retval : -EIO;
+	if (retval == -EACCES)
+		retval = -EAGAIN;
+
+	if (retval != -EAGAIN && retval != -EBUSY)
+		dev->power.runtime_error = retval;
+
+	return retval;
 }
 
 /**
@@ -725,21 +730,18 @@ 
 	dev->power.deferred_resume = false;
 	wake_up_all(&dev->power.wait_queue);
 
-	if (retval == -EAGAIN || retval == -EBUSY) {
-		dev->power.runtime_error = 0;
+	/*
+	 * On transient errors, if the callback routine failed an autosuspend,
+	 * and if the last_busy time has been updated so that there is a new
+	 * autosuspend expiration time, automatically reschedule another
+	 * autosuspend.
+	 */
+	if (!dev->power.runtime_error && (rpmflags & RPM_AUTO) &&
+	    pm_runtime_autosuspend_expiration(dev) != 0)
+		goto repeat;
+
+	pm_runtime_cancel_pending(dev);
 
-		/*
-		 * If the callback routine failed an autosuspend, and
-		 * if the last_busy time has been updated so that there
-		 * is a new autosuspend expiration time, automatically
-		 * reschedule another autosuspend.
-		 */
-		if ((rpmflags & RPM_AUTO) &&
-		    pm_runtime_autosuspend_expiration(dev) != 0)
-			goto repeat;
-	} else {
-		pm_runtime_cancel_pending(dev);
-	}
 	goto out;
 }