* [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.