[PATCHSET] scsi: don't use flush_scheduled_work()

[PATCHSET] scsi: don't use flush_scheduled_work()

[Tejun Heo]

Hello,

This patchset removes the use of flush_scheduled_work() from scsi
drivers and contains the following six 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
 0005-scsi-remove-bogus-use-of-struct-execute_work-in-sg.patch
 0006-scsi-don-t-use-execute_in_process_context.patch

The patches are on top of the current scsi-misc-2.6#master +
wq#for-2.6.38[1].  The latter is necessary for commit c8efcc25
(workqueue: allow chained queueing during destruction) as 0003 and
0006 depend on destroy_workqueue() handling chained works correctly.

James, how does 0006 look to you now?  If you think it's acceptable,
please feel free to pull from wq#for-2.6.38, the branch is stable and
won't be rebased.

Thank you.

--
tejun

[1] git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-2.6.38

[PATCH 1/6] 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.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 17e3df4..36cc467 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 de2e09e..91a40f9 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -8871,7 +8871,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.c b/drivers/scsi/megaraid/megaraid_sas.c
index 7451bc0..6da090a 100644
--- a/drivers/scsi/megaraid/megaraid_sas.c
+++ b/drivers/scsi/megaraid/megaraid_sas.c
@@ -4036,9 +4036,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;
 	}
 
@@ -4186,9 +4185,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 300d59f..1d54e7a 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -5459,7 +5459,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/6] 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 f8c86b2..40cf80d 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

[PATCH 3/6] 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

[PATCH 4/6] 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

[PATCH 5/6] scsi: remove bogus use of struct execute_work in sg

[Tejun Heo]

execute_work usage in sg is bogus.  execute_in_process_context() is
never used and ew is used purely as wrapper around work_struct which
is superflous.  Use work_struct directly.

Signed-off-by: Tejun Heo <tj@kernel.org>
Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Acked-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/sg.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 909ed9e..403998c 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -139,7 +139,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 	volatile char done;	/* 0->before bh, 1->before read, 2->read */
 	struct request *rq;
 	struct bio *bio;
-	struct execute_work ew;
+	struct work_struct work;
 } Sg_request;
 
 typedef struct sg_fd {		/* holds the state of a file descriptor */
@@ -162,7 +162,7 @@ typedef struct sg_fd {		/* holds the state of a file descriptor */
 	char keep_orphan;	/* 0 -> drop orphan (def), 1 -> keep for read() */
 	char mmap_called;	/* 0 -> mmap() never called on this fd */
 	struct kref f_ref;
-	struct execute_work ew;
+	struct work_struct work;
 } Sg_fd;
 
 typedef struct sg_device { /* holds the state of each scsi generic device */
@@ -1248,7 +1248,7 @@ sg_mmap(struct file *filp, struct vm_area_struct *vma)
 
 static void sg_rq_end_io_usercontext(struct work_struct *work)
 {
-	struct sg_request *srp = container_of(work, struct sg_request, ew.work);
+	struct sg_request *srp = container_of(work, struct sg_request, work);
 	struct sg_fd *sfp = srp->parentfp;
 
 	sg_finish_rem_req(srp);
@@ -1335,8 +1335,8 @@ static void sg_rq_end_io(struct request *rq, int uptodate)
 		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_IN);
 		kref_put(&sfp->f_ref, sg_remove_sfp);
 	} else {
-		INIT_WORK(&srp->ew.work, sg_rq_end_io_usercontext);
-		schedule_work(&srp->ew.work);
+		INIT_WORK(&srp->work, sg_rq_end_io_usercontext);
+		schedule_work(&srp->work);
 	}
 }
 
@@ -2089,7 +2089,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
 
 static void sg_remove_sfp_usercontext(struct work_struct *work)
 {
-	struct sg_fd *sfp = container_of(work, struct sg_fd, ew.work);
+	struct sg_fd *sfp = container_of(work, struct sg_fd, work);
 	struct sg_device *sdp = sfp->parentdp;
 
 	/* Cleanup any responses which were never read(). */
@@ -2126,8 +2126,8 @@ static void sg_remove_sfp(struct kref *kref)
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 	wake_up_interruptible(&sdp->o_excl_wait);
 
-	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
-	schedule_work(&sfp->ew.work);
+	INIT_WORK(&sfp->work, sg_remove_sfp_usercontext);
+	schedule_work(&sfp->work);
 }
 
 static int
