* [PATCH v3] PM: check for complete cb before device lock in dpm_complete @ 2015-10-16 22:45 Todd Brandt 2015-10-16 22:49 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Todd Brandt @ 2015-10-16 22:45 UTC (permalink / raw) To: linux-pm, rafael.j.wysocki; +Cc: rjw, todd.e.brandt, Todd Brandt If a device has no pm_ops complete callback it shouldn't have to be locked in order to skip it in the dpm_complete call. This causes problems when a device without a complete callback has already begun operation after its resume cb is called. It can end up holding up the system resume as the pm subsystem tries to get a device lock just to check for a callback that isn't there. This is basically the original v1 patch but updated for the latest kernel. Signed-off-by: Todd Brandt <todd.e.brandt@linux.intel.com> --- drivers/base/power/main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 1710c26..9bb8ff0 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -899,8 +899,6 @@ static void device_complete(struct device *dev, pm_message_t state) if (dev->power.syscore) return; - device_lock(dev); - if (dev->pm_domain) { info = "completing power domain "; callback = dev->pm_domain->ops.complete; @@ -920,13 +918,15 @@ static void device_complete(struct device *dev, pm_message_t state) callback = dev->driver->pm->complete; } - if (callback) { - pm_dev_dbg(dev, state, info); - callback(dev); - } + if (!callback) + goto Complete; + device_lock(dev); + pm_dev_dbg(dev, state, info); + callback(dev); device_unlock(dev); +Complete: pm_runtime_put(dev); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] PM: check for complete cb before device lock in dpm_complete 2015-10-16 22:45 [PATCH v3] PM: check for complete cb before device lock in dpm_complete Todd Brandt @ 2015-10-16 22:49 ` Rafael J. Wysocki 2015-10-16 23:06 ` Todd E Brandt 2015-11-29 5:33 ` Dmitry Torokhov 0 siblings, 2 replies; 6+ messages in thread From: Rafael J. Wysocki @ 2015-10-16 22:49 UTC (permalink / raw) To: Todd Brandt Cc: linux-pm@vger.kernel.org, Rafael Wysocki, Rafael J. Wysocki, todd.e.brandt Hi, On Sat, Oct 17, 2015 at 12:45 AM, Todd Brandt <todd.e.brandt@linux.intel.com> wrote: > If a device has no pm_ops complete callback it shouldn't have > to be locked in order to skip it in the dpm_complete call. This causes > problems when a device without a complete callback has already begun > operation after its resume cb is called. It can end up holding up the > system resume as the pm subsystem tries to get a device lock just to > check for a callback that isn't there. > > This is basically the original v1 patch but updated for the latest kernel. > > Signed-off-by: Todd Brandt <todd.e.brandt@linux.intel.com> > --- > drivers/base/power/main.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 1710c26..9bb8ff0 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -899,8 +899,6 @@ static void device_complete(struct device *dev, pm_message_t state) > if (dev->power.syscore) > return; > > - device_lock(dev); > - > if (dev->pm_domain) { > info = "completing power domain "; > callback = dev->pm_domain->ops.complete; > @@ -920,13 +918,15 @@ static void device_complete(struct device *dev, pm_message_t state) > callback = dev->driver->pm->complete; > } > > - if (callback) { > - pm_dev_dbg(dev, state, info); > - callback(dev); > - } > + if (!callback) > + goto Complete; > Suppose that the devices is unregistered at this point and callback was pointing to the driver callback. > + device_lock(dev); > + pm_dev_dbg(dev, state, info); > + callback(dev); I don't think it is correct to execute that callback here in that case, is it? > device_unlock(dev); > > +Complete: > pm_runtime_put(dev); > } Thanks, Rafael ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] PM: check for complete cb before device lock in dpm_complete 2015-10-16 22:49 ` Rafael J. Wysocki @ 2015-10-16 23:06 ` Todd E Brandt 2015-10-17 15:04 ` Alan Stern 2015-11-29 5:33 ` Dmitry Torokhov 1 sibling, 1 reply; 6+ messages in thread From: Todd E Brandt @ 2015-10-16 23:06 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-pm@vger.kernel.org, Rafael Wysocki, Rafael J. Wysocki, todd.e.brandt On Sat, Oct 17, 2015 at 12:49:04AM +0200, Rafael J. Wysocki wrote: > Hi, > > On Sat, Oct 17, 2015 at 12:45 AM, Todd Brandt > <todd.e.brandt@linux.intel.com> wrote: > > If a device has no pm_ops complete callback it shouldn't have > > to be locked in order to skip it in the dpm_complete call. This causes > > problems when a device without a complete callback has already begun > > operation after its resume cb is called. It can end up holding up the > > system resume as the pm subsystem tries to get a device lock just to > > check for a callback that isn't there. > > > > This is basically the original v1 patch but updated for the latest kernel. > > > > Signed-off-by: Todd Brandt <todd.e.brandt@linux.intel.com> > > --- > > drivers/base/power/main.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > > index 1710c26..9bb8ff0 100644 > > --- a/drivers/base/power/main.c > > +++ b/drivers/base/power/main.c > > @@ -899,8 +899,6 @@ static void device_complete(struct device *dev, pm_message_t state) > > if (dev->power.syscore) > > return; > > > > - device_lock(dev); > > - > > if (dev->pm_domain) { > > info = "completing power domain "; > > callback = dev->pm_domain->ops.complete; > > @@ -920,13 +918,15 @@ static void device_complete(struct device *dev, pm_message_t state) > > callback = dev->driver->pm->complete; > > } > > > > - if (callback) { > > - pm_dev_dbg(dev, state, info); > > - callback(dev); > > - } > > + if (!callback) > > + goto Complete; > > > > Suppose that the devices is unregistered at this point and callback > was pointing to the driver callback. > > > + device_lock(dev); > > + pm_dev_dbg(dev, state, info); > > + callback(dev); > > I don't think it is correct to execute that callback here in that case, is it? Ahh, so we *should* be worried about an unregister in between the check and the call. I seem to remember a discussion where this was deemed a non-issue but I must have misheard (it was a while back). It sounds to me then that there's just no way to do this safely :(. I'll experiment with the i8042 driver to see if theres a way to fix this at a lower level. Thus far it's the only driver which has this issue. Thanks for the feedback. > > > device_unlock(dev); > > > > +Complete: > > pm_runtime_put(dev); > > } > > Thanks, > Rafael ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] PM: check for complete cb before device lock in dpm_complete 2015-10-16 23:06 ` Todd E Brandt @ 2015-10-17 15:04 ` Alan Stern 0 siblings, 0 replies; 6+ messages in thread From: Alan Stern @ 2015-10-17 15:04 UTC (permalink / raw) To: Todd E Brandt Cc: Rafael J. Wysocki, linux-pm@vger.kernel.org, Rafael Wysocki, Rafael J. Wysocki, todd.e.brandt On Fri, 16 Oct 2015, Todd E Brandt wrote: > On Sat, Oct 17, 2015 at 12:49:04AM +0200, Rafael J. Wysocki wrote: > > Hi, > > > > On Sat, Oct 17, 2015 at 12:45 AM, Todd Brandt > > <todd.e.brandt@linux.intel.com> wrote: > > > If a device has no pm_ops complete callback it shouldn't have > > > to be locked in order to skip it in the dpm_complete call. This causes > > > problems when a device without a complete callback has already begun > > > operation after its resume cb is called. It can end up holding up the > > > system resume as the pm subsystem tries to get a device lock just to > > > check for a callback that isn't there. > > > > > > This is basically the original v1 patch but updated for the latest kernel. > > > > > > Signed-off-by: Todd Brandt <todd.e.brandt@linux.intel.com> > > > --- > > > drivers/base/power/main.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > > > index 1710c26..9bb8ff0 100644 > > > --- a/drivers/base/power/main.c > > > +++ b/drivers/base/power/main.c > > > @@ -899,8 +899,6 @@ static void device_complete(struct device *dev, pm_message_t state) > > > if (dev->power.syscore) > > > return; > > > > > > - device_lock(dev); > > > - > > > if (dev->pm_domain) { > > > info = "completing power domain "; > > > callback = dev->pm_domain->ops.complete; > > > @@ -920,13 +918,15 @@ static void device_complete(struct device *dev, pm_message_t state) > > > callback = dev->driver->pm->complete; > > > } > > > > > > - if (callback) { > > > - pm_dev_dbg(dev, state, info); > > > - callback(dev); > > > - } > > > + if (!callback) > > > + goto Complete; > > > > > > > Suppose that the devices is unregistered at this point and callback > > was pointing to the driver callback. > > > > > + device_lock(dev); > > > + pm_dev_dbg(dev, state, info); > > > + callback(dev); > > > > I don't think it is correct to execute that callback here in that case, is it? > > Ahh, so we *should* be worried about an unregister in between the check and the call. > I seem to remember a discussion where this was deemed a non-issue but I must have > misheard (it was a while back). > > It sounds to me then that there's just no way to do this safely :(. I'll experiment > with the i8042 driver to see if theres a way to fix this at a lower level. Thus far > it's the only driver which has this issue. Fixing it in the driver would be best. (Why does the i8042 device stay locked long enough to matter? And doesn't the driver use async suspend/resume anyway?) Still, if you want to do it here safely, there is a way: Compute the callback's address If it is not NULL: Lock the device Compute the callback's address again If it is not NULL: Invoke the callback Unlock the device However, duplicating the computation likek this would be a waste for almost all devices. So unless there's a real need to do this, we should avoid it. Alan Stern ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] PM: check for complete cb before device lock in dpm_complete 2015-10-16 22:49 ` Rafael J. Wysocki 2015-10-16 23:06 ` Todd E Brandt @ 2015-11-29 5:33 ` Dmitry Torokhov 2015-11-30 2:15 ` Rafael J. Wysocki 1 sibling, 1 reply; 6+ messages in thread From: Dmitry Torokhov @ 2015-11-29 5:33 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Todd Brandt, linux-pm@vger.kernel.org, Rafael Wysocki, Rafael J. Wysocki, todd.e.brandt On Fri, Oct 16, 2015 at 3:49 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > Hi, > > On Sat, Oct 17, 2015 at 12:45 AM, Todd Brandt > <todd.e.brandt@linux.intel.com> wrote: >> If a device has no pm_ops complete callback it shouldn't have >> to be locked in order to skip it in the dpm_complete call. This causes >> problems when a device without a complete callback has already begun >> operation after its resume cb is called. It can end up holding up the >> system resume as the pm subsystem tries to get a device lock just to >> check for a callback that isn't there. >> >> This is basically the original v1 patch but updated for the latest kernel. >> >> Signed-off-by: Todd Brandt <todd.e.brandt@linux.intel.com> >> --- >> drivers/base/power/main.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c >> index 1710c26..9bb8ff0 100644 >> --- a/drivers/base/power/main.c >> +++ b/drivers/base/power/main.c >> @@ -899,8 +899,6 @@ static void device_complete(struct device *dev, pm_message_t state) >> if (dev->power.syscore) >> return; >> >> - device_lock(dev); >> - >> if (dev->pm_domain) { >> info = "completing power domain "; >> callback = dev->pm_domain->ops.complete; >> @@ -920,13 +918,15 @@ static void device_complete(struct device *dev, pm_message_t state) >> callback = dev->driver->pm->complete; >> } >> >> - if (callback) { >> - pm_dev_dbg(dev, state, info); >> - callback(dev); >> - } >> + if (!callback) >> + goto Complete; >> > > Suppose that the devices is unregistered at this point and callback > was pointing to the driver callback. > >> + device_lock(dev); >> + pm_dev_dbg(dev, state, info); >> + callback(dev); > > I don't think it is correct to execute that callback here in that case, is it? Continuing this line of thought: what if driver was unbound and rebound again before we got to executing device_complete()? In that case we should not be calling ->complete() even if it is present, since it is as if prepare() was not called, and there is nothing for ->complete() to undo. It seems to me we should not be allowing bind/unbind transitions here. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] PM: check for complete cb before device lock in dpm_complete 2015-11-29 5:33 ` Dmitry Torokhov @ 2015-11-30 2:15 ` Rafael J. Wysocki 0 siblings, 0 replies; 6+ messages in thread From: Rafael J. Wysocki @ 2015-11-30 2:15 UTC (permalink / raw) To: Dmitry Torokhov Cc: Rafael J. Wysocki, Todd Brandt, linux-pm@vger.kernel.org, Rafael Wysocki, todd.e.brandt On Saturday, November 28, 2015 09:33:20 PM Dmitry Torokhov wrote: > On Fri, Oct 16, 2015 at 3:49 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > > Hi, > > > > On Sat, Oct 17, 2015 at 12:45 AM, Todd Brandt > > <todd.e.brandt@linux.intel.com> wrote: > >> If a device has no pm_ops complete callback it shouldn't have > >> to be locked in order to skip it in the dpm_complete call. This causes > >> problems when a device without a complete callback has already begun > >> operation after its resume cb is called. It can end up holding up the > >> system resume as the pm subsystem tries to get a device lock just to > >> check for a callback that isn't there. > >> > >> This is basically the original v1 patch but updated for the latest kernel. > >> > >> Signed-off-by: Todd Brandt <todd.e.brandt@linux.intel.com> > >> --- > >> drivers/base/power/main.c | 12 ++++++------ > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > >> index 1710c26..9bb8ff0 100644 > >> --- a/drivers/base/power/main.c > >> +++ b/drivers/base/power/main.c > >> @@ -899,8 +899,6 @@ static void device_complete(struct device *dev, pm_message_t state) > >> if (dev->power.syscore) > >> return; > >> > >> - device_lock(dev); > >> - > >> if (dev->pm_domain) { > >> info = "completing power domain "; > >> callback = dev->pm_domain->ops.complete; > >> @@ -920,13 +918,15 @@ static void device_complete(struct device *dev, pm_message_t state) > >> callback = dev->driver->pm->complete; > >> } > >> > >> - if (callback) { > >> - pm_dev_dbg(dev, state, info); > >> - callback(dev); > >> - } > >> + if (!callback) > >> + goto Complete; > >> > > > > Suppose that the devices is unregistered at this point and callback > > was pointing to the driver callback. > > > >> + device_lock(dev); > >> + pm_dev_dbg(dev, state, info); > >> + callback(dev); > > > > I don't think it is correct to execute that callback here in that case, is it? > > Continuing this line of thought: what if driver was unbound and > rebound again before we got to executing device_complete()? In that > case we should not be calling ->complete() even if it is present, > since it is as if prepare() was not called, and there is nothing for > ->complete() to undo. > > It seems to me we should not be allowing bind/unbind transitions here. Probes will be avoided after this patch: https://patchwork.kernel.org/patch/7589121/ which I'm going to queue up for 4.5. I'm not sure about unbinds, though. Some parts of the kernel seem to want to be able to remove devices during system suspend/resume. Thanks, Rafael ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-30 1:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-16 22:45 [PATCH v3] PM: check for complete cb before device lock in dpm_complete Todd Brandt 2015-10-16 22:49 ` Rafael J. Wysocki 2015-10-16 23:06 ` Todd E Brandt 2015-10-17 15:04 ` Alan Stern 2015-11-29 5:33 ` Dmitry Torokhov 2015-11-30 2:15 ` Rafael J. Wysocki
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.