| Message ID | 20220620144231.GA23345@axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | PM runtime_error handling missing in many drivers? | expand |
On 20.06.22 16:42, Vincent Whitchurch wrote: Hi, > Many drivers do something like the following to resume their hardware > before performing some hardware access when user space ask for it: > > ret = pm_runtime_resume_and_get(dev); > if (ret) > return ret; > > But if the ->runtime_resume() callback fails, then the > power.runtime_error is set and any further attempts to use > pm_runtime_resume_and_get() will fail, as documented in > Documentation/power/runtime_pm.rst. Whether this is properly documented is debatable. 4. Runtime PM Device Helper Functions (as a chapter) does _not_ document it. > [110778.050000][ T27] rpm_resume: 0-0009 flags-4 cnt-1 dep-0 auto-1 p-0 irq-0 child-0 > [110778.050000][ T27] rpm_return_int: rpm_resume+0x24d/0x11d0:0-0009 ret=-22 > > The following patch fixes the issue on vcnl4000, but is this the right in the > fix? And, unless I'm missing something, there are dozens of drivers > with the same problem. Yes. The point of pm_runtime_resume_and_get() is to remove the need for handling errors when the resume fails. So I fail to see why a permanent record of a failure makes sense for this API. > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c > index e02e92bc2928..082b8969fe2f 100644 > --- a/drivers/iio/light/vcnl4000.c > +++ b/drivers/iio/light/vcnl4000.c > @@ -414,6 +414,8 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on) > > if (on) { > ret = pm_runtime_resume_and_get(dev); > + if (ret) > + pm_runtime_set_suspended(dev); > } else { > pm_runtime_mark_last_busy(dev); > ret = pm_runtime_put_autosuspend(dev); If you need to add this to every driver, you can just as well add it to pm_runtime_resume_and_get() to avoid the duplication. But I am afraid we need to ask a deeper question. Is there a point in recording failures to resume? The error code is reported back. If a driver wishes to act upon it, it can. The core really only uses the result to block new PM operations. But nobody requests a resume unless it is necessary. Thus I fail to see the point of checking this flag in resume as opposed to suspend. If we fail, we fail, why not retry? It seems to me that the record should be used only during runtime suspend. And as an immediate band aid, some errors like ENOMEM should never be recorded. Regards Oliver
On Tue, Jun 21, 2022 at 11:38:33AM +0200, Oliver Neukum wrote: > On 20.06.22 16:42, Vincent Whitchurch wrote: > > [110778.050000][ T27] rpm_resume: 0-0009 flags-4 cnt-1 dep-0 auto-1 p-0 irq-0 child-0 > > [110778.050000][ T27] rpm_return_int: rpm_resume+0x24d/0x11d0:0-0009 ret=-22 > > > > The following patch fixes the issue on vcnl4000, but is this the right in the > > fix? And, unless I'm missing something, there are dozens of drivers > > with the same problem. > > Yes. The point of pm_runtime_resume_and_get() is to remove the need > for handling errors when the resume fails. So I fail to see why a > permanent record of a failure makes sense for this API. I don't understand it either. > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c > > index e02e92bc2928..082b8969fe2f 100644 > > --- a/drivers/iio/light/vcnl4000.c > > +++ b/drivers/iio/light/vcnl4000.c > > @@ -414,6 +414,8 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on) > > > > if (on) { > > ret = pm_runtime_resume_and_get(dev); > > + if (ret) > > + pm_runtime_set_suspended(dev); > > } else { > > pm_runtime_mark_last_busy(dev); > > ret = pm_runtime_put_autosuspend(dev); > > If you need to add this to every driver, you can just as well add it to > pm_runtime_resume_and_get() to avoid the duplication. Yes, the documentation says that the error should be cleared, but it's unclear why the driver is expected to do it. From the documentation it looks the driver is supposed to choose between pm_runtime_set_active() and pm_runtime_set_suspended() to clear the error, but how/why is this choice supposed to be made in the driver when the driver doesn't know more than the framework about the status of the device? Perhaps Rafael can shed some light on this. > But I am afraid we need to ask a deeper question. Is there a point > in recording failures to resume? The error code is reported back. > If a driver wishes to act upon it, it can. The core really only > uses the result to block new PM operations. > But nobody requests a resume unless it is necessary. Thus I fail > to see the point of checking this flag in resume as opposed to > suspend. If we fail, we fail, why not retry? It seems to me that the > record should be used only during runtime suspend. I guess this is also a question for Rafael. Even if the error recording is removed from runtime_resume and only done on suspend failures, all these drivers still have the problem of not clearing the error, since the next resume will fail if that is not done. > And as an immediate band aid, some errors like ENOMEM should > never be recorded.
On 7/8/2022 1:03 PM, Vincent Whitchurch wrote: > On Tue, Jun 21, 2022 at 11:38:33AM +0200, Oliver Neukum wrote: >> On 20.06.22 16:42, Vincent Whitchurch wrote: >>> [110778.050000][ T27] rpm_resume: 0-0009 flags-4 cnt-1 dep-0 auto-1 p-0 irq-0 child-0 >>> [110778.050000][ T27] rpm_return_int: rpm_resume+0x24d/0x11d0:0-0009 ret=-22 >>> >>> The following patch fixes the issue on vcnl4000, but is this the right in the >>> fix? And, unless I'm missing something, there are dozens of drivers >>> with the same problem. >> Yes. The point of pm_runtime_resume_and_get() is to remove the need >> for handling errors when the resume fails. So I fail to see why a >> permanent record of a failure makes sense for this API. > I don't understand it either. > >>> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c >>> index e02e92bc2928..082b8969fe2f 100644 >>> --- a/drivers/iio/light/vcnl4000.c >>> +++ b/drivers/iio/light/vcnl4000.c >>> @@ -414,6 +414,8 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on) >>> >>> if (on) { >>> ret = pm_runtime_resume_and_get(dev); >>> + if (ret) >>> + pm_runtime_set_suspended(dev); >>> } else { >>> pm_runtime_mark_last_busy(dev); >>> ret = pm_runtime_put_autosuspend(dev); >> If you need to add this to every driver, you can just as well add it to >> pm_runtime_resume_and_get() to avoid the duplication. > Yes, the documentation says that the error should be cleared, but it's > unclear why the driver is expected to do it. From the documentation it > looks the driver is supposed to choose between pm_runtime_set_active() > and pm_runtime_set_suspended() to clear the error, but how/why is this > choice supposed to be made in the driver when the driver doesn't know > more than the framework about the status of the device? > > Perhaps Rafael can shed some light on this. The driver always knows more than the framework about the device's actual state. The framework only knows that something failed, but it doesn't know what it was and what way it failed. >> But I am afraid we need to ask a deeper question. Is there a point >> in recording failures to resume? The error code is reported back. >> If a driver wishes to act upon it, it can. The core really only >> uses the result to block new PM operations. >> But nobody requests a resume unless it is necessary. Thus I fail >> to see the point of checking this flag in resume as opposed to >> suspend. If we fail, we fail, why not retry? It seems to me that the >> record should be used only during runtime suspend. > I guess this is also a question for Rafael. > > Even if the error recording is removed from runtime_resume and only done > on suspend failures, all these drivers still have the problem of not > clearing the error, since the next resume will fail if that is not done. The idea was that drivers would clear these errors. >> And as an immediate band aid, some errors like ENOMEM should >> never be recorded.
On 08.07.22 22:10, Rafael J. Wysocki wrote: > On 7/8/2022 1:03 PM, Vincent Whitchurch wrote: >> Perhaps Rafael can shed some light on this. > > The driver always knows more than the framework about the device's > actual state. The framework only knows that something failed, but it > doesn't know what it was and what way it failed. Hi, thinking long and deeply about this I do not think that this seemingly obvious assertion is actually correct. > The idea was that drivers would clear these errors. I am afraid that is a deeply hidden layering violation. Yes, a driver's resume() method may have failed. In that case, if that is the same driver, it will obviously already know about the failure. PM operations, however, are operating on a tree. A driver requesting a resume may get an error code. But it has no idea where this error comes from. The generic code knows at least that. Let's look at at a USB storage device. The request to resume comes from sd.c. sd.c is certainly not equipped to handle a PCI error condition that has prevented a USB host controller from resuming. I am afraid this part of the API has issues. And they keep growing the more we divorce the device driver from the bus driver, which actually does the PM operation. Regards Oliver
On Tue, Jul 26, 2022 at 11:05 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > On 08.07.22 22:10, Rafael J. Wysocki wrote: > > On 7/8/2022 1:03 PM, Vincent Whitchurch wrote: > > >> Perhaps Rafael can shed some light on this. > > > > The driver always knows more than the framework about the device's > > actual state. The framework only knows that something failed, but it > > doesn't know what it was and what way it failed. > > Hi, > > thinking long and deeply about this I do not think that this seemingly > obvious assertion is actually correct. I guess that depends on what is regarded as "the framework". I mean the PM-runtime code, excluding the bus type or equivalent. > > The idea was that drivers would clear these errors. > > I am afraid that is a deeply hidden layering violation. Yes, a driver's > resume() method may have failed. In that case, if that is the same > driver, it will obviously already know about the failure. So presumably it will do something to recover and avoid returning the error in the first place.
On 26.07.22 17:41, Rafael J. Wysocki wrote: > On Tue, Jul 26, 2022 at 11:05 AM Oliver Neukum <oneukum@suse.com> wrote: > I guess that depends on what is regarded as "the framework". I mean > the PM-runtime code, excluding the bus type or equivalent. Yes, we have multiple candidates in the generic case. Easy to overengineer. >>> The idea was that drivers would clear these errors. >> >> I am afraid that is a deeply hidden layering violation. Yes, a driver's >> resume() method may have failed. In that case, if that is the same >> driver, it will obviously already know about the failure. > > So presumably it will do something to recover and avoid returning the > error in the first place. Yes, but that does not help us if they do return an error. > From the PM-runtime core code perspective, if an error is returned by > a suspend callback and it is not -EBUSY or -EAGAIN, the subsequent > suspend is also likely to fail. True. > If a resume callback returns an error, any subsequent suspend or > resume operations are likely to fail. Also true, but the consequences are different. > Storing the error effectively prevents subsequent operations from > being carried out in both cases and that's why it is done. I am afraid seeing these two operations as equivalent for this purpose is a problem for two reasons: 1. suspend can be initiated by the generic framework 2. a failure to suspend leads to worse power consumption, while a failure to resume is -EIO, at best >> PM operations, however, are operating on a tree. A driver requesting >> a resume may get an error code. But it has no idea where this error >> comes from. The generic code knows at least that. > > Well, what do you mean by "the generic code"? In this case the device model, which has the tree and all dependencies. Error handling here is potentially very complicated because 1. a driver can experience an error from a node higher in the tree 2. a driver can trigger a failure in a sibling 3. a driver for a node can be less specific than the drivers higher up Reducing this to a single error condition is difficult. Suppose you have a USB device with two interfaces. The driver for A initiates a resume. Interface A is resumed; B reports an error. Should this block further attempts to suspend the whole device? >> Let's look at at a USB storage device. The request to resume comes >> from sd.c. sd.c is certainly not equipped to handle a PCI error >> condition that has prevented a USB host controller from resuming. > > Sure, but this doesn't mean that suspending or resuming the device is > a good idea until the error condition gets resolved. Suspending clearly yes. Resuming is another matter. It has to work if you want to operate without errors. >> I am afraid this part of the API has issues. And they keep growing >> the more we divorce the device driver from the bus driver, which >> actually does the PM operation. > > Well, in general suspending or resuming a device is a collaborative > effort and if one of the pieces falls over, making it work again > involves fixing up the failing piece and notifying the others that it > is ready again. However, that part isn't covered and I'm not sure if > it can be covered in a sufficiently generic way. True. But that still cannot solve the question what is to be done if error handling fails. Hence my proposal: - record all failures - heed the record only when suspending Regards Oliver
On Wed, Jul 27, 2022 at 10:08 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > On 26.07.22 17:41, Rafael J. Wysocki wrote: > > On Tue, Jul 26, 2022 at 11:05 AM Oliver Neukum <oneukum@suse.com> wrote: > > > I guess that depends on what is regarded as "the framework". I mean > > the PM-runtime code, excluding the bus type or equivalent. > > Yes, we have multiple candidates in the generic case. Easy to overengineer. > > >>> The idea was that drivers would clear these errors. > >> > >> I am afraid that is a deeply hidden layering violation. Yes, a driver's > >> resume() method may have failed. In that case, if that is the same > >> driver, it will obviously already know about the failure. > > > > So presumably it will do something to recover and avoid returning the > > error in the first place. > > Yes, but that does not help us if they do return an error. > > > From the PM-runtime core code perspective, if an error is returned by > > a suspend callback and it is not -EBUSY or -EAGAIN, the subsequent > > suspend is also likely to fail. > > True. > > > If a resume callback returns an error, any subsequent suspend or > > resume operations are likely to fail. > > Also true, but the consequences are different. > > > Storing the error effectively prevents subsequent operations from > > being carried out in both cases and that's why it is done. > > I am afraid seeing these two operations as equivalent for this > purpose is a problem for two reasons: > > 1. suspend can be initiated by the generic framework Resume can be initiated by generic code too. > 2. a failure to suspend leads to worse power consumption, > while a failure to resume is -EIO, at best Yes, a failure to resume is a big deal. > >> PM operations, however, are operating on a tree. A driver requesting > >> a resume may get an error code. But it has no idea where this error > >> comes from. The generic code knows at least that. > > > > Well, what do you mean by "the generic code"? > > In this case the device model, which has the tree and all dependencies. > Error handling here is potentially very complicated because > > 1. a driver can experience an error from a node higher in the tree Well, there can be an error coming from a parent or a supplier, but the driver will not receive it directly. > 2. a driver can trigger a failure in a sibling > 3. a driver for a node can be less specific than the drivers higher up I'm not sure I understand the above correctly. > Reducing this to a single error condition is difficult. Fair enough. > Suppose you have a USB device with two interfaces. The driver for A > initiates a resume. Interface A is resumed; B reports an error. > Should this block further attempts to suspend the whole device? It should IMV. > >> Let's look at at a USB storage device. The request to resume comes > >> from sd.c. sd.c is certainly not equipped to handle a PCI error > >> condition that has prevented a USB host controller from resuming. > > > > Sure, but this doesn't mean that suspending or resuming the device is > > a good idea until the error condition gets resolved. > > Suspending clearly yes. Resuming is another matter. It has to work > if you want to operate without errors. Well, it has to physically work in the first place. If it doesn't, the rest is a bit moot, because you end up with a non-functional device that appears to be permanently suspended. > >> I am afraid this part of the API has issues. And they keep growing > >> the more we divorce the device driver from the bus driver, which > >> actually does the PM operation. > > > > Well, in general suspending or resuming a device is a collaborative > > effort and if one of the pieces falls over, making it work again > > involves fixing up the failing piece and notifying the others that it > > is ready again. However, that part isn't covered and I'm not sure if > > it can be covered in a sufficiently generic way. > > True. But that still cannot solve the question what is to be done > if error handling fails. Hence my proposal: > - record all failures > - heed the record only when suspending I guess that would boil down to moving the power.runtime_error update from rpm_callback() to rpm_suspend()?
On Wed, Jul 27, 2022 at 06:31:48PM +0200, Rafael J. Wysocki wrote: > On Wed, Jul 27, 2022 at 10:08 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > > > > > On 26.07.22 17:41, Rafael J. Wysocki wrote: > > > On Tue, Jul 26, 2022 at 11:05 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > > I guess that depends on what is regarded as "the framework". I mean > > > the PM-runtime code, excluding the bus type or equivalent. > > > > Yes, we have multiple candidates in the generic case. Easy to overengineer. > > > > >>> The idea was that drivers would clear these errors. > > >> > > >> I am afraid that is a deeply hidden layering violation. Yes, a driver's > > >> resume() method may have failed. In that case, if that is the same > > >> driver, it will obviously already know about the failure. > > > > > > So presumably it will do something to recover and avoid returning the > > > error in the first place. > > > > Yes, but that does not help us if they do return an error. > > > > > From the PM-runtime core code perspective, if an error is returned by > > > a suspend callback and it is not -EBUSY or -EAGAIN, the subsequent > > > suspend is also likely to fail. > > > > True. > > > > > If a resume callback returns an error, any subsequent suspend or > > > resume operations are likely to fail. > > > > Also true, but the consequences are different. > > > > > Storing the error effectively prevents subsequent operations from > > > being carried out in both cases and that's why it is done. > > > > I am afraid seeing these two operations as equivalent for this > > purpose is a problem for two reasons: > > > > 1. suspend can be initiated by the generic framework > > Resume can be initiated by generic code too. > > > 2. a failure to suspend leads to worse power consumption, > > while a failure to resume is -EIO, at best > > Yes, a failure to resume is a big deal. > > > >> PM operations, however, are operating on a tree. A driver requesting > > >> a resume may get an error code. But it has no idea where this error > > >> comes from. The generic code knows at least that. > > > > > > Well, what do you mean by "the generic code"? > > > > In this case the device model, which has the tree and all dependencies. > > Error handling here is potentially very complicated because > > > > 1. a driver can experience an error from a node higher in the tree > > Well, there can be an error coming from a parent or a supplier, but > the driver will not receive it directly. > > > 2. a driver can trigger a failure in a sibling > > 3. a driver for a node can be less specific than the drivers higher up > > I'm not sure I understand the above correctly. > > > Reducing this to a single error condition is difficult. > > Fair enough. > > > Suppose you have a USB device with two interfaces. The driver for A > > initiates a resume. Interface A is resumed; B reports an error. > > Should this block further attempts to suspend the whole device? > > It should IMV. > > > >> Let's look at at a USB storage device. The request to resume comes > > >> from sd.c. sd.c is certainly not equipped to handle a PCI error > > >> condition that has prevented a USB host controller from resuming. > > > > > > Sure, but this doesn't mean that suspending or resuming the device is > > > a good idea until the error condition gets resolved. > > > > Suspending clearly yes. Resuming is another matter. It has to work > > if you want to operate without errors. > > Well, it has to physically work in the first place. If it doesn't, > the rest is a bit moot, because you end up with a non-functional > device that appears to be permanently suspended. > > > >> I am afraid this part of the API has issues. And they keep growing > > >> the more we divorce the device driver from the bus driver, which > > >> actually does the PM operation. > > > > > > Well, in general suspending or resuming a device is a collaborative > > > effort and if one of the pieces falls over, making it work again > > > involves fixing up the failing piece and notifying the others that it > > > is ready again. However, that part isn't covered and I'm not sure if > > > it can be covered in a sufficiently generic way. > > > > True. But that still cannot solve the question what is to be done > > if error handling fails. Hence my proposal: > > - record all failures > > - heed the record only when suspending > > I guess that would boil down to moving the power.runtime_error update > from rpm_callback() to rpm_suspend()? Resuming this discussion. One of the ways the device drivers are clearing the runtime_error flag is by calling pm_runtime_set_suspended [1]. To me, it feels weird that a device driver calls pm_runtime_set_suspended if the runtime_resume() has failed. It should be implied that the device is in suspended state if the resume failed. So how really should the runtime_error flag be cleared? Should there be a new API exposed to device drivers for this? Or should we plan for it in the framework itself? 1: https://lore.kernel.org/all/20250129124009.1039982-3-jacek.lawrynowicz@linux.intel.com/
Hi Ajay, On Mon, Feb 10, 2025 at 09:02:41AM +0530, Ajay Agarwal wrote: > On Wed, Jul 27, 2022 at 06:31:48PM +0200, Rafael J. Wysocki wrote: > > On Wed, Jul 27, 2022 at 10:08 AM Oliver Neukum <oneukum@suse.com> wrote: > > > On 26.07.22 17:41, Rafael J. Wysocki wrote: > > > > Well, in general suspending or resuming a device is a collaborative > > > > effort and if one of the pieces falls over, making it work again > > > > involves fixing up the failing piece and notifying the others that it > > > > is ready again. However, that part isn't covered and I'm not sure if > > > > it can be covered in a sufficiently generic way. > > > > > > True. But that still cannot solve the question what is to be done > > > if error handling fails. Hence my proposal: > > > - record all failures > > > - heed the record only when suspending > > > > I guess that would boil down to moving the power.runtime_error update > > from rpm_callback() to rpm_suspend()? > Resuming this discussion. One of the ways the device drivers are > clearing the runtime_error flag is by calling pm_runtime_set_suspended > [1]. > > To me, it feels weird that a device driver calls pm_runtime_set_suspended > if the runtime_resume() has failed. It should be implied that the device > is in suspended state if the resume failed. > > So how really should the runtime_error flag be cleared? Should there be > a new API exposed to device drivers for this? Or should we plan for it > in the framework itself? While the API naming is unclear, that's exactly what pm_runtime_set_suspended() is about. Personally, I find it nice when a driver adds the comment "clear runtime_error flag", because otherwise it's not really obvious why a driver has to take care of "suspending" after a failed resume. But that's not the biggest question here, IMO. The real reson I pointed you at this thread was because I think it's useful to pursue the proposal above: to avoid setting a persistent "runtime_error" for resume failures. This seems to just create a pitfall for clients, as asked by Vincent and Oliver upthread. And along this line, there are relatively few drivers that actually bother to reset this error flag ever (e.g., commit f2bc2afe34c1 ("accel/ivpu: Clear runtime_error after pm_runtime_resume_and_get() fails")). So to me, we should simply answer Rafael's question: (repeated:) > > I guess that would boil down to moving the power.runtime_error update > > from rpm_callback() to rpm_suspend()? Yes, I think so. (Although I'm not sure if this leaves undesirable spam where persistent .runtime_resume() failures occur.) ...and then write/test/submit such a patch, provided it achieves the desired results. Unless of course one of the thread participants here has some other update in the intervening 2.5 years, or if Rafael was simply asking the above rhetorically, and wasn't actually interested in fielding such a change. Brian
On Tue, Feb 11, 2025 at 11:21 PM Brian Norris <briannorris@google.com> wrote: > > Hi Ajay, > > On Mon, Feb 10, 2025 at 09:02:41AM +0530, Ajay Agarwal wrote: > > On Wed, Jul 27, 2022 at 06:31:48PM +0200, Rafael J. Wysocki wrote: > > > On Wed, Jul 27, 2022 at 10:08 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > On 26.07.22 17:41, Rafael J. Wysocki wrote: > > > > > Well, in general suspending or resuming a device is a collaborative > > > > > effort and if one of the pieces falls over, making it work again > > > > > involves fixing up the failing piece and notifying the others that it > > > > > is ready again. However, that part isn't covered and I'm not sure if > > > > > it can be covered in a sufficiently generic way. > > > > > > > > True. But that still cannot solve the question what is to be done > > > > if error handling fails. Hence my proposal: > > > > - record all failures > > > > - heed the record only when suspending > > > > > > I guess that would boil down to moving the power.runtime_error update > > > from rpm_callback() to rpm_suspend()? > > Resuming this discussion. One of the ways the device drivers are > > clearing the runtime_error flag is by calling pm_runtime_set_suspended > > [1]. I personally think that jumping on a 2.5 years old thread is not a good idea. It would be better to restate the problem statement and provide the link to the previous discussion. > > To me, it feels weird that a device driver calls pm_runtime_set_suspended > > if the runtime_resume() has failed. It should be implied that the device > > is in suspended state if the resume failed. > > > > So how really should the runtime_error flag be cleared? Should there be > > a new API exposed to device drivers for this? Or should we plan for it > > in the framework itself? > > While the API naming is unclear, that's exactly what > pm_runtime_set_suspended() is about. Personally, I find it nice when a > driver adds the comment "clear runtime_error flag", because otherwise > it's not really obvious why a driver has to take care of "suspending" > after a failed resume. But that's not the biggest question here, IMO. > > The real reson I pointed you at this thread was because I think it's > useful to pursue the proposal above: to avoid setting a persistent > "runtime_error" for resume failures. This seems to just create a pitfall > for clients, as asked by Vincent and Oliver upthread. > > And along this line, there are relatively few drivers that actually > bother to reset this error flag ever (e.g., commit f2bc2afe34c1 > ("accel/ivpu: Clear runtime_error after pm_runtime_resume_and_get() > fails")). > > So to me, we should simply answer Rafael's question: > > (repeated:) > > > I guess that would boil down to moving the power.runtime_error update > > > from rpm_callback() to rpm_suspend()? > > Yes, I think so. (Although I'm not sure if this leaves undesirable spam > where persistent .runtime_resume() failures occur.) > > ...and then write/test/submit such a patch, provided it achieves the > desired results. > > Unless of course one of the thread participants here has some other > update in the intervening 2.5 years, or if Rafael was simply asking the > above rhetorically, and wasn't actually interested in fielding such a > change. The reason why runtime_error is there is to prevent runtime PM callbacks from being run until something is done about the error, under the assumption that running them in that case may make the problem worse. I'm not sure if I see a substantial difference between suspend and resume in that respect: If any of them fails, the state of the device is kind of unstable. In particular, if resume fails and the device doesn't actually resume, something needs to be done about it or it just becomes unusable. Now, the way of clearing the error may not be super-convenient, which was a bit hard to figure out upfront, so I'm not against making any changes as long as there are sufficient reasons for making them.
On Wed, Feb 12, 2025 at 08:29:34PM +0100, Rafael J. Wysocki wrote: > On Tue, Feb 11, 2025 at 11:21 PM Brian Norris <briannorris@google.com> wrote: > > > > Hi Ajay, > > > > On Mon, Feb 10, 2025 at 09:02:41AM +0530, Ajay Agarwal wrote: > > > On Wed, Jul 27, 2022 at 06:31:48PM +0200, Rafael J. Wysocki wrote: > > > > On Wed, Jul 27, 2022 at 10:08 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > > On 26.07.22 17:41, Rafael J. Wysocki wrote: > > > > > > Well, in general suspending or resuming a device is a collaborative > > > > > > effort and if one of the pieces falls over, making it work again > > > > > > involves fixing up the failing piece and notifying the others that it > > > > > > is ready again. However, that part isn't covered and I'm not sure if > > > > > > it can be covered in a sufficiently generic way. > > > > > > > > > > True. But that still cannot solve the question what is to be done > > > > > if error handling fails. Hence my proposal: > > > > > - record all failures > > > > > - heed the record only when suspending > > > > > > > > I guess that would boil down to moving the power.runtime_error update > > > > from rpm_callback() to rpm_suspend()? > > > Resuming this discussion. One of the ways the device drivers are > > > clearing the runtime_error flag is by calling pm_runtime_set_suspended > > > [1]. > > I personally think that jumping on a 2.5 years old thread is not a > good idea. It would be better to restate the problem statement and > provide the link to the previous discussion. > > > > To me, it feels weird that a device driver calls pm_runtime_set_suspended > > > if the runtime_resume() has failed. It should be implied that the device > > > is in suspended state if the resume failed. > > > > > > So how really should the runtime_error flag be cleared? Should there be > > > a new API exposed to device drivers for this? Or should we plan for it > > > in the framework itself? > > > > While the API naming is unclear, that's exactly what > > pm_runtime_set_suspended() is about. Personally, I find it nice when a > > driver adds the comment "clear runtime_error flag", because otherwise > > it's not really obvious why a driver has to take care of "suspending" > > after a failed resume. But that's not the biggest question here, IMO. > > > > The real reson I pointed you at this thread was because I think it's > > useful to pursue the proposal above: to avoid setting a persistent > > "runtime_error" for resume failures. This seems to just create a pitfall > > for clients, as asked by Vincent and Oliver upthread. > > > > And along this line, there are relatively few drivers that actually > > bother to reset this error flag ever (e.g., commit f2bc2afe34c1 > > ("accel/ivpu: Clear runtime_error after pm_runtime_resume_and_get() > > fails")). > > > > So to me, we should simply answer Rafael's question: > > > > (repeated:) > > > > I guess that would boil down to moving the power.runtime_error update > > > > from rpm_callback() to rpm_suspend()? > > > > Yes, I think so. (Although I'm not sure if this leaves undesirable spam > > where persistent .runtime_resume() failures occur.) > > > > ...and then write/test/submit such a patch, provided it achieves the > > desired results. > > > > Unless of course one of the thread participants here has some other > > update in the intervening 2.5 years, or if Rafael was simply asking the > > above rhetorically, and wasn't actually interested in fielding such a > > change. > > The reason why runtime_error is there is to prevent runtime PM > callbacks from being run until something is done about the error, > under the assumption that running them in that case may make the > problem worse. > > I'm not sure if I see a substantial difference between suspend and > resume in that respect: If any of them fails, the state of the device > is kind of unstable. In particular, if resume fails and the device > doesn't actually resume, something needs to be done about it or it > just becomes unusable. > > Now, the way of clearing the error may not be super-convenient, which > was a bit hard to figure out upfront, so I'm not against making any > changes as long as there are sufficient reasons for making them. I am thinking if we can start with a change to not check runtime_error in rpm_resume, and let it go through even if the previous rpm_resume attempt failed. Something like this: ``` static int rpm_resume(struct device *dev, int rpmflags) trace_rpm_resume(dev, rpmflags); repeat: - if (dev->power.runtime_error) { - retval = -EINVAL; - } else if (dev->power.disable_depth > 0) { + if (dev->power.disable_depth > 0) { if (dev->power.runtime_status == RPM_ACTIVE && dev->power.last_status == RPM_ACTIVE) retval = 1; ``` I think setting the runtime_error in rpm_callback, i.e. for both resume and suspend is still a good idea for book-keeping purposes, e.g. the user reading the runtime_status of the device from sysfs.
On Mon, Feb 17, 2025 at 4:49 AM Ajay Agarwal <ajayagarwal@google.com> wrote: > > On Wed, Feb 12, 2025 at 08:29:34PM +0100, Rafael J. Wysocki wrote: > > On Tue, Feb 11, 2025 at 11:21 PM Brian Norris <briannorris@google.com> wrote: > > > > > > Hi Ajay, > > > > > > On Mon, Feb 10, 2025 at 09:02:41AM +0530, Ajay Agarwal wrote: > > > > On Wed, Jul 27, 2022 at 06:31:48PM +0200, Rafael J. Wysocki wrote: > > > > > On Wed, Jul 27, 2022 at 10:08 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > > > On 26.07.22 17:41, Rafael J. Wysocki wrote: > > > > > > > Well, in general suspending or resuming a device is a collaborative > > > > > > > effort and if one of the pieces falls over, making it work again > > > > > > > involves fixing up the failing piece and notifying the others that it > > > > > > > is ready again. However, that part isn't covered and I'm not sure if > > > > > > > it can be covered in a sufficiently generic way. > > > > > > > > > > > > True. But that still cannot solve the question what is to be done > > > > > > if error handling fails. Hence my proposal: > > > > > > - record all failures > > > > > > - heed the record only when suspending > > > > > > > > > > I guess that would boil down to moving the power.runtime_error update > > > > > from rpm_callback() to rpm_suspend()? > > > > Resuming this discussion. One of the ways the device drivers are > > > > clearing the runtime_error flag is by calling pm_runtime_set_suspended > > > > [1]. > > > > I personally think that jumping on a 2.5 years old thread is not a > > good idea. It would be better to restate the problem statement and > > provide the link to the previous discussion. > > > > > > To me, it feels weird that a device driver calls pm_runtime_set_suspended > > > > if the runtime_resume() has failed. It should be implied that the device > > > > is in suspended state if the resume failed. > > > > > > > > So how really should the runtime_error flag be cleared? Should there be > > > > a new API exposed to device drivers for this? Or should we plan for it > > > > in the framework itself? > > > > > > While the API naming is unclear, that's exactly what > > > pm_runtime_set_suspended() is about. Personally, I find it nice when a > > > driver adds the comment "clear runtime_error flag", because otherwise > > > it's not really obvious why a driver has to take care of "suspending" > > > after a failed resume. But that's not the biggest question here, IMO. > > > > > > The real reson I pointed you at this thread was because I think it's > > > useful to pursue the proposal above: to avoid setting a persistent > > > "runtime_error" for resume failures. This seems to just create a pitfall > > > for clients, as asked by Vincent and Oliver upthread. > > > > > > And along this line, there are relatively few drivers that actually > > > bother to reset this error flag ever (e.g., commit f2bc2afe34c1 > > > ("accel/ivpu: Clear runtime_error after pm_runtime_resume_and_get() > > > fails")). > > > > > > So to me, we should simply answer Rafael's question: > > > > > > (repeated:) > > > > > I guess that would boil down to moving the power.runtime_error update > > > > > from rpm_callback() to rpm_suspend()? > > > > > > Yes, I think so. (Although I'm not sure if this leaves undesirable spam > > > where persistent .runtime_resume() failures occur.) > > > > > > ...and then write/test/submit such a patch, provided it achieves the > > > desired results. > > > > > > Unless of course one of the thread participants here has some other > > > update in the intervening 2.5 years, or if Rafael was simply asking the > > > above rhetorically, and wasn't actually interested in fielding such a > > > change. > > > > The reason why runtime_error is there is to prevent runtime PM > > callbacks from being run until something is done about the error, > > under the assumption that running them in that case may make the > > problem worse. > > > > I'm not sure if I see a substantial difference between suspend and > > resume in that respect: If any of them fails, the state of the device > > is kind of unstable. In particular, if resume fails and the device > > doesn't actually resume, something needs to be done about it or it > > just becomes unusable. > > > > Now, the way of clearing the error may not be super-convenient, which > > was a bit hard to figure out upfront, so I'm not against making any > > changes as long as there are sufficient reasons for making them. > > I am thinking if we can start with a change to not check runtime_error > in rpm_resume, and let it go through even if the previous rpm_resume > attempt failed. Something like this: > > ``` > static int rpm_resume(struct device *dev, int rpmflags) > trace_rpm_resume(dev, rpmflags); > > repeat: > - if (dev->power.runtime_error) { > - retval = -EINVAL; > - } else if (dev->power.disable_depth > 0) { > + if (dev->power.disable_depth > 0) { > if (dev->power.runtime_status == RPM_ACTIVE && > dev->power.last_status == RPM_ACTIVE) > retval = 1; > ``` > > I think setting the runtime_error in rpm_callback, i.e. for both resume > and suspend is still a good idea for book-keeping purposes, e.g. the > user reading the runtime_status of the device from sysfs. What would be the benefit of this change?
On Tue, Feb 18, 2025 at 11:07:19AM +0530, Ajay Agarwal wrote: > On Mon, Feb 17, 2025 at 09:23:18PM +0100, Rafael J. Wysocki wrote: > > On Mon, Feb 17, 2025 at 4:49 AM Ajay Agarwal <ajayagarwal@google.com> wrote: > > > > > > On Wed, Feb 12, 2025 at 08:29:34PM +0100, Rafael J. Wysocki wrote: > > > > On Tue, Feb 11, 2025 at 11:21 PM Brian Norris <briannorris@google.com> wrote: > > > > > > > > > > Hi Ajay, > > > > > > > > > > On Mon, Feb 10, 2025 at 09:02:41AM +0530, Ajay Agarwal wrote: > > > > > > On Wed, Jul 27, 2022 at 06:31:48PM +0200, Rafael J. Wysocki wrote: > > > > > > > On Wed, Jul 27, 2022 at 10:08 AM Oliver Neukum <oneukum@suse.com> wrote: > > > > > > > > On 26.07.22 17:41, Rafael J. Wysocki wrote: > > > > > > > > > Well, in general suspending or resuming a device is a collaborative > > > > > > > > > effort and if one of the pieces falls over, making it work again > > > > > > > > > involves fixing up the failing piece and notifying the others that it > > > > > > > > > is ready again. However, that part isn't covered and I'm not sure if > > > > > > > > > it can be covered in a sufficiently generic way. > > > > > > > > > > > > > > > > True. But that still cannot solve the question what is to be done > > > > > > > > if error handling fails. Hence my proposal: > > > > > > > > - record all failures > > > > > > > > - heed the record only when suspending > > > > > > > > > > > > > > I guess that would boil down to moving the power.runtime_error update > > > > > > > from rpm_callback() to rpm_suspend()? > > > > > > Resuming this discussion. One of the ways the device drivers are > > > > > > clearing the runtime_error flag is by calling pm_runtime_set_suspended > > > > > > [1]. > > > > > > > > I personally think that jumping on a 2.5 years old thread is not a > > > > good idea. It would be better to restate the problem statement and > > > > provide the link to the previous discussion. > > > > > > > > > > To me, it feels weird that a device driver calls pm_runtime_set_suspended > > > > > > if the runtime_resume() has failed. It should be implied that the device > > > > > > is in suspended state if the resume failed. > > > > > > > > > > > > So how really should the runtime_error flag be cleared? Should there be > > > > > > a new API exposed to device drivers for this? Or should we plan for it > > > > > > in the framework itself? > > > > > > > > > > While the API naming is unclear, that's exactly what > > > > > pm_runtime_set_suspended() is about. Personally, I find it nice when a > > > > > driver adds the comment "clear runtime_error flag", because otherwise > > > > > it's not really obvious why a driver has to take care of "suspending" > > > > > after a failed resume. But that's not the biggest question here, IMO. > > > > > > > > > > The real reson I pointed you at this thread was because I think it's > > > > > useful to pursue the proposal above: to avoid setting a persistent > > > > > "runtime_error" for resume failures. This seems to just create a pitfall > > > > > for clients, as asked by Vincent and Oliver upthread. > > > > > > > > > > And along this line, there are relatively few drivers that actually > > > > > bother to reset this error flag ever (e.g., commit f2bc2afe34c1 > > > > > ("accel/ivpu: Clear runtime_error after pm_runtime_resume_and_get() > > > > > fails")). > > > > > > > > > > So to me, we should simply answer Rafael's question: > > > > > > > > > > (repeated:) > > > > > > > I guess that would boil down to moving the power.runtime_error update > > > > > > > from rpm_callback() to rpm_suspend()? > > > > > > > > > > Yes, I think so. (Although I'm not sure if this leaves undesirable spam > > > > > where persistent .runtime_resume() failures occur.) > > > > > > > > > > ...and then write/test/submit such a patch, provided it achieves the > > > > > desired results. > > > > > > > > > > Unless of course one of the thread participants here has some other > > > > > update in the intervening 2.5 years, or if Rafael was simply asking the > > > > > above rhetorically, and wasn't actually interested in fielding such a > > > > > change. > > > > > > > > The reason why runtime_error is there is to prevent runtime PM > > > > callbacks from being run until something is done about the error, > > > > under the assumption that running them in that case may make the > > > > problem worse. > > > > > > > > I'm not sure if I see a substantial difference between suspend and > > > > resume in that respect: If any of them fails, the state of the device > > > > is kind of unstable. In particular, if resume fails and the device > > > > doesn't actually resume, something needs to be done about it or it > > > > just becomes unusable. > > > > > > > > Now, the way of clearing the error may not be super-convenient, which > > > > was a bit hard to figure out upfront, so I'm not against making any > > > > changes as long as there are sufficient reasons for making them. > > > > > > I am thinking if we can start with a change to not check runtime_error > > > in rpm_resume, and let it go through even if the previous rpm_resume > > > attempt failed. Something like this: > > > > > > ``` > > > static int rpm_resume(struct device *dev, int rpmflags) > > > trace_rpm_resume(dev, rpmflags); > > > > > > repeat: > > > - if (dev->power.runtime_error) { > > > - retval = -EINVAL; > > > - } else if (dev->power.disable_depth > 0) { > > > + if (dev->power.disable_depth > 0) { > > > if (dev->power.runtime_status == RPM_ACTIVE && > > > dev->power.last_status == RPM_ACTIVE) > > > retval = 1; > > > ``` > > > > > > I think setting the runtime_error in rpm_callback, i.e. for both resume > > > and suspend is still a good idea for book-keeping purposes, e.g. the > > > user reading the runtime_status of the device from sysfs. > > > > What would be the benefit of this change? > The benefit would be that the runtime_resume would be re-attempted even if > the previous attempt failed. Actually, I wanted to propose the removal of `runtime_error` flag completely from the code. But it sounded too disruptive to me. Hence, I proposed the milder patch of removal of `runtime_error` check from rpm_resume so that the drivers do not have to call `pm_runtime_set_suspended` explicitly. Basically, we still do not have a good solution for the situation where one of the ancestors fails to resume. We do not know how to make the ancestor working again. But I guess a re-attempt is better than not doing anything about it?
On Wed, Feb 12, 2025 at 08:29:34PM +0100, Rafael J. Wysocki wrote: > The reason why runtime_error is there is to prevent runtime PM > callbacks from being run until something is done about the error, > under the assumption that running them in that case may make the > problem worse. What makes you think it will make the problem worse? That seems like a rather large assumption to me. What kind of things do you think go wrong, that it requires the framework to stop any future attempts? Just spam (e.g., logging noise, if -EIO is persistent)? Or something worse? And OTOH, there are clearly cases where retrying would be not only acceptable, but expected -- so giving special case to -EAGAIN and -EBUSY, per another branch of this thread, seems wise. I'd also note that AFAICT, there is no similar feature in system PM. If suspend() fails, we unwind and report the error ... but still allow future system suspend requests. resume() is even "worse" -- errors are essentially logged and ignored. > I'm not sure if I see a substantial difference between suspend and > resume in that respect: If any of them fails, the state of the device > is kind of unstable. In particular, if resume fails and the device > doesn't actually resume, something needs to be done about it or it > just becomes unusable. To me, it's about the state of the device. If suspend failed, the device may still be active and functional -- but not power-efficient. If resume failed, the device may be suspended and non-functional. But anyway, I don't think I require asymmetry; I'm just more interested in unnecessary non-functionality. (Power inefficiency is less important, as in the worst case, we can at least save our data, reboot, and try again.) Brian
On 19.02.25 23:15, Brian Norris wrote: > On Wed, Feb 12, 2025 at 08:29:34PM +0100, Rafael J. Wysocki wrote: >> The reason why runtime_error is there is to prevent runtime PM >> callbacks from being run until something is done about the error, >> under the assumption that running them in that case may make the >> problem worse. > > What makes you think it will make the problem worse? That seems like a > rather large assumption to me. What kind of things do you think go > wrong, that it requires the framework to stop any future attempts? Just > spam (e.g., logging noise, if -EIO is persistent)? Or something worse?e suspend() is three operations, potentially a) record device state b) arm remote wakeup c) transition to a lower power state I wouldn't trust a device to perform the first two steps without error handling either. It is an unnecessary risk. > And OTOH, there are clearly cases where retrying would be not only > acceptable, but expected -- so giving special case to -EAGAIN and > -EBUSY, per another branch of this thread, seems wise. Yes > > I'd also note that AFAICT, there is no similar feature in system PM. If > suspend() fails, we unwind and report the error ... but still allow > future system suspend requests. resume() is even "worse" -- errors are > essentially logged and ignored. Suspend requests from runtime PM are different. They happen spontaneously. Secondly, failures to suspend in runtime PM are far cheaper. >> I'm not sure if I see a substantial difference between suspend and >> resume in that respect: If any of them fails, the state of the device >> is kind of unstable. In particular, if resume fails and the device >> doesn't actually resume, something needs to be done about it or it >> just becomes unusable. Again, if you look at it in an abstract manner, this is a mess. Resume() is actually two functions a) transition to a power state that allows an operation b) restore device settings It is possible for the second step to fail after the first has worked. > To me, it's about the state of the device. If suspend failed, the device > may still be active and functional -- but not power-efficient. If resume > failed, the device may be suspended and non-functional. > > But anyway, I don't think I require asymmetry; I'm just more interested > in unnecessary non-functionality. (Power inefficiency is less important, > as in the worst case, we can at least save our data, reboot, and try > again.) You are calling for asymmetry ;-) If you fail to resume, you will need to return an error. The functions are just not equal in terms of consequences. We don't resume for fun. We do, however, suspend just because a timer fires. Regards Oliver
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c index e02e92bc2928..082b8969fe2f 100644 --- a/drivers/iio/light/vcnl4000.c +++ b/drivers/iio/light/vcnl4000.c @@ -414,6 +414,8 @@ static int vcnl4000_set_pm_runtime_state(struct vcnl4000_data *data, bool on) if (on) { ret = pm_runtime_resume_and_get(dev); + if (ret) + pm_runtime_set_suspended(dev); } else { pm_runtime_mark_last_busy(dev); ret = pm_runtime_put_autosuspend(dev);