[PATCHSET] scsi: remove use of flush_scheduled_work()

[PATCHSET] scsi: remove use of flush_scheduled_work()

[Tejun Heo]

Hello,

This series updates scsi so that flush_scheduled_work(), which is
scheduled for deprecation, isn't used anymore.  It includes four
patches.

 0001-scsi-remove-flush_scheduled_work-usages.patch
 0002-scsi-pm8001-simplify-workqueue-usage.patch
 0003-fcoe-use-dedicated-workqueue-instead-of-system_wq.patch
 0004-fusion-don-t-use-flush_scheduled_work.patch

0001 converts all the easy ones.  0002-0004 do non-trivial conversions
one by one.

The patches are on top of 2.6.38-rc2.  Diffstat follows.

 drivers/message/fusion/mptscsih.c         |    1 
 drivers/message/fusion/mptspi.c           |   33 ++++++++++++++++++++++----
 drivers/scsi/NCR5380.c                    |    3 --
 drivers/scsi/arcmsr/arcmsr_hba.c          |    4 +--
 drivers/scsi/fcoe/fcoe.c                  |   25 ++++++++++++--------
 drivers/scsi/ipr.c                        |    2 -
 drivers/scsi/megaraid/megaraid_sas_base.c |    6 +---
 drivers/scsi/mpt2sas/mpt2sas_scsih.c      |    1 
 drivers/scsi/pm8001/pm8001_hwi.c          |   37 +++++++++++++-----------------
 drivers/scsi/pm8001/pm8001_init.c         |   27 ++++++++++++++-------
 drivers/scsi/pm8001/pm8001_sas.h          |   10 ++++----
 drivers/scsi/pmcraid.c                    |    2 -
 12 files changed, 92 insertions(+), 59 deletions(-)

Thanks.

--
tejun

[PATCH 1/4] scsi: remove flush_scheduled_work() usages

[Tejun Heo]

Simple conversions to drop flush_scheduled_work() usages in
drivers/scsi.  More involved ones will be done in separate patches.

* NCR5380, megaraid_sas: cancel_delayed_work() +
  flush_scheduled_work() -> cancel_delayed_work_sync().

* mpt2sas_scsih: drop unnecessary flush_scheduled_work().

