[PATCH 0/2] PM / sleep: prohibit devices probing during suspend/hibernation

[PATCH 0/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Grygorii Strashko]

The main goal of this patchset is to ensure that devices probing will not
happen during system transition to low power states like suspend or
hibernation, because it is unsafe [1] 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]).

Tested on dra7-evm using a simple kernel module which only does 
msleep(5000) in its probe.
Test cmd: insmod test_probe.ko & echo mem > sys/power/state
 - suspend will wait for test_probe to finish

[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>
Grygorii Strashko (2):
  PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe
  PM / sleep: prohibit devices probing during suspend/hibernation

 drivers/base/base.h       |  1 +
 drivers/base/dd.c         | 39 ++++++++++++++++++++++++++++++++++++++-
 drivers/base/power/main.c | 13 +++++++++++++
 3 files changed, 52 insertions(+), 1 deletion(-)

-- 
2.5.1

[PATCH 1/2] PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe

[Grygorii Strashko]

Now wait_for_device_probe() waits for currently executing probes to finish,
but it doesn't take into account deferred probing mechanism. As result,
nothing prevents deferred probe workqueue to continue probing devices right
after wait_for_device_probe() is finished.

Hence, lest ensure deferred probe workqueue is finished in
wait_for_device_probe() before proceeding.

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>
---
 drivers/base/dd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index be0eb46..98de1a5 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -391,6 +391,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();
-- 
2.5.1

Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe

[Rafael J. Wysocki]

On Thursday, October 08, 2015 11:57:06 AM Grygorii Strashko wrote:
> Now wait_for_device_probe() waits for currently executing probes to finish,
> but it doesn't take into account deferred probing mechanism. As result,
> nothing prevents deferred probe workqueue to continue probing devices right
> after wait_for_device_probe() is finished.
> 
> Hence, lest ensure deferred probe workqueue is finished in

s/lest/let's/

> wait_for_device_probe() before proceeding.
> 
> 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>
> ---
>  drivers/base/dd.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index be0eb46..98de1a5 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -391,6 +391,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();

I'm not sure if this is sufficient.

Something may be added to the workqueue right after you've flushed it and
then be reporobed after the wait_event() in theory.  Or am I missing anything?

Thanks,
Rafael

Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe

[Alan Stern]

On Thu, 8 Oct 2015, Rafael J. Wysocki wrote:

> > @@ -391,6 +391,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();
> 
> I'm not sure if this is sufficient.
> 
> Something may be added to the workqueue right after you've flushed it and
> then be reporobed after the wait_event() in theory.  Or am I missing anything?

Maybe I'm missing part of this, but I think the point is to make sure 
that every probe which began or was queued before this function got 
called, has finished before the function returns.

Thus, in the case at hand we want to defer all probes starting from
some point in the system sleep transition.  Grygorii sets his
defer_all_probes variable and then calls this function.  It waits for
any probes that were initiated before the function call.  Any probe
that was initiated after the function call (for example, the ones
you're concerned about between the flush_workqueue and wait_event) will
see that defer_all_probes is set and so will defer itself.

Now, I'm not sure what happens when a probe that was deferred tries to 
defer itself again.  Do we end up in an infinite probing loop?  Is
the deferred_wq workqueue freezable?

Alan Stern

Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe

[Grygorii Strashko]

On 10/08/2015 03:53 PM, Alan Stern wrote:
> On Thu, 8 Oct 2015, Rafael J. Wysocki wrote:
> 
>>> @@ -391,6 +391,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();
>>
>> I'm not sure if this is sufficient.
>>
>> Something may be added to the workqueue right after you've flushed it and
>> then be reporobed after the wait_event() in theory.  Or am I missing anything?
> 
> Maybe I'm missing part of this, but I think the point is to make sure
> that every probe which began or was queued before this function got
> called, has finished before the function returns.
> 
> Thus, in the case at hand we want to defer all probes starting from
> some point in the system sleep transition.  Grygorii sets his
> defer_all_probes variable and then calls this function.  It waits for
> any probes that were initiated before the function call.  Any probe
> that was initiated after the function call (for example, the ones
> you're concerned about between the flush_workqueue and wait_event) will
> see that defer_all_probes is set and so will defer itself.

Yes. It will work as expected with the next patch.
For all other case, where this API is used alone -
it will make things more safe, but there is no way to completely block
scheduling of new probes.

> 
> Now, I'm not sure what happens when a probe that was deferred tries to
> defer itself again.  Do we end up in an infinite probing loop?

No. handling of defered probes will be re triggered only if
some probe was finished successfully.
> Is  the deferred_wq workqueue freezable?

seems WQ_FREEZABLE is not set for this WQ.


-- 
regards,
-grygorii

Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe

[Rafael J. Wysocki]

On Friday, October 09, 2015 09:38:13 AM Grygorii Strashko wrote:
> On 10/08/2015 03:53 PM, Alan Stern wrote:
> > On Thu, 8 Oct 2015, Rafael J. Wysocki wrote:
> > 
> >>> @@ -391,6 +391,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();
> >>
> >> I'm not sure if this is sufficient.
> >>
> >> Something may be added to the workqueue right after you've flushed it and
> >> then be reporobed after the wait_event() in theory.  Or am I missing anything?
> > 
> > Maybe I'm missing part of this, but I think the point is to make sure
> > that every probe which began or was queued before this function got
> > called, has finished before the function returns.
> > 
> > Thus, in the case at hand we want to defer all probes starting from
> > some point in the system sleep transition.  Grygorii sets his
> > defer_all_probes variable and then calls this function.  It waits for
> > any probes that were initiated before the function call.  Any probe
> > that was initiated after the function call (for example, the ones
> > you're concerned about between the flush_workqueue and wait_event) will
> > see that defer_all_probes is set and so will defer itself.
> 
> Yes. It will work as expected with the next patch.
> For all other case, where this API is used alone -
> it will make things more safe, but there is no way to completely block
> scheduling of new probes.

Well, in that case why don't you make it part of the second patch after all
instead of making a false impression of fixing a more general problem?

Thanks,
Rafael

Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe

[Grygorii Strashko]

On 10/10/2015 12:16 AM, Rafael J. Wysocki wrote:
> On Friday, October 09, 2015 09:38:13 AM Grygorii Strashko wrote:
>> On 10/08/2015 03:53 PM, Alan Stern wrote:
>>> On Thu, 8 Oct 2015, Rafael J. Wysocki wrote:
>>>
>>>>> @@ -391,6 +391,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();
>>>>
>>>> I'm not sure if this is sufficient.
>>>>
>>>> Something may be added to the workqueue right after you've flushed it and
>>>> then be reporobed after the wait_event() in theory.  Or am I missing anything?
>>>
>>> Maybe I'm missing part of this, but I think the point is to make sure
>>> that every probe which began or was queued before this function got
>>> called, has finished before the function returns.
>>>
>>> Thus, in the case at hand we want to defer all probes starting from
>>> some point in the system sleep transition.  Grygorii sets his
>>> defer_all_probes variable and then calls this function.  It waits for
>>> any probes that were initiated before the function call.  Any probe
>>> that was initiated after the function call (for example, the ones
>>> you're concerned about between the flush_workqueue and wait_event) will
>>> see that defer_all_probes is set and so will defer itself.
>>
>> Yes. It will work as expected with the next patch.
>> For all other case, where this API is used alone -
>> it will make things more safe, but there is no way to completely block
>> scheduling of new probes.
>
> Well, in that case why don't you make it part of the second patch after all
> instead of making a false impression of fixing a more general problem?
>

ok.


-- 
regards,
-grygorii

[PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Grygorii Strashko]

It is unsafe [1] if probing of devices will happen during suspend or
hibernation and system behavior will be unpredictable in this case.
So, lets prohibit device's probing in dpm_prepare() and defer their
probing instead. The normal behavior will be restored in
dpm_complete().

This patch introduces new DD core API device_defer_all_probes(bool enable)
to enable/disable probing of devices:
 if @enable = true
   It will disable probing of devices and defer their probes.
 otherwise
   It will restore normal behavior and trigger re-probing of deferred devices.

[1] https://lkml.org/lkml/2015/9/11/554
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>
---
 drivers/base/base.h       |  1 +
 drivers/base/dd.c         | 35 ++++++++++++++++++++++++++++++++++-
 drivers/base/power/main.c | 13 +++++++++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 1782f3a..6c9684b 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -131,6 +131,7 @@ 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(bool enable);
 
 /* /sys/devices directory */
 extern struct kset *devices_kset;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 98de1a5..d600c14 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,26 @@ static void driver_deferred_probe_trigger(void)
 }
 
 /**
+ * device_defer_all_probes() - Enable/disable probing of devices
+ * @enable:  Enable/disable probing of devices
+ *
+ * if @enable = true
+ *	It will disable probing of devices and defer their probes.
+ * otherwise
+ *	It will restore normal behavior and trigger re-probing of deferred
+ *	devices.
+ */
+void device_defer_all_probes(bool enable)
+{
+	defer_all_probes = enable;
+	if (enable)
+		/* sync with probes to avoid any races. */
+		wait_for_device_probe();
+	else
+		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 +304,15 @@ 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) {
+		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));
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 1710c26..98c1da36 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(false);
 	trace_suspend_resume(TPS("dpm_complete"), state.event, false);
 }
 
@@ -1624,6 +1627,16 @@ 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 probing. */
+	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, lets prohibit device's probing here and defer their probes
+	 * instead. The normal behavior will be restored in dpm_complete().
+	 */
+	device_defer_all_probes(true);
+
 	mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_list)) {
 		struct device *dev = to_device(dpm_list.next);
-- 
2.5.1

Re: [PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Alan Stern]

On Thu, 8 Oct 2015, 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.
> So, lets prohibit device's probing in dpm_prepare() and defer their

s/lets/let's/, and same for the comment in the code.

> probing instead. The normal behavior will be restored in
> dpm_complete().


> @@ -172,6 +179,26 @@ static void driver_deferred_probe_trigger(void)
>  }
>  
>  /**
> + * device_defer_all_probes() - Enable/disable probing of devices
> + * @enable:  Enable/disable probing of devices
> + *
> + * if @enable = true
> + *	It will disable probing of devices and defer their probes.
> + * otherwise
> + *	It will restore normal behavior and trigger re-probing of deferred
> + *	devices.
> + */
> +void device_defer_all_probes(bool enable)
> +{
> +	defer_all_probes = enable;
> +	if (enable)
> +		/* sync with probes to avoid any races. */
> +		wait_for_device_probe();
> +	else
> +		driver_deferred_probe_trigger();
> +}

