usb-storage: fix freezing of the scanning thread

This patch (as1521b) fixes the interaction between usb-storage's scanning thread and the freezer. The current implementation has a race: If the device is unplugged shortly after being plugged in and just as a system sleep begins, the scanning thread may get frozen before the khubd task. Khubd won't be able to freeze until the disconnect processing is complete, and the disconnect processing can't proceed until the scanning thread finishes, so the sleep transition will fail. The implementation in the 3.2 kernel suffers from an additional problem. There the scanning thread calls set_freezable_with_signal(), and the signals sent by the freezer will mess up the thread's I/O delays, which are all interruptible. The solution to both problems is the same: Replace the kernel thread used for scanning with a delayed-work routine on the system freezable work queue. Freezable work queues have the nice property that you can cancel a work item even while the work queue is frozen, and no signals are needed. The 3.2 version of this patch solves the problem in Bugzilla #42730. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Acked-by: Seth Forshee <seth.forshee@canonical.com> CC: stable <stable@vger.kernel.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

usb-storage: fix freezing of the scanning thread
This patch (as1521b) fixes the interaction between usb-storage's scanning thread and the freezer. The current implementation has a race: If the device is unplugged shortly after being plugged in and just as a system sleep begins, the scanning thread may get frozen before the khubd task. Khubd won't be able to freeze until the disconnect processing is complete, and the disconnect processing can't proceed until the scanning thread finishes, so the sleep transition will fail. The implementation in the 3.2 kernel suffers from an additional problem. There the scanning thread calls set_freezable_with_signal(), and the signals sent by the freezer will mess up the thread's I/O delays, which are all interruptible. The solution to both problems is the same: Replace the kernel thread used for scanning with a delayed-work routine on the system freezable work queue. Freezable work queues have the nice property that you can cancel a work item even while the work queue is frozen, and no signals are needed. The 3.2 version of this patch solves the problem in Bugzilla #42730. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Acked-by: Seth Forshee <seth.forshee@canonical.com> CC: stable <stable@vger.kernel.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Alan Stern · Greg Kroah-Hartman
1 parent 9a9a71b77c
Showing 2 changed files with 35 additions and 62 deletions Side-by-side Diff
drivers/usb/storage/usb.c
drivers/usb/storage/usb.h
@@ -788,16 +788,20 @@
 	struct Scsi_Host *host = us_to_host(us);
  
 	/* If the device is really gone, cut short reset delays */
-	if (us->pusb_dev->state == USB_STATE_NOTATTACHED)
+	if (us->pusb_dev->state == USB_STATE_NOTATTACHED) {
 		set_bit(US_FLIDX_DISCONNECTING, &us->dflags);
+		wake_up(&us->delay_wait);
+	}
  
-	/* Prevent SCSI-scanning (if it hasn't started yet)
-	 * and wait for the SCSI-scanning thread to stop.
+	/* Prevent SCSI scanning (if it hasn't started yet)
+	 * or wait for the SCSI-scanning routine to stop.
 	 */
-	set_bit(US_FLIDX_DONT_SCAN, &us->dflags);
-	wake_up(&us->delay_wait);
-	wait_for_completion(&us->scanning_done);
+	cancel_delayed_work_sync(&us->scan_dwork);
  
+	/* Balance autopm calls if scanning was cancelled */
+	if (test_bit(US_FLIDX_SCAN_PENDING, &us->dflags))
+		usb_autopm_put_interface_no_suspend(us->pusb_intf);
+
 	/* Removing the host will perform an orderly shutdown: caches
 	 * synchronized, disks spun down, etc.
 	 */
  
  
  
  
  
  
  
@@ -823,53 +827,28 @@
 	scsi_host_put(us_to_host(us));
 }
  
