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

Groups > linux.kernel > #1452656 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] device probe: add self triggered delayed work request Qing Huang <qing.huang@oracle.com> - 2016-07-30 07:40 +0200
    Re: [PATCH] device probe: add self triggered delayed work request Frank Rowand <frowand.list@gmail.com> - 2016-08-08 22:50 +0200
      Re: [PATCH] device probe: add self triggered delayed work request Qing Huang <qing.huang@oracle.com> - 2016-08-08 23:50 +0200
        Re: [PATCH] device probe: add self triggered delayed work request Frank Rowand <frowand.list@gmail.com> - 2016-08-09 03:20 +0200
          Re: [PATCH] device probe: add self triggered delayed work request Santosh Shilimkar <santosh.shilimkar@oracle.com> - 2016-08-09 03:20 +0200
            Re: [PATCH] device probe: add self triggered delayed work request Frank Rowand <frowand.list@gmail.com> - 2016-08-09 09:20 +0200

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

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

[toc] | [next] | [standalone]

#1458237

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

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

#1458266

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

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

#1458355

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 >

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

#1458356

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>

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

#1458471

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web