[PATCHSET] workqueue: another assorted flush_scheduled_work() removals

[PATCHSET] workqueue: another assorted flush_scheduled_work() removals

[Tejun Heo]

Hello,

This is another set of patches to remove flush_scheduled_work() users.

 0001-init-don-t-call-flush_scheduled_work-from-do_initcal.patch
 0002-ioc4-use-static-work_struct-for-ioc4_load_modules.patch
 0003-media-video-explicitly-flush-request_module-work.patch
 0004-media-video-don-t-use-flush_scheduled_work.patch
 0005-speedtch-don-t-abuse-struct-delayed_work.patch
 0006-usb-don-t-use-flush_scheduled_work.patch

Andrew, implementing proper work function in init text test requires
tracking an additional state for each worker.  I don't think it'll be
worth the effort or complexity as removal of init sections itself will
make the bugs, if exists, quite visible anyway, so I just updated
patch description of 0001.  What do you think?

0002 updates the ioc4 driver to use static work_struct and removes
flush_scheduled_work() call in the process.

0003-0004 convert the video drivers and 0005-0006 usb.

These patches are available in the following git tree.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git net-dev-kill-flush_scheduled_work

Note that the branch is on top of the previous set of
flush_scheduled_work() removal patches; however, they're independent
and this series cleanly applies on top of 2.6.37-rc5
(6313e3c21743cc88bb5bd8aa72948ee1e83937b6).

diffstat follows.  Thanks.

 drivers/media/video/bt8xx/bttv-driver.c       |    9 ++++++++
 drivers/media/video/bt8xx/bttv-input.c        |    5 ----
 drivers/media/video/cx18/cx18-driver.c        |    8 +++++++
 drivers/media/video/cx231xx/cx231xx-cards.c   |    8 +++++++
 drivers/media/video/cx23885/cx23885-input.c   |    2 -
 drivers/media/video/cx88/cx88-mpeg.c          |    8 +++++++
 drivers/media/video/em28xx/em28xx-cards.c     |    8 +++++++
 drivers/media/video/omap24xxcam.c             |    6 ++---
 drivers/media/video/saa7134/saa7134-core.c    |   11 ++++++++-
 drivers/media/video/saa7134/saa7134-empress.c |    2 -
 drivers/misc/ioc4.c                           |   29 ++++++++------------------
 drivers/usb/atm/speedtch.c                    |   27 ++++++++++++------------
 drivers/usb/gadget/u_ether.c                  |    4 ---
 drivers/usb/host/ohci-hcd.c                   |    3 +-
 drivers/usb/otg/isp1301_omap.c                |    2 -
 drivers/usb/serial/oti6858.c                  |    5 +---
 drivers/video/fb_defio.c                      |    5 +---
 init/main.c                                   |    3 --
 18 files changed, 86 insertions(+), 59 deletions(-)

--
tejun

[PATCH 1/6] init: don't call flush_scheduled_work() from do_initcalls()

[Tejun Heo]

The call to flush_scheduled_work() in do_initcalls() is there to make
sure all works queued to system_wq by initcalls finish before the init
sections are dropped.

However, the call doesn't make much sense at this point - there
already are multiple different workqueues and different subsystems are
free to create and use their own.  Ordering requirements are and
should be expressed explicitly.

Drop the call to prepare for the deprecation and removal of
flush_scheduled_work().

Andrew suggested adding sanity check where the workqueue code checks
whether any pending or running work has the work function in the init
text section.  However, checking this for running works requires the
worker to keep track of the current function being executed, and
checking only the pending works will miss most cases.  As a violation
will almost always be caught by the usual page fault mechanism, I
don't think it would be worthwhile to make the workqueue code track
extra state just for this.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
This is part of a series to remove flush_scheduled_work() usage to
prepare for deprecation of flush_scheduled_work().  Patches in this
series are self contained and mostly straight-forward.

Please feel free to take it into the appropriate tree, or just ack it.
In the latter case, I'll merge the patch through the workqueue tree
during the next merge window.

Thank you.

 init/main.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/init/main.c b/init/main.c
index 8646401..5421e8f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -775,9 +775,6 @@ static void __init do_initcalls(void)
 
 	for (fn = __early_initcall_end; fn < __initcall_end; fn++)
 		do_one_initcall(*fn);
-
-	/* Make sure there is no pending stuff from the initcall sequence */
-	flush_scheduled_work();
 }
 
 /*
-- 
1.7.1

[PATCH 2/6] ioc4: use static work_struct for ioc4_load_modules()

[Tejun Heo]

There is no reason to dynamically allocate work_struct for
ioc4_load_modules().  It makes the code more complex and makes it
impossible to flush the work directly.  Use static work
ioc4_load_modules_work instead and flush it directly on exit.

This removes the use of flush_scheduled_work() which is being
deprecated.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Brent Casavant <bcasavan@sgi.com>
---
This is part of a series to remove flush_scheduled_work() usage to
prepare for deprecation of flush_scheduled_work().  Patches in this
series are self contained and mostly straight-forward.

Please feel free to take it into the appropriate tree, or just ack it.
In the latter case, I'll merge the patch through the workqueue tree
during the next merge window.

Thank you.

 drivers/misc/ioc4.c |   29 +++++++++--------------------
 1 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
index 1932066..668d41e 100644
--- a/drivers/misc/ioc4.c
+++ b/drivers/misc/ioc4.c
@@ -273,13 +273,11 @@ ioc4_variant(struct ioc4_driver_data *idd)
 static void __devinit
 ioc4_load_modules(struct work_struct *work)
 {
-	/* arg just has to be freed */
-
 	request_module("sgiioc4");
-
-	kfree(work);
 }
 