-/* Thread to carry out delayed SCSI-device scanning */
-static int usb_stor_scan_thread(void * __us)
+/* Delayed-work routine to carry out SCSI-device scanning */
+static void usb_stor_scan_dwork(struct work_struct *work)
 {
-	struct us_data *us = (struct us_data *)__us;
+	struct us_data *us = container_of(work, struct us_data,
+			scan_dwork.work);
 	struct device *dev = &us->pusb_intf->dev;
  
-	dev_dbg(dev, "device found\n");
+	dev_dbg(dev, "starting scan\n");
  
-	set_freezable();
-
-	/*
-	 * Wait for the timeout to expire or for a disconnect
-	 *
-	 * We can't freeze in this thread or we risk causing khubd to
-	 * fail to freeze, but we can't be non-freezable either. Nor can
-	 * khubd freeze while waiting for scanning to complete as it may
-	 * hold the device lock, causing a hang when suspending devices.
-	 * So instead of using wait_event_freezable(), explicitly test
-	 * for (DONT_SCAN || freezing) in interruptible wait and proceed
-	 * if any of DONT_SCAN, freezing or timeout has happened.
-	 */
-	if (delay_use > 0) {
-		dev_dbg(dev, "waiting for device to settle "
-				"before scanning\n");
-		wait_event_interruptible_timeout(us->delay_wait,
-				test_bit(US_FLIDX_DONT_SCAN, &us->dflags) ||
-				freezing(current), delay_use * HZ);
+	/* For bulk-only devices, determine the max LUN value */
+	if (us->protocol == USB_PR_BULK && !(us->fflags & US_FL_SINGLE_LUN)) {
+		mutex_lock(&us->dev_mutex);
+		us->max_lun = usb_stor_Bulk_max_lun(us);
+		mutex_unlock(&us->dev_mutex);
 	}
+	scsi_scan_host(us_to_host(us));
+	dev_dbg(dev, "scan complete\n");
  
-	/* If the device is still connected, perform the scanning */
-	if (!test_bit(US_FLIDX_DONT_SCAN, &us->dflags)) {
+	/* Should we unbind if no devices were detected? */
  
-		/* For bulk-only devices, determine the max LUN value */
-		if (us->protocol == USB_PR_BULK &&
-				!(us->fflags & US_FL_SINGLE_LUN)) {
-			mutex_lock(&us->dev_mutex);
-			us->max_lun = usb_stor_Bulk_max_lun(us);
-			mutex_unlock(&us->dev_mutex);
-		}
-		scsi_scan_host(us_to_host(us));
-		dev_dbg(dev, "scan complete\n");
-
-		/* Should we unbind if no devices were detected? */
-	}
-
 	usb_autopm_put_interface(us->pusb_intf);
-	complete_and_exit(&us->scanning_done, 0);
+	clear_bit(US_FLIDX_SCAN_PENDING, &us->dflags);
 }
  
 static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf)
@@ -916,7 +895,7 @@
 	init_completion(&us->cmnd_ready);
 	init_completion(&(us->notify));
 	init_waitqueue_head(&us->delay_wait);
-	init_completion(&us->scanning_done);
+	INIT_DELAYED_WORK(&us->scan_dwork, usb_stor_scan_dwork);
  
 	/* Associate the us_data structure with the USB device */
 	result = associate_dev(us, intf);
@@ -947,7 +926,6 @@
 /* Second part of general USB mass-storage probing */
 int usb_stor_probe2(struct us_data *us)
 {
-	struct task_struct *th;
 	int result;
 	struct device *dev = &us->pusb_intf->dev;
  
  
  
@@ -988,20 +966,14 @@
 		goto BadDevice;
 	}
  
-	/* Start up the thread for delayed SCSI-device scanning */
-	th = kthread_create(usb_stor_scan_thread, us, "usb-stor-scan");
-	if (IS_ERR(th)) {
-		dev_warn(dev,
-				"Unable to start the device-scanning thread\n");
-		complete(&us->scanning_done);
-		quiesce_and_remove_host(us);
-		result = PTR_ERR(th);
-		goto BadDevice;
-	}
-
+	/* Submit the delayed_work for SCSI-device scanning */
 	usb_autopm_get_interface_no_resume(us->pusb_intf);
-	wake_up_process(th);
+	set_bit(US_FLIDX_SCAN_PENDING, &us->dflags);
  
+	if (delay_use > 0)
+		dev_dbg(dev, "waiting for device to settle before scanning\n");
+	queue_delayed_work(system_freezable_wq, &us->scan_dwork,
+			delay_use * HZ);
 	return 0;
  
 	/* We come here if there are any problems */
@@ -47,6 +47,7 @@
 #include <linux/blkdev.h>
 #include <linux/completion.h>
 #include <linux/mutex.h>
+#include <linux/workqueue.h>
 #include <scsi/scsi_host.h>
  
 struct us_data;
@@ -72,7 +73,7 @@
 #define US_FLIDX_DISCONNECTING	3	/* disconnect in progress   */
 #define US_FLIDX_RESETTING	4	/* device reset in progress */
 #define US_FLIDX_TIMED_OUT	5	/* SCSI midlayer timed out  */
-#define US_FLIDX_DONT_SCAN	6	/* don't scan (disconnect)  */
+#define US_FLIDX_SCAN_PENDING	6	/* scanning not yet done    */
 #define US_FLIDX_REDO_READ10	7	/* redo READ(10) command    */
 #define US_FLIDX_READ10_WORKED	8	/* previous READ(10) succeeded */
  
