[PATCH] device probe: add self triggered delayed work request

[PATCH] device probe: add self triggered delayed work request

[Qing Huang]

In normal condition, the device probe requests kept in deferred
queue would only be triggered for re-probing when another new device
probe is finished successfully. This change will set up a delayed
trigger work request if the current deferred probe being added is
the only one in the queue. This delayed work request will try to
reactivate any device from the deferred queue for re-probing later.

By doing this, if the last device being probed in system boot process
has a deferred probe error, this particular device will still be able
to be probed again.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Santosh Shilimkar <santosh.shilimkar@oracle.com>
Signed-off-by: Qing Huang <qing.huang@oracle.com>
---
 drivers/base/dd.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 16688f5..251042d 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -53,6 +53,9 @@ static LIST_HEAD(deferred_probe_pending_list);
 static LIST_HEAD(deferred_probe_active_list);
 static struct workqueue_struct *deferred_wq;
 static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
+static atomic_t deferred_self_trigger_count = ATOMIC_INIT(0);
+#define DEFERRED_SELF_TRIGGER_INTERVAL 30 /* 30 secs */
+#define DEFERRED_SELF_TRIGGER_ATTEMPTS 5  /* 5 times */
 
 /*
  * In some cases, like suspend to RAM or hibernation, It might be reasonable
@@ -115,10 +118,23 @@ static void deferred_probe_work_func(struct work_struct *work)
 	mutex_unlock(&deferred_probe_mutex);
 }
 static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);
+void driver_deferred_probe_trigger_wrapper(struct work_struct *work);
+static DECLARE_DELAYED_WORK(deferred_probe_trigger_work,
+			    driver_deferred_probe_trigger_wrapper);
 
 static void driver_deferred_probe_add(struct device *dev)
 {
+	int local_self_trigger_count;
+
 	mutex_lock(&deferred_probe_mutex);
+	local_self_trigger_count = atomic_read(&deferred_self_trigger_count);
+	if (list_empty(&deferred_probe_pending_list) &&
+	    local_self_trigger_count <= DEFERRED_SELF_TRIGGER_ATTEMPTS) {
+		cancel_delayed_work(&deferred_probe_trigger_work);
+		queue_delayed_work(deferred_wq, &deferred_probe_trigger_work,
+				   HZ * DEFERRED_SELF_TRIGGER_INTERVAL);
+	}
+
 	if (list_empty(&dev->p->deferred_probe)) {
 		dev_dbg(dev, "Added to deferred list\n");
 		list_add_tail(&dev->p->deferred_probe, &deferred_probe_pending_list);
@@ -202,6 +218,15 @@ void device_unblock_probing(void)
 	driver_deferred_probe_trigger();
 }
 
+/*
+ * Wrapper function for delayed trigger work requests
+ */
+void driver_deferred_probe_trigger_wrapper(struct work_struct *work)
+{
+	atomic_inc(&deferred_self_trigger_count);
+	driver_deferred_probe_trigger();
+}
+
 /**
  * deferred_probe_initcall() - Enable probing of deferred devices
  *
-- 
1.7.1

Re: [PATCH] device probe: add self triggered delayed work request

[Frank Rowand]

On 07/29/16 22:39, Qing Huang wrote:
> In normal condition, the device probe requests kept in deferred
> queue would only be triggered for re-probing when another new device
> probe is finished successfully. This change will set up a delayed
> trigger work request if the current deferred probe being added is
> the only one in the queue. This delayed work request will try to
> reactivate any device from the deferred queue for re-probing later.
> 
> By doing this, if the last device being probed in system boot process
> has a deferred probe error, this particular device will still be able
> to be probed again.

I am trying to understand the use case.

Can you explain the scenario you are trying to fix?  If I understand
correctly, you expect that something will change such that a later
probe attempt will succeed.  How will that change occur and why
will the deferred probe list not be processed in this case?

Why are you conditioning this on the deferred_probe_pending_list
being empty?

-Frank



> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Santosh Shilimkar <santosh.shilimkar@oracle.com>
> Signed-off-by: Qing Huang <qing.huang@oracle.com>
> ---
>  drivers/base/dd.c |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 16688f5..251042d 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -53,6 +53,9 @@ static LIST_HEAD(deferred_probe_pending_list);
>  static LIST_HEAD(deferred_probe_active_list);
>  static struct workqueue_struct *deferred_wq;
>  static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
> +static atomic_t deferred_self_trigger_count = ATOMIC_INIT(0);
> +#define DEFERRED_SELF_TRIGGER_INTERVAL 30 /* 30 secs */
> +#define DEFERRED_SELF_TRIGGER_ATTEMPTS 5  /* 5 times */
>  
>  /*
>   * In some cases, like suspend to RAM or hibernation, It might be reasonable
> @@ -115,10 +118,23 @@ static void deferred_probe_work_func(struct work_struct *work)
>  	mutex_unlock(&deferred_probe_mutex);
>  }
>  static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);
> +void driver_deferred_probe_trigger_wrapper(struct work_struct *work);
> +static DECLARE_DELAYED_WORK(deferred_probe_trigger_work,
> +			    driver_deferred_probe_trigger_wrapper);
>  
>  static void driver_deferred_probe_add(struct device *dev)
>  {
> +	int local_self_trigger_count;
> +
>  	mutex_lock(&deferred_probe_mutex);
> +	local_self_trigger_count = atomic_read(&deferred_self_trigger_count);
> +	if (list_empty(&deferred_probe_pending_list) &&
> +	    local_self_trigger_count <= DEFERRED_SELF_TRIGGER_ATTEMPTS) {
> +		cancel_delayed_work(&deferred_probe_trigger_work);
> +		queue_delayed_work(deferred_wq, &deferred_probe_trigger_work,
> +				   HZ * DEFERRED_SELF_TRIGGER_INTERVAL);
> +	}
> +
>  	if (list_empty(&dev->p->deferred_probe)) {
>  		dev_dbg(dev, "Added to deferred list\n");
>  		list_add_tail(&dev->p->deferred_probe, &deferred_probe_pending_list);
> @@ -202,6 +218,15 @@ void device_unblock_probing(void)
>  	driver_deferred_probe_trigger();
>  }
>  
> +/*
> + * Wrapper function for delayed trigger work requests
> + */
> +void driver_deferred_probe_trigger_wrapper(struct work_struct *work)
> +{
> +	atomic_inc(&deferred_self_trigger_count);
> +	driver_deferred_probe_trigger();
> +}
> +
>  /**
>   * deferred_probe_initcall() - Enable probing of deferred devices
>   *
> 

Re: [PATCH] device probe: add self triggered delayed work request

[Qing Huang]

On 08/08/2016 01:44 PM, Frank Rowand wrote:
> On 07/29/16 22:39, Qing Huang wrote:
>> In normal condition, the device probe requests kept in deferred
>> queue would only be triggered for re-probing when another new device
>> probe is finished successfully. This change will set up a delayed
>> trigger work request if the current deferred probe being added is
>> the only one in the queue. This delayed work request will try to
>> reactivate any device from the deferred queue for re-probing later.
>>
>> By doing this, if the last device being probed in system boot process
>> has a deferred probe error, this particular device will still be able
>> to be probed again.
> I am trying to understand the use case.
>
> Can you explain the scenario you are trying to fix?  If I understand
> correctly, you expect that something will change such that a later
> probe attempt will succeed.  How will that change occur and why
> will the deferred probe list not be processed in this case?
>
> Why are you conditioning this on the deferred_probe_pending_list
> being empty?
>
> -Frank

It turns out one corner case which we worried about has already been 
solved in the really_probe() function by comparing 
'deferred_trigger_count' values.

Another use case we are investigating now: when we probe a device, the 
main thread returns EPROBE_DEFER from the driver after we spawn a child 
thread to do the actual init work. So we can initialize multiple similar 
devices at the same time. After the child thread finishes its task, we 
can call driver_deferred_probe_trigger() directly from child thread to 
re-probe the device(driver_deferred_probe_trigger() has to be exported 
though). Or we could rely on something in this patch to re-probe the 
deferred devices from the pending list...

What do you suggest?

Thanks,
-Qing

>> Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>> Cc: Grant Likely<grant.likely@linaro.org>
>> Cc: Santosh Shilimkar<santosh.shilimkar@oracle.com>
>> Signed-off-by: Qing Huang<qing.huang@oracle.com>
>> ---
>>   drivers/base/dd.c |   25 +++++++++++++++++++++++++
>>   1 files changed, 25 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 16688f5..251042d 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -53,6 +53,9 @@ static LIST_HEAD(deferred_probe_pending_list);
>>   static LIST_HEAD(deferred_probe_active_list);
>>   static struct workqueue_struct *deferred_wq;
>>   static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
>> +static atomic_t deferred_self_trigger_count = ATOMIC_INIT(0);
>> +#define DEFERRED_SELF_TRIGGER_INTERVAL 30 /* 30 secs */
>> +#define DEFERRED_SELF_TRIGGER_ATTEMPTS 5  /* 5 times */
>>   
>>   /*
>>    * In some cases, like suspend to RAM or hibernation, It might be reasonable
>> @@ -115,10 +118,23 @@ static void deferred_probe_work_func(struct work_struct *work)
>>   	mutex_unlock(&deferred_probe_mutex);
>>   }
>>   static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);
>> +void driver_deferred_probe_trigger_wrapper(struct work_struct *work);
>> +static DECLARE_DELAYED_WORK(deferred_probe_trigger_work,
>> +			    driver_deferred_probe_trigger_wrapper);
>>   
>>   static void driver_deferred_probe_add(struct device *dev)
>>   {
>> +	int local_self_trigger_count;
>> +
>>   	mutex_lock(&deferred_probe_mutex);
>> +	local_self_trigger_count = atomic_read(&deferred_self_trigger_count);
>> +	if (list_empty(&deferred_probe_pending_list) &&
>> +	    local_self_trigger_count <= DEFERRED_SELF_TRIGGER_ATTEMPTS) {
>> +		cancel_delayed_work(&deferred_probe_trigger_work);
>> +		queue_delayed_work(deferred_wq, &deferred_probe_trigger_work,
>> +				   HZ * DEFERRED_SELF_TRIGGER_INTERVAL);
>> +	}
>> +
>>   	if (list_empty(&dev->p->deferred_probe)) {
>>   		dev_dbg(dev, "Added to deferred list\n");
>>   		list_add_tail(&dev->p->deferred_probe, &deferred_probe_pending_list);
>> @@ -202,6 +218,15 @@ void device_unblock_probing(void)
>>   	driver_deferred_probe_trigger();
>>   }
>>   
>> +/*
>> + * Wrapper function for delayed trigger work requests
>> + */
>> +void driver_deferred_probe_trigger_wrapper(struct work_struct *work)
>> +{
>> +	atomic_inc(&deferred_self_trigger_count);
>> +	driver_deferred_probe_trigger();
>> +}
>> +
>>   /**
>>    * deferred_probe_initcall() - Enable probing of deferred devices
>>    *
>>

Re: [PATCH] device probe: add self triggered delayed work request

[Frank Rowand]

On 08/08/16 14:51, Qing Huang wrote:
> 
> 
> On 08/08/2016 01:44 PM, Frank Rowand wrote:
>> On 07/29/16 22:39, Qing Huang wrote:
>>> In normal condition, the device probe requests kept in deferred
>>> queue would only be triggered for re-probing when another new device
>>> probe is finished successfully. This change will set up a delayed
>>> trigger work request if the current deferred probe being added is
>>> the only one in the queue. This delayed work request will try to
>>> reactivate any device from the deferred queue for re-probing later.
>>>
>>> By doing this, if the last device being probed in system boot process
>>> has a deferred probe error, this particular device will still be able
>>> to be probed again.
>> I am trying to understand the use case.
>>
>> Can you explain the scenario you are trying to fix?  If I understand
>> correctly, you expect that something will change such that a later
>> probe attempt will succeed.  How will that change occur and why
>> will the deferred probe list not be processed in this case?
>>
>> Why are you conditioning this on the deferred_probe_pending_list
>> being empty?
>>
>> -Frank
> 
> It turns out one corner case which we worried about has already been
> solved in the really_probe() function by comparing
> 'deferred_trigger_count' values.
> 
> Another use case we are investigating now: when we probe a device,
> the main thread returns EPROBE_DEFER from the driver after we spawn a
> child thread to do the actual init work. So we can initialize
> multiple similar devices at the same time. After the child thread
> finishes its task, we can call driver_deferred_probe_trigger()
> directly from child thread to re-probe the
> device(driver_deferred_probe_trigger() has to be exported though). Or
> we could rely on something in this patch to re-probe the deferred
> devices from the pending list...
> What do you suggest?

See commit 735a7ffb739b6efeaeb1e720306ba308eaaeb20e for how multi-threaded
probes were intended to be handled.  I don't know if this approach is used
much or even usable, but that is the framework that was created.


> Thanks,
> -Qing

< snip >

Re: [PATCH] device probe: add self triggered delayed work request

[Santosh Shilimkar]

On 8/8/2016 6:11 PM, Frank Rowand wrote:
> On 08/08/16 14:51, Qing Huang wrote:
>>
>>
>> On 08/08/2016 01:44 PM, Frank Rowand wrote:
>>> On 07/29/16 22:39, Qing Huang wrote:
>>>> In normal condition, the device probe requests kept in deferred
>>>> queue would only be triggered for re-probing when another new device
>>>> probe is finished successfully. This change will set up a delayed
>>>> trigger work request if the current deferred probe being added is
>>>> the only one in the queue. This delayed work request will try to
>>>> reactivate any device from the deferred queue for re-probing later.
>>>>
>>>> By doing this, if the last device being probed in system boot process
>>>> has a deferred probe error, this particular device will still be able
>>>> to be probed again.
>>> I am trying to understand the use case.
>>>
>>> Can you explain the scenario you are trying to fix?  If I understand
>>> correctly, you expect that something will change such that a later
>>> probe attempt will succeed.  How will that change occur and why
>>> will the deferred probe list not be processed in this case?
>>>
>>> Why are you conditioning this on the deferred_probe_pending_list
>>> being empty?
>>>
>>> -Frank
>>
>> It turns out one corner case which we worried about has already been
>> solved in the really_probe() function by comparing
>> 'deferred_trigger_count' values.
>>
>> Another use case we are investigating now: when we probe a device,
>> the main thread returns EPROBE_DEFER from the driver after we spawn a
>> child thread to do the actual init work. So we can initialize
>> multiple similar devices at the same time. After the child thread
>> finishes its task, we can call driver_deferred_probe_trigger()
>> directly from child thread to re-probe the
>> device(driver_deferred_probe_trigger() has to be exported though). Or
>> we could rely on something in this patch to re-probe the deferred
>> devices from the pending list...
>> What do you suggest?
>
> See commit 735a7ffb739b6efeaeb1e720306ba308eaaeb20e for how multi-threaded
> probes were intended to be handled.  I don't know if this approach is used
> much or even usable, but that is the framework that was created.
>
That infrastructure got removed as part of below commit :-(

commit 5adc55da4a7758021bcc374904b0f8b076508a11
Author: Adrian Bunk <bunk@stusta.de>
Date:   Tue Mar 27 03:02:51 2007 +0200

     PCI: remove the broken PCI_MULTITHREAD_PROBE option

     This patch removes the PCI_MULTITHREAD_PROBE option that had already
     been marked as broken.

     Signed-off-by: Adrian Bunk <bunk@stusta.de>
     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

Re: [PATCH] device probe: add self triggered delayed work request

[Frank Rowand]

On 08/08/16 18:15, Santosh Shilimkar wrote:
> 
> 
> On 8/8/2016 6:11 PM, Frank Rowand wrote:
>> On 08/08/16 14:51, Qing Huang wrote:
>>>
>>>
>>> On 08/08/2016 01:44 PM, Frank Rowand wrote:
>>>> On 07/29/16 22:39, Qing Huang wrote:
>>>>> In normal condition, the device probe requests kept in deferred
>>>>> queue would only be triggered for re-probing when another new device
>>>>> probe is finished successfully. This change will set up a delayed
>>>>> trigger work request if the current deferred probe being added is
>>>>> the only one in the queue. This delayed work request will try to
>>>>> reactivate any device from the deferred queue for re-probing later.
>>>>>
>>>>> By doing this, if the last device being probed in system boot process
>>>>> has a deferred probe error, this particular device will still be able
>>>>> to be probed again.
>>>> I am trying to understand the use case.
>>>>
>>>> Can you explain the scenario you are trying to fix?  If I understand
>>>> correctly, you expect that something will change such that a later
>>>> probe attempt will succeed.  How will that change occur and why
>>>> will the deferred probe list not be processed in this case?
>>>>
>>>> Why are you conditioning this on the deferred_probe_pending_list
>>>> being empty?
>>>>
>>>> -Frank
>>>
>>> It turns out one corner case which we worried about has already been
>>> solved in the really_probe() function by comparing
>>> 'deferred_trigger_count' values.
>>>
>>> Another use case we are investigating now: when we probe a device,
>>> the main thread returns EPROBE_DEFER from the driver after we spawn a
>>> child thread to do the actual init work. So we can initialize
>>> multiple similar devices at the same time. After the child thread
>>> finishes its task, we can call driver_deferred_probe_trigger()
>>> directly from child thread to re-probe the
>>> device(driver_deferred_probe_trigger() has to be exported though). Or
>>> we could rely on something in this patch to re-probe the deferred
>>> devices from the pending list...
>>> What do you suggest?
>>
>> See commit 735a7ffb739b6efeaeb1e720306ba308eaaeb20e for how multi-threaded
>> probes were intended to be handled.  I don't know if this approach is used
>> much or even usable, but that is the framework that was created.
>>
> That infrastructure got removed as part of below commit :-(
> 
> commit 5adc55da4a7758021bcc374904b0f8b076508a11
> Author: Adrian Bunk <bunk@stusta.de>
> Date:   Tue Mar 27 03:02:51 2007 +0200
> 
>     PCI: remove the broken PCI_MULTITHREAD_PROBE option
> 
>     This patch removes the PCI_MULTITHREAD_PROBE option that had already
>     been marked as broken.
> 
>     Signed-off-by: Adrian Bunk <bunk@stusta.de>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

Hmmmm.  :-( indeed.

The *_initcall_sync defines are still there, but yep, the wait_for_probes()
part is gone.  Thanks for the info about the partial removal.

Re: [PATCH] device probe: add self triggered delayed work request

[Shamir Rabinovitch]

Hi Qing,

I suspect there is potential dead-lock with this patch:

cpu0                                                    cpu1

driver_deferred_probe_add                               deferred_probe_work_func
 ...                                                     mutex_unlock(&deferred_probe_mutex)
 mutex_lock(&deferred_probe_mutex)                        bus_probe_device(dev)
  ...                                                      device return -EPROBE_DEFER
  ...                                                       driver_deferred_probe_add
  ...                                                        mutex_lock(&deferred_probe_mutex)
  ...                                                         <deadlock!>
  cancel_delayed_work(&deferred_probe_trigger_work)
   <work will never end - deadlock!>

Please confirm if this scenario is possible.

BR, Shamir Rabinovitch

Re: [PATCH] device probe: add self triggered delayed work request

[Qing Huang]

On 08/08/2016 03:42 AM, Shamir Rabinovitch wrote:
> Hi Qing,
>
> I suspect there is potential dead-lock with this patch:
>
> cpu0                                                    cpu1
>
> driver_deferred_probe_add                               deferred_probe_work_func
>   ...                                                     mutex_unlock(&deferred_probe_mutex)
>   mutex_lock(&deferred_probe_mutex)                        bus_probe_device(dev)
>    ...                                                      device return -EPROBE_DEFER
>    ...                                                       driver_deferred_probe_add
>    ...                                                        mutex_lock(&deferred_probe_mutex)
>    ...                                                         <deadlock!>
>    cancel_delayed_work(&deferred_probe_trigger_work)
>     <work will never end - deadlock!>
Not sure if I understood your scenario. Why there is a deadlock here?

>
> Please confirm if this scenario is possible.
>
> BR, Shamir Rabinovitch

Re: [PATCH] device probe: add self triggered delayed work request

[Shamir Rabinovitch]

On Mon, Aug 08, 2016 at 05:10:05PM -0700, Qing Huang wrote:
> 
> Not sure if I understood your scenario. Why there is a deadlock here?
> 

 CPU0                                                   | CPU1
---------------------------------------------------------------------------------------------
 driver_deferred_probe_add                              | driver_deferred_probe_trigger_wrapper   
  mutex_lock(&deferred_probe_mutex)                     |  driver_deferred_probe_trigger
   cancel_delayed_work(&deferred_probe_trigger_work)    |   mutex_lock(&deferred_probe_mutex)
    wait for "driver_deferred_probe_trigger_wrapper"    |    wait for "deferred_probe_mutex"

is this possible scenario with this patch?

if yes then CPU0 will wait for CPU1 to finish the delayed work whith
mutex deferred_probe_mutex held while CPU1 will try to finish the
delayed work and will wait for the same mutex forever.

it seems like dead lock scenario to me.

please say if this scenario is possible.

BR, Shamir Rabinovitch

Re: [PATCH] device probe: add self triggered delayed work request

[Qing Huang]

On 08/09/2016 03:11 AM, Shamir Rabinovitch wrote:
> On Mon, Aug 08, 2016 at 05:10:05PM -0700, Qing Huang wrote:
>> Not sure if I understood your scenario. Why there is a deadlock here?
>>
>   CPU0                                                   | CPU1
> ---------------------------------------------------------------------------------------------
>   driver_deferred_probe_add                              | driver_deferred_probe_trigger_wrapper
>    mutex_lock(&deferred_probe_mutex)                     |  driver_deferred_probe_trigger
>     cancel_delayed_work(&deferred_probe_trigger_work)    |   mutex_lock(&deferred_probe_mutex)
>      wait for "driver_deferred_probe_trigger_wrapper"    |    wait for "deferred_probe_mutex"
>
> is this possible scenario with this patch?
>
> if yes then CPU0 will wait for CPU1 to finish the delayed work whith
> mutex deferred_probe_mutex held while CPU1 will try to finish the
> delayed work and will wait for the same mutex forever.

CPU0 will not wait for "driver_deferred_probe_trigger_wrapper" to 
finish, it simply puts the work request onto the queue and returns.

Qing

>
> it seems like dead lock scenario to me.
>
> please say if this scenario is possible.
>
> BR, Shamir Rabinovitch