+static DECLARE_WORK(ioc4_load_modules_work, ioc4_load_modules);
+
 /* Adds a new instance of an IOC4 card */
 static int __devinit
 ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
@@ -396,21 +394,12 @@ ioc4_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id)
 	 * PCI device.
 	 */
 	if (idd->idd_variant != IOC4_VARIANT_PCI_RT) {
-		struct work_struct *work;
-		work = kzalloc(sizeof(struct work_struct), GFP_KERNEL);
-		if (!work) {
-			printk(KERN_WARNING
-			       "%s: IOC4 unable to allocate memory for "
-			       "load of sub-modules.\n", __func__);
-		} else {
-			/* Request the module from a work procedure as the
-			 * modprobe goes out to a userland helper and that
-			 * will hang if done directly from ioc4_probe().
-			 */
-			printk(KERN_INFO "IOC4 loading sgiioc4 submodule\n");
-			INIT_WORK(work, ioc4_load_modules);
-			schedule_work(work);
-		}
+		/* Request the module from a work procedure as the modprobe
+		 * goes out to a userland helper and that will hang if done
+		 * directly from ioc4_probe().
+		 */
+		printk(KERN_INFO "IOC4 loading sgiioc4 submodule\n");
+		schedule_work(&ioc4_load_modules_work);
 	}
 
 	return 0;
@@ -498,7 +487,7 @@ static void __exit
 ioc4_exit(void)
 {
 	/* Ensure ioc4_load_modules() has completed before exiting */
-	flush_scheduled_work();
+	flush_work_sync(&ioc4_load_modules_work);
 	pci_unregister_driver(&ioc4_driver);
 }
 
-- 
1.7.1

[PATCH 3/6] media/video: explicitly flush request_module work

[Tejun Heo]

Video drivers request submodules using a work during probe and calls
flush_scheduled_work() on exit to make sure the work is complete
before being unloaded.  This patch makes these drivers flush the work
directly instead of using flush_scheduled_work().

While at it, relocate request_submodules() call in saa7134_initdev()
right right before successful return as in other drivers to avoid
failing after the work is scheduled and returning failure without the
work still active.

This is in preparation for the deprecation of flush_scheduled_work().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
---
This is part of a series to remove flush_scheduled_work() usage to
prepare for deprecation of flush_scheduled_work().  Patches in this
series are self contained and mostly straight-forward.

Please feel free to take it into the appropriate tree, or just ack it.
In the latter case, I'll merge the patch through the workqueue tree
during the next merge window.

Thank you.

 drivers/media/video/bt8xx/bttv-driver.c     |    9 +++++++++
 drivers/media/video/cx18/cx18-driver.c      |    8 ++++++++
 drivers/media/video/cx231xx/cx231xx-cards.c |    8 ++++++++
 drivers/media/video/cx88/cx88-mpeg.c        |    8 ++++++++
 drivers/media/video/em28xx/em28xx-cards.c   |    8 ++++++++
 drivers/media/video/saa7134/saa7134-core.c  |   11 +++++++++--
 6 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/bt8xx/bttv-driver.c b/drivers/media/video/bt8xx/bttv-driver.c
index a529619..53285aa 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -189,8 +189,14 @@ static void request_modules(struct bttv *dev)
 	INIT_WORK(&dev->request_module_wk, request_module_async);
 	schedule_work(&dev->request_module_wk);
 }
+
+static void flush_request_modules(struct bttv *dev)
+{
+	flush_work_sync(&dev->request_module_wk);
+}
 #else
 #define request_modules(dev)
+#define flush_request_modules(dev)
 #endif /* CONFIG_MODULES */
 
 
@@ -4573,6 +4579,9 @@ static void __devexit bttv_remove(struct pci_dev *pci_dev)
 	if (bttv_verbose)
 		printk("bttv%d: unloading\n",btv->c.nr);
 
+	if (bttv_tvcards[btv->c.type].has_dvb)
+		flush_request_modules(btv);
+
 	/* shutdown everything (DMA+IRQs) */
 	btand(~15, BT848_GPIO_DMA_CTL);
 	btwrite(0, BT848_INT_MASK);
diff --git a/drivers/media/video/cx18/cx18-driver.c b/drivers/media/video/cx18/cx18-driver.c
index df60f27..f6fdcfb 100644
--- a/drivers/media/video/cx18/cx18-driver.c
+++ b/drivers/media/video/cx18/cx18-driver.c
@@ -266,8 +266,14 @@ static void request_modules(struct cx18 *dev)
 	INIT_WORK(&dev->request_module_wk, request_module_async);
 	schedule_work(&dev->request_module_wk);
 }
+
+static void flush_request_modules(struct cx18 *dev)
+{
+	flush_work_sync(&dev->request_module_wk);
+}
 #else
 #define request_modules(dev)
+#define flush_request_modules(dev)
 #endif /* CONFIG_MODULES */
 
 /* Generic utility functions */
@@ -1226,6 +1232,8 @@ static void cx18_remove(struct pci_dev *pci_dev)
 
 	CX18_DEBUG_INFO("Removing Card\n");
 
+	flush_request_modules(cx);
+
 	/* Stop all captures */
 	CX18_DEBUG_INFO("Stopping all streams\n");
 	if (atomic_read(&cx->tot_capturing) > 0)
diff --git a/drivers/media/video/cx231xx/cx231xx-cards.c b/drivers/media/video/cx231xx/cx231xx-cards.c
index 2c78d18..d342ab2 100644
--- a/drivers/media/video/cx231xx/cx231xx-cards.c
+++ b/drivers/media/video/cx231xx/cx231xx-cards.c
@@ -762,8 +762,14 @@ static void request_modules(struct cx231xx *dev)
 	INIT_WORK(&dev->request_module_wk, request_module_async);
 	schedule_work(&dev->request_module_wk);
 }