@@ -147,8 +148,8 @@
 	/* mutual exclusion and synchronization structures */
 	struct completion	cmnd_ready;	 /* to sleep thread on	    */
 	struct completion	notify;		 /* thread begin/end	    */
-	wait_queue_head_t	delay_wait;	 /* wait during scan, reset */
-	struct completion	scanning_done;	 /* wait for scan thread    */
+	wait_queue_head_t	delay_wait;	 /* wait during reset	    */
+	struct delayed_work	scan_dwork;	 /* for async scanning      */
  
 	/* subdriver information */
 	void			*extra;		 /* Any extra data          */
...	...	@@ -788,16 +788,20 @@
788	788	struct Scsi_Host *host = us_to_host(us);
789	789
790	790	/* If the device is really gone, cut short reset delays */
791		- if (us->pusb_dev->state == USB_STATE_NOTATTACHED)
	791	+ if (us->pusb_dev->state == USB_STATE_NOTATTACHED) {
792	792	set_bit(US_FLIDX_DISCONNECTING, &us->dflags);
	793	+ wake_up(&us->delay_wait);
	794	+ }
793	795
794		- /* Prevent SCSI-scanning (if it hasn't started yet)
795		- * and wait for the SCSI-scanning thread to stop.
	796	+ /* Prevent SCSI scanning (if it hasn't started yet)
	797	+ * or wait for the SCSI-scanning routine to stop.
796	798	*/
797		- set_bit(US_FLIDX_DONT_SCAN, &us->dflags);
798		- wake_up(&us->delay_wait);
799		- wait_for_completion(&us->scanning_done);
	799	+ cancel_delayed_work_sync(&us->scan_dwork);
800	800
	801	+ /* Balance autopm calls if scanning was cancelled */
	802	+ if (test_bit(US_FLIDX_SCAN_PENDING, &us->dflags))
	803	+ usb_autopm_put_interface_no_suspend(us->pusb_intf);
	804	+
801	805	/* Removing the host will perform an orderly shutdown: caches
802	806	* synchronized, disks spun down, etc.
803	807	*/
804	808
805	809
806	810
807	811
808	812
809	813
810	814
...	...	@@ -823,53 +827,28 @@
823	827	scsi_host_put(us_to_host(us));
824	828	}
825	829
826		-/* Thread to carry out delayed SCSI-device scanning */
827		-static int usb_stor_scan_thread(void * __us)
	830	+/* Delayed-work routine to carry out SCSI-device scanning */
	831	+static void usb_stor_scan_dwork(struct work_struct *work)
828	832	{
829		- struct us_data us = (struct us_data )__us;
	833	+ struct us_data *us = container_of(work, struct us_data,
	834	+ scan_dwork.work);
830	835	struct device *dev = &us->pusb_intf->dev;
831	836
832		- dev_dbg(dev, "device found\n");
	837	+ dev_dbg(dev, "starting scan\n");
833	838
834		- set_freezable();
835		-
836		- /*
837		- * Wait for the timeout to expire or for a disconnect
838		- *
839		- * We can't freeze in this thread or we risk causing khubd to
840		- * fail to freeze, but we can't be non-freezable either. Nor can
841		- * khubd freeze while waiting for scanning to complete as it may
842		- * hold the device lock, causing a hang when suspending devices.
843		- * So instead of using wait_event_freezable(), explicitly test
844		- * for (DONT_SCAN \|\| freezing) in interruptible wait and proceed
845		- * if any of DONT_SCAN, freezing or timeout has happened.
846		- */
847		- if (delay_use > 0) {
848		- dev_dbg(dev, "waiting for device to settle "
849		- "before scanning\n");
850		- wait_event_interruptible_timeout(us->delay_wait,
851		- test_bit(US_FLIDX_DONT_SCAN, &us->dflags) \|\|
852		- freezing(current), delay_use * HZ);
	839	+ /* For bulk-only devices, determine the max LUN value */
	840	+ if (us->protocol == USB_PR_BULK && !(us->fflags & US_FL_SINGLE_LUN)) {
	841	+ mutex_lock(&us->dev_mutex);
	842	+ us->max_lun = usb_stor_Bulk_max_lun(us);
	843	+ mutex_unlock(&us->dev_mutex);
853	844	}
	845	+ scsi_scan_host(us_to_host(us));
	846	+ dev_dbg(dev, "scan complete\n");
854	847
855		- /* If the device is still connected, perform the scanning */
856		- if (!test_bit(US_FLIDX_DONT_SCAN, &us->dflags)) {
	848	+ /* Should we unbind if no devices were detected? */
857	849
858		- /* For bulk-only devices, determine the max LUN value */
859		- if (us->protocol == USB_PR_BULK &&
860		- !(us->fflags & US_FL_SINGLE_LUN)) {
861		- mutex_lock(&us->dev_mutex);
862		- us->max_lun = usb_stor_Bulk_max_lun(us);
863		- mutex_unlock(&us->dev_mutex);
864		- }
865		- scsi_scan_host(us_to_host(us));
866		- dev_dbg(dev, "scan complete\n");
867		-
868		- /* Should we unbind if no devices were detected? */
869		- }
870		-
871	850	usb_autopm_put_interface(us->pusb_intf);
872		- complete_and_exit(&us->scanning_done, 0);
	851	+ clear_bit(US_FLIDX_SCAN_PENDING, &us->dflags);
873	852	}
874	853
875	854	static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf)
...	...	@@ -916,7 +895,7 @@
916	895	init_completion(&us->cmnd_ready);
917	896	init_completion(&(us->notify));
918	897	init_waitqueue_head(&us->delay_wait);
919		- init_completion(&us->scanning_done);
	898	+ INIT_DELAYED_WORK(&us->scan_dwork, usb_stor_scan_dwork);
