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

Posted by Rafael J. Wysocki 9 months, 4 weeks ago

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;
+
+	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;
 }

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

Posted by Raag Jadav 9 months, 3 weeks ago

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.

Raag

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

Posted by Rafael J. Wysocki 9 months, 3 weeks ago

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?

In that case, I'd like to know who may be confused and why.

Thanks!

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

Posted by Raag Jadav 9 months, 3 weeks ago

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?

Raag

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

Posted by Rafael J. Wysocki 9 months, 3 weeks ago

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.

Thanks!

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

Posted by Raag Jadav 9 months, 3 weeks ago

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