* arcmsr_hba, ipr, pmcraid: flush the used work explicitly instead of
  using flush_scheduled_work().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/scsi/NCR5380.c                    |    3 +--
 drivers/scsi/arcmsr/arcmsr_hba.c          |    4 ++--
 drivers/scsi/ipr.c                        |    2 +-
 drivers/scsi/megaraid/megaraid_sas_base.c |    6 ++----
 drivers/scsi/mpt2sas/mpt2sas_scsih.c      |    1 -
 drivers/scsi/pmcraid.c                    |    2 +-
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 9a5629f..e7cd2fc 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -936,8 +936,7 @@ static void NCR5380_exit(struct Scsi_Host *instance)
 {
 	struct NCR5380_hostdata *hostdata = (struct NCR5380_hostdata *) instance->hostdata;
 
-	cancel_delayed_work(&hostdata->coroutine);
-	flush_scheduled_work();
+	cancel_delayed_work_sync(&hostdata->coroutine);
 }
 
 /**
diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 1cadcd6..d6d17a1 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -1022,7 +1022,7 @@ static void arcmsr_remove(struct pci_dev *pdev)
 	int poll_count = 0;
 	arcmsr_free_sysfs_attr(acb);
 	scsi_remove_host(host);
-	flush_scheduled_work();
+	flush_work_sync(&acb->arcmsr_do_message_isr_bh);
 	del_timer_sync(&acb->eternal_timer);
 	arcmsr_disable_outbound_ints(acb);
 	arcmsr_stop_adapter_bgrb(acb);
@@ -1068,7 +1068,7 @@ static void arcmsr_shutdown(struct pci_dev *pdev)
 		(struct AdapterControlBlock *)host->hostdata;
 	del_timer_sync(&acb->eternal_timer);
 	arcmsr_disable_outbound_ints(acb);
-	flush_scheduled_work();
+	flush_work_sync(&acb->arcmsr_do_message_isr_bh);
 	arcmsr_stop_adapter_bgrb(acb);
 	arcmsr_flush_adapter_cache(acb);
 }
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 9c5c8be..a8e24f0 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -8865,7 +8865,7 @@ static void __ipr_remove(struct pci_dev *pdev)
 
 	spin_unlock_irqrestore(ioa_cfg->host->host_lock, host_lock_flags);
 	wait_event(ioa_cfg->reset_wait_q, !ioa_cfg->in_reset_reload);
-	flush_scheduled_work();
+	flush_work_sync(&ioa_cfg->work_q);
 	spin_lock_irqsave(ioa_cfg->host->host_lock, host_lock_flags);
 
 	spin_lock(&ipr_driver_lock);
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 5d6d07b..e7c9b41 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -4242,9 +4242,8 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state)
 	/* cancel the delayed work if this work still in queue */
 	if (instance->ev != NULL) {
 		struct megasas_aen_event *ev = instance->ev;
-		cancel_delayed_work(
+		cancel_delayed_work_sync(
 			(struct delayed_work *)&ev->hotplug_work);
-		flush_scheduled_work();
 		instance->ev = NULL;
 	}
 
@@ -4417,9 +4416,8 @@ static void __devexit megasas_detach_one(struct pci_dev *pdev)
 	/* cancel the delayed work if this work still in queue*/
 	if (instance->ev != NULL) {
 		struct megasas_aen_event *ev = instance->ev;
-		cancel_delayed_work(
+		cancel_delayed_work_sync(
 			(struct delayed_work *)&ev->hotplug_work);
-		flush_scheduled_work();
 		instance->ev = NULL;
 	}
 
diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index eda347c..b375150 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -6935,7 +6935,6 @@ _scsih_suspend(struct pci_dev *pdev, pm_message_t state)
 	u32 device_state;
 
 	mpt2sas_base_stop_watchdog(ioc);
-	flush_scheduled_work();
 	scsi_block_requests(shost);
 	device_state = pci_choose_state(pdev, state);
 	printk(MPT2SAS_INFO_FMT "pdev=0x%p, slot=%s, entering "
diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index 321cf3a..bcf858e 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -5454,7 +5454,7 @@ static void __devexit pmcraid_remove(struct pci_dev *pdev)
 	pmcraid_shutdown(pdev);
 
 	pmcraid_disable_interrupts(pinstance, ~0);
-	flush_scheduled_work();
+	flush_work_sync(&pinstance->worker_q);
 
 	pmcraid_kill_tasklets(pinstance);
 	pmcraid_unregister_interrupt_handler(pinstance);
-- 
1.7.1

[PATCH 2/4] scsi: pm8001: simplify workqueue usage

[Tejun Heo]

pm8001 manages its own list of pending works and cancel them on device
free.  It is unnecessarily complex and has a race condition - the
works are canceled but not synced, so the work could still be running
during and after the data structures are freed.

This patch simplifies workqueue usage.

* A driver specific workqueue pm8001_wq is created to serve these
  work items.

* To avoid confusion, the "queue" suffixes are dropped from work items
  and functions.

* Delayed queueing was never used.  pm8001_work now uses work_struct
  instead.

* The driver no longer keeps track of pending works.  All pm8001_works
  are queued to pm8001_wq and the workqueue is flushed as necessary.

flush_scheduled_work() usage is removed during conversion.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/scsi/pm8001/pm8001_hwi.c  |   37 +++++++++++++++++--------------------
 drivers/scsi/pm8001/pm8001_init.c |   27 ++++++++++++++++++---------
 drivers/scsi/pm8001/pm8001_sas.h  |   10 ++++++----
 3 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index d8db013..18b6c55 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -1382,53 +1382,50 @@ static u32 mpi_msg_consume(struct pm8001_hba_info *pm8001_ha,
 	return MPI_IO_STATUS_BUSY;
 }
 
-static void pm8001_work_queue(struct work_struct *work)
+static void pm8001_work_fn(struct work_struct *work)
 {
-	struct delayed_work *dw = container_of(work, struct delayed_work, work);
-	struct pm8001_wq *wq = container_of(dw, struct pm8001_wq, work_q);
+	struct pm8001_work *pw = container_of(work, struct pm8001_work, work);
 	struct pm8001_device *pm8001_dev;
-	struct domain_device	*dev;
+	struct domain_device *dev;
 
-	switch (wq->handler) {
+	switch (pw->handler) {
 	case IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS:
-		pm8001_dev = wq->data;
+		pm8001_dev = pw->data;
 		dev = pm8001_dev->sas_device;
 		pm8001_I_T_nexus_reset(dev);
 		break;
 	case IO_OPEN_CNX_ERROR_STP_RESOURCES_BUSY:
-		pm8001_dev = wq->data;
+		pm8001_dev = pw->data;
 		dev = pm8001_dev->sas_device;
 		pm8001_I_T_nexus_reset(dev);
 		break;
 	case IO_DS_IN_ERROR:
-		pm8001_dev = wq->data;
+		pm8001_dev = pw->data;
 		dev = pm8001_dev->sas_device;
 		pm8001_I_T_nexus_reset(dev);
 		break;
 	case IO_DS_NON_OPERATIONAL:
-		pm8001_dev = wq->data;
+		pm8001_dev = pw->data;
 		dev = pm8001_dev->sas_device;
 		pm8001_I_T_nexus_reset(dev);
 		break;
 	}
-	list_del(&wq->entry);
-	kfree(wq);
+	kfree(pw);
 }
 
 static int pm8001_handle_event(struct pm8001_hba_info *pm8001_ha, void *data,
 			       int handler)
 {
-	struct pm8001_wq *wq;
+	struct pm8001_work *pw;
 	int ret = 0;
 
-	wq = kmalloc(sizeof(struct pm8001_wq), GFP_ATOMIC);
-	if (wq) {
-		wq->pm8001_ha = pm8001_ha;
-		wq->data = data;
-		wq->handler = handler;
-		INIT_DELAYED_WORK(&wq->work_q, pm8001_work_queue);
-		list_add_tail(&wq->entry, &pm8001_ha->wq_list);
-		schedule_delayed_work(&wq->work_q, 0);
+	pw = kmalloc(sizeof(struct pm8001_work), GFP_ATOMIC);
+	if (pw) {
+		pw->pm8001_ha = pm8001_ha;
+		pw->data = data;
+		pw->handler = handler;
+		INIT_WORK(&pw->work, pm8001_work_fn);
+		queue_work(pm8001_wq, &pw->work);
 	} else
 		ret = -ENOMEM;
 
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index b95285f..002360d 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -51,6 +51,8 @@ static int pm8001_id;
 
 LIST_HEAD(hba_list);
 
+struct workqueue_struct *pm8001_wq;
+
 /**
  * The main structure which LLDD must register for scsi core.
  */
@@ -134,7 +136,6 @@ static void __devinit pm8001_phy_init(struct pm8001_hba_info *pm8001_ha,
 static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
 {
 	int i;
-	struct pm8001_wq *wq;
 
 	if (!pm8001_ha)
 		return;
@@ -150,8 +151,7 @@ static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
 	PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
 	if (pm8001_ha->shost)
 		scsi_host_put(pm8001_ha->shost);
-	list_for_each_entry(wq, &pm8001_ha->wq_list, entry)
-		cancel_delayed_work(&wq->work_q);
+	flush_workqueue(pm8001_wq);
 	kfree(pm8001_ha->tags);
 	kfree(pm8001_ha);
 }
@@ -381,7 +381,6 @@ pm8001_pci_alloc(struct pci_dev *pdev, u32 chip_id, struct Scsi_Host *shost)
 	pm8001_ha->sas = sha;
 	pm8001_ha->shost = shost;
 	pm8001_ha->id = pm8001_id++;
-	INIT_LIST_HEAD(&pm8001_ha->wq_list);
 	pm8001_ha->logging_level = 0x01;
 	sprintf(pm8001_ha->name, "%s%d", DRV_NAME, pm8001_ha->id);
 #ifdef PM8001_USE_TASKLET
@@ -758,7 +757,7 @@ static int pm8001_pci_suspend(struct pci_dev *pdev, pm_message_t state)
 	int i , pos;
 	u32 device_state;
 	pm8001_ha = sha->lldd_ha;
-	flush_scheduled_work();
+	flush_workqueue(pm8001_wq);
 	scsi_block_requests(pm8001_ha->shost);
 	pos = pci_find_capability(pdev, PCI_CAP_ID_PM);
 	if (pos == 0) {
@@ -870,17 +869,26 @@ static struct pci_driver pm8001_pci_driver = {
  */
 static int __init pm8001_init(void)
 {
-	int rc;
+	int rc = -ENOMEM;
+
+	pm8001_wq = alloc_workqueue("pm8001", 0, 0);
+	if (!pm8001_wq)
+		goto err;
+
 	pm8001_id = 0;
 	pm8001_stt = sas_domain_attach_transport(&pm8001_transport_ops);
 	if (!pm8001_stt)
-		return -ENOMEM;
+		goto err_wq;
 	rc = pci_register_driver(&pm8001_pci_driver);
 	if (rc)
-		goto err_out;
+		goto err_tp;
 	return 0;
-err_out:
+
+err_tp:
 	sas_release_transport(pm8001_stt);
+err_wq:
+	destroy_workqueue(pm8001_wq);
+err:
 	return rc;
 }
 
@@ -888,6 +896,7 @@ static void __exit pm8001_exit(void)
 {
 	pci_unregister_driver(&pm8001_pci_driver);
 	sas_release_transport(pm8001_stt);
+	destroy_workqueue(pm8001_wq);
 }
 
 module_init(pm8001_init);
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 7f064f9..bdb6b27 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -50,6 +50,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/pci.h>
 #include <linux/interrupt.h>
+#include <linux/workqueue.h>
 #include <scsi/libsas.h>
 #include <scsi/scsi_tcq.h>
 #include <scsi/sas_ata.h>
@@ -379,18 +380,16 @@ struct pm8001_hba_info {
 #ifdef PM8001_USE_TASKLET
 	struct tasklet_struct	tasklet;
 #endif
-	struct list_head 	wq_list;
 	u32			logging_level;
 	u32			fw_status;
 	const struct firmware 	*fw_image;
 };
 
-struct pm8001_wq {
-	struct delayed_work work_q;
+struct pm8001_work {
+	struct work_struct work;
 	struct pm8001_hba_info *pm8001_ha;
 	void *data;
 	int handler;
-	struct list_head entry;
 };
 
 struct pm8001_fw_image_header {
@@ -460,6 +459,9 @@ struct fw_control_ex {
 	void			*param3;
 };
 
+/* pm8001 workqueue */
+extern struct workqueue_struct *pm8001_wq;
+
 /******************** function prototype *********************/
 int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out);
 void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha);
-- 
1.7.1

Re: [PATCH 2/4] scsi: pm8001: simplify workqueue usage

[James Bottomley]

On Mon, 2011-01-24 at 14:57 +0100, Tejun Heo wrote:
> pm8001 manages its own list of pending works and cancel them on device
> free.  It is unnecessarily complex and has a race condition - the
> works are canceled but not synced, so the work could still be running
> during and after the data structures are freed.
> 
> This patch simplifies workqueue usage.
> 
> * A driver specific workqueue pm8001_wq is created to serve these
>   work items.
> 
> * To avoid confusion, the "queue" suffixes are dropped from work items
>   and functions.
> 
> * Delayed queueing was never used.  pm8001_work now uses work_struct
>   instead.
> 
> * The driver no longer keeps track of pending works.  All pm8001_works
>   are queued to pm8001_wq and the workqueue is flushed as necessary.
> 
> flush_scheduled_work() usage is removed during conversion.

cc'ing pm8001 maintainers for an opinion.  Jack could you look at this
and ack, please.

Thanks,

James


> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c  |   37 +++++++++++++++++--------------------
>  drivers/scsi/pm8001/pm8001_init.c |   27 ++++++++++++++++++---------
>  drivers/scsi/pm8001/pm8001_sas.h  |   10 ++++++----
>  3 files changed, 41 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
> index d8db013..18b6c55 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -1382,53 +1382,50 @@ static u32 mpi_msg_consume(struct pm8001_hba_info *pm8001_ha,
>  	return MPI_IO_STATUS_BUSY;
>  }
>  
> -static void pm8001_work_queue(struct work_struct *work)
> +static void pm8001_work_fn(struct work_struct *work)
>  {
> -	struct delayed_work *dw = container_of(work, struct delayed_work, work);
> -	struct pm8001_wq *wq = container_of(dw, struct pm8001_wq, work_q);
> +	struct pm8001_work *pw = container_of(work, struct pm8001_work, work);
>  	struct pm8001_device *pm8001_dev;
> -	struct domain_device	*dev;
> +	struct domain_device *dev;
>  
> -	switch (wq->handler) {
> +	switch (pw->handler) {
>  	case IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS:
> -		pm8001_dev = wq->data;
> +		pm8001_dev = pw->data;
>  		dev = pm8001_dev->sas_device;
>  		pm8001_I_T_nexus_reset(dev);
>  		break;
>  	case IO_OPEN_CNX_ERROR_STP_RESOURCES_BUSY:
> -		pm8001_dev = wq->data;
> +		pm8001_dev = pw->data;
>  		dev = pm8001_dev->sas_device;
>  		pm8001_I_T_nexus_reset(dev);
>  		break;
>  	case IO_DS_IN_ERROR:
> -		pm8001_dev = wq->data;
> +		pm8001_dev = pw->data;
>  		dev = pm8001_dev->sas_device;
>  		pm8001_I_T_nexus_reset(dev);
>  		break;
>  	case IO_DS_NON_OPERATIONAL:
> -		pm8001_dev = wq->data;
> +		pm8001_dev = pw->data;
>  		dev = pm8001_dev->sas_device;
>  		pm8001_I_T_nexus_reset(dev);
>  		break;
>  	}
> -	list_del(&wq->entry);
> -	kfree(wq);
> +	kfree(pw);
>  }
>  
>  static int pm8001_handle_event(struct pm8001_hba_info *pm8001_ha, void *data,
>  			       int handler)
>  {
> -	struct pm8001_wq *wq;
> +	struct pm8001_work *pw;
>  	int ret = 0;
>  
> -	wq = kmalloc(sizeof(struct pm8001_wq), GFP_ATOMIC);
> -	if (wq) {
> -		wq->pm8001_ha = pm8001_ha;
> -		wq->data = data;
> -		wq->handler = handler;
> -		INIT_DELAYED_WORK(&wq->work_q, pm8001_work_queue);
> -		list_add_tail(&wq->entry, &pm8001_ha->wq_list);
> -		schedule_delayed_work(&wq->work_q, 0);
> +	pw = kmalloc(sizeof(struct pm8001_work), GFP_ATOMIC);
> +	if (pw) {
> +		pw->pm8001_ha = pm8001_ha;
> +		pw->data = data;
> +		pw->handler = handler;
> +		INIT_WORK(&pw->work, pm8001_work_fn);
> +		queue_work(pm8001_wq, &pw->work);
>  	} else
>  		ret = -ENOMEM;
>  
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index b95285f..002360d 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -51,6 +51,8 @@ static int pm8001_id;
>  
>  LIST_HEAD(hba_list);
>  
> +struct workqueue_struct *pm8001_wq;
> +
>  /**
>   * The main structure which LLDD must register for scsi core.
>   */
> @@ -134,7 +136,6 @@ static void __devinit pm8001_phy_init(struct pm8001_hba_info *pm8001_ha,
>  static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
>  {
>  	int i;
> -	struct pm8001_wq *wq;
>  
>  	if (!pm8001_ha)
>  		return;
> @@ -150,8 +151,7 @@ static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
>  	PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
>  	if (pm8001_ha->shost)
>  		scsi_host_put(pm8001_ha->shost);
> -	list_for_each_entry(wq, &pm8001_ha->wq_list, entry)
> -		cancel_delayed_work(&wq->work_q);
> +	flush_workqueue(pm8001_wq);
>  	kfree(pm8001_ha->tags);
>  	kfree(pm8001_ha);
>  }
> @@ -381,7 +381,6 @@ pm8001_pci_alloc(struct pci_dev *pdev, u32 chip_id, struct Scsi_Host *shost)
>  	pm8001_ha->sas = sha;
>  	pm8001_ha->shost = shost;
>  	pm8001_ha->id = pm8001_id++;
> -	INIT_LIST_HEAD(&pm8001_ha->wq_list);
>  	pm8001_ha->logging_level = 0x01;
>  	sprintf(pm8001_ha->name, "%s%d", DRV_NAME, pm8001_ha->id);
>  #ifdef PM8001_USE_TASKLET
> @@ -758,7 +757,7 @@ static int pm8001_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>  	int i , pos;
>  	u32 device_state;
>  	pm8001_ha = sha->lldd_ha;
> -	flush_scheduled_work();
> +	flush_workqueue(pm8001_wq);
>  	scsi_block_requests(pm8001_ha->shost);
>  	pos = pci_find_capability(pdev, PCI_CAP_ID_PM);
>  	if (pos == 0) {
> @@ -870,17 +869,26 @@ static struct pci_driver pm8001_pci_driver = {
>   */
>  static int __init pm8001_init(void)
>  {
> -	int rc;
> +	int rc = -ENOMEM;
> +
> +	pm8001_wq = alloc_workqueue("pm8001", 0, 0);
> +	if (!pm8001_wq)
> +		goto err;
> +
>  	pm8001_id = 0;
>  	pm8001_stt = sas_domain_attach_transport(&pm8001_transport_ops);
>  	if (!pm8001_stt)
> -		return -ENOMEM;
> +		goto err_wq;
>  	rc = pci_register_driver(&pm8001_pci_driver);
>  	if (rc)
> -		goto err_out;
> +		goto err_tp;
>  	return 0;
> -err_out:
> +
> +err_tp:
>  	sas_release_transport(pm8001_stt);
> +err_wq:
> +	destroy_workqueue(pm8001_wq);
> +err:
>  	return rc;
>  }
>  
> @@ -888,6 +896,7 @@ static void __exit pm8001_exit(void)
>  {
>  	pci_unregister_driver(&pm8001_pci_driver);
>  	sas_release_transport(pm8001_stt);
> +	destroy_workqueue(pm8001_wq);
>  }
>  
>  module_init(pm8001_init);
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 7f064f9..bdb6b27 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -50,6 +50,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/pci.h>
>  #include <linux/interrupt.h>
> +#include <linux/workqueue.h>
>  #include <scsi/libsas.h>
>  #include <scsi/scsi_tcq.h>
>  #include <scsi/sas_ata.h>
> @@ -379,18 +380,16 @@ struct pm8001_hba_info {
>  #ifdef PM8001_USE_TASKLET
>  	struct tasklet_struct	tasklet;
>  #endif
> -	struct list_head 	wq_list;
>  	u32			logging_level;
>  	u32			fw_status;
>  	const struct firmware 	*fw_image;
>  };
>  
> -struct pm8001_wq {
> -	struct delayed_work work_q;
> +struct pm8001_work {
> +	struct work_struct work;
>  	struct pm8001_hba_info *pm8001_ha;
>  	void *data;
>  	int handler;
> -	struct list_head entry;
>  };
>  
>  struct pm8001_fw_image_header {
> @@ -460,6 +459,9 @@ struct fw_control_ex {
>  	void			*param3;
>  };
>  
> +/* pm8001 workqueue */
> +extern struct workqueue_struct *pm8001_wq;
> +
>  /******************** function prototype *********************/
>  int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out);
>  void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha);

RE: [PATCH 2/4] scsi: pm8001: simplify workqueue usage

[Jack Wang]

 > 
> On Mon, 2011-01-24 at 14:57 +0100, Tejun Heo wrote:
> > pm8001 manages its own list of pending works and cancel them on device
> > free.  It is unnecessarily complex and has a race condition - the
> > works are canceled but not synced, so the work could still be running
> > during and after the data structures are freed.
> >
> > This patch simplifies workqueue usage.
> >
> > * A driver specific workqueue pm8001_wq is created to serve these
> >   work items.
> >
> > * To avoid confusion, the "queue" suffixes are dropped from work items
> >   and functions.
> >
> > * Delayed queueing was never used.  pm8001_work now uses work_struct
> >   instead.
> >
> > * The driver no longer keeps track of pending works.  All pm8001_works
> >   are queued to pm8001_wq and the workqueue is flushed as necessary.
> >
> > flush_scheduled_work() usage is removed during conversion.
> 
> cc'ing pm8001 maintainers for an opinion.  Jack could you look at this
> and ack, please.
> 
> Thanks,
> 
> James
> 
[Jack Wang] Looks Okay to me. Ack-by: Jack Wang <jack_wang@usish.com>
	 Thanks

[PATCH 3/4] fcoe: use dedicated workqueue instead of system_wq

[Tejun Heo]

fcoe uses the system_wq to destroy ports and the work items need to be
flushed before the driver is unloaded.  As the work items free the
containing data structure, they can't be flushed directly.  The
workqueue should be flushed instead.

Also, the destruction works can be chained - ie. destruction of a port
may lead to destruction of another port where the work item for the
former queues the work for the latter.  Currently, the depth of chain
can be at most two and fcoe_exit() makes sure everything is complete
by calling flush_scheduled_work() twice.

With commit c8efcc25 (workqueue: allow chained queueing during
destruction), destroy_workqueue() can take care of chained works on
workqueue destruction.  Add and use fcoe_wq instead.  Simply
destroying fcoe_wq on driver unload takes care of flushing.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/scsi/fcoe/fcoe.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 9f9600b..e2f5820 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -31,6 +31,7 @@
 #include <linux/fs.h>
 #include <linux/sysfs.h>
 #include <linux/ctype.h>
+#include <linux/workqueue.h>
 #include <scsi/scsi_tcq.h>
 #include <scsi/scsicam.h>
 #include <scsi/scsi_transport.h>
@@ -58,6 +59,8 @@ MODULE_PARM_DESC(ddp_min, "Minimum I/O size in bytes for "	\
 
 DEFINE_MUTEX(fcoe_config_mutex);
 
+static struct workqueue_struct *fcoe_wq;
+
 /* fcoe_percpu_clean completion.  Waiter protected by fcoe_create_mutex */
 static DECLARE_COMPLETION(fcoe_flush_completion);
 
@@ -1874,7 +1877,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 		list_del(&fcoe->list);
 		port = lport_priv(fcoe->ctlr.lp);
 		fcoe_interface_cleanup(fcoe);
-		schedule_work(&port->destroy_work);
+		queue_work(fcoe_wq, &port->destroy_work);
 		goto out;
 		break;
 	case NETDEV_FEAT_CHANGE:
@@ -2412,6 +2415,10 @@ static int __init fcoe_init(void)
 	unsigned int cpu;
 	int rc = 0;
 
+	fcoe_wq = alloc_workqueue("fcoe", 0, 0);
+	if (!fcoe_wq)
+		return -ENOMEM;
+
 	mutex_lock(&fcoe_config_mutex);
 
 	for_each_possible_cpu(cpu) {
@@ -2442,6 +2449,7 @@ out_free:
 		fcoe_percpu_thread_destroy(cpu);
 	}
 	mutex_unlock(&fcoe_config_mutex);
+	destroy_workqueue(fcoe_wq);
 	return rc;
 }
 module_init(fcoe_init);
@@ -2467,7 +2475,7 @@ static void __exit fcoe_exit(void)
 		list_del(&fcoe->list);
 		port = lport_priv(fcoe->ctlr.lp);
 		fcoe_interface_cleanup(fcoe);
-		schedule_work(&port->destroy_work);
+		queue_work(fcoe_wq, &port->destroy_work);
 	}
 	rtnl_unlock();
 
@@ -2478,12 +2486,11 @@ static void __exit fcoe_exit(void)
 
 	mutex_unlock(&fcoe_config_mutex);
 
-	/* flush any asyncronous interface destroys,
-	 * this should happen after the netdev notifier is unregistered */
-	flush_scheduled_work();
-	/* That will flush out all the N_Ports on the hostlist, but now we
-	 * may have NPIV VN_Ports scheduled for destruction */
-	flush_scheduled_work();
+	/*
+	 * destroy_work's may be chained but destroy_workqueue() can take
+	 * care of them.  Just kill the wq.
+	 */
+	destroy_workqueue(fcoe_wq);
 
 	/* detach from scsi transport
 	 * must happen after all destroys are done, therefor after the flush */
@@ -2632,7 +2639,7 @@ static int fcoe_vport_destroy(struct fc_vport *vport)
 	mutex_lock(&n_port->lp_mutex);
 	list_del(&vn_port->list);
 	mutex_unlock(&n_port->lp_mutex);
-	schedule_work(&port->destroy_work);
+	queue_work(fcoe_wq, &port->destroy_work);
 	return 0;
 }
 
-- 
1.7.1

Re: [PATCH 3/4] fcoe: use dedicated workqueue instead of system_wq

[James Bottomley]

On Mon, 2011-01-24 at 14:57 +0100, Tejun Heo wrote:
> fcoe uses the system_wq to destroy ports and the work items need to be
> flushed before the driver is unloaded.  As the work items free the
> containing data structure, they can't be flushed directly.  The
> workqueue should be flushed instead.
> 
> Also, the destruction works can be chained - ie. destruction of a port
> may lead to destruction of another port where the work item for the
> former queues the work for the latter.  Currently, the depth of chain
> can be at most two and fcoe_exit() makes sure everything is complete
> by calling flush_scheduled_work() twice.
> 
> With commit c8efcc25 (workqueue: allow chained queueing during
> destruction), destroy_workqueue() can take care of chained works on
> workqueue destruction.  Add and use fcoe_wq instead.  Simply
> destroying fcoe_wq on driver unload takes care of flushing.

Cc'ing FCoE maintainer for an opinion.

Robert, I assume this will come back via your queue with an ack.

James


> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/scsi/fcoe/fcoe.c |   25 ++++++++++++++++---------
>  1 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index 9f9600b..e2f5820 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -31,6 +31,7 @@
>  #include <linux/fs.h>
>  #include <linux/sysfs.h>
>  #include <linux/ctype.h>
> +#include <linux/workqueue.h>
>  #include <scsi/scsi_tcq.h>
>  #include <scsi/scsicam.h>
>  #include <scsi/scsi_transport.h>
> @@ -58,6 +59,8 @@ MODULE_PARM_DESC(ddp_min, "Minimum I/O size in bytes for "	\
>  
>  DEFINE_MUTEX(fcoe_config_mutex);
>  
> +static struct workqueue_struct *fcoe_wq;
> +
>  /* fcoe_percpu_clean completion.  Waiter protected by fcoe_create_mutex */
>  static DECLARE_COMPLETION(fcoe_flush_completion);
>  
> @@ -1874,7 +1877,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
>  		list_del(&fcoe->list);
>  		port = lport_priv(fcoe->ctlr.lp);
>  		fcoe_interface_cleanup(fcoe);
> -		schedule_work(&port->destroy_work);
> +		queue_work(fcoe_wq, &port->destroy_work);
>  		goto out;
>  		break;
>  	case NETDEV_FEAT_CHANGE:
> @@ -2412,6 +2415,10 @@ static int __init fcoe_init(void)
>  	unsigned int cpu;
>  	int rc = 0;
>  
> +	fcoe_wq = alloc_workqueue("fcoe", 0, 0);
> +	if (!fcoe_wq)
> +		return -ENOMEM;
> +
>  	mutex_lock(&fcoe_config_mutex);
>  
>  	for_each_possible_cpu(cpu) {
> @@ -2442,6 +2449,7 @@ out_free:
>  		fcoe_percpu_thread_destroy(cpu);
>  	}
>  	mutex_unlock(&fcoe_config_mutex);
> +	destroy_workqueue(fcoe_wq);
>  	return rc;
>  }
>  module_init(fcoe_init);
> @@ -2467,7 +2475,7 @@ static void __exit fcoe_exit(void)
>  		list_del(&fcoe->list);
>  		port = lport_priv(fcoe->ctlr.lp);
>  		fcoe_interface_cleanup(fcoe);
> -		schedule_work(&port->destroy_work);
> +		queue_work(fcoe_wq, &port->destroy_work);
>  	}
>  	rtnl_unlock();
>  
> @@ -2478,12 +2486,11 @@ static void __exit fcoe_exit(void)
>  
>  	mutex_unlock(&fcoe_config_mutex);
>  
> -	/* flush any asyncronous interface destroys,
> -	 * this should happen after the netdev notifier is unregistered */
> -	flush_scheduled_work();
> -	/* That will flush out all the N_Ports on the hostlist, but now we
> -	 * may have NPIV VN_Ports scheduled for destruction */
> -	flush_scheduled_work();
> +	/*
> +	 * destroy_work's may be chained but destroy_workqueue() can take
> +	 * care of them.  Just kill the wq.
> +	 */
> +	destroy_workqueue(fcoe_wq);
>  
>  	/* detach from scsi transport
>  	 * must happen after all destroys are done, therefor after the flush */
> @@ -2632,7 +2639,7 @@ static int fcoe_vport_destroy(struct fc_vport *vport)
>  	mutex_lock(&n_port->lp_mutex);
>  	list_del(&vn_port->list);
>  	mutex_unlock(&n_port->lp_mutex);
> -	schedule_work(&port->destroy_work);
> +	queue_work(fcoe_wq, &port->destroy_work);
>  	return 0;
>  }
>  

Re: [PATCH 3/4] fcoe: use dedicated workqueue instead of system_wq

[Robert Love]

On Mon, 2011-01-24 at 20:43 -0800, James Bottomley wrote:
> On Mon, 2011-01-24 at 14:57 +0100, Tejun Heo wrote:
> > fcoe uses the system_wq to destroy ports and the work items need to be
> > flushed before the driver is unloaded.  As the work items free the
> > containing data structure, they can't be flushed directly.  The
> > workqueue should be flushed instead.
> > 
> > Also, the destruction works can be chained - ie. destruction of a port
> > may lead to destruction of another port where the work item for the
> > former queues the work for the latter.  Currently, the depth of chain
> > can be at most two and fcoe_exit() makes sure everything is complete
> > by calling flush_scheduled_work() twice.
> > 
> > With commit c8efcc25 (workqueue: allow chained queueing during
> > destruction), destroy_workqueue() can take care of chained works on
> > workqueue destruction.  Add and use fcoe_wq instead.  Simply
> > destroying fcoe_wq on driver unload takes care of flushing.
> 
> Cc'ing FCoE maintainer for an opinion.
> 
> Robert, I assume this will come back via your queue with an ack.
> 
> James
> 
Yes, I will take care of it. There is going to be a lot of code movement
in the next fcoe update to make way for a new fcoe model that will allow
for new transports. It's the pre-work that the bnx2fc driver can plug
into. I think having this go through my tree is a good idea.

Thanks, //Rob

[PATCH 4/4] fusion: don't use flush_scheduled_work()

[Tejun Heo]

mptscsih_suspend() does flush_scheduled_work() but mptscsih is the
only one using the system_wq.  Make mptspi use its own workqueue, and
on suspend, flush it and then call mptscsih_suspend().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Eric Moore <Eric.Moore@lsi.com>
---
 drivers/message/fusion/mptscsih.c |    1 -
 drivers/message/fusion/mptspi.c   |   33 ++++++++++++++++++++++++++++-----
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c
index 59b8f53..c99043a 100644
--- a/drivers/message/fusion/mptscsih.c
+++ b/drivers/message/fusion/mptscsih.c
@@ -1227,7 +1227,6 @@ mptscsih_suspend(struct pci_dev *pdev, pm_message_t state)
 	MPT_ADAPTER 		*ioc = pci_get_drvdata(pdev);
 
 	scsi_block_requests(ioc->sh);
-	flush_scheduled_work();
 	mptscsih_shutdown(pdev);
 	return mpt_suspend(pdev,state);
 }
diff --git a/drivers/message/fusion/mptspi.c b/drivers/message/fusion/mptspi.c
index 6d9568d..e7de938 100644
--- a/drivers/message/fusion/mptspi.c
+++ b/drivers/message/fusion/mptspi.c
@@ -90,6 +90,7 @@ static int mptspi_write_spi_device_pg1(struct scsi_target *,
 				       struct _CONFIG_PAGE_SCSI_DEVICE_1 *);
 
 static struct scsi_transport_template *mptspi_transport_template = NULL;
+static struct workqueue_struct *mptspi_wq;
 
 static u8	mptspiDoneCtx = MPT_MAX_PROTOCOL_DRIVERS;
 static u8	mptspiTaskCtx = MPT_MAX_PROTOCOL_DRIVERS;
@@ -1150,7 +1151,7 @@ static void mpt_dv_raid(struct _MPT_SCSI_HOST *hd, int disk)
 	wqw->hd = hd;
 	wqw->disk = disk;
 
-	schedule_work(&wqw->work);
+	queue_work(mptspi_wq, &wqw->work);
 }
 
 static int
@@ -1281,7 +1282,7 @@ mptspi_dv_renegotiate(struct _MPT_SCSI_HOST *hd)
 	INIT_WORK(&wqw->work, mptspi_dv_renegotiate_work);
 	wqw->hd = hd;
 
-	schedule_work(&wqw->work);
+	queue_work(mptspi_wq, &wqw->work);
 }
 
 /*
@@ -1524,6 +1525,18 @@ out_mptspi_probe:
 	return error;
 }
 
+/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
+/*
+ *	mptspi_suspend - Fusion MPT SPI driver suspend routine.
+ *
+ *
+ */
+int mptspi_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	flush_workqueue(mptspi_wq);
+	return mptscsih_suspend(pdev, state);
+}
+
 static struct pci_driver mptspi_driver = {
 	.name		= "mptspi",
 	.id_table	= mptspi_pci_table,
@@ -1531,7 +1544,7 @@ static struct pci_driver mptspi_driver = {
 	.remove		= __devexit_p(mptscsih_remove),
 	.shutdown	= mptscsih_shutdown,
 #ifdef CONFIG_PM
-	.suspend	= mptscsih_suspend,
+	.suspend	= mptspi_suspend,
 	.resume		= mptspi_resume,
 #endif
 };
@@ -1549,9 +1562,15 @@ mptspi_init(void)
 
 	show_mptmod_ver(my_NAME, my_VERSION);
 
+	mptspi_wq = alloc_workqueue("mptspi", 0, 0);
+	if (!mptspi_wq)
+		return -ENOMEM;
+
 	mptspi_transport_template = spi_attach_transport(&mptspi_transport_functions);
-	if (!mptspi_transport_template)
+	if (!mptspi_transport_template) {
+		destroy_workqueue(mptspi_wq);
 		return -ENODEV;
+	}
 
 	mptspiDoneCtx = mpt_register(mptscsih_io_done, MPTSPI_DRIVER,
 	    "mptscsih_io_done");
@@ -1564,8 +1583,10 @@ mptspi_init(void)
 	mpt_reset_register(mptspiDoneCtx, mptspi_ioc_reset);
 
 	error = pci_register_driver(&mptspi_driver);
-	if (error)
+	if (error) {
+		destroy_workqueue(mptspi_wq);
 		spi_release_transport(mptspi_transport_template);
+	}
 
 	return error;
 }
@@ -1587,6 +1608,8 @@ mptspi_exit(void)
 	mpt_deregister(mptspiTaskCtx);
 	mpt_deregister(mptspiDoneCtx);
 	spi_release_transport(mptspi_transport_template);
+
+	destroy_workqueue(mptspi_wq);
 }
 
 module_init(mptspi_init);
-- 
1.7.1

Re: [PATCH 4/4] fusion: don't use flush_scheduled_work()

[Tejun Heo]

On Mon, Jan 24, 2011 at 02:57:31PM +0100, Tejun Heo wrote:
> mptscsih_suspend() does flush_scheduled_work() but mptscsih is the
> only one using the system_wq.  Make mptspi use its own workqueue, and
> on suspend, flush it and then call mptscsih_suspend().
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Eric Moore <Eric.Moore@lsi.com>

Ping on this one too.  I'm quite close to marking
flush_scheduled_work() deprecated in linux-next and this is one of few
holdouts outside of staging.  It would be great if this patch can
appear in linux-next soon.

Thank you.

-- 
tejun