+
+static void flush_request_modules(struct cx231xx *dev)
+{
+	flush_work_sync(&dev->request_module_wk);
+}
 #else
 #define request_modules(dev)
+#define flush_request_modules(dev)
 #endif /* CONFIG_MODULES */
 
 /*
@@ -1096,6 +1102,8 @@ static void cx231xx_usb_disconnect(struct usb_interface *interface)
 	if (!dev->udev)
 		return;
 
+	flush_request_modules(dev);
+
 	/* delete v4l2 device */
 	v4l2_device_unregister(&dev->v4l2_dev);
 
diff --git a/drivers/media/video/cx88/cx88-mpeg.c b/drivers/media/video/cx88/cx88-mpeg.c
index f7d71ac..addf954 100644
--- a/drivers/media/video/cx88/cx88-mpeg.c
+++ b/drivers/media/video/cx88/cx88-mpeg.c
@@ -66,8 +66,14 @@ static void request_modules(struct cx8802_dev *dev)
 	INIT_WORK(&dev->request_module_wk, request_module_async);
 	schedule_work(&dev->request_module_wk);
 }
+
+static void flush_request_modules(struct cx8802_dev *dev)
+{
+	flush_work_sync(&dev->request_module_wk);
+}
 #else
 #define request_modules(dev)
+#define flush_request_modules(dev)
 #endif /* CONFIG_MODULES */
 
 
@@ -819,6 +825,8 @@ static void __devexit cx8802_remove(struct pci_dev *pci_dev)
 
 	dprintk( 1, "%s\n", __func__);
 
+	flush_request_modules(dev);
+
 	if (!list_empty(&dev->drvlist)) {
 		struct cx8802_driver *drv, *tmp;
 		int err;
diff --git a/drivers/media/video/em28xx/em28xx-cards.c b/drivers/media/video/em28xx/em28xx-cards.c
index f7e9168..f55cd0b 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -2632,8 +2632,14 @@ static void request_modules(struct em28xx *dev)
 	INIT_WORK(&dev->request_module_wk, request_module_async);
 	schedule_work(&dev->request_module_wk);
 }
+
+static void flush_request_modules(struct em28xx *dev)
+{
+	flush_work_sync(&dev->request_module_wk);
+}
 #else
 #define request_modules(dev)
+#define flush_request_modules(dev)
 #endif /* CONFIG_MODULES */
 
 /*
@@ -3060,6 +3066,8 @@ static void em28xx_usb_disconnect(struct usb_interface *interface)
 
 	em28xx_info("disconnecting %s\n", dev->vdev->name);
 
+	flush_request_modules(dev);
+
 	/* wait until all current v4l2 io is finished then deallocate
 	   resources */
 	mutex_lock(&dev->lock);
diff --git a/drivers/media/video/saa7134/saa7134-core.c b/drivers/media/video/saa7134/saa7134-core.c
index 756a278..6abeecf 100644
--- a/drivers/media/video/saa7134/saa7134-core.c
+++ b/drivers/media/video/saa7134/saa7134-core.c
@@ -166,8 +166,14 @@ static void request_submodules(struct saa7134_dev *dev)
 	schedule_work(&dev->request_module_wk);
 }
 
+static void flush_request_submodules(struct saa7134_dev *dev)
+{
+	flush_work_sync(&dev->request_module_wk);
+}
+
 #else
 #define request_submodules(dev)
+#define flush_request_submodules(dev)
 #endif /* CONFIG_MODULES */
 
 /* ------------------------------------------------------------------ */
@@ -1010,8 +1016,6 @@ static int __devinit saa7134_initdev(struct pci_dev *pci_dev,
 		}
 	}
 
-	request_submodules(dev);
-
 	v4l2_prio_init(&dev->prio);
 
 	mutex_lock(&saa7134_devlist_lock);