920	899
921	900	/* Associate the us_data structure with the USB device */
922	901	result = associate_dev(us, intf);
...	...	@@ -947,7 +926,6 @@
947	926	/* Second part of general USB mass-storage probing */
948	927	int usb_stor_probe2(struct us_data *us)
949	928	{
950		- struct task_struct *th;
951	929	int result;
952	930	struct device *dev = &us->pusb_intf->dev;
953	931
954	932
955	933
...	...	@@ -988,20 +966,14 @@
988	966	goto BadDevice;
989	967	}
990	968
991		- /* Start up the thread for delayed SCSI-device scanning */
992		- th = kthread_create(usb_stor_scan_thread, us, "usb-stor-scan");
993		- if (IS_ERR(th)) {
994		- dev_warn(dev,
995		- "Unable to start the device-scanning thread\n");
996		- complete(&us->scanning_done);
997		- quiesce_and_remove_host(us);
998		- result = PTR_ERR(th);
999		- goto BadDevice;
1000		- }
1001		-
	969	+ /* Submit the delayed_work for SCSI-device scanning */
1002	970	usb_autopm_get_interface_no_resume(us->pusb_intf);
1003		- wake_up_process(th);
	971	+ set_bit(US_FLIDX_SCAN_PENDING, &us->dflags);
1004	972
	973	+ if (delay_use > 0)
	974	+ dev_dbg(dev, "waiting for device to settle before scanning\n");
	975	+ queue_delayed_work(system_freezable_wq, &us->scan_dwork,
	976	+ delay_use * HZ);
1005	977	return 0;
1006	978
1007	979	/* We come here if there are any problems */
...	...	@@ -47,6 +47,7 @@
47	47	#include <linux/blkdev.h>
48	48	#include <linux/completion.h>
49	49	#include <linux/mutex.h>
	50	+#include <linux/workqueue.h>
50	51	#include <scsi/scsi_host.h>
51	52
52	53	struct us_data;
...	...	@@ -72,7 +73,7 @@
72	73	#define US_FLIDX_DISCONNECTING 3 /* disconnect in progress */
73	74	#define US_FLIDX_RESETTING 4 /* device reset in progress */
74	75	#define US_FLIDX_TIMED_OUT 5 /* SCSI midlayer timed out */
75		-#define US_FLIDX_DONT_SCAN 6 /* don't scan (disconnect) */
	76	+#define US_FLIDX_SCAN_PENDING 6 /* scanning not yet done */
76	77	#define US_FLIDX_REDO_READ10 7 /* redo READ(10) command */
77	78	#define US_FLIDX_READ10_WORKED 8 /* previous READ(10) succeeded */
78	79
...	...	@@ -147,8 +148,8 @@
147	148	/* mutual exclusion and synchronization structures */
148	149	struct completion cmnd_ready; /* to sleep thread on */
149	150	struct completion notify; /* thread begin/end */
150		- wait_queue_head_t delay_wait; /* wait during scan, reset */
151		- struct completion scanning_done; /* wait for scan thread */
	151	+ wait_queue_head_t delay_wait; /* wait during reset */
	152	+ struct delayed_work scan_dwork; /* for async scanning */
152	153
153	154	/* subdriver information */
154	155	void extra; / Any extra data */