Re: [PATCH] firewire: sbp2: parallelize login/inquiry, reconnect, and shutdown

[Tejun Heo <tj@kernel.org>]

Hello,

On 10/10/2010 04:57 PM, Stefan Richter wrote:
...
> Simply swap out the driver workqueue by system_nrt_wq.  It provides all
> the parallelism we will ever need, accepts long-running work, and
> provides the here necessary guarantee of non-reentrance across all CPUs.
               ^^^^
               this probably should go away?

> (The work items are scheduled from firewire-core's fw_device work items
> which themselves may change CPUs.)
> 
> This simple approach requires though that targets with multiple logical
> units accept multiple outstanding management requests.  The SBP-2 spec
> implicitly requires this capability, but who knows how real firmwares
> react.
> 
> I only have access to two dual-LU targets:  A OXUF924DSB based one from
> MacPower and a INIC-2430 based one from IOI.  They both work fine with
> this change, as do several single-LU targets of course.
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
>  drivers/firewire/sbp2.c |    9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> Index: b/drivers/firewire/sbp2.c
> ===================================================================
> --- a/drivers/firewire/sbp2.c
> +++ b/drivers/firewire/sbp2.c
> @@ -835,8 +835,6 @@ static void sbp2_target_put(struct sbp2_
>  	kref_put(&tgt->kref, sbp2_release_target);
>  }
>  
> -static struct workqueue_struct *sbp2_wq;
> -
>  /*
>   * Always get the target's kref when scheduling work on one its units.
>   * Each workqueue job is responsible to call sbp2_target_put() upon return.
> @@ -844,7 +842,7 @@ static struct workqueue_struct *sbp2_wq;
>  static void sbp2_queue_work(struct sbp2_logical_unit *lu, unsigned long delay)
>  {
>  	sbp2_target_get(lu->tgt);
> -	if (!queue_delayed_work(sbp2_wq, &lu->work, delay))
> +	if (!queue_delayed_work(system_nrt_wq, &lu->work, delay))
>  		sbp2_target_put(lu->tgt);
>  }
>  
> @@ -1656,17 +1654,12 @@ MODULE_ALIAS("sbp2");
>  
>  static int __init sbp2_init(void)
>  {
> -	sbp2_wq = create_singlethread_workqueue(KBUILD_MODNAME);
> -	if (!sbp2_wq)
> -		return -ENOMEM;
> -
>  	return driver_register(&sbp2_driver.driver);
>  }
>  
>  static void __exit sbp2_cleanup(void)
>  {
>  	driver_unregister(&sbp2_driver.driver);
> -	destroy_workqueue(sbp2_wq);

Hmmm... from glancing the code, there doesn't seem to anything which
can guarantee sbp2_release_target/reconnect() are finished before
sbp2_cleanup() returns, so the code section might go away with code
still running.  It seems like the right thing to do here would be
using alloc_workqueue(KBUILD_MODNAME, WQ_NON_REENTRANT, 0).  Am I
missing something?

Thanks.

-- 
tejun