@@ -1066,6 +1070,7 @@ static int __devinit saa7134_initdev(struct pci_dev *pci_dev,
 	if (saa7134_dmasound_init && !dev->dmasound.priv_data)
 		saa7134_dmasound_init(dev);
 
+	request_submodules(dev);
 	return 0;
 
  fail4:
@@ -1091,6 +1096,8 @@ static void __devexit saa7134_finidev(struct pci_dev *pci_dev)
 	struct saa7134_dev *dev = container_of(v4l2_dev, struct saa7134_dev, v4l2_dev);
 	struct saa7134_mpeg_ops *mops;
 
+	flush_request_submodules(dev);
+
 	/* Release DMA sound modules if present */
 	if (saa7134_dmasound_exit && dev->dmasound.priv_data) {
 		saa7134_dmasound_exit(dev);
-- 
1.7.1

[PATCH 4/6] media/video: don't use flush_scheduled_work()

[Tejun Heo]

This patch converts the remaining users of flush_scheduled_work() in
media/video.

* bttv-input.c and cx23885-input.c don't use workqueue at all.  No
  need to flush.

* Make omap24xxcam.c and saa7134-empress.c flush the used work
  directly.

* In fd_defio.c, replace cancel_delayed_work() +
  flush_scheduled_work() with cancel_delayed_work_sync().  While at
  it, replace the deprecated cancel_rearming_delayed_work() with
  cancel_delayed_work_sync().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
---
This is part of a series to remove flush_scheduled_work() usage to
prepare for deprecation of flush_scheduled_work().  Patches in this
series are self contained and mostly straight-forward.

Please feel free to take it into the appropriate tree, or just ack it.
In the latter case, I'll merge the patch through the workqueue tree
during the next merge window.

Thank you.

 drivers/media/video/bt8xx/bttv-input.c        |    5 +----
 drivers/media/video/cx23885/cx23885-input.c   |    2 --
 drivers/media/video/omap24xxcam.c             |    6 +++---
 drivers/media/video/saa7134/saa7134-empress.c |    2 +-
 drivers/video/fb_defio.c                      |    5 ++---
 5 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/bt8xx/bttv-input.c b/drivers/media/video/bt8xx/bttv-input.c
index 6bf05a7..1989f00 100644
--- a/drivers/media/video/bt8xx/bttv-input.c
+++ b/drivers/media/video/bt8xx/bttv-input.c
@@ -229,16 +229,13 @@ static void bttv_ir_start(struct bttv *btv, struct card_ir *ir)
 
 static void bttv_ir_stop(struct bttv *btv)
 {
-	if (btv->remote->polling) {
+	if (btv->remote->polling)
 		del_timer_sync(&btv->remote->timer);
-		flush_scheduled_work();
-	}
 
 	if (btv->remote->rc5_gpio) {
 		u32 gpio;
 
 		del_timer_sync(&btv->remote->timer_end);
-		flush_scheduled_work();
 
 		gpio = bttv_gpio_read(&btv->c);
 		bttv_gpio_write(&btv->c, gpio & ~(1 << 4));
diff --git a/drivers/media/video/cx23885/cx23885-input.c b/drivers/media/video/cx23885/cx23885-input.c
index bb61870..4a326fe 100644
--- a/drivers/media/video/cx23885/cx23885-input.c
+++ b/drivers/media/video/cx23885/cx23885-input.c
@@ -230,8 +230,6 @@ static void cx23885_input_ir_stop(struct cx23885_dev *dev)
 		v4l2_subdev_call(dev->sd_ir, ir, rx_s_parameters, &params);
 		v4l2_subdev_call(dev->sd_ir, ir, rx_g_parameters, &params);
 	}
-
-	flush_scheduled_work();
 }
 
 static void cx23885_input_ir_close(void *priv)
diff --git a/drivers/media/video/omap24xxcam.c b/drivers/media/video/omap24xxcam.c
index 378b094..0175527 100644
--- a/drivers/media/video/omap24xxcam.c
+++ b/drivers/media/video/omap24xxcam.c
@@ -1198,7 +1198,7 @@ static int vidioc_streamoff(struct file *file, void *fh, enum v4l2_buf_type i)
 
 	atomic_inc(&cam->reset_disable);
 
-	flush_scheduled_work();
+	flush_work_sync(&cam->sensor_reset_work);
 
 	rval = videobuf_streamoff(q);
 	if (!rval) {
@@ -1512,7 +1512,7 @@ static int omap24xxcam_release(struct file *file)
 
 	atomic_inc(&cam->reset_disable);
 
-	flush_scheduled_work();
+	flush_work_sync(&cam->sensor_reset_work);
 
 	/* stop streaming capture */
 	videobuf_streamoff(&fh->vbq);
@@ -1536,7 +1536,7 @@ static int omap24xxcam_release(struct file *file)
 	 * not be scheduled anymore since streaming is already
 	 * disabled.)
 	 */
-	flush_scheduled_work();
+	flush_work_sync(&cam->sensor_reset_work);
 
 	mutex_lock(&cam->mutex);
 	if (atomic_dec_return(&cam->users) == 0) {
diff --git a/drivers/media/video/saa7134/saa7134-empress.c b/drivers/media/video/saa7134/saa7134-empress.c
index b890aaf..6b8459c 100644
--- a/drivers/media/video/saa7134/saa7134-empress.c
+++ b/drivers/media/video/saa7134/saa7134-empress.c
@@ -553,7 +553,7 @@ static int empress_fini(struct saa7134_dev *dev)
 
 	if (NULL == dev->empress_dev)
 		return 0;
-	flush_scheduled_work();
+	flush_work_sync(&dev->empress_workqueue);
 	video_unregister_device(dev->empress_dev);
 	dev->empress_dev = NULL;
 	return 0;
diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
index 6b93ef9..a79c0de 100644
--- a/drivers/video/fb_defio.c
+++ b/drivers/video/fb_defio.c
@@ -75,7 +75,7 @@ int fb_deferred_io_fsync(struct file *file, int datasync)
 		return 0;
 
 	/* Kill off the delayed work */
-	cancel_rearming_delayed_work(&info->deferred_work);
+	cancel_delayed_work_sync(&info->deferred_work);
 
 	/* Run it immediately */
 	return schedule_delayed_work(&info->deferred_work, 0);
@@ -216,8 +216,7 @@ void fb_deferred_io_cleanup(struct fb_info *info)
 	int i;
 
 	BUG_ON(!fbdefio);
-	cancel_delayed_work(&info->deferred_work);
-	flush_scheduled_work();
+	cancel_delayed_work_sync(&info->deferred_work);
 
 	/* clear out the mapping that we setup */
 	for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
-- 
1.7.1

[PATCH 5/6] speedtch: don't abuse struct delayed_work

[Tejun Heo]

speedtch directly uses the internal timer and work members of a struct
delayed_work.  Use a separate work item and timer instead.

Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Duncan Sands <duncan.sands@free.fr>
Cc: linux-usb@vger.kernel.org
---
This is part of a series to remove flush_scheduled_work() usage to
prepare for deprecation of flush_scheduled_work().  Patches in this
series are self contained and mostly straight-forward.

Please feel free to take it into the appropriate tree, or just ack it.
In the latter case, I'll merge the patch through the workqueue tree
during the next merge window.

Thank you.

 drivers/usb/atm/speedtch.c |   25 +++++++++++++------------
 1 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/atm/speedtch.c b/drivers/usb/atm/speedtch.c
index 4716e70..b591906 100644
--- a/drivers/usb/atm/speedtch.c
+++ b/drivers/usb/atm/speedtch.c
@@ -139,7 +139,8 @@ struct speedtch_instance_data {
 
 	struct speedtch_params params; /* set in probe, constant afterwards */
 
-	struct delayed_work status_checker;
+	struct timer_list status_check_timer;
+	struct work_struct status_check_work;
 
 	unsigned char last_status;
 
@@ -498,7 +499,7 @@ static void speedtch_check_status(struct work_struct *work)
 {
 	struct speedtch_instance_data *instance =
 		container_of(work, struct speedtch_instance_data,
-			     status_checker.work);
+			     status_check_work);
 	struct usbatm_data *usbatm = instance->usbatm;
 	struct atm_dev *atm_dev = usbatm->atm_dev;
 	unsigned char *buf = instance->scratch_buffer;
@@ -575,11 +576,11 @@ static void speedtch_status_poll(unsigned long data)
 {
 	struct speedtch_instance_data *instance = (void *)data;
 
-	schedule_delayed_work(&instance->status_checker, 0);
+	schedule_work(&instance->status_check_work);
 
 	/* The following check is racy, but the race is harmless */
 	if (instance->poll_delay < MAX_POLL_DELAY)
-		mod_timer(&instance->status_checker.timer, jiffies + msecs_to_jiffies(instance->poll_delay));
+		mod_timer(&instance->status_check_timer, jiffies + msecs_to_jiffies(instance->poll_delay));
 	else
 		atm_warn(instance->usbatm, "Too many failures - disabling line status polling\n");
 }
@@ -595,7 +596,7 @@ static void speedtch_resubmit_int(unsigned long data)
 	if (int_urb) {
 		ret = usb_submit_urb(int_urb, GFP_ATOMIC);
 		if (!ret)
-			schedule_delayed_work(&instance->status_checker, 0);
+			schedule_work(&instance->status_check_work);
 		else {
 			atm_dbg(instance->usbatm, "%s: usb_submit_urb failed with result %d\n", __func__, ret);
 			mod_timer(&instance->resubmit_timer, jiffies + msecs_to_jiffies(RESUBMIT_DELAY));
@@ -624,7 +625,7 @@ static void speedtch_handle_int(struct urb *int_urb)
 	}
 
 	if ((count == 6) && !memcmp(up_int, instance->int_data, 6)) {
-		del_timer(&instance->status_checker.timer);
+		del_timer(&instance->status_check_timer);
 		atm_info(usbatm, "DSL line goes up\n");
 	} else if ((count == 6) && !memcmp(down_int, instance->int_data, 6)) {
 		atm_info(usbatm, "DSL line goes down\n");
@@ -640,7 +641,7 @@ static void speedtch_handle_int(struct urb *int_urb)
 
 	if ((int_urb = instance->int_urb)) {
 		ret = usb_submit_urb(int_urb, GFP_ATOMIC);
-		schedule_delayed_work(&instance->status_checker, 0);
+		schedule_work(&instance->status_check_work);
 		if (ret < 0) {
 			atm_dbg(usbatm, "%s: usb_submit_urb failed with result %d\n", __func__, ret);
 			goto fail;
@@ -686,7 +687,7 @@ static int speedtch_atm_start(struct usbatm_data *usbatm, struct atm_dev *atm_de
 	}
 
 	/* Start status polling */
-	mod_timer(&instance->status_checker.timer, jiffies + msecs_to_jiffies(1000));
+	mod_timer(&instance->status_check_timer, jiffies + msecs_to_jiffies(1000));
 
 	return 0;
 }
@@ -698,7 +699,7 @@ static void speedtch_atm_stop(struct usbatm_data *usbatm, struct atm_dev *atm_de
 
 	atm_dbg(usbatm, "%s entered\n", __func__);
 
-	del_timer_sync(&instance->status_checker.timer);
+	del_timer_sync(&instance->status_check_timer);
 
 	/*
 	 * Since resubmit_timer and int_urb can schedule themselves and
@@ -869,10 +870,10 @@ static int speedtch_bind(struct usbatm_data *usbatm,
 
 	usbatm->flags |= (use_isoc ? UDSL_USE_ISOC : 0);
 
-	INIT_DELAYED_WORK(&instance->status_checker, speedtch_check_status);
+	INIT_WORK(&instance->status_check_work, speedtch_check_status);
 
-	instance->status_checker.timer.function = speedtch_status_poll;
-	instance->status_checker.timer.data = (unsigned long)instance;
+	instance->status_check_timer.function = speedtch_status_poll;
+	instance->status_check_timer.data = (unsigned long)instance;
 	instance->last_status = 0xff;
 	instance->poll_delay = MIN_POLL_DELAY;
 
-- 
1.7.1

Re: [PATCH 5/6] speedtch: don't abuse struct delayed_work

[Greg KH]

On Sun, Dec 12, 2010 at 05:48:18PM +0100, Tejun Heo wrote:
> speedtch directly uses the internal timer and work members of a struct
> delayed_work.  Use a separate work item and timer instead.
> 
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Duncan Sands <duncan.sands@free.fr>
> Cc: linux-usb@vger.kernel.org
> ---
> This is part of a series to remove flush_scheduled_work() usage to
> prepare for deprecation of flush_scheduled_work().  Patches in this
> series are self contained and mostly straight-forward.
> 
> Please feel free to take it into the appropriate tree, or just ack it.
> In the latter case, I'll merge the patch through the workqueue tree
> during the next merge window.

You forgot to add a "Signed-off-by:" on these patches, so I can't take
them in my tree :(

But feel free to add:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

to both 5/6 and 6/6 of this series and take them through your tree.

thanks,

greg k-h

Re: [PATCH 5/6] speedtch: don't abuse struct delayed_work

[Tejun Heo]

Hello,

On 12/16/2010 10:30 PM, Greg KH wrote:
> You forgot to add a "Signed-off-by:" on these patches, so I can't take
> them in my tree :(

Oops, sorry about that.  Where did they go?  :-)

> But feel free to add:
> 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> to both 5/6 and 6/6 of this series and take them through your tree.

Yeap, will do so.

Thanks.

-- 
tejun

[PATCH 6/6] usb: don't use flush_scheduled_work()

[Tejun Heo]

flush_scheduled_work() is being deprecated.  Directly flush or cancel
work items instead.

* u_ether, isp1301_omap, speedtch conversions are straight-forward.

* ochi-hcd should only flush when quirk_nec() is true as otherwise the
  work wouldn't have been initialized.

* In oti6858, cancel_delayed_work() + flush_scheduled_work() ->
  cancel_delayed_work_sync().

Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Duncan Sands <duncan.sands@free.fr>
Cc: linux-usb@vger.kernel.org
---
This is part of a series to remove flush_scheduled_work() usage to
prepare for deprecation of flush_scheduled_work().  Patches in this
series are self contained and mostly straight-forward.

Please feel free to take it into the appropriate tree, or just ack it.
In the latter case, I'll merge the patch through the workqueue tree
during the next merge window.

Thank you.

 drivers/usb/atm/speedtch.c     |    2 +-
 drivers/usb/gadget/u_ether.c   |    4 +---
 drivers/usb/host/ohci-hcd.c    |    3 ++-
 drivers/usb/otg/isp1301_omap.c |    2 +-
 drivers/usb/serial/oti6858.c   |    5 ++---
 5 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/atm/speedtch.c b/drivers/usb/atm/speedtch.c
index b591906..ff0f219 100644
--- a/drivers/usb/atm/speedtch.c
+++ b/drivers/usb/atm/speedtch.c
@@ -718,7 +718,7 @@ static void speedtch_atm_stop(struct usbatm_data *usbatm, struct atm_dev *atm_de
 	del_timer_sync(&instance->resubmit_timer);
 	usb_free_urb(int_urb);
 
-	flush_scheduled_work();
+	flush_work_sync(&instance->status_check_work);
 }
 
 static int speedtch_pre_reset(struct usb_interface *intf)
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index fbe86ca..00a7824 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -829,11 +829,9 @@ void gether_cleanup(void)
 		return;
 
 	unregister_netdev(the_dev->net);
+	flush_work_sync(&the_dev->work);
 	free_netdev(the_dev->net);
 
-	/* assuming we used keventd, it must quiesce too */
-	flush_scheduled_work();
-
 	the_dev = NULL;
 }
 
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 5179acb..bd5eff7 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -901,7 +901,8 @@ static void ohci_stop (struct usb_hcd *hcd)
 
 	ohci_dump (ohci, 1);
 
-	flush_scheduled_work();
+	if (quirk_nec(ohci))
+		flush_work_sync(&ohci->nec_work);
 
 	ohci_usb_reset (ohci);
 	ohci_writel (ohci, OHCI_INTR_MIE, &ohci->regs->intrdisable);
diff --git a/drivers/usb/otg/isp1301_omap.c b/drivers/usb/otg/isp1301_omap.c
index 4569694..e00fa1b 100644
--- a/drivers/usb/otg/isp1301_omap.c
+++ b/drivers/usb/otg/isp1301_omap.c
@@ -1247,7 +1247,7 @@ static int __exit isp1301_remove(struct i2c_client *i2c)
 	isp->timer.data = 0;
 	set_bit(WORK_STOP, &isp->todo);
 	del_timer_sync(&isp->timer);
-	flush_scheduled_work();
+	flush_work_sync(&isp->work);
 
 	put_device(&i2c->dev);
 	the_transceiver = NULL;
diff --git a/drivers/usb/serial/oti6858.c b/drivers/usb/serial/oti6858.c
index e199b0f..5be866b 100644
--- a/drivers/usb/serial/oti6858.c
+++ b/drivers/usb/serial/oti6858.c
@@ -613,9 +613,8 @@ static void oti6858_close(struct usb_serial_port *port)
 	dbg("%s(): after buf_clear()", __func__);
 
 	/* cancel scheduled setup */
-	cancel_delayed_work(&priv->delayed_setup_work);
-	cancel_delayed_work(&priv->delayed_write_work);
-	flush_scheduled_work();
+	cancel_delayed_work_sync(&priv->delayed_setup_work);
+	cancel_delayed_work_sync(&priv->delayed_write_work);
 
 	/* shutdown our urbs */
 	dbg("%s(): shutting down urbs", __func__);
-- 
1.7.1

Re: speedtch: don't abuse struct delayed_work

[Nicolas Kaiser]

> On Sun, Dec 12, 2010 at 05:48:18PM +0100, Tejun Heo wrote:
>> speedtch directly uses the internal timer and work members of a struct
>> delayed_work.  Use a separate work item and timer instead.

>> This is part of a series to remove flush_scheduled_work() usage to
>> prepare for deprecation of flush_scheduled_work().  Patches in this
>> series are self contained and mostly straight-forward

I tested this patch with 2.6.37-rc6. Not sure whether 
I'm doing something wrong, but I failed to start the
PPP daemon with my speedtouch usb:

Dec 17 14:17:24 absol pppd[4853]: Plugin pppoatm.so loaded.
Dec 17 14:17:24 absol pppd[4853]: PPPoATM plugin_init
Dec 17 14:17:24 absol pppd[4853]: PPPoATM setdevname_pppoatm - SUCCESS:8.48
Dec 17 14:17:24 absol pppd[4854]: pppd 2.4.4 started by root, uid 0
Dec 17 14:17:24 absol pppd[4854]: connect(8.48): No such device
Dec 17 14:17:24 absol pppd[4854]: Exit.

Dec 17 14:17:26 absol rc-scripts: Failed to start the PPP daemon


Without the patch:

Dec 17 14:19:19 absol pppd[4847]: Plugin pppoatm.so loaded.
Dec 17 14:19:19 absol pppd[4847]: PPPoATM plugin_init
Dec 17 14:19:19 absol pppd[4847]: PPPoATM setdevname_pppoatm - SUCCESS:8.48
Dec 17 14:19:19 absol pppd[4848]: pppd 2.4.4 started by root, uid 0
Dec 17 14:19:19 absol pppd[4848]: using channel 1
Dec 17 14:19:19 absol pppd[4848]: Using interface ppp0
Dec 17 14:19:19 absol pppd[4848]: Connect: ppp0 <--> 8.48


Best regards,
Nicolas Kaiser

Re: speedtch: don't abuse struct delayed_work

[Nicolas Kaiser]

* Nicolas Kaiser <nikai@nikai.net>:
> > On Sun, Dec 12, 2010 at 05:48:18PM +0100, Tejun Heo wrote:
> >> speedtch directly uses the internal timer and work members of a struct
> >> delayed_work.  Use a separate work item and timer instead.
> 
> >> This is part of a series to remove flush_scheduled_work() usage to
> >> prepare for deprecation of flush_scheduled_work().  Patches in this
> >> series are self contained and mostly straight-forward
> 
> I tested this patch with 2.6.37-rc6. Not sure whether 
> I'm doing something wrong, but I failed to start the
> PPP daemon with my speedtouch usb:

I narrowed down my problem, and noticed that speedtch_atm_start
never returns, but gets stuck trying to start status polling.

Now when I delete this mod_timer, everything appears to work
nicely at my place. With additional debug output:


Dec 18 12:24:40 absol kernel: ATM dev 0: speedtch_atm_start entered
Dec 18 12:24:40 absol kernel: ATM dev 0: speedtch_start_synchro entered
Dec 18 12:24:40 absol kernel: ATM dev 0: speedtch_start_synchro: modem prodded. 2 bytes returned: 00 00
 Dec 18 12:24:40 absol kernel: return from speedtch_start_synchro
 Dec 18 12:24:40 absol kernel: return from speedtch_atm_start

Dec 18 12:24:49 absol pppd[4844]: Plugin pppoatm.so loaded.
Dec 18 12:24:49 absol pppd[4844]: PPPoATM plugin_init
Dec 18 12:24:49 absol pppd[4844]: PPPoATM setdevname_pppoatm - SUCCESS:8.48
Dec 18 12:24:49 absol pppd[4845]: pppd 2.4.4 started by root, uid 0
Dec 18 12:24:49 absol pppd[4845]: using channel 1
Dec 18 12:24:49 absol pppd[4845]: Using interface ppp0
Dec 18 12:24:49 absol pppd[4845]: Connect: ppp0 <--> 8.48

Dec 18 12:24:54 absol kernel: ATM dev 0: speedtch_handle_int entered
Dec 18 12:24:54 absol kernel: ATM dev 0: DSL line goes up
 Dec 18 12:24:54 absol kernel: return from speedtch_handle_int
 Dec 18 12:24:54 absol kernel: entered speedtch_check_status
 Dec 18 12:24:54 absol kernel: entered speedtch_read_status
 Dec 18 12:24:54 absol kernel: return from speedtch_read_status
Dec 18 12:24:54 absol kernel: ATM dev 0: speedtch_check_status: line state 0x20
Dec 18 12:24:54 absol kernel: ATM dev 0: ADSL line is up (6176 kb/s down | 512 kb/s up)
 Dec 18 12:24:54 absol kernel: return from speedtch_check_status


Best regards,
Nicolas Kaiser

Signed-off-by: Nicolas Kaiser <nikai@nikai.net>
---
 drivers/usb/atm/speedtch.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/atm/speedtch.c b/drivers/usb/atm/speedtch.c
index b591906..e6d8847 100644
--- a/drivers/usb/atm/speedtch.c
+++ b/drivers/usb/atm/speedtch.c
@@ -686,9 +686,6 @@ static int speedtch_atm_start(struct usbatm_data *usbatm, struct atm_dev *atm_de
 		}
 	}
 
-	/* Start status polling */
-	mod_timer(&instance->status_check_timer, jiffies + msecs_to_jiffies(1000));
-
 	return 0;
 }
 
-- 
1.7.2.2

[PATCH UPDATED 5/6] speedtch: don't abuse struct delayed_work

[Tejun Heo]

speedtch directly uses the internal timer and work members of a struct
delayed_work.  Use a separate work item and timer instead.

* Nicolas Kaiser discovered that timer init was missing.  Fixed.

Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Duncan Sands <duncan.sands@free.fr>
Cc: linux-usb@vger.kernel.org
Cc: Nicolas Kaiser <nikai@nikai.net>
---
Can you please test this one?  Thanks.

 drivers/usb/atm/speedtch.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Index: work/drivers/usb/atm/speedtch.c
===================================================================
--- work.orig/drivers/usb/atm/speedtch.c
+++ work/drivers/usb/atm/speedtch.c
@@ -139,7 +139,8 @@ struct speedtch_instance_data {

 	struct speedtch_params params; /* set in probe, constant afterwards */

-	struct delayed_work status_checker;
+	struct timer_list status_check_timer;
+	struct work_struct status_check_work;

 	unsigned char last_status;

@@ -498,7 +499,7 @@ static void speedtch_check_status(struct
 {
 	struct speedtch_instance_data *instance =
 		container_of(work, struct speedtch_instance_data,
-			     status_checker.work);
+			     status_check_work);
 	struct usbatm_data *usbatm = instance->usbatm;
 	struct atm_dev *atm_dev = usbatm->atm_dev;
 	unsigned char *buf = instance->scratch_buffer;
@@ -575,11 +576,11 @@ static void speedtch_status_poll(unsigne
 {
 	struct speedtch_instance_data *instance = (void *)data;

-	schedule_delayed_work(&instance->status_checker, 0);
+	schedule_work(&instance->status_check_work);

 	/* The following check is racy, but the race is harmless */
 	if (instance->poll_delay < MAX_POLL_DELAY)
-		mod_timer(&instance->status_checker.timer, jiffies + msecs_to_jiffies(instance->poll_delay));
+		mod_timer(&instance->status_check_timer, jiffies + msecs_to_jiffies(instance->poll_delay));
 	else
 		atm_warn(instance->usbatm, "Too many failures - disabling line status polling\n");
 }
@@ -595,7 +596,7 @@ static void speedtch_resubmit_int(unsign
 	if (int_urb) {
 		ret = usb_submit_urb(int_urb, GFP_ATOMIC);
 		if (!ret)
-			schedule_delayed_work(&instance->status_checker, 0);
+			schedule_work(&instance->status_check_work);
 		else {
 			atm_dbg(instance->usbatm, "%s: usb_submit_urb failed with result %d\n", __func__, ret);
 			mod_timer(&instance->resubmit_timer, jiffies + msecs_to_jiffies(RESUBMIT_DELAY));
@@ -624,7 +625,7 @@ static void speedtch_handle_int(struct u
 	}

 	if ((count == 6) && !memcmp(up_int, instance->int_data, 6)) {
-		del_timer(&instance->status_checker.timer);
+		del_timer(&instance->status_check_timer);
 		atm_info(usbatm, "DSL line goes up\n");
 	} else if ((count == 6) && !memcmp(down_int, instance->int_data, 6)) {
 		atm_info(usbatm, "DSL line goes down\n");
@@ -640,7 +641,7 @@ static void speedtch_handle_int(struct u

 	if ((int_urb = instance->int_urb)) {
 		ret = usb_submit_urb(int_urb, GFP_ATOMIC);
-		schedule_delayed_work(&instance->status_checker, 0);
+		schedule_work(&instance->status_check_work);
 		if (ret < 0) {
 			atm_dbg(usbatm, "%s: usb_submit_urb failed with result %d\n", __func__, ret);
 			goto fail;
@@ -686,7 +687,7 @@ static int speedtch_atm_start(struct usb
 	}

 	/* Start status polling */
-	mod_timer(&instance->status_checker.timer, jiffies + msecs_to_jiffies(1000));
+	mod_timer(&instance->status_check_timer, jiffies + msecs_to_jiffies(1000));

 	return 0;
 }
@@ -698,7 +699,7 @@ static void speedtch_atm_stop(struct usb

 	atm_dbg(usbatm, "%s entered\n", __func__);

-	del_timer_sync(&instance->status_checker.timer);
+	del_timer_sync(&instance->status_check_timer);

 	/*
 	 * Since resubmit_timer and int_urb can schedule themselves and
@@ -869,10 +870,11 @@ static int speedtch_bind(struct usbatm_d

 	usbatm->flags |= (use_isoc ? UDSL_USE_ISOC : 0);

-	INIT_DELAYED_WORK(&instance->status_checker, speedtch_check_status);
+	INIT_WORK(&instance->status_check_work, speedtch_check_status);
+	init_timer(&instance->status_check_timer);

-	instance->status_checker.timer.function = speedtch_status_poll;
-	instance->status_checker.timer.data = (unsigned long)instance;
+	instance->status_check_timer.function = speedtch_status_poll;
+	instance->status_check_timer.data = (unsigned long)instance;
 	instance->last_status = 0xff;
 	instance->poll_delay = MIN_POLL_DELAY;

Re: [PATCH UPDATED 5/6] speedtch: don't abuse struct delayed_work

[Nicolas Kaiser]

* Tejun Heo <tj@kernel.org>:
> speedtch directly uses the internal timer and work members of a struct
> delayed_work.  Use a separate work item and timer instead.
> 
> * Nicolas Kaiser discovered that timer init was missing.  Fixed.

> Can you please test this one?  Thanks.

Works for me, thanks!

Tested-by: Nicolas Kaiser <nikai@nikai.net>

I'm about to replace this device, and will have to return it
to my ISP in about a fortnight. Until then I can perform
further tests if needed.

Best regards,
Nicolas Kaiser

Re: [PATCHSET] workqueue: another assorted flush_scheduled_work() removals

[Tejun Heo]

On Sun, Dec 12, 2010 at 05:48:13PM +0100, Tejun Heo wrote:
> Hello,
> 
> This is another set of patches to remove flush_scheduled_work() users.
> 
>  0001-init-don-t-call-flush_scheduled_work-from-do_initcal.patch
>  0002-ioc4-use-static-work_struct-for-ioc4_load_modules.patch
>  0003-media-video-explicitly-flush-request_module-work.patch
>  0004-media-video-don-t-use-flush_scheduled_work.patch
>  0005-speedtch-don-t-abuse-struct-delayed_work.patch
>  0006-usb-don-t-use-flush_scheduled_work.patch

Merged into wq#for-next.  Thanks.

-- 
tejun