scsi: remove use of flush_scheduled_work()
Subsystems (8)
PATCH 1/1 scsi: remove use of flush_scheduled_work()
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
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().
--- 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(-)
drivers/scsi/NCR5380.c +1 -2
@@ -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); }/**
drivers/scsi/arcmsr/arcmsr_hba.c +2 -2
@@ -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); }
drivers/scsi/ipr.c +1 -1
@@ -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);
drivers/scsi/megaraid/megaraid_sas_base.c +2 -4
@@ -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 ; }
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0 -1
@@ -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 "
drivers/scsi/pmcraid.c +1 -1
@@ -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);
Signature · 1 line
1.7.1
PATCH 2/4 scsi: pm8001: simplify workqueue usage
In reply to Tejun Heopm8001 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.
--- 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(-)
drivers/scsi/pm8001/pm8001_hwi.c +17 -20
@@ -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 ;
drivers/scsi/pm8001/pm8001_init.c +18 -9
@@ -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);
drivers/scsi/pm8001/pm8001_sas.h +6 -4
@@ -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);
Signature · 1 line
1.7.1
PATCH 3/4 fcoe: use dedicated workqueue instead of system_wq
In reply to Tejun Heofcoe 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.
--- drivers/scsi/fcoe/fcoe.c | 25 ++++++++++++++++--------- 1 files changed, 16 insertions(+), 9 deletions(-)
drivers/scsi/fcoe/fcoe.c +16 -9
@@ -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 ; }
Signature · 1 line
1.7.1
PATCH 4/4 fusion: don't use flush_scheduled_work()
In reply to Tejun Heomptscsih_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().
--- drivers/message/fusion/mptscsih.c | 1 - drivers/message/fusion/mptspi.c | 33 ++++++++++++++++++++++++++++----- 2 files changed, 28 insertions(+), 6 deletions(-)
drivers/message/fusion/mptscsih.c +0 -1
@@ -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); }
drivers/message/fusion/mptspi.c +28 -5
@@ -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);
Signature · 1 line
1.7.1
James Bottomley
maintainerRe: [PATCH 2/4] scsi: pm8001: simplify workqueue usage
In reply to Tejun HeoQuoted 13 lines · 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_wqis created to serve these work items.* To avoid confusion, the "queue" suffixes are dropped from work items and functions.
Quoted 7 lines
* Delayed queueing was never used.
pm8001_worknow useswork_structinstead.* The driver no longer keeps track of pending works. All
pm8001_worksare queued topm8001_wqand 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
Quoted 212 lines
Signed-off-by Tejun Heo <[email protected]>--- 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(-)drivers/scsi/pm8001/pm8001_hwi.c +17
-20@@ -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 ;drivers/scsi/pm8001/pm8001_init.c +18
-9@@ -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);drivers/scsi/pm8001/pm8001_sas.h +6
-4@@ -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);
James Bottomley
maintainerRe: [PATCH 3/4] fcoe: use dedicated workqueue instead of system_wq
In reply to Tejun HeoQuoted 11 lines · Tejun Heo
fcoe uses the
system_wqto 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 callingflush_scheduled_work()twice.
Quoted 4 lines
With commit
c8efcc25(workqueue: allow chained queueing during destruction),destroy_workqueue()can take care of chained works on workqueue destruction. Add and usefcoe_wqinstead. Simply destroyingfcoe_wqon 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
Quoted 90 lines
Signed-off-by Tejun Heo <[email protected]>--- drivers/scsi/fcoe/fcoe.c | 25 ++++++++++++++++--------- 1 files changed, 16 insertions(+), 9 deletions(-)drivers/scsi/fcoe/fcoe.c +16
-9@@ -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 2/4] scsi: pm8001: simplify workqueue usage
In reply to James BottomleyQuoted 21 lines · Tejun Heo
Quoted 19 lines
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_wqis 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_worknow useswork_structinstead.* The driver no longer keeps track of pending works. All
pm8001_worksare queued topm8001_wqand the workqueue is flushed as necessary.
Quoted 9 lines
Quoted 1 line
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 <[email protected]>
Thanks
Re: [PATCH 3/4] fcoe: use dedicated workqueue instead of system_wq
In reply to James BottomleyQuoted 17 lines · James Bottomley
Quoted 11 lines · Tejun Heo
fcoe uses the
system_wqto 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 callingflush_scheduled_work()twice.Quoted 4 lines
With commit
c8efcc25(workqueue: allow chained queueing during destruction),destroy_workqueue()can take care of chained works on workqueue destruction. Add and usefcoe_wqinstead. Simply destroyingfcoe_wqon driver unload takes care of flushing.
Quoted 6 lines
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
Re: [PATCH 4/4] fusion: don't use flush_scheduled_work()
In reply to Tejun HeoQuoted 6 lines · Tejun Heo
mptscsih_suspend()doesflush_scheduled_work()but mptscsih is the only one using thesystem_wq. Make mptspi use its own workqueue, and on suspend, flush it and then callmptscsih_suspend().Signed-off-by Tejun Heo <[email protected]>Cc Eric Moore <[email protected]>
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.
Signature · 1 line
tejun