| Message ID | 1445288064-25299-1-git-send-email-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Monday, October 19, 2015 11:54:24 PM Grygorii Strashko wrote: > It is unsafe [1] if probing of devices will happen during suspend or > hibernation and system behavior will be unpredictable in this case > (for example: after successful probe the device potentially has a different > set of PM callbacks than before [2]). > So, let's prohibit device's probing in dpm_prepare() and defer their > probes instead. The normal behavior will be restored in dpm_complete(). > > This patch introduces new DD core APIs: > device_defer_all_probes_enable() > It will disable probing of devices and defer their probes. > device_defer_all_probes_disable() > It will restore normal behavior and trigger re-probing of deferred > devices. > > [1] https://lkml.org/lkml/2015/9/11/554 > [2] https://lkml.org/lkml/2015/9/15/1039 > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Greg, any objections against this one? > --- > Changes in v2: > - DD core API device_defer_all_probes(bool enable) split on two > void device_defer_all_probes_enable(void); > void device_defer_all_probes_disable(void); > - more comments added > > Link on v1: > - https://lkml.org/lkml/2015/10/8/681 > > drivers/base/base.h | 2 ++ > drivers/base/dd.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- > drivers/base/power/main.c | 17 +++++++++++++++++ > 3 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index 1782f3a..c332b68 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -131,6 +131,8 @@ extern void device_remove_groups(struct device *dev, > extern char *make_class_name(const char *name, struct kobject *kobj); > > extern int devres_release_all(struct device *dev); > +extern void device_defer_all_probes_enable(void); > +extern void device_defer_all_probes_disable(void); > > /* /sys/devices directory */ > extern struct kset *devices_kset; > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index be0eb46..b8d9e70 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -55,6 +55,13 @@ static struct workqueue_struct *deferred_wq; > static atomic_t deferred_trigger_count = ATOMIC_INIT(0); > > /* > + * In some cases, like suspend to RAM or hibernation, It might be reasonable > + * to prohibit probing of devices as it could be unsafe. > + * Once defer_all_probes is true all drivers probes will be forcibly deferred. > + */ > +static bool defer_all_probes; > + > +/* > * deferred_probe_work_func() - Retry probing devices in the active list. > */ > static void deferred_probe_work_func(struct work_struct *work) > @@ -172,6 +179,30 @@ static void driver_deferred_probe_trigger(void) > } > > /** > + * device_defer_all_probes_enable() - Enable deferring of device's probes > + * > + * It will disable probing of devices and defer their probes. > + */ > +void device_defer_all_probes_enable(void) > +{ > + defer_all_probes = true; > + /* sync with probes to avoid races. */ > + wait_for_device_probe(); > +} > + > +/** > + * device_defer_all_probes_disable() - Disable deferring of device's probes > + * > + * It will restore normal behavior and trigger re-probing of deferred > + * devices. > + */ > +void device_defer_all_probes_disable(void) > +{ > + defer_all_probes = false; > + driver_deferred_probe_trigger(); > +} > + > +/** > * deferred_probe_initcall() - Enable probing of deferred devices > * > * We don't want to get in the way when the bulk of drivers are getting probed. > @@ -277,9 +308,20 @@ static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue); > > static int really_probe(struct device *dev, struct device_driver *drv) > { > - int ret = 0; > + int ret = -EPROBE_DEFER; > int local_trigger_count = atomic_read(&deferred_trigger_count); > > + if (defer_all_probes) { > + /* > + * Value of defer_all_probes can be set only by > + * device_defer_all_probes_enable() which, in turn, will call > + * wait_for_device_probe() right after that to avoid any races. > + */ > + dev_dbg(dev, "Driver %s force probe deferral\n", drv->name); > + driver_deferred_probe_add(dev); > + return ret; > + } > + > atomic_inc(&probe_count); > pr_debug("bus: '%s': %s: probing driver %s with device %s\n", > drv->bus->name, __func__, drv->name, dev_name(dev)); > @@ -391,6 +433,10 @@ int driver_probe_done(void) > */ > void wait_for_device_probe(void) > { > + /* wait for the deferred probe workqueue to finish */ > + if (driver_deferred_probe_enable) > + flush_workqueue(deferred_wq); > + > /* wait for the known devices to complete their probing */ > wait_event(probe_waitqueue, atomic_read(&probe_count) == 0); > async_synchronize_full(); > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 1710c26..eeb2bb7 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -963,6 +963,9 @@ void dpm_complete(pm_message_t state) > } > list_splice(&list, &dpm_list); > mutex_unlock(&dpm_list_mtx); > + > + /* Allow device probing and trigger re-probing of deferred devices */ > + device_defer_all_probes_disable(); > trace_suspend_resume(TPS("dpm_complete"), state.event, false); > } > > @@ -1624,6 +1627,20 @@ int dpm_prepare(pm_message_t state) > trace_suspend_resume(TPS("dpm_prepare"), state.event, true); > might_sleep(); > > + /* > + * Give a chance for the known devices to complete their probes, before > + * disable probing of devices. This sync point is important at least > + * at boot time + hibernation restore. > + */ > + wait_for_device_probe(); > + /* > + * It is unsafe if probing of devices will happen during suspend or > + * hibernation and system behavior will be unpredictable in this case. > + * So, let's prohibit device's probing here and defer their probes > + * instead. The normal behavior will be restored in dpm_complete(). > + */ > + device_defer_all_probes_enable(); > + > mutex_lock(&dpm_list_mtx); > while (!list_empty(&dpm_list)) { > struct device *dev = to_device(dpm_list.next); > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 2015-10-19 23:54:24, Grygorii Strashko wrote: > It is unsafe [1] if probing of devices will happen during suspend or > hibernation and system behavior will be unpredictable in this case > (for example: after successful probe the device potentially has a different > set of PM callbacks than before [2]). > So, let's prohibit device's probing in dpm_prepare() and defer their > probes instead. The normal behavior will be restored in dpm_complete(). > > This patch introduces new DD core APIs: > device_defer_all_probes_enable() > It will disable probing of devices and defer their probes. > device_defer_all_probes_disable() > It will restore normal behavior and trigger re-probing of deferred > devices. > > [1] https://lkml.org/lkml/2015/9/11/554 > [2] https://lkml.org/lkml/2015/9/15/1039 > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > Changes in v2: > - DD core API device_defer_all_probes(bool enable) split on two > void device_defer_all_probes_enable(void); > void device_defer_all_probes_disable(void); > - more comments added > > Link on v1: > - https://lkml.org/lkml/2015/10/8/681 > > drivers/base/base.h | 2 ++ > drivers/base/dd.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- > drivers/base/power/main.c | 17 +++++++++++++++++ > 3 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/base.h b/drivers/base/base.h > index 1782f3a..c332b68 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -131,6 +131,8 @@ extern void device_remove_groups(struct device *dev, > extern char *make_class_name(const char *name, struct kobject *kobj); > > extern int devres_release_all(struct device *dev); > +extern void device_defer_all_probes_enable(void); > +extern void device_defer_all_probes_disable(void); > > /* /sys/devices directory */ > extern struct kset *devices_kset; > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index be0eb46..b8d9e70 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -55,6 +55,13 @@ static struct workqueue_struct *deferred_wq; > static atomic_t deferred_trigger_count = ATOMIC_INIT(0); > > /* > + * In some cases, like suspend to RAM or hibernation, It might be reasonable > + * to prohibit probing of devices as it could be unsafe. > + * Once defer_all_probes is true all drivers probes will be forcibly deferred. > + */ > +static bool defer_all_probes; > + > +/* > * deferred_probe_work_func() - Retry probing devices in the active list. > */ > static void deferred_probe_work_func(struct work_struct *work) > @@ -172,6 +179,30 @@ static void driver_deferred_probe_trigger(void) > } > > /** > + * device_defer_all_probes_enable() - Enable deferring of device's probes > + * > + * It will disable probing of devices and defer their probes. > + */ > +void device_defer_all_probes_enable(void) > +{ > + defer_all_probes = true; > + /* sync with probes to avoid races. */ > + wait_for_device_probe(); > +} device_pause_probing()? > +/** > + * device_defer_all_probes_disable() - Disable deferring of device's probes > + * > + * It will restore normal behavior and trigger re-probing of deferred > + * devices. > + */ Hmm. This is not quite a double negative... but it sounds like one. device_restart_probing()? Thanks, Pavel
On Monday, November 02, 2015 02:25:00 AM Rafael J. Wysocki wrote: > On Monday, October 19, 2015 11:54:24 PM Grygorii Strashko wrote: > > It is unsafe [1] if probing of devices will happen during suspend or > > hibernation and system behavior will be unpredictable in this case > > (for example: after successful probe the device potentially has a different > > set of PM callbacks than before [2]). > > So, let's prohibit device's probing in dpm_prepare() and defer their > > probes instead. The normal behavior will be restored in dpm_complete(). > > > > This patch introduces new DD core APIs: > > device_defer_all_probes_enable() > > It will disable probing of devices and defer their probes. > > device_defer_all_probes_disable() > > It will restore normal behavior and trigger re-probing of deferred > > devices. > > > > [1] https://lkml.org/lkml/2015/9/11/554 > > [2] https://lkml.org/lkml/2015/9/15/1039 > > Cc: Alan Stern <stern@rowland.harvard.edu> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > Cc: Thierry Reding <thierry.reding@gmail.com> > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > Greg, any objections against this one? Greg, if this isn't problematic, I'd still like to take it for v4.4. Any chance you can have a look at it? > > --- > > Changes in v2: > > - DD core API device_defer_all_probes(bool enable) split on two > > void device_defer_all_probes_enable(void); > > void device_defer_all_probes_disable(void); > > - more comments added > > > > Link on v1: > > - https://lkml.org/lkml/2015/10/8/681 > > > > drivers/base/base.h | 2 ++ > > drivers/base/dd.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- > > drivers/base/power/main.c | 17 +++++++++++++++++ > > 3 files changed, 66 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/base.h b/drivers/base/base.h > > index 1782f3a..c332b68 100644 > > --- a/drivers/base/base.h > > +++ b/drivers/base/base.h > > @@ -131,6 +131,8 @@ extern void device_remove_groups(struct device *dev, > > extern char *make_class_name(const char *name, struct kobject *kobj); > > > > extern int devres_release_all(struct device *dev); > > +extern void device_defer_all_probes_enable(void); > > +extern void device_defer_all_probes_disable(void); > > > > /* /sys/devices directory */ > > extern struct kset *devices_kset; > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > > index be0eb46..b8d9e70 100644 > > --- a/drivers/base/dd.c > > +++ b/drivers/base/dd.c > > @@ -55,6 +55,13 @@ static struct workqueue_struct *deferred_wq; > > static atomic_t deferred_trigger_count = ATOMIC_INIT(0); > > > > /* > > + * In some cases, like suspend to RAM or hibernation, It might be reasonable > > + * to prohibit probing of devices as it could be unsafe. > > + * Once defer_all_probes is true all drivers probes will be forcibly deferred. > > + */ > > +static bool defer_all_probes; > > + > > +/* > > * deferred_probe_work_func() - Retry probing devices in the active list. > > */ > > static void deferred_probe_work_func(struct work_struct *work) > > @@ -172,6 +179,30 @@ static void driver_deferred_probe_trigger(void) > > } > > > > /** > > + * device_defer_all_probes_enable() - Enable deferring of device's probes > > + * > > + * It will disable probing of devices and defer their probes. > > + */ > > +void device_defer_all_probes_enable(void) > > +{ > > + defer_all_probes = true; > > + /* sync with probes to avoid races. */ > > + wait_for_device_probe(); > > +} > > + > > +/** > > + * device_defer_all_probes_disable() - Disable deferring of device's probes > > + * > > + * It will restore normal behavior and trigger re-probing of deferred > > + * devices. > > + */ > > +void device_defer_all_probes_disable(void) > > +{ > > + defer_all_probes = false; > > + driver_deferred_probe_trigger(); > > +} > > + > > +/** > > * deferred_probe_initcall() - Enable probing of deferred devices > > * > > * We don't want to get in the way when the bulk of drivers are getting probed. > > @@ -277,9 +308,20 @@ static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue); > > > > static int really_probe(struct device *dev, struct device_driver *drv) > > { > > - int ret = 0; > > + int ret = -EPROBE_DEFER; > > int local_trigger_count = atomic_read(&deferred_trigger_count); > > > > + if (defer_all_probes) { > > + /* > > + * Value of defer_all_probes can be set only by > > + * device_defer_all_probes_enable() which, in turn, will call > > + * wait_for_device_probe() right after that to avoid any races. > > + */ > > + dev_dbg(dev, "Driver %s force probe deferral\n", drv->name); > > + driver_deferred_probe_add(dev); > > + return ret; > > + } > > + > > atomic_inc(&probe_count); > > pr_debug("bus: '%s': %s: probing driver %s with device %s\n", > > drv->bus->name, __func__, drv->name, dev_name(dev)); > > @@ -391,6 +433,10 @@ int driver_probe_done(void) > > */ > > void wait_for_device_probe(void) > > { > > + /* wait for the deferred probe workqueue to finish */ > > + if (driver_deferred_probe_enable) > > + flush_workqueue(deferred_wq); > > + > > /* wait for the known devices to complete their probing */ > > wait_event(probe_waitqueue, atomic_read(&probe_count) == 0); > > async_synchronize_full(); > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > > index 1710c26..eeb2bb7 100644 > > --- a/drivers/base/power/main.c > > +++ b/drivers/base/power/main.c > > @@ -963,6 +963,9 @@ void dpm_complete(pm_message_t state) > > } > > list_splice(&list, &dpm_list); > > mutex_unlock(&dpm_list_mtx); > > + > > + /* Allow device probing and trigger re-probing of deferred devices */ > > + device_defer_all_probes_disable(); > > trace_suspend_resume(TPS("dpm_complete"), state.event, false); > > } > > > > @@ -1624,6 +1627,20 @@ int dpm_prepare(pm_message_t state) > > trace_suspend_resume(TPS("dpm_prepare"), state.event, true); > > might_sleep(); > > > > + /* > > + * Give a chance for the known devices to complete their probes, before > > + * disable probing of devices. This sync point is important at least > > + * at boot time + hibernation restore. > > + */ > > + wait_for_device_probe(); > > + /* > > + * It is unsafe if probing of devices will happen during suspend or > > + * hibernation and system behavior will be unpredictable in this case. > > + * So, let's prohibit device's probing here and defer their probes > > + * instead. The normal behavior will be restored in dpm_complete(). > > + */ > > + device_defer_all_probes_enable(); > > + > > mutex_lock(&dpm_list_mtx); > > while (!list_empty(&dpm_list)) { > > struct device *dev = to_device(dpm_list.next); > > > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 05, 2015 at 11:38:23PM +0100, Rafael J. Wysocki wrote: > On Monday, November 02, 2015 02:25:00 AM Rafael J. Wysocki wrote: > > On Monday, October 19, 2015 11:54:24 PM Grygorii Strashko wrote: > > > It is unsafe [1] if probing of devices will happen during suspend or > > > hibernation and system behavior will be unpredictable in this case > > > (for example: after successful probe the device potentially has a different > > > set of PM callbacks than before [2]). > > > So, let's prohibit device's probing in dpm_prepare() and defer their > > > probes instead. The normal behavior will be restored in dpm_complete(). > > > > > > This patch introduces new DD core APIs: > > > device_defer_all_probes_enable() > > > It will disable probing of devices and defer their probes. > > > device_defer_all_probes_disable() > > > It will restore normal behavior and trigger re-probing of deferred > > > devices. > > > > > > [1] https://lkml.org/lkml/2015/9/11/554 > > > [2] https://lkml.org/lkml/2015/9/15/1039 > > > Cc: Alan Stern <stern@rowland.harvard.edu> > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > > Cc: Thierry Reding <thierry.reding@gmail.com> > > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > > > Greg, any objections against this one? > > Greg, if this isn't problematic, I'd still like to take it for v4.4. What? The merge window for new stuff closed a few weeks ago, how can you add this for 4.4 when it needs to be in linux-next for a while first? I don't have any real objection to it, but I don't see how defering probing until "later" really solves anything, no one should be "changing pm callbacks" after some other probe succeeds, that sounds like someone just wants to have a broken system. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 05, 2015 11:19:03 PM Pavel Machek wrote: > On Mon 2015-10-19 23:54:24, Grygorii Strashko wrote: > > It is unsafe [1] if probing of devices will happen during suspend or > > hibernation and system behavior will be unpredictable in this case > > (for example: after successful probe the device potentially has a different > > set of PM callbacks than before [2]). > > So, let's prohibit device's probing in dpm_prepare() and defer their > > probes instead. The normal behavior will be restored in dpm_complete(). > > > > This patch introduces new DD core APIs: > > device_defer_all_probes_enable() > > It will disable probing of devices and defer their probes. > > device_defer_all_probes_disable() > > It will restore normal behavior and trigger re-probing of deferred > > devices. > > > > [1] https://lkml.org/lkml/2015/9/11/554 > > [2] https://lkml.org/lkml/2015/9/15/1039 > > Cc: Alan Stern <stern@rowland.harvard.edu> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > Cc: Thierry Reding <thierry.reding@gmail.com> > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > --- > > Changes in v2: > > - DD core API device_defer_all_probes(bool enable) split on two > > void device_defer_all_probes_enable(void); > > void device_defer_all_probes_disable(void); > > - more comments added > > > > Link on v1: > > - https://lkml.org/lkml/2015/10/8/681 > > > > drivers/base/base.h | 2 ++ > > drivers/base/dd.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- > > drivers/base/power/main.c | 17 +++++++++++++++++ > > 3 files changed, 66 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/base.h b/drivers/base/base.h > > index 1782f3a..c332b68 100644 > > --- a/drivers/base/base.h > > +++ b/drivers/base/base.h > > @@ -131,6 +131,8 @@ extern void device_remove_groups(struct device *dev, > > extern char *make_class_name(const char *name, struct kobject *kobj); > > > > extern int devres_release_all(struct device *dev); > > +extern void device_defer_all_probes_enable(void); > > +extern void device_defer_all_probes_disable(void); > > > > /* /sys/devices directory */ > > extern struct kset *devices_kset; > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > > index be0eb46..b8d9e70 100644 > > --- a/drivers/base/dd.c > > +++ b/drivers/base/dd.c > > @@ -55,6 +55,13 @@ static struct workqueue_struct *deferred_wq; > > static atomic_t deferred_trigger_count = ATOMIC_INIT(0); > > > > /* > > + * In some cases, like suspend to RAM or hibernation, It might be reasonable > > + * to prohibit probing of devices as it could be unsafe. > > + * Once defer_all_probes is true all drivers probes will be forcibly deferred. > > + */ > > +static bool defer_all_probes; > > + > > +/* > > * deferred_probe_work_func() - Retry probing devices in the active list. > > */ > > static void deferred_probe_work_func(struct work_struct *work) > > @@ -172,6 +179,30 @@ static void driver_deferred_probe_trigger(void) > > } > > > > /** > > + * device_defer_all_probes_enable() - Enable deferring of device's probes > > + * > > + * It will disable probing of devices and defer their probes. > > + */ > > +void device_defer_all_probes_enable(void) > > +{ > > + defer_all_probes = true; > > + /* sync with probes to avoid races. */ > > + wait_for_device_probe(); > > +} > > device_pause_probing()? > > > +/** > > + * device_defer_all_probes_disable() - Disable deferring of device's probes > > + * > > + * It will restore normal behavior and trigger re-probing of deferred > > + * devices. > > + */ > > Hmm. This is not quite a double negative... but it sounds like one. > > device_restart_probing()? I _start/_stop would be fine by me too. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 05, 2015 04:09:57 PM Greg Kroah-Hartman wrote: > On Thu, Nov 05, 2015 at 11:38:23PM +0100, Rafael J. Wysocki wrote: > > On Monday, November 02, 2015 02:25:00 AM Rafael J. Wysocki wrote: > > > On Monday, October 19, 2015 11:54:24 PM Grygorii Strashko wrote: > > > > It is unsafe [1] if probing of devices will happen during suspend or > > > > hibernation and system behavior will be unpredictable in this case > > > > (for example: after successful probe the device potentially has a different > > > > set of PM callbacks than before [2]). > > > > So, let's prohibit device's probing in dpm_prepare() and defer their > > > > probes instead. The normal behavior will be restored in dpm_complete(). > > > > > > > > This patch introduces new DD core APIs: > > > > device_defer_all_probes_enable() > > > > It will disable probing of devices and defer their probes. > > > > device_defer_all_probes_disable() > > > > It will restore normal behavior and trigger re-probing of deferred > > > > devices. > > > > > > > > [1] https://lkml.org/lkml/2015/9/11/554 > > > > [2] https://lkml.org/lkml/2015/9/15/1039 > > > > Cc: Alan Stern <stern@rowland.harvard.edu> > > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > > > Cc: Thierry Reding <thierry.reding@gmail.com> > > > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > > > > > Greg, any objections against this one? > > > > Greg, if this isn't problematic, I'd still like to take it for v4.4. > > What? The merge window for new stuff closed a few weeks ago, how can > you add this for 4.4 when it needs to be in linux-next for a while > first? Well, it's not really scary and it was posted way before the merge window, but no one has had the time to look at it since then, apparently. Of course, I can queue it up for the next release too. > I don't have any real objection to it, but I don't see how defering > probing until "later" really solves anything, no one should be "changing > pm callbacks" after some other probe succeeds, that sounds like someone > just wants to have a broken system. This really is about preventing things from being probed during system suspend/resume, because (successful) probing effectively changes the set of PM callbacks for the probed device (the ones provided by the driver are now available), so allowing it to happen during suspend/resume is a bad idea (as you said). This is just a safety measure to provide certain guarantee ("your callbacks are not going to change in the middle of a suspend/resume sequence"). Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 06, 2015 at 03:04:10AM +0100, Rafael J. Wysocki wrote: > On Thursday, November 05, 2015 04:09:57 PM Greg Kroah-Hartman wrote: > > On Thu, Nov 05, 2015 at 11:38:23PM +0100, Rafael J. Wysocki wrote: > > > On Monday, November 02, 2015 02:25:00 AM Rafael J. Wysocki wrote: > > > > On Monday, October 19, 2015 11:54:24 PM Grygorii Strashko wrote: > > > > > It is unsafe [1] if probing of devices will happen during suspend or > > > > > hibernation and system behavior will be unpredictable in this case > > > > > (for example: after successful probe the device potentially has a different > > > > > set of PM callbacks than before [2]). > > > > > So, let's prohibit device's probing in dpm_prepare() and defer their > > > > > probes instead. The normal behavior will be restored in dpm_complete(). > > > > > > > > > > This patch introduces new DD core APIs: > > > > > device_defer_all_probes_enable() > > > > > It will disable probing of devices and defer their probes. > > > > > device_defer_all_probes_disable() > > > > > It will restore normal behavior and trigger re-probing of deferred > > > > > devices. > > > > > > > > > > [1] https://lkml.org/lkml/2015/9/11/554 > > > > > [2] https://lkml.org/lkml/2015/9/15/1039 > > > > > Cc: Alan Stern <stern@rowland.harvard.edu> > > > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > > > > Cc: Thierry Reding <thierry.reding@gmail.com> > > > > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > > > > > > > Greg, any objections against this one? > > > > > > Greg, if this isn't problematic, I'd still like to take it for v4.4. > > > > What? The merge window for new stuff closed a few weeks ago, how can > > you add this for 4.4 when it needs to be in linux-next for a while > > first? > > Well, it's not really scary and it was posted way before the merge window, > but no one has had the time to look at it since then, apparently. > > Of course, I can queue it up for the next release too. That would be best, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/06/2015 02:13 AM, Rafael J. Wysocki wrote: > On Thursday, November 05, 2015 11:19:03 PM Pavel Machek wrote: >> On Mon 2015-10-19 23:54:24, Grygorii Strashko wrote: >>> It is unsafe [1] if probing of devices will happen during suspend or >>> hibernation and system behavior will be unpredictable in this case >>> (for example: after successful probe the device potentially has a different >>> set of PM callbacks than before [2]). >>> So, let's prohibit device's probing in dpm_prepare() and defer their >>> probes instead. The normal behavior will be restored in dpm_complete(). >>> >>> This patch introduces new DD core APIs: >>> device_defer_all_probes_enable() >>> It will disable probing of devices and defer their probes. >>> device_defer_all_probes_disable() >>> It will restore normal behavior and trigger re-probing of deferred >>> devices. >>> >>> [1] https://lkml.org/lkml/2015/9/11/554 >>> [2] https://lkml.org/lkml/2015/9/15/1039 >>> Cc: Alan Stern <stern@rowland.harvard.edu> >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>> Cc: Thierry Reding <thierry.reding@gmail.com> >>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>> --- >>> Changes in v2: >>> - DD core API device_defer_all_probes(bool enable) split on two >>> void device_defer_all_probes_enable(void); >>> void device_defer_all_probes_disable(void); >>> - more comments added >>> >>> Link on v1: >>> - https://lkml.org/lkml/2015/10/8/681 >>> >>> drivers/base/base.h | 2 ++ >>> drivers/base/dd.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- >>> drivers/base/power/main.c | 17 +++++++++++++++++ >>> 3 files changed, 66 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/base/base.h b/drivers/base/base.h >>> index 1782f3a..c332b68 100644 >>> --- a/drivers/base/base.h >>> +++ b/drivers/base/base.h >>> @@ -131,6 +131,8 @@ extern void device_remove_groups(struct device *dev, >>> extern char *make_class_name(const char *name, struct kobject *kobj); >>> >>> extern int devres_release_all(struct device *dev); >>> +extern void device_defer_all_probes_enable(void); >>> +extern void device_defer_all_probes_disable(void); >>> >>> /* /sys/devices directory */ >>> extern struct kset *devices_kset; >>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >>> index be0eb46..b8d9e70 100644 >>> --- a/drivers/base/dd.c >>> +++ b/drivers/base/dd.c >>> @@ -55,6 +55,13 @@ static struct workqueue_struct *deferred_wq; >>> static atomic_t deferred_trigger_count = ATOMIC_INIT(0); >>> >>> /* >>> + * In some cases, like suspend to RAM or hibernation, It might be reasonable >>> + * to prohibit probing of devices as it could be unsafe. >>> + * Once defer_all_probes is true all drivers probes will be forcibly deferred. >>> + */ >>> +static bool defer_all_probes; >>> + >>> +/* >>> * deferred_probe_work_func() - Retry probing devices in the active list. >>> */ >>> static void deferred_probe_work_func(struct work_struct *work) >>> @@ -172,6 +179,30 @@ static void driver_deferred_probe_trigger(void) >>> } >>> >>> /** >>> + * device_defer_all_probes_enable() - Enable deferring of device's probes >>> + * >>> + * It will disable probing of devices and defer their probes. >>> + */ >>> +void device_defer_all_probes_enable(void) >>> +{ >>> + defer_all_probes = true; >>> + /* sync with probes to avoid races. */ >>> + wait_for_device_probe(); >>> +} >> >> device_pause_probing()? device_stop_probing(). Right? >> >>> +/** >>> + * device_defer_all_probes_disable() - Disable deferring of device's probes >>> + * >>> + * It will restore normal behavior and trigger re-probing of deferred >>> + * devices. >>> + */ device_start_probing(). Right? >> >> Hmm. This is not quite a double negative... but it sounds like one. >> >> device_restart_probing()? > > I _start/_stop would be fine by me too. Ok. So do I need rename/resend it again? Probably, I'll need to wait for -rc1 before resending.
On Friday, November 06, 2015 11:58:40 AM Grygorii Strashko wrote: > On 11/06/2015 02:13 AM, Rafael J. Wysocki wrote: > > On Thursday, November 05, 2015 11:19:03 PM Pavel Machek wrote: > >> On Mon 2015-10-19 23:54:24, Grygorii Strashko wrote: > >>> It is unsafe [1] if probing of devices will happen during suspend or > >>> hibernation and system behavior will be unpredictable in this case > >>> (for example: after successful probe the device potentially has a different > >>> set of PM callbacks than before [2]). > >>> So, let's prohibit device's probing in dpm_prepare() and defer their > >>> probes instead. The normal behavior will be restored in dpm_complete(). > >>> > >>> This patch introduces new DD core APIs: > >>> device_defer_all_probes_enable() > >>> It will disable probing of devices and defer their probes. > >>> device_defer_all_probes_disable() > >>> It will restore normal behavior and trigger re-probing of deferred > >>> devices. > >>> > >>> [1] https://lkml.org/lkml/2015/9/11/554 > >>> [2] https://lkml.org/lkml/2015/9/15/1039 > >>> Cc: Alan Stern <stern@rowland.harvard.edu> > >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > >>> Cc: Thierry Reding <thierry.reding@gmail.com> > >>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > >>> --- > >>> Changes in v2: > >>> - DD core API device_defer_all_probes(bool enable) split on two > >>> void device_defer_all_probes_enable(void); > >>> void device_defer_all_probes_disable(void); > >>> - more comments added > >>> > >>> Link on v1: > >>> - https://lkml.org/lkml/2015/10/8/681 > >>> > >>> drivers/base/base.h | 2 ++ > >>> drivers/base/dd.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- > >>> drivers/base/power/main.c | 17 +++++++++++++++++ > >>> 3 files changed, 66 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/base/base.h b/drivers/base/base.h > >>> index 1782f3a..c332b68 100644 > >>> --- a/drivers/base/base.h > >>> +++ b/drivers/base/base.h > >>> @@ -131,6 +131,8 @@ extern void device_remove_groups(struct device *dev, > >>> extern char *make_class_name(const char *name, struct kobject *kobj); > >>> > >>> extern int devres_release_all(struct device *dev); > >>> +extern void device_defer_all_probes_enable(void); > >>> +extern void device_defer_all_probes_disable(void); > >>> > >>> /* /sys/devices directory */ > >>> extern struct kset *devices_kset; > >>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c > >>> index be0eb46..b8d9e70 100644 > >>> --- a/drivers/base/dd.c > >>> +++ b/drivers/base/dd.c > >>> @@ -55,6 +55,13 @@ static struct workqueue_struct *deferred_wq; > >>> static atomic_t deferred_trigger_count = ATOMIC_INIT(0); > >>> > >>> /* > >>> + * In some cases, like suspend to RAM or hibernation, It might be reasonable > >>> + * to prohibit probing of devices as it could be unsafe. > >>> + * Once defer_all_probes is true all drivers probes will be forcibly deferred. > >>> + */ > >>> +static bool defer_all_probes; > >>> + > >>> +/* > >>> * deferred_probe_work_func() - Retry probing devices in the active list. > >>> */ > >>> static void deferred_probe_work_func(struct work_struct *work) > >>> @@ -172,6 +179,30 @@ static void driver_deferred_probe_trigger(void) > >>> } > >>> > >>> /** > >>> + * device_defer_all_probes_enable() - Enable deferring of device's probes > >>> + * > >>> + * It will disable probing of devices and defer their probes. > >>> + */ > >>> +void device_defer_all_probes_enable(void) > >>> +{ > >>> + defer_all_probes = true; > >>> + /* sync with probes to avoid races. */ > >>> + wait_for_device_probe(); > >>> +} > >> > >> device_pause_probing()? > > device_stop_probing(). Right? Or device_block_probing() > >> > >>> +/** > >>> + * device_defer_all_probes_disable() - Disable deferring of device's probes > >>> + * > >>> + * It will restore normal behavior and trigger re-probing of deferred > >>> + * devices. > >>> + */ > > device_start_probing(). Right? and device_unblock_probing() > > >> > >> Hmm. This is not quite a double negative... but it sounds like one. > >> > >> device_restart_probing()? > > > > I _start/_stop would be fine by me too. > > Ok. So do I need rename/resend it again? Yes, please. > Probably, I'll need to wait for -rc1 before resending. No, you don't. Please resend when you're ready. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 05, 2015 06:07:45 PM Greg Kroah-Hartman wrote: > On Fri, Nov 06, 2015 at 03:04:10AM +0100, Rafael J. Wysocki wrote: > > On Thursday, November 05, 2015 04:09:57 PM Greg Kroah-Hartman wrote: > > > On Thu, Nov 05, 2015 at 11:38:23PM +0100, Rafael J. Wysocki wrote: > > > > On Monday, November 02, 2015 02:25:00 AM Rafael J. Wysocki wrote: > > > > > On Monday, October 19, 2015 11:54:24 PM Grygorii Strashko wrote: > > > > > > It is unsafe [1] if probing of devices will happen during suspend or > > > > > > hibernation and system behavior will be unpredictable in this case > > > > > > (for example: after successful probe the device potentially has a different > > > > > > set of PM callbacks than before [2]). > > > > > > So, let's prohibit device's probing in dpm_prepare() and defer their > > > > > > probes instead. The normal behavior will be restored in dpm_complete(). > > > > > > > > > > > > This patch introduces new DD core APIs: > > > > > > device_defer_all_probes_enable() > > > > > > It will disable probing of devices and defer their probes. > > > > > > device_defer_all_probes_disable() > > > > > > It will restore normal behavior and trigger re-probing of deferred > > > > > > devices. > > > > > > > > > > > > [1] https://lkml.org/lkml/2015/9/11/554 > > > > > > [2] https://lkml.org/lkml/2015/9/15/1039 > > > > > > Cc: Alan Stern <stern@rowland.harvard.edu> > > > > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > > > > > Cc: Thierry Reding <thierry.reding@gmail.com> > > > > > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > > > > > > > > > Greg, any objections against this one? > > > > > > > > Greg, if this isn't problematic, I'd still like to take it for v4.4. > > > > > > What? The merge window for new stuff closed a few weeks ago, how can > > > you add this for 4.4 when it needs to be in linux-next for a while > > > first? > > > > Well, it's not really scary and it was posted way before the merge window, > > but no one has had the time to look at it since then, apparently. > > > > Of course, I can queue it up for the next release too. > > That would be best, thanks. OK -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/base.h b/drivers/base/base.h index 1782f3a..c332b68 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -131,6 +131,8 @@ extern void device_remove_groups(struct device *dev, extern char *make_class_name(const char *name, struct kobject *kobj); extern int devres_release_all(struct device *dev); +extern void device_defer_all_probes_enable(void); +extern void device_defer_all_probes_disable(void); /* /sys/devices directory */ extern struct kset *devices_kset; diff --git a/drivers/base/dd.c b/drivers/base/dd.c index be0eb46..b8d9e70 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -55,6 +55,13 @@ static struct workqueue_struct *deferred_wq; static atomic_t deferred_trigger_count = ATOMIC_INIT(0); /* + * In some cases, like suspend to RAM or hibernation, It might be reasonable + * to prohibit probing of devices as it could be unsafe. + * Once defer_all_probes is true all drivers probes will be forcibly deferred. + */ +static bool defer_all_probes; + +/* * deferred_probe_work_func() - Retry probing devices in the active list. */ static void deferred_probe_work_func(struct work_struct *work) @@ -172,6 +179,30 @@ static void driver_deferred_probe_trigger(void) } /** + * device_defer_all_probes_enable() - Enable deferring of device's probes + * + * It will disable probing of devices and defer their probes. + */ +void device_defer_all_probes_enable(void) +{ + defer_all_probes = true; + /* sync with probes to avoid races. */ + wait_for_device_probe(); +} + +/** + * device_defer_all_probes_disable() - Disable deferring of device's probes + * + * It will restore normal behavior and trigger re-probing of deferred + * devices. + */ +void device_defer_all_probes_disable(void) +{ + defer_all_probes = false; + driver_deferred_probe_trigger(); +} + +/** * deferred_probe_initcall() - Enable probing of deferred devices * * We don't want to get in the way when the bulk of drivers are getting probed. @@ -277,9 +308,20 @@ static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue); static int really_probe(struct device *dev, struct device_driver *drv) { - int ret = 0; + int ret = -EPROBE_DEFER; int local_trigger_count = atomic_read(&deferred_trigger_count); + if (defer_all_probes) { + /* + * Value of defer_all_probes can be set only by + * device_defer_all_probes_enable() which, in turn, will call + * wait_for_device_probe() right after that to avoid any races. + */ + dev_dbg(dev, "Driver %s force probe deferral\n", drv->name); + driver_deferred_probe_add(dev); + return ret; + } + atomic_inc(&probe_count); pr_debug("bus: '%s': %s: probing driver %s with device %s\n", drv->bus->name, __func__, drv->name, dev_name(dev)); @@ -391,6 +433,10 @@ int driver_probe_done(void) */ void wait_for_device_probe(void) { + /* wait for the deferred probe workqueue to finish */ + if (driver_deferred_probe_enable) + flush_workqueue(deferred_wq); + /* wait for the known devices to complete their probing */ wait_event(probe_waitqueue, atomic_read(&probe_count) == 0); async_synchronize_full(); diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 1710c26..eeb2bb7 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -963,6 +963,9 @@ void dpm_complete(pm_message_t state) } list_splice(&list, &dpm_list); mutex_unlock(&dpm_list_mtx); + + /* Allow device probing and trigger re-probing of deferred devices */ + device_defer_all_probes_disable(); trace_suspend_resume(TPS("dpm_complete"), state.event, false); } @@ -1624,6 +1627,20 @@ int dpm_prepare(pm_message_t state) trace_suspend_resume(TPS("dpm_prepare"), state.event, true); might_sleep(); + /* + * Give a chance for the known devices to complete their probes, before + * disable probing of devices. This sync point is important at least + * at boot time + hibernation restore. + */ + wait_for_device_probe(); + /* + * It is unsafe if probing of devices will happen during suspend or + * hibernation and system behavior will be unpredictable in this case. + * So, let's prohibit device's probing here and defer their probes + * instead. The normal behavior will be restored in dpm_complete(). + */ + device_defer_all_probes_enable(); + mutex_lock(&dpm_list_mtx); while (!list_empty(&dpm_list)) { struct device *dev = to_device(dpm_list.next);
It is unsafe [1] if probing of devices will happen during suspend or hibernation and system behavior will be unpredictable in this case (for example: after successful probe the device potentially has a different set of PM callbacks than before [2]). So, let's prohibit device's probing in dpm_prepare() and defer their probes instead. The normal behavior will be restored in dpm_complete(). This patch introduces new DD core APIs: device_defer_all_probes_enable() It will disable probing of devices and defer their probes. device_defer_all_probes_disable() It will restore normal behavior and trigger re-probing of deferred devices. [1] https://lkml.org/lkml/2015/9/11/554 [2] https://lkml.org/lkml/2015/9/15/1039 Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Thierry Reding <thierry.reding@gmail.com> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- Changes in v2: - DD core API device_defer_all_probes(bool enable) split on two void device_defer_all_probes_enable(void); void device_defer_all_probes_disable(void); - more comments added Link on v1: - https://lkml.org/lkml/2015/10/8/681 drivers/base/base.h | 2 ++ drivers/base/dd.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- drivers/base/power/main.c | 17 +++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-)