Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1242606 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe Grygorii Strashko <grygorii.strashko@ti.com> - 2015-10-08 19:00 +0200
    Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-08 22:30 +0200
      Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is  finished in wait_for_device_probe Alan Stern <stern@rowland.harvard.edu> - 2015-10-08 23:00 +0200
        Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is  finished in wait_for_device_probe Grygorii Strashko <grygorii.strashko@ti.com> - 2015-10-09 16:40 +0200
          Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is finished in wait_for_device_probe "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-09 22:50 +0200
            Re: [PATCH 1/2] PM / sleep: ensure deferred probe workqueue is  finished in wait_for_device_probe Grygorii Strashko <grygorii.strashko@ti.com> - 2015-10-13 20:30 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1242718

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243640

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web