Some people might prefer to see two separate functions, an enable
routine and a disable routine.  I don't much care.

> @@ -277,9 +304,15 @@ 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) {
> +		dev_dbg(dev, "Driver %s force probe deferral\n", drv->name);
> +		driver_deferred_probe_add(dev);
> +		return ret;
> +	}

In theory there's a race here.  If one CPU sets defer_all_probes, the 
new value might not be perceived by another CPU until a little while 
later.  Is there an easy way to insure that this race won't cause any 
problems?

Or do we already know that when this mechanism gets used, the system is 
already running on a single CPU?  I forget when that happens.

> @@ -1624,6 +1627,16 @@ 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 probing. */
> +	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, lets prohibit device's probing here and defer their probes
> +	 * instead. The normal behavior will be restored in dpm_complete().
> +	 */
> +	device_defer_all_probes(true);

Don't you want to call these two functions in the opposite order?  
First prevent new probes from occurring, then wait for any probes that 
are already in progress?  The way you have it here, a new probe could 
start between these two lines.

Alan Stern

Re: [PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Grygorii Strashko]

On 10/08/2015 12:24 PM, Alan Stern wrote:
> On Thu, 8 Oct 2015, 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.
>> So, lets prohibit device's probing in dpm_prepare() and defer their
> 
> s/lets/let's/, and same for the comment in the code.
> 
>> probing instead. The normal behavior will be restored in
>> dpm_complete().
> 
> 
>> @@ -172,6 +179,26 @@ static void driver_deferred_probe_trigger(void)
>>   }
>>   
>>   /**
>> + * device_defer_all_probes() - Enable/disable probing of devices
>> + * @enable:  Enable/disable probing of devices
>> + *
>> + * if @enable = true
>> + *	It will disable probing of devices and defer their probes.
>> + * otherwise
>> + *	It will restore normal behavior and trigger re-probing of deferred
>> + *	devices.
>> + */
>> +void device_defer_all_probes(bool enable)
>> +{
>> +	defer_all_probes = enable;
>> +	if (enable)
>> +		/* sync with probes to avoid any races. */
>> +		wait_for_device_probe();

^ pls, pay attention on above code line

>> +	else
>> +		driver_deferred_probe_trigger();
>> +}
> 
> Some people might prefer to see two separate functions, an enable
> routine and a disable routine.  I don't much care.

May be. Should I change it?

> 
>> @@ -277,9 +304,15 @@ 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) {
>> +		dev_dbg(dev, "Driver %s force probe deferral\n", drv->name);
>> +		driver_deferred_probe_add(dev);
>> +		return ret;
>> +	}
> 
> In theory there's a race here.  If one CPU sets defer_all_probes, the
> new value might not be perceived by another CPU until a little while
> later.  Is there an easy way to insure that this race won't cause any
> problems?

Yes. this question was raised by Rafael also [1].

> 
> Or do we already know that when this mechanism gets used, the system is
> already running on a single CPU?  I forget when that happens.

No. nonboot cpus are still  on.

> 
>> @@ -1624,6 +1627,16 @@ 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 probing. */
>> +	wait_for_device_probe();

^ this sync point is important at least at boot time + hibernation restore

>> +	/*
>> +	 * It is unsafe if probing of devices will happen during suspend or
>> +	 * hibernation and system behavior will be unpredictable in this case.
>> +	 * So, lets prohibit device's probing here and defer their probes
>> +	 * instead. The normal behavior will be restored in dpm_complete().
>> +	 */
>> +	device_defer_all_probes(true);
> 
> Don't you want to call these two functions in the opposite order?
> First prevent new probes from occurring, then wait for any probes that
> are already in progress?  The way you have it here, a new probe could
> start between these two lines.

No. Initially I did it as below:
     wait_for_device_probe(); <- wait for active probes
     device_defer_all_probes(true); <- prohibit probing
     wait_for_device_probe(); <- sync again to avoid races

then I decided to move second wait_for_device_probe() call inside
device_defer_all_probes() because it's always required.

[1] https://lkml.org/lkml/2015/9/17/857

-- 
regards,
-grygorii

Re: [PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Alan Stern]

On Thu, 8 Oct 2015, Grygorii Strashko wrote:

> >>   /**
> >> + * device_defer_all_probes() - Enable/disable probing of devices
> >> + * @enable:  Enable/disable probing of devices
> >> + *
> >> + * if @enable = true
> >> + *	It will disable probing of devices and defer their probes.
> >> + * otherwise
> >> + *	It will restore normal behavior and trigger re-probing of deferred
> >> + *	devices.
> >> + */
> >> +void device_defer_all_probes(bool enable)
> >> +{
> >> +	defer_all_probes = enable;
> >> +	if (enable)
> >> +		/* sync with probes to avoid any races. */
> >> +		wait_for_device_probe();
> 
> ^ pls, pay attention on above code line
> 
> >> +	else
> >> +		driver_deferred_probe_trigger();
> >> +}
> > 
> > Some people might prefer to see two separate functions, an enable
> > routine and a disable routine.  I don't much care.
> 
> May be. Should I change it?

It would then be more in line with functions like 
pm_runtime_set_{active|suspended} or pm_runtime_[dont_]use_autosuspend.

> >> @@ -277,9 +304,15 @@ 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) {
> >> +		dev_dbg(dev, "Driver %s force probe deferral\n", drv->name);
> >> +		driver_deferred_probe_add(dev);
> >> +		return ret;
> >> +	}
> > 
> > In theory there's a race here.  If one CPU sets defer_all_probes, the
> > new value might not be perceived by another CPU until a little while
> > later.  Is there an easy way to insure that this race won't cause any
> > problems?
> 
> Yes. this question was raised by Rafael also [1].

I see.  Can you add a comment explaining all of this?

Alan Stern

Re: [PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Grygorii Strashko]

On 10/08/2015 02:20 PM, Alan Stern wrote:
> On Thu, 8 Oct 2015, Grygorii Strashko wrote:
> 
>>>>    /**
>>>> + * device_defer_all_probes() - Enable/disable probing of devices
>>>> + * @enable:  Enable/disable probing of devices
>>>> + *
>>>> + * if @enable = true
>>>> + *	It will disable probing of devices and defer their probes.
>>>> + * otherwise
>>>> + *	It will restore normal behavior and trigger re-probing of deferred
>>>> + *	devices.
>>>> + */
>>>> +void device_defer_all_probes(bool enable)
>>>> +{
>>>> +	defer_all_probes = enable;
>>>> +	if (enable)
>>>> +		/* sync with probes to avoid any races. */
>>>> +		wait_for_device_probe();
>>
>> ^ pls, pay attention on above code line
>>
>>>> +	else
>>>> +		driver_deferred_probe_trigger();
>>>> +}
>>>
>>> Some people might prefer to see two separate functions, an enable
>>> routine and a disable routine.  I don't much care.
>>
>> May be. Should I change it?
> 
> It would then be more in line with functions like
> pm_runtime_set_{active|suspended} or pm_runtime_[dont_]use_autosuspend.

ok

> 
>>>> @@ -277,9 +304,15 @@ 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) {

Will it be ok If I add below comment here?
		/*
		 * 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;
>>>> +	}
>>>
>>> In theory there's a race here.  If one CPU sets defer_all_probes, the
>>> new value might not be perceived by another CPU until a little while
>>> later.  Is there an easy way to insure that this race won't cause any
>>> problems?
>>
>> Yes. this question was raised by Rafael also [1].
> 
> I see.  Can you add a comment explaining all of this?



-- 
regards,
-grygorii

Re: [PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Alan Stern]

On Thu, 8 Oct 2015, Grygorii Strashko wrote:

> >>>> @@ -277,9 +304,15 @@ 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) {
> 
> Will it be ok If I add below comment here?
> 		/*
> 		 * 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.
> 		 */

That will help.  You could also add a comment where
wait_for_device_probe is called before device_defer_all_probes_enable,
explaining that is needed.

Alan Stern

Re: [PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Rafael J. Wysocki]

On Thursday, October 08, 2015 03:20:08 PM Alan Stern wrote:
> On Thu, 8 Oct 2015, Grygorii Strashko wrote:
> 
> > >>   /**
> > >> + * device_defer_all_probes() - Enable/disable probing of devices
> > >> + * @enable:  Enable/disable probing of devices
> > >> + *
> > >> + * if @enable = true
> > >> + *	It will disable probing of devices and defer their probes.
> > >> + * otherwise
> > >> + *	It will restore normal behavior and trigger re-probing of deferred
> > >> + *	devices.
> > >> + */
> > >> +void device_defer_all_probes(bool enable)
> > >> +{
> > >> +	defer_all_probes = enable;
> > >> +	if (enable)
> > >> +		/* sync with probes to avoid any races. */
> > >> +		wait_for_device_probe();
> > 
> > ^ pls, pay attention on above code line
> > 
> > >> +	else
> > >> +		driver_deferred_probe_trigger();
> > >> +}
> > > 
> > > Some people might prefer to see two separate functions, an enable
> > > routine and a disable routine.  I don't much care.
> > 
> > May be. Should I change it?
> 
> It would then be more in line with functions like 
> pm_runtime_set_{active|suspended} or pm_runtime_[dont_]use_autosuspend.

Also it would be cleaner code without conditionals:

enable:
	defer_all_probes = true;
	wait_for_device_probe();

disable:
	defer_all_probes = false;
	driver_deferred_probe_trigger();

Cleaner, no?

Thanks,
Rafael

Re: [PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

[Grygorii Strashko]

On 10/08/2015 03:46 PM, Rafael J. Wysocki wrote:
> On Thursday, October 08, 2015 03:20:08 PM Alan Stern wrote:
>> On Thu, 8 Oct 2015, Grygorii Strashko wrote:
>>
>>>>>    /**
>>>>> + * device_defer_all_probes() - Enable/disable probing of devices
>>>>> + * @enable:  Enable/disable probing of devices
>>>>> + *
>>>>> + * if @enable = true
>>>>> + *	It will disable probing of devices and defer their probes.
>>>>> + * otherwise
>>>>> + *	It will restore normal behavior and trigger re-probing of deferred
>>>>> + *	devices.
>>>>> + */
>>>>> +void device_defer_all_probes(bool enable)
>>>>> +{
>>>>> +	defer_all_probes = enable;
>>>>> +	if (enable)
>>>>> +		/* sync with probes to avoid any races. */
>>>>> +		wait_for_device_probe();
>>>
>>> ^ pls, pay attention on above code line
>>>
>>>>> +	else
>>>>> +		driver_deferred_probe_trigger();
>>>>> +}
>>>>
>>>> Some people might prefer to see two separate functions, an enable
>>>> routine and a disable routine.  I don't much care.
>>>
>>> May be. Should I change it?
>>
>> It would then be more in line with functions like
>> pm_runtime_set_{active|suspended} or pm_runtime_[dont_]use_autosuspend.
>
> Also it would be cleaner code without conditionals:
>
> enable:
> 	defer_all_probes = true;
> 	wait_for_device_probe();
>
> disable:
> 	defer_all_probes = false;
> 	driver_deferred_probe_trigger();
>
> Cleaner, no?
>

NP. Will do.

-- 
regards,
-grygorii