-- 
1.7.1

[PATCH 6/6] scsi: don't use execute_in_process_context()

[Tejun Heo]

SCSI is the only subsystem which uses execute_in_process_context() and
its use is racy against module unload.  ie. the reap work is not
properly flushed and could still be running after the scsi module is
unloaded.

Although execute_in_process_context() can be more efficient when the
caller already has a context, in this case, the call sites are quite
cold and the difference is practically meaningless.  With commit
c8efcc25 (workqueue: allow chained queueing during destruction), the
race condition can easily be fixed by using a dedicated workqueue and
destroying it on module unload.

Create and use scsi_wq instead of execute_in_process_context().

* scsi_device->ew is replaced with release_work.  scsi_target->ew is
  replaced with reap_work.

* Both works are initialized with the respective release/reap function
  during device/target init.  scsi_target_reap_usercontext() is moved
  upwards to avoid needing forward declaration.

* scsi_alloc_target() now explicitly flushes the reap_work of the
  found dying target before putting it instead of depending on
  flush_scheduled_work().

For more info on the issues, please read the following thread.

 http://thread.gmane.org/gmane.linux.scsi/62923

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/scsi/scsi.c        |   15 +++++++++++++--
 drivers/scsi/scsi_scan.c   |   26 +++++++++++++-------------
 drivers/scsi/scsi_sysfs.c  |    8 +++++---
 include/scsi/scsi_device.h |    6 ++++--
 4 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2aeb2e9..12ebcbc 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -70,6 +70,11 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/scsi.h>
 
+/*
+ * Utility multithreaded workqueue for SCSI.
+ */
+struct workqueue_struct *scsi_wq;
+
 static void scsi_done(struct scsi_cmnd *cmd);
 
 /*
@@ -1306,11 +1311,14 @@ MODULE_PARM_DESC(scsi_logging_level, "a bit mask of logging levels");
 
 static int __init init_scsi(void)
 {
-	int error;
+	int error = -ENOMEM;
 
+	scsi_wq = alloc_workqueue("scsi", 0, 0);
+	if (!scsi_wq)
+		return error;
 	error = scsi_init_queue();
 	if (error)
-		return error;
+		goto cleanup_wq;
 	error = scsi_init_procfs();
 	if (error)
 		goto cleanup_queue;
@@ -1342,6 +1350,8 @@ cleanup_procfs:
 	scsi_exit_procfs();
 cleanup_queue:
 	scsi_exit_queue();
+cleanup_wq:
+	destroy_workqueue(scsi_wq);
 	printk(KERN_ERR "SCSI subsystem failed to initialize, error = %d\n",
 	       -error);
 	return error;
@@ -1356,6 +1366,7 @@ static void __exit exit_scsi(void)
 	scsi_exit_devinfo();
 	scsi_exit_procfs();
 	scsi_exit_queue();
+	destroy_workqueue(scsi_wq);
 }
 
 subsys_initcall(init_scsi);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 087821f..4222b58 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -362,6 +362,16 @@ int scsi_is_target_device(const struct device *dev)
 }
 EXPORT_SYMBOL(scsi_is_target_device);
 
+static void scsi_target_reap_usercontext(struct work_struct *work)
+{
+	struct scsi_target *starget =
+		container_of(work, struct scsi_target, reap_work);
+
+	transport_remove_device(&starget->dev);
+	device_del(&starget->dev);
+	scsi_target_destroy(starget);
+}
+
 static struct scsi_target *__scsi_find_target(struct device *parent,
 					      int channel, uint id)
 {
@@ -427,6 +437,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
+	INIT_WORK(&starget->reap_work, scsi_target_reap_usercontext);
  retry:
 	spin_lock_irqsave(shost->host_lock, flags);
 
@@ -462,21 +473,11 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	}
 	/* Unfortunately, we found a dying target; need to
 	 * wait until it's dead before we can get a new one */
+	flush_work(&found_target->reap_work);
 	put_device(&found_target->dev);
-	flush_scheduled_work();
 	goto retry;
 }
 
-static void scsi_target_reap_usercontext(struct work_struct *work)
-{
-	struct scsi_target *starget =
-		container_of(work, struct scsi_target, ew.work);
-
-	transport_remove_device(&starget->dev);
-	device_del(&starget->dev);
-	scsi_target_destroy(starget);
-}
-
 /**
  * scsi_target_reap - check to see if target is in use and destroy if not
  * @starget: target to be checked
@@ -507,8 +508,7 @@ void scsi_target_reap(struct scsi_target *starget)
 	if (state == STARGET_CREATED)
 		scsi_target_destroy(starget);
 	else
-		execute_in_process_context(scsi_target_reap_usercontext,
-					   &starget->ew);
+		queue_work(scsi_wq, &starget->reap_work);
 }
 
 /**
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 76ee2e7..b0cae21 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -300,7 +300,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	struct list_head *this, *tmp;
 	unsigned long flags;
 
-	sdev = container_of(work, struct scsi_device, ew.work);
+	sdev = container_of(work, struct scsi_device, release_work);
 
 	parent = sdev->sdev_gendev.parent;
 	starget = to_scsi_target(parent);
@@ -343,8 +343,8 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 static void scsi_device_dev_release(struct device *dev)
 {
 	struct scsi_device *sdp = to_scsi_device(dev);
-	execute_in_process_context(scsi_device_dev_release_usercontext,
-				   &sdp->ew);
+
+	queue_work(scsi_wq, &sdp->release_work);
 }
 
 static struct class sdev_class = {
@@ -1069,6 +1069,8 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 	dev_set_name(&sdev->sdev_dev, "%d:%d:%d:%d",
 		     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
 	sdev->scsi_level = starget->scsi_level;
+	INIT_WORK(&sdev->release_work, scsi_device_dev_release_usercontext);
+
 	transport_setup_device(&sdev->sdev_gendev);
 	spin_lock_irqsave(shost->host_lock, flags);
 	list_add_tail(&sdev->same_target_siblings, &starget->devices);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 85867dc..64de831 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -168,7 +168,7 @@ struct scsi_device {
 	struct device		sdev_gendev,
 				sdev_dev;
 
-	struct execute_work	ew; /* used to get process context on put */
+	struct work_struct	release_work; /* for process context on put */
 
 	struct scsi_dh_data	*scsi_dh_data;
 	enum scsi_device_state sdev_state;
@@ -258,7 +258,7 @@ struct scsi_target {
 #define SCSI_DEFAULT_TARGET_BLOCKED	3
 
 	char			scsi_level;
-	struct execute_work	ew;
+	struct work_struct	reap_work;
 	enum scsi_target_state	state;
 	void 			*hostdata; /* available to low-level driver */
 	unsigned long		starget_data[0]; /* for the transport */
@@ -276,6 +276,8 @@ static inline struct scsi_target *scsi_target(struct scsi_device *sdev)
 #define starget_printk(prefix, starget, fmt, a...)	\
 	dev_printk(prefix, &(starget)->dev, fmt, ##a)
 
+extern struct workqueue_struct *scsi_wq;
+
 extern struct scsi_device *__scsi_add_device(struct Scsi_Host *,
 		uint, uint, uint, void *hostdata);
 extern int scsi_add_device(struct Scsi_Host *host, uint channel,
-- 
1.7.1

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

[Desai, Kashyap]

Tejun,

Need to add header of function mptspi_suspend.
Logical part of the patch looks OK to me. Please consider this patch as an ACKed by me.

Thanks, Kashyap

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Tejun Heo
> Sent: Tuesday, December 21, 2010 8:32 PM
> To: linux-scsi@vger.kernel.org; James.Bottomley@suse.de;
> fujita.tomonori@lab.ntt.co.jp; linux-kernel@vger.kernel.org; Moore,
> Eric; dgilbert@interlog.com
> Cc: Tejun Heo
> Subject: [PATCH 4/6] fusion: don't use flush_scheduled_work()
> 
> 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tejun Heo]

On Mon, Jan 10, 2011 at 01:00:58PM +0530, Desai, Kashyap wrote:
> Need to add header of function mptspi_suspend.  Logical part of the
> patch looks OK to me. Please consider this patch as an ACKed by me.

Will do so.  Thank you.

-- 
tejun