| 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()?
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);