[PATCH 0/8] floppy: cleanup patches

[PATCH 0/8] floppy: cleanup patches

[Stephen Hemminger]

When going through floppy driver addressing some of the timing
bugs, I also saw these leftover things that ought to be cleaned up.

[PATCH 1/8] floppy: initialize debug jiffies offset

[Stephen Hemminger]

[-- Attachment #1: floppy-debugt.patch --]
[-- Type: text/plain, Size: 527 bytes --]

Set debug jiffies offset at initialization. Avoids wierd values
showing up if debugging enabled.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/block/floppy.c	2010-06-08 23:06:35.142657727 -0700
+++ b/drivers/block/floppy.c	2010-06-08 23:07:59.205810459 -0700
@@ -4175,6 +4175,9 @@ static int __init floppy_init(void)
 	int i, unit, drive;
 	int err, dr;
 
+	set_debugt();
+	interruptjiffies = resultjiffies = jiffies;
+
 #if defined(CONFIG_PPC)
 	if (check_legacy_ioport(FDC1))
 		return -ENODEV;

[PATCH 2/8] floppy: remove unnecessary inlines

[Stephen Hemminger]

[-- Attachment #1: floppy-deinline.patch --]
[-- Type: text/plain, Size: 2111 bytes --]

These routines are all big enough that is better to let
the compiler decide to inline or not.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/block/floppy.c	2010-06-08 14:54:33.726619979 -0700
+++ b/drivers/block/floppy.c	2010-06-08 14:54:39.187135875 -0700
@@ -899,7 +899,7 @@ static int _lock_fdc(int drive, bool int
 	_lock_fdc(drive, interruptible, __LINE__)
 
 /* unlocks the driver */
-static inline void unlock_fdc(void)
+static void unlock_fdc(void)
 {
 	unsigned long flags;
 
@@ -1224,7 +1224,7 @@ static int need_more_output(void)
 /* Set perpendicular mode as required, based on data rate, if supported.
  * 82077 Now tested. 1Mbps data rate only possible with 82077-1.
  */
-static inline void perpendicular_mode(void)
+static void perpendicular_mode(void)
 {
 	unsigned char perp_mode;
 
@@ -3033,7 +3033,7 @@ static inline int fd_copyin(void __user 
 	return copy_from_user(address, param, size) ? -EFAULT : 0;
 }
 
-static inline const char *drive_name(int type, int drive)
+static const char *drive_name(int type, int drive)
 {
 	struct floppy_struct *floppy;
 
@@ -3103,7 +3103,7 @@ static struct cont_t raw_cmd_cont = {
 	.done		= raw_cmd_done
 };
 
-static inline int raw_cmd_copyout(int cmd, void __user *param,
+static int raw_cmd_copyout(int cmd, void __user *param,
 				  struct floppy_raw_cmd *ptr)
 {
 	int ret;
@@ -3148,7 +3148,7 @@ static void raw_cmd_free(struct floppy_r
 	}
 }
 
-static inline int raw_cmd_copyin(int cmd, void __user *param,
+static int raw_cmd_copyin(int cmd, void __user *param,
 				 struct floppy_raw_cmd **rcmd)
 {
 	struct floppy_raw_cmd *ptr;
@@ -3266,7 +3266,7 @@ static int invalidate_drive(struct block
 	return 0;
 }
 
-static inline int set_geometry(unsigned int cmd, struct floppy_struct *g,
+static int set_geometry(unsigned int cmd, struct floppy_struct *g,
 			       int drive, int type, struct block_device *bdev)
 {
 	int cnt;
@@ -3365,7 +3365,7 @@ static int ioctl_table[] = {
 	FDTWADDLE
 };
 
-static inline int normalize_ioctl(int *cmd, int *size)
+static int normalize_ioctl(int *cmd, int *size)
 {
 	int i;
 

[PATCH 3/8] floppy: silence warning during disk test

[Stephen Hemminger]

[-- Attachment #1: floppy-quiet.patch --]
[-- Type: text/plain, Size: 643 bytes --]

The first thing the floppy does is read block 0 to test geometry
and to test for disk presence. If disk is not present this causes a
console warning message about failed I/O.  Set flag to silence.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/block/floppy.c	2010-06-08 14:54:39.187135875 -0700
+++ b/drivers/block/floppy.c	2010-06-08 14:54:40.307241699 -0700
@@ -3829,6 +3829,7 @@ static int __floppy_read_block_0(struct 
 	bio.bi_size = size;
 	bio.bi_bdev = bdev;
 	bio.bi_sector = 0;
+	bio.bi_flags = BIO_QUIET;
 	init_completion(&complete);
 	bio.bi_private = &complete;
 	bio.bi_end_io = floppy_rb0_complete;

[PATCH 4/8] floppy: use atomic type for usage_count

[Stephen Hemminger]

[-- Attachment #1: floppy-usage-atomic.patch --]
[-- Type: text/plain, Size: 4013 bytes --]

The usage_count was being protected by a lock which
was only there to create an atomic counter.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/block/floppy.c	2010-06-08 23:08:20.846622833 -0700
+++ b/drivers/block/floppy.c	2010-06-08 23:08:23.326715957 -0700
@@ -578,7 +578,7 @@ static void reset_fdc(void);
 #define NEED_1_RECAL	-2
 #define NEED_2_RECAL	-3
 
-static int usage_count;
+static atomic_t usage_count = ATOMIC_INIT(0);
 
 /* buffer related variables */
 static int buffer_track = -1;
@@ -860,7 +860,7 @@ static void set_fdc(int drive)
 /* locks the driver */
 static int _lock_fdc(int drive, bool interruptible, int line)
 {
-	if (!usage_count) {
+	if (atomic_read(&usage_count) == 0) {
 		pr_err("Trying to lock fdc while usage count=0 at line %d\n",
 		       line);
 		return -1;
@@ -2941,7 +2941,7 @@ static void do_fd_request(struct request
 		return;
 	}
 
-	if (usage_count == 0) {
+	if (atomic_read(&usage_count) == 0) {
 		pr_info("warning: usage count=0, current_req=%p exiting\n",
 			current_req);
 		pr_info("sect=%ld type=%x flags=%x\n",
@@ -3858,7 +3858,7 @@ static int floppy_revalidate(struct gend
 	if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags) ||
 	    test_bit(FD_VERIFY_BIT, &UDRS->flags) ||
 	    test_bit(drive, &fake_change) || NO_GEOM) {
-		if (usage_count == 0) {
+		if (atomic_read(&usage_count) == 0) {
 			pr_info("VFS: revalidate called on non-open device.\n");
 			return -EFAULT;
 		}
@@ -4357,7 +4357,7 @@ out_unreg_platform_dev:
 	platform_device_unregister(&floppy_device[drive]);
 out_flush_work:
 	flush_scheduled_work();
-	if (usage_count)
+	if (atomic_read(&usage_count))
 		floppy_release_irq_and_dma();
 out_unreg_region:
 	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
@@ -4374,8 +4374,6 @@ out_put_disk:
 	return err;
 }
 
-static DEFINE_SPINLOCK(floppy_usage_lock);
-
 static const struct io_region {
 	int offset;
 	int size;
@@ -4421,14 +4419,8 @@ static void floppy_release_regions(int f
 
 static int floppy_grab_irq_and_dma(void)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&floppy_usage_lock, flags);
-	if (usage_count++) {
-		spin_unlock_irqrestore(&floppy_usage_lock, flags);
+	if (atomic_inc_return(&usage_count) > 1)
 		return 0;
-	}
-	spin_unlock_irqrestore(&floppy_usage_lock, flags);
 
 	/*
 	 * We might have scheduled a free_irq(), wait it to
@@ -4439,9 +4431,7 @@ static int floppy_grab_irq_and_dma(void)
 	if (fd_request_irq()) {
 		DPRINT("Unable to grab IRQ%d for the floppy driver\n",
 		       FLOPPY_IRQ);
-		spin_lock_irqsave(&floppy_usage_lock, flags);
-		usage_count--;
-		spin_unlock_irqrestore(&floppy_usage_lock, flags);
+		atomic_dec(&usage_count);
 		return -1;
 	}
 	if (fd_request_dma()) {
@@ -4451,9 +4441,7 @@ static int floppy_grab_irq_and_dma(void)
 			use_virtual_dma = can_use_virtual_dma = 1;
 		if (!(can_use_virtual_dma & 1)) {
 			fd_free_irq();
-			spin_lock_irqsave(&floppy_usage_lock, flags);
-			usage_count--;
-			spin_unlock_irqrestore(&floppy_usage_lock, flags);
+			atomic_dec(&usage_count);
 			return -1;
 		}
 	}
@@ -4488,9 +4476,7 @@ cleanup:
 	fd_free_dma();
 	while (--fdc >= 0)
 		floppy_release_regions(fdc);
-	spin_lock_irqsave(&floppy_usage_lock, flags);
-	usage_count--;
-	spin_unlock_irqrestore(&floppy_usage_lock, flags);
+	atomic_dec(&usage_count);
 	return -1;
 }
 
@@ -4502,14 +4488,10 @@ static void floppy_release_irq_and_dma(v
 #endif
 	long tmpsize;
 	unsigned long tmpaddr;
-	unsigned long flags;
 
-	spin_lock_irqsave(&floppy_usage_lock, flags);
-	if (--usage_count) {
-		spin_unlock_irqrestore(&floppy_usage_lock, flags);
+	if (!atomic_dec_and_test(&usage_count))
 		return;
-	}
-	spin_unlock_irqrestore(&floppy_usage_lock, flags);
+
 	if (irqdma_allocated) {
 		fd_disable_dma();
 		fd_free_dma();
@@ -4602,7 +4584,7 @@ static void __exit floppy_module_exit(vo
 	del_timer_sync(&fd_timer);
 	blk_cleanup_queue(floppy_queue);
 
-	if (usage_count)
+	if (atomic_read(&usage_count))
 		floppy_release_irq_and_dma();
 
 	/* eject disk, if any */

[PATCH 5/8] floppy: cmos attribute should be static

[Stephen Hemminger]

[-- Attachment #1: floppy-cmos-local.patch --]
[-- Type: text/plain, Size: 522 bytes --]

As reported by sparse, cmos attribute is local.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/block/floppy.c	2010-06-08 23:08:23.326715957 -0700
+++ b/drivers/block/floppy.c	2010-06-08 23:08:29.136934127 -0700
@@ -4127,7 +4127,7 @@ static ssize_t floppy_cmos_show(struct d
 	return sprintf(buf, "%X\n", UDP->cmos);
 }
 
-DEVICE_ATTR(cmos, S_IRUGO, floppy_cmos_show, NULL);
+static DEVICE_ATTR(cmos, S_IRUGO, floppy_cmos_show, NULL);
 
 static void floppy_device_release(struct device *dev)
 {

[PATCH 6/8] floppy: fix signed/unsigned warnings

[Stephen Hemminger]

[-- Attachment #1: floppy-sparse-signed.patch --]
[-- Type: text/plain, Size: 640 bytes --]

Ioctl cmd value is unsigned, so change normalize_ioctl

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/block/floppy.c	2010-06-08 23:08:29.136934127 -0700
+++ b/drivers/block/floppy.c	2010-06-08 23:08:32.697067816 -0700
@@ -3337,7 +3337,7 @@ static int set_geometry(unsigned int cmd
 }
 
 /* handle obsolete ioctl's */
-static int ioctl_table[] = {
+static unsigned int ioctl_table[] = {
 	FDCLRPRM,
 	FDSETPRM,
 	FDDEFPRM,
@@ -3365,7 +3365,7 @@ static int ioctl_table[] = {
 	FDTWADDLE
 };
 
-static int normalize_ioctl(int *cmd, int *size)
+static int normalize_ioctl(unsigned int *cmd, int *size)
 {
 	int i;
 

[PATCH 7/8] floppy: use wait_event_interruptible

[Stephen Hemminger]

[-- Attachment #1: floppy-wait-event.patch --]
[-- Type: text/plain, Size: 2526 bytes --]

Convert wait loops to use wait_event_ macros.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/block/floppy.c	2010-06-09 11:34:11.326812498 -0700
+++ b/drivers/block/floppy.c	2010-06-09 11:34:12.416645239 -0700
@@ -514,8 +514,6 @@ static unsigned long fdc_busy;
 static DECLARE_WAIT_QUEUE_HEAD(fdc_wait);
 static DECLARE_WAIT_QUEUE_HEAD(command_done);
 
-#define NO_SIGNAL (!interruptible || !signal_pending(current))
-
 /* Errors during formatting are counted here. */
 static int format_errors;
 
@@ -858,36 +856,15 @@ static void set_fdc(int drive)
 }
 
 /* locks the driver */
-static int _lock_fdc(int drive, bool interruptible, int line)
+static int lock_fdc(int drive, bool interruptible)
 {
-	if (atomic_read(&usage_count) == 0) {
-		pr_err("Trying to lock fdc while usage count=0 at line %d\n",
-		       line);
+	if (WARN(atomic_read(&usage_count) == 0,
+		 "Trying to lock fdc while usage count=0\n"))
 		return -1;
-	}
 
-	if (test_and_set_bit(0, &fdc_busy)) {
-		DECLARE_WAITQUEUE(wait, current);
-		add_wait_queue(&fdc_wait, &wait);
-
-		for (;;) {
-			set_current_state(TASK_INTERRUPTIBLE);
-
-			if (!test_and_set_bit(0, &fdc_busy))
-				break;
-
-			schedule();
-
-			if (!NO_SIGNAL) {
-				remove_wait_queue(&fdc_wait, &wait);
-				return -EINTR;
-			}
-		}
+	if (wait_event_interruptible(fdc_wait, !test_and_set_bit(0, &fdc_busy)))
+		return -EINTR;
 
-		set_current_state(TASK_RUNNING);
-		remove_wait_queue(&fdc_wait, &wait);
-		flush_scheduled_work();
-	}
 	command_status = FD_COMMAND_NONE;
 
 	__reschedule_timeout(drive, "lock fdc");
@@ -895,9 +872,6 @@ static int _lock_fdc(int drive, bool int
 	return 0;
 }
 
-#define lock_fdc(drive, interruptible)			\
-	_lock_fdc(drive, interruptible, __LINE__)
-
 /* unlocks the driver */
 static void unlock_fdc(void)
 {
@@ -2015,25 +1989,10 @@ static int wait_til_done(void (*handler)
 
 	schedule_bh(handler);
 
-	if (command_status < 2 && NO_SIGNAL) {
-		DECLARE_WAITQUEUE(wait, current);
-
-		add_wait_queue(&command_done, &wait);
-		for (;;) {
-			set_current_state(interruptible ?
-					  TASK_INTERRUPTIBLE :
-					  TASK_UNINTERRUPTIBLE);
-
-			if (command_status >= 2 || !NO_SIGNAL)
-				break;
-
-			is_alive(__func__, "");
-			schedule();
-		}
-
-		set_current_state(TASK_RUNNING);
-		remove_wait_queue(&command_done, &wait);
-	}
+	if (interruptible)
+		wait_event_interruptible(command_done, command_status >= 2);
+	else
+		wait_event(command_done, command_status >= 2);
 
 	if (command_status < 2) {
 		cancel_activity();

[PATCH 8/8] floppy: use warning macros

[Stephen Hemminger]

[-- Attachment #1: floppy-warn.patch --]
[-- Type: text/plain, Size: 2215 bytes --]

Convert assertions to use WARN().  There are several error checks
in the code for things that should never happen. Convert them to
standard warnings so kerneloops.org will see them.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/block/floppy.c	2010-06-08 08:08:36.363245707 -0700
+++ b/drivers/block/floppy.c	2010-06-08 08:08:40.360468297 -0700
@@ -2532,10 +2532,8 @@ static int make_raw_rw_request(void)
 	int tracksize;
 	int ssize;
 
-	if (max_buffer_sectors == 0) {
-		pr_info("VFS: Block I/O scheduled on unopened device\n");
+	if (WARN(max_buffer_sectors == 0, "VFS: Block I/O scheduled on unopened device\n"))
 		return 0;
-	}
 
 	set_fdc((long)current_req->rq_disk->private_data);
 
@@ -2895,19 +2893,16 @@ static void process_fd_request(void)
 
 static void do_fd_request(struct request_queue *q)
 {
-	if (max_buffer_sectors == 0) {
-		pr_info("VFS: %s called on non-open device\n", __func__);
+	if (WARN(max_buffer_sectors == 0,
+		 "VFS: %s called on non-open device\n", __func__))
 		return;
-	}
 
-	if (atomic_read(&usage_count) == 0) {
-		pr_info("warning: usage count=0, current_req=%p exiting\n",
-			current_req);
-		pr_info("sect=%ld type=%x flags=%x\n",
-			(long)blk_rq_pos(current_req), current_req->cmd_type,
-			current_req->cmd_flags);
+	if (WARN(atomic_read(&usage_count) == 0,
+		 "warning: usage count=0, current_req=%p sect=%ld type=%x flags=%x\n",
+		 current_req, (long)blk_rq_pos(current_req), current_req->cmd_type,
+		 current_req->cmd_flags))
 		return;
-	}
+
 	if (test_bit(0, &fdc_busy)) {
 		/* fdc busy, this new request will be treated when the
 		   current one is done */
@@ -3817,10 +3812,10 @@ static int floppy_revalidate(struct gend
 	if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags) ||
 	    test_bit(FD_VERIFY_BIT, &UDRS->flags) ||
 	    test_bit(drive, &fake_change) || NO_GEOM) {
-		if (atomic_read(&usage_count) == 0) {
-			pr_info("VFS: revalidate called on non-open device.\n");
+		if (WARN(atomic_read(&usage_count) == 0,
+			 "VFS: revalidate called on non-open device.\n"))
 			return -EFAULT;
-		}
+
 		lock_fdc(drive, false);
 		cf = (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags) ||
 		      test_bit(FD_VERIFY_BIT, &UDRS->flags));

Re: [PATCH 0/8] floppy: cleanup patches

[Linus Torvalds]

On Wed, 9 Jun 2010, Stephen Hemminger wrote:
>
> When going through floppy driver addressing some of the timing
> bugs, I also saw these leftover things that ought to be cleaned up.

Ack on them all as far as I'm concerned. I suspect the _real_ bugs in 
floppy.c are about various paths not holding the floppy_lock spinlock, but 
the patches all look fine and certainly don't make anything worse.

Mind re-sending them after 2.6.35, though? (Or maybe somebody like Greg 
wants to queue them up in their drievr trees)

			Linus

Re: [PATCH 0/8] floppy: cleanup patches

[Stephen Hemminger]

On Wed, 9 Jun 2010 11:46:13 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Wed, 9 Jun 2010, Stephen Hemminger wrote:
> >
> > When going through floppy driver addressing some of the timing
> > bugs, I also saw these leftover things that ought to be cleaned up.
> 
> Ack on them all as far as I'm concerned. I suspect the _real_ bugs in 
> floppy.c are about various paths not holding the floppy_lock spinlock, but 
> the patches all look fine and certainly don't make anything worse.
> 
> Mind re-sending them after 2.6.35, though? (Or maybe somebody like Greg 
> wants to queue them up in their drievr trees)
> 
> 			Linus

Current plan is to convert all the timers and old BH code to go through
a single workqueue. This avoids all the races and cleans up the cancellation
logic as well without having lots of other changes.

-- 

Re: [PATCH 0/8] floppy: cleanup patches

[Linus Torvalds]

On Wed, 9 Jun 2010, Stephen Hemminger wrote:
> 
> Current plan is to convert all the timers and old BH code to go through
> a single workqueue. This avoids all the races and cleans up the cancellation
> logic as well without having lots of other changes.

That will definitely help. We still end up with the things that call the 
request queue directly as a second (nonworkqueue) thread, but I think 
that's what the floppy_lock has _traditionally_ protected against, so it 
might all work out and largely fix the locking.

			Linus

Re: [PATCH 0/8] floppy: cleanup patches

[Thomas Meyer]

Am Mittwoch, den 09.06.2010, 11:34 -0700 schrieb Stephen Hemminger:
> When going through floppy driver addressing some of the timing
> bugs, I also saw these leftover things that ought to be cleaned up.
> 

With your patches applied on top of 2.6.34 I still get this timeout
while resuming from ram:

[ 1086.971283] ata2.00: configured for UDMA/133
[ 1089.986024] 
[ 1089.986026] floppy driver state
[ 1089.986026] -------------------
[ 1089.986032] now=4295757282 last interrupt=4295281727 diff=475555 last called handler=main_command_interrupt
[ 1089.986033] timeout_message=lock fdc
[ 1089.986034] last output bytes:
[ 1089.986035]  2 90 4295192821
[ 1089.986036]  1 90 4295192821
[ 1089.986037]  d 90 4295192821
[ 1089.986038]  2 90 4295192821
[ 1089.986039]  e 90 4295192821
[ 1089.986040] 1b 90 4295192821
[ 1089.986041] ff 90 4295192821
[ 1089.986042]  f 80 4295281121
[ 1089.986043]  0 90 4295281121
[ 1089.986044]  0 91 4295281121
[ 1089.986045]  8 81 4295281129
[ 1089.986046] 45 80 4295281499
[ 1089.986047]  0 90 4295281499
[ 1089.986048]  0 90 4295281499
[ 1089.986049]  0 90 4295281499
[ 1089.986050]  3 90 4295281499
[ 1089.986051]  2 90 4295281499
[ 1089.986052]  4 90 4295281499
[ 1089.986053] 1b 90 4295281499
[ 1089.986054] ff 90 4295281499
[ 1089.986055] last result at 4295281741
[ 1089.986056] last redo_fd_request at 4295281741
[ 1089.986060] status=a
[ 1089.986061] fdc_busy=1
[ 1089.986063] do_floppy=reset_interrupt
[ 1089.986064] cont=ffffffffa0009ae0
[ 1089.986065] current_req=(null)
[ 1089.986066] command_status=-1
[ 1089.986067] 
[ 1089.986069] floppy0: floppy timeout called
[ 1089.986094] PM: resume of devices complete after 10536.229 msecs
[ 1089.986319] PM: resume devices took 10.537 seconds
[ 1089.986320] ------------[ cut here ]------------
[ 1089.986325] WARNING: at kernel/power/suspend_test.c:53 suspend_devices_and_enter+0xd2/0x220()
[ 1089.986326] Hardware name: MS-7250
[ 1089.986327] Component: resume devices, time: 10537
[ 1089.986328] Modules linked in: rndis_wlan olympic forcedeth floppy [last unloaded: scsi_wait_scan]
[ 1089.986333] Pid: 15510, comm: pm-suspend Not tainted 2.6.34 #1
[ 1089.986334] Call Trace:
[ 1089.986340]  [<ffffffff81063283>] ? warn_slowpath_common+0x73/0xb0
[ 1089.986343]  [<ffffffff81063320>] ? warn_slowpath_fmt+0x40/0x50
[ 1089.986345]  [<ffffffff81095132>] ? suspend_devices_and_enter+0xd2/0x220
[ 1089.986348]  [<ffffffff810953a8>] ? enter_state+0x128/0x150
[ 1089.986350]  [<ffffffff810949cd>] ? state_store+0x8d/0xf0
[ 1089.986353]  [<ffffffff81165215>] ? sysfs_write_file+0xd5/0x160
[ 1089.986356]  [<ffffffff811029e8>] ? vfs_write+0xb8/0x1a0
[ 1089.986360]  [<ffffffff810aff51>] ? audit_syscall_entry+0x1c1/0x1f0
[ 1089.986362]  [<ffffffff811032ce>] ? sys_write+0x4e/0x90
[ 1089.986365]  [<ffffffff81027e6b>] ? system_call_fastpath+0x16/0x1b
[ 1089.986367] ---[ end trace 210bc7fac7be9fbd ]---
[ 1089.986432] PM: Finishing wakeup.
[ 1089.986433] Restarting tasks ... 

Is this okay?

Re: [PATCH 0/8] floppy: cleanup patches

[Stephen Hemminger]

On Thu, 10 Jun 2010 08:43:32 +0200
Thomas Meyer <thomas@m3y3r.de> wrote:

> Am Mittwoch, den 09.06.2010, 11:34 -0700 schrieb Stephen Hemminger:
> > When going through floppy driver addressing some of the timing
> > bugs, I also saw these leftover things that ought to be cleaned up.
> >   
> 
> With your patches applied on top of 2.6.34 I still get this timeout
> while resuming from ram:

Not clear, did the problem occur before my patches?  The posted set
of patches should not change visible functionalty or fix any observable
bug. Floppy driver would be bug for bug the same.



-- 

[RFC] floppy: use single threaded workqueue

[Stephen Hemminger]

Fix races between timers and bottom half by using delayed work
and a single threaded queue.

Still needs more testing.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 drivers/block/floppy.c |  172 ++++++++++++++++++++++++++-----------------------
 1 file changed, 94 insertions(+), 78 deletions(-)

--- a/drivers/block/floppy.c	2010-06-11 09:47:59.000000000 -0700
+++ b/drivers/block/floppy.c	2010-06-11 10:06:01.451555907 -0700
@@ -553,7 +553,7 @@ static void floppy_ready(void);
 static void floppy_start(void);
 static void process_fd_request(void);
 static void recalibrate_floppy(void);
-static void floppy_shutdown(unsigned long);
+static void floppy_shutdown(struct work_struct *);
 
 static int floppy_request_regions(int);
 static void floppy_release_regions(int);
@@ -590,6 +590,8 @@ static int buffer_max = -1;
 static struct floppy_fdc_state fdc_state[N_FDC];
 static int fdc;			/* current fdc */
 
+static struct workqueue_struct *floppy_workq;
+
 static struct floppy_struct *_floppy = floppy_type;
 static unsigned char current_drive;
 static long current_count_sectors;
@@ -626,16 +628,15 @@ static inline void set_debugt(void) { }
 static inline void debugt(const char *func, const char *msg) { }
 #endif /* DEBUGT */
 
-typedef void (*timeout_fn)(unsigned long);
-static DEFINE_TIMER(fd_timeout, floppy_shutdown, 0, 0);
 
+static DECLARE_DELAYED_WORK(fd_timeout, floppy_shutdown);
 static const char *timeout_message;
 
 static void is_alive(const char *func, const char *message)
 {
 	/* this routine checks whether the floppy driver is "alive" */
 	if (test_bit(0, &fdc_busy) && command_status < 2 &&
-	    !timer_pending(&fd_timeout)) {
+	    !delayed_work_pending(&fd_timeout)) {
 		DPRINT("%s: timeout handler died.  %s\n", func, message);
 	}
 }
@@ -663,15 +664,18 @@ static int output_log_pos;
 
 static void __reschedule_timeout(int drive, const char *message)
 {
+	unsigned long delay;
+
 	if (drive == current_reqD)
 		drive = current_drive;
-	del_timer(&fd_timeout);
+
 	if (drive < 0 || drive >= N_DRIVE) {
-		fd_timeout.expires = jiffies + 20UL * HZ;
+		delay = 20UL * HZ;
 		drive = 0;
 	} else
-		fd_timeout.expires = jiffies + UDP->timeout;
-	add_timer(&fd_timeout);
+		delay = UDP->timeout;
+
+	queue_delayed_work(floppy_workq, &fd_timeout, delay);
 	if (UDP->flags & FD_DEBUG)
 		DPRINT("reschedule timeout %s\n", message);
 	timeout_message = message;
@@ -886,11 +890,11 @@ static int _lock_fdc(int drive, bool int
 
 		set_current_state(TASK_RUNNING);
 		remove_wait_queue(&fdc_wait, &wait);
-		flush_scheduled_work();
+		flush_workqueue(floppy_workq);
 	}
 	command_status = FD_COMMAND_NONE;
 
-	__reschedule_timeout(drive, "lock fdc");
+	reschedule_timeout(drive, "lock fdc");
 	set_fdc(drive);
 	return 0;
 }
@@ -901,23 +905,15 @@ static int _lock_fdc(int drive, bool int
 /* unlocks the driver */
 static void unlock_fdc(void)
 {
-	unsigned long flags;
-
-	raw_cmd = NULL;
 	if (!test_bit(0, &fdc_busy))
 		DPRINT("FDC access conflict!\n");
 
-	if (do_floppy)
-		DPRINT("device interrupt still active at FDC release: %pf!\n",
-		       do_floppy);
+	raw_cmd = NULL;
 	command_status = FD_COMMAND_NONE;
-	spin_lock_irqsave(&floppy_lock, flags);
-	del_timer(&fd_timeout);
+	__cancel_delayed_work(&fd_timeout);
+	do_floppy = NULL;
 	cont = NULL;
 	clear_bit(0, &fdc_busy);
-	if (current_req || blk_peek_request(floppy_queue))
-		do_fd_request(floppy_queue);
-	spin_unlock_irqrestore(&floppy_lock, flags);
 	wake_up(&fdc_wait);
 }
 
@@ -989,26 +985,24 @@ static DECLARE_WORK(floppy_work, NULL);
 
 static void schedule_bh(void (*handler)(void))
 {
+	WARN_ON(work_pending(&floppy_work));
+
 	PREPARE_WORK(&floppy_work, (work_func_t)handler);
-	schedule_work(&floppy_work);
+	queue_work(floppy_workq, &floppy_work);
 }
 
-static DEFINE_TIMER(fd_timer, NULL, 0, 0);
+static DECLARE_DELAYED_WORK(fd_timer, NULL);
 
 static void cancel_activity(void)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&floppy_lock, flags);
 	do_floppy = NULL;
-	PREPARE_WORK(&floppy_work, (work_func_t)empty);
-	del_timer(&fd_timer);
-	spin_unlock_irqrestore(&floppy_lock, flags);
+	cancel_delayed_work_sync(&fd_timer);
+	cancel_work_sync(&floppy_work);
 }
 
 /* this function makes sure that the disk stays in the drive during the
  * transfer */
-static void fd_watchdog(void)
+static void fd_watchdog(struct work_struct *arg)
 {
 	debug_dcl(DP->flags, "calling disk change from watchdog\n");
 
@@ -1018,21 +1012,21 @@ static void fd_watchdog(void)
 		cont->done(0);
 		reset_fdc();
 	} else {
-		del_timer(&fd_timer);
-		fd_timer.function = (timeout_fn)fd_watchdog;
-		fd_timer.expires = jiffies + HZ / 10;
-		add_timer(&fd_timer);
+		cancel_delayed_work(&fd_timer);
+		PREPARE_DELAYED_WORK(&fd_timer, fd_watchdog);
+		queue_delayed_work(floppy_workq,
+				   &fd_timer, HZ / 10);
 	}
 }
 
 static void main_command_interrupt(void)
 {
-	del_timer(&fd_timer);
+	cancel_delayed_work(&fd_timer);
 	cont->interrupt();
 }
 
 /* waits for a delay (spinup or select) to pass */
-static int fd_wait_for_completion(unsigned long delay, timeout_fn function)
+static int fd_wait_for_completion(unsigned long expires, work_func_t function)
 {
 	if (FDCS->reset) {
 		reset_fdc();	/* do the reset during sleep to win time
@@ -1041,11 +1035,11 @@ static int fd_wait_for_completion(unsign
 		return 1;
 	}
 
-	if (time_before(jiffies, delay)) {
-		del_timer(&fd_timer);
-		fd_timer.function = function;
-		fd_timer.expires = delay;
-		add_timer(&fd_timer);
+	if (time_before(jiffies, expires)) {
+		cancel_delayed_work(&fd_timer);
+		PREPARE_DELAYED_WORK(&fd_timer, function);
+		queue_delayed_work(floppy_workq, &fd_timer,
+				   expires - jiffies);
 		return 1;
 	}
 	return 0;
@@ -1394,7 +1388,7 @@ static int fdc_dtr(void)
 	 */
 	FDCS->dtr = raw_cmd->rate & 3;
 	return fd_wait_for_completion(jiffies + 2UL * HZ / 100,
-				      (timeout_fn)floppy_ready);
+				      (work_func_t)floppy_ready);
 }				/* fdc_dtr */
 
 static void tell_sector(void)
@@ -1499,7 +1493,7 @@ static void setup_rw_floppy(void)
 	int flags;
 	int dflags;
 	unsigned long ready_date;
-	timeout_fn function;
+	work_func_t function;
 
 	flags = raw_cmd->flags;
 	if (flags & (FD_RAW_READ | FD_RAW_WRITE))
@@ -1513,9 +1507,9 @@ static void setup_rw_floppy(void)
 		 */
 		if (time_after(ready_date, jiffies + DP->select_delay)) {
 			ready_date -= DP->select_delay;
-			function = (timeout_fn)floppy_start;
+			function = (work_func_t)floppy_start;
 		} else
-			function = (timeout_fn)setup_rw_floppy;
+			function = (work_func_t)setup_rw_floppy;
 
 		/* wait until the floppy is spinning fast enough */
 		if (fd_wait_for_completion(ready_date, function))
@@ -1545,7 +1539,7 @@ static void setup_rw_floppy(void)
 		inr = result();
 		cont->interrupt();
 	} else if (flags & FD_RAW_NEED_DISK)
-		fd_watchdog();
+		fd_watchdog(NULL);
 }
 
 static int blind_seek;
@@ -1855,20 +1849,22 @@ static void show_floppy(void)
 		pr_info("do_floppy=%pf\n", do_floppy);
 	if (work_pending(&floppy_work))
 		pr_info("floppy_work.func=%pf\n", floppy_work.func);
-	if (timer_pending(&fd_timer))
-		pr_info("fd_timer.function=%pf\n", fd_timer.function);
-	if (timer_pending(&fd_timeout)) {
-		pr_info("timer_function=%pf\n", fd_timeout.function);
-		pr_info("expires=%lu\n", fd_timeout.expires - jiffies);
-		pr_info("now=%lu\n", jiffies);
-	}
+	if (delayed_work_pending(&fd_timer))
+		pr_info("delayed work.function=%p expires=%ld\n",
+		       fd_timer.work.func,
+		       fd_timer.timer.expires - jiffies);
+	if (delayed_work_pending(&fd_timeout))
+		pr_info("timer_function=%p expires=%ld\n",
+		       fd_timeout.work.func,
+		       fd_timeout.timer.expires - jiffies);
+
 	pr_info("cont=%p\n", cont);
 	pr_info("current_req=%p\n", current_req);
 	pr_info("command_status=%d\n", command_status);
 	pr_info("\n");
 }
 
-static void floppy_shutdown(unsigned long data)
+static void floppy_shutdown(struct work_struct *arg)
 {
 	unsigned long flags;
 
@@ -1923,7 +1919,7 @@ static int start_motor(void (*function)(
 
 	/* wait_for_completion also schedules reset if needed. */
 	return fd_wait_for_completion(DRS->select_date + DP->select_delay,
-				      (timeout_fn)function);
+				      (work_func_t)function);
 }
 
 static void floppy_ready(void)
@@ -2853,6 +2849,20 @@ static int make_raw_rw_request(void)
 	return 2;
 }
 
+static struct request *get_fd_request(void)
+{
+	struct request *req;
+
+	spin_lock_bh(floppy_queue->queue_lock);
+	req = blk_fetch_request(floppy_queue);
+	if (!req)
+		unlock_fdc();
+	spin_unlock_bh(floppy_queue->queue_lock);
+
+	return req;
+}
+
+
 static void redo_fd_request(void)
 {
 	int drive;
@@ -2864,16 +2874,10 @@ static void redo_fd_request(void)
 
 do_request:
 	if (!current_req) {
-		struct request *req;
+		struct request *req = get_fd_request();
 
-		spin_lock_irq(floppy_queue->queue_lock);
-		req = blk_fetch_request(floppy_queue);
-		spin_unlock_irq(floppy_queue->queue_lock);
-		if (!req) {
-			do_floppy = NULL;
-			unlock_fdc();
+		if (!req)
 			return;
-		}
 		current_req = req;
 	}
 	drive = (long)current_req->rq_disk->private_data;
@@ -2949,13 +2953,17 @@ static void do_fd_request(struct request
 			current_req->cmd_flags);
 		return;
 	}
-	if (test_bit(0, &fdc_busy)) {
+	if (test_and_set_bit(0, &fdc_busy)) {
 		/* fdc busy, this new request will be treated when the
 		   current one is done */
 		is_alive(__func__, "old request running");
 		return;
 	}
-	lock_fdc(MAXTIMEOUT, false);
+
+	command_status = FD_COMMAND_NONE;
+
+	__reschedule_timeout(MAXTIMEOUT, "fd_request");
+	set_fdc(0);
 	process_fd_request();
 	is_alive(__func__, "");
 }
@@ -4211,10 +4219,16 @@ static int __init floppy_init(void)
 	if (err)
 		goto out_unreg_blkdev;
 
+	floppy_workq = create_singlethread_workqueue("floppy");
+	if (!floppy_workq) {
+		err = -ENOMEM;
+		goto out_unreg_driver;
+	}
+
 	floppy_queue = blk_init_queue(do_fd_request, &floppy_lock);
 	if (!floppy_queue) {
 		err = -ENOMEM;
-		goto out_unreg_driver;
+		goto out_destroy_workq;
 	}
 	blk_queue_max_hw_sectors(floppy_queue, 64);
 
@@ -4247,7 +4261,7 @@ static int __init floppy_init(void)
 	use_virtual_dma = can_use_virtual_dma & 1;
 	fdc_state[0].address = FDC1;
 	if (fdc_state[0].address == -1) {
-		del_timer(&fd_timeout);
+		cancel_delayed_work(&fd_timeout);
 		err = -ENODEV;
 		goto out_unreg_region;
 	}
@@ -4258,7 +4272,7 @@ static int __init floppy_init(void)
 	fdc = 0;		/* reset fdc in case of unexpected interrupt */
 	err = floppy_grab_irq_and_dma();
 	if (err) {
-		del_timer(&fd_timeout);
+		cancel_delayed_work(&fd_timeout);
 		err = -EBUSY;
 		goto out_unreg_region;
 	}
@@ -4315,13 +4329,13 @@ static int __init floppy_init(void)
 		user_reset_fdc(-1, FD_RESET_ALWAYS, false);
 	}
 	fdc = 0;
-	del_timer(&fd_timeout);
+	cancel_delayed_work(&fd_timeout);
 	current_drive = 0;
 	initialized = true;
 	if (have_no_fdc) {
 		DPRINT("no floppy controllers found\n");
 		err = have_no_fdc;
-		goto out_flush_work;
+		goto out_release_dma;
 	}
 
 	for (drive = 0; drive < N_DRIVE; drive++) {
@@ -4336,7 +4350,7 @@ static int __init floppy_init(void)
 
 		err = platform_device_register(&floppy_device[drive]);
 		if (err)
-			goto out_flush_work;
+			goto out_release_dma;
 
 		err = device_create_file(&floppy_device[drive].dev,
 					 &dev_attr_cmos);
@@ -4355,13 +4369,14 @@ static int __init floppy_init(void)
 
 out_unreg_platform_dev:
 	platform_device_unregister(&floppy_device[drive]);
-out_flush_work:
-	flush_scheduled_work();
+out_release_dma:
 	if (atomic_read(&usage_count))
 		floppy_release_irq_and_dma();
 out_unreg_region:
 	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
 	blk_cleanup_queue(floppy_queue);
+out_destroy_workq:
+	destroy_workqueue(floppy_workq);
 out_unreg_driver:
 	platform_driver_unregister(&floppy_driver);
 out_unreg_blkdev:
@@ -4426,7 +4441,7 @@ static int floppy_grab_irq_and_dma(void)
 	 * We might have scheduled a free_irq(), wait it to
 	 * drain first:
 	 */
-	flush_scheduled_work();
+	flush_workqueue(floppy_workq);
 
 	if (fd_request_irq()) {
 		DPRINT("Unable to grab IRQ%d for the floppy driver\n",
@@ -4518,9 +4533,9 @@ static void floppy_release_irq_and_dma(v
 			pr_info("motor off timer %d still active\n", drive);
 #endif
 
-	if (timer_pending(&fd_timeout))
+	if (delayed_work_pending(&fd_timeout))
 		pr_info("floppy timer still active:%s\n", timeout_message);
-	if (timer_pending(&fd_timer))
+	if (delayed_work_pending(&fd_timer))
 		pr_info("auxiliary floppy timer still active\n");
 	if (work_pending(&floppy_work))
 		pr_info("work still pending\n");
@@ -4580,8 +4595,9 @@ static void __exit floppy_module_exit(vo
 		put_disk(disks[drive]);
 	}
 
-	del_timer_sync(&fd_timeout);
-	del_timer_sync(&fd_timer);
+	cancel_delayed_work_sync(&fd_timeout);
+	cancel_delayed_work_sync(&fd_timer);
+	destroy_workqueue(floppy_workq);
 	blk_cleanup_queue(floppy_queue);
 
 	if (atomic_read(&usage_count))

Re: [RFC] floppy: use single threaded workqueue

[Linus Torvalds]

On Fri, Jun 11, 2010 at 10:32 AM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> Fix races between timers and bottom half by using delayed work
> and a single threaded queue.
>
> Still needs more testing.

Looks simple and straightforward to me. Ack. (And ack on the "needs
more testing" part too, of course).

                  Linus

Re: [PATCH 0/8] floppy: cleanup patches

[Thomas Meyer]

----- Original Message ----- 
From: "Stephen Hemminger" <shemminger@vyatta.com>
> On Thu, 10 Jun 2010 08:43:32 +0200
> Thomas Meyer <thomas@m3y3r.de> wrote:
> 
>> Am Mittwoch, den 09.06.2010, 11:34 -0700 schrieb Stephen Hemminger:
>> > When going through floppy driver addressing some of the timing
>> > bugs, I also saw these leftover things that ought to be cleaned up.
>> >   
>> 
>> With your patches applied on top of 2.6.34 I still get this timeout
>> while resuming from ram:
> 
> Not clear, did the problem occur before my patches?

Yes, the timeout occured before your patches, i.e. plain 2.6.34.

> The posted set
> of patches should not change visible functionalty or fix any observable
> bug. Floppy driver would be bug for bug the same.

I guess, it's okay then.

kind regards
thomas

Re: [RFC] floppy: use single threaded workqueue

[Marcelo Tosatti]

On Fri, Jun 11, 2010 at 10:32:23AM -0700, Stephen Hemminger wrote:
> Fix races between timers and bottom half by using delayed work
> and a single threaded queue.
> 
> Still needs more testing.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
>  drivers/block/floppy.c |  172 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 94 insertions(+), 78 deletions(-)

Stephen,

Can confirm this fixes the following oops seen under KVM guests.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000035
IP: [<ffffffff812141f0>] setup_rw_floppy+0x229/0x2ca
PGD 284b8067 PUD 3d79f067 PMD 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/devices/pci0000:00/0000:00:03.0/resource
CPU 1
Modules linked in: xt_tcpudp iptable_filter ip_tables x_tables
scsi_wait_scan uhci_hcd ohci_hcd ehci_hcd

Pid: 28, comm: events/1 Not tainted 2.6.35-rc5-35033-g14e84ff #221
/Bochs RIP: 0010:[<ffffffff812141f0>]  [<ffffffff812141f0>] setup_rw_floppy+0x229/0x2ca
RSP: 0018:ffff88003e1a1e60  EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000009 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000286 RDI: 0000000000000000
RBP: 00000000000000da R08: 0000000000000000 R09: ffffffff81214bdb
R10: 0000000000000000 R11: ffffffff817f7660 R12: 0000000000000000
R13: 0000000000000008 R14: ffffffff81214bdb R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880001a40000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000035 CR3: 000000003a428000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process events/1 (pid: 28, threadinfo ffff88003e1a0000, task
ffff88003e19a8f0)
Stack:
 0000000000000000 ffff88003e1a1ef8 ffffffff81692f00 ffff880001a56680
<0> ffff88003e19a8f0 ffffffff810466f1 0000000000000000 ffff88003e19a8f0
<0> ffffffff810495ad ffff88003e1a1ea8 ffff88003e1a1ea8 ffff88003e1a1ef8
Call Trace:
 [<ffffffff810466f1>] ? worker_thread+0x150/0x1f0
 [<ffffffff810495ad>] ? autoremove_wake_function+0x0/0x2e
 [<ffffffff810465a1>] ? worker_thread+0x0/0x1f0
 [<ffffffff810492bf>] ? kthread+0x79/0x81
 [<ffffffff81002be4>] ? kernel_thread_helper+0x4/0x10

Re: [RFC] floppy: use single threaded workqueue

[Linus Torvalds]

On Wed, Jul 21, 2010 at 12:55 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
>
> Can confirm this fixes the following oops seen under KVM guests.
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000035

Hmm. That does make me want to merge it early, but at the same time I
suspect it has gotten almost no testing on real hardware. So despite
looking "obvious" and fixing one bug, I'm a bit nervous and I suspect
that the prudent thing to do is still to take it early in the 2.3.36
merge window, and just mark it for stable.

Is anybody a big floppy user (on _real_ hardware, preferably with more
than a couple machines) and could test it? I do feel kind of stupid
for not taking it, but I don't have the hardware or the interest in
really installing any in my current machines, and even if it looks
fine and fixes some issue, I just don't feel safe enough without some
more confirmation... As far as I know, the floppy oops thing has
_only_ been seen on KVM (likely due to timing that just doesn't happen
on real hardware), and while the KVM floppy emulation is probably
fairly good, it also clearly isn't equivalent to hardware.

                               Linus

Re: [RFC] floppy: use single threaded workqueue

[david]

I don't use floopies much anymore, but I've got a fair number of machines 
around that have floppy drives (purchased across about 6 years with from a 
couple vendors and a variety of motherboards)

If someone can go to the effort of documenting what testing you want done 
I may be able to do it.

I would be installing new builds onto the machines to test them, so 
you can do testing that locks up the boxes without impacting anything.

the easier it is to do the tests, the more machines I can test on 
(bootable CD images that I could just pop in and then check the screen 
later would be ideal ;-)

these systems will not have network access.

David Lang


  On Thu, 22 Jul 2010, Linus 
Torvalds wrote:

> On Wed, Jul 21, 2010 at 12:55 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
>>
>> Can confirm this fixes the following oops seen under KVM guests.
>>
>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000035
>
> Hmm. That does make me want to merge it early, but at the same time I
> suspect it has gotten almost no testing on real hardware. So despite
> looking "obvious" and fixing one bug, I'm a bit nervous and I suspect
> that the prudent thing to do is still to take it early in the 2.3.36
> merge window, and just mark it for stable.
>
> Is anybody a big floppy user (on _real_ hardware, preferably with more
> than a couple machines) and could test it? I do feel kind of stupid
> for not taking it, but I don't have the hardware or the interest in
> really installing any in my current machines, and even if it looks
> fine and fixes some issue, I just don't feel safe enough without some
> more confirmation... As far as I know, the floppy oops thing has
> _only_ been seen on KVM (likely due to timing that just doesn't happen
> on real hardware), and while the KVM floppy emulation is probably
> fairly good, it also clearly isn't equivalent to hardware.
>
>                               Linus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [RFC] floppy: use single threaded workqueue

[Linus Torvalds]

On Thu, Jul 22, 2010 at 3:22 PM,  <david@lang.hm> wrote:
>
> I don't use floopies much anymore, but I've got a fair number of machines
> around that have floppy drives (purchased across about 6 years with from a
> couple vendors and a variety of motherboards)
>
> If someone can go to the effort of documenting what testing you want done I
> may be able to do it.

It doesn't really need to be all that extensive. The "interesting"
operations tend to be (apart from just reading and writing data, of
course):

 - formatting a floppy (it's special, and historically relatively
often broke without people noticing for a while)

 - the floppy format auto-detection

 - floppy disk change detection

so just formatting floppies to a couple of different formats (ie do
you possibly have DD and HD floppies?), mkfs them and write something
to them, and then moving them to another machine and checking that
reading the data off them through the auto-detected formats (/dev/fd0)
works and gives the right results (just sha1sum the files).

The disk change detect can be a bit hard to see. It's unreliable with
some media, so iirc we always flush caches after the last close (we
didn't use to do that, and the disk change detection needed to just be
reliable - and you could test that the disk change logic worked by
just timing a read of the media and seeing if it came out of the
cache). I think for floppies, the thing to see is if format detection
works correctly when you switch between formats. Perhaps also the
read-only marker (ie switch the floppy between read-only and
read-write, and see that the status of the floppy is correctly
noticed: you should get a nice error if you try to write a read-only
floppy, rather than getting IO errors).

I can't think of anything else relevant.

                      Linus

Re: [RFC] floppy: use single threaded workqueue

[Amos Kong]

On Fri, Jul 23, 2010 at 6:22 AM,  <david@lang.hm> wrote:
> I don't use floopies much anymore, but I've got a fair number of machines
> around that have floppy drives (purchased across about 6 years with from a
> couple vendors and a variety of motherboards)
>
> If someone can go to the effort of documenting what testing you want done I
> may be able to do it.
>
> I would be installing new builds onto the machines to test them, so you can
> do testing that locks up the boxes without impacting anything.
>
> the easier it is to do the tests, the more machines I can test on (bootable
> CD images that I could just pop in and then check the screen later would be
> ideal ;-)
>
> these systems will not have network access.
>
> David Lang


Hello Davide,

Any feedback of testing this patch on real hardware with Linus' methods ?
Thanks.

Amos

Re: [RFC] floppy: use single threaded workqueue

[Stephen Hemminger]

On Sun, 22 Aug 2010 17:07:49 +0800
Amos Kong <kongjianjun@gmail.com> wrote:

> On Fri, Jul 23, 2010 at 6:22 AM,  <david@lang.hm> wrote:
> > I don't use floopies much anymore, but I've got a fair number of machines
> > around that have floppy drives (purchased across about 6 years with from a
> > couple vendors and a variety of motherboards)
> >
> > If someone can go to the effort of documenting what testing you want done I
> > may be able to do it.
> >
> > I would be installing new builds onto the machines to test them, so you can
> > do testing that locks up the boxes without impacting anything.
> >
> > the easier it is to do the tests, the more machines I can test on (bootable
> > CD images that I could just pop in and then check the screen later would be
> > ideal ;-)
> >
> > these systems will not have network access.
> >
> > David Lang
> 
> 
> Hello Davide,
> 
> Any feedback of testing this patch on real hardware with Linus' methods ?
> Thanks.

I did some testing, saw a glitch but wasn't sure if it was a 2.6.35
or floppy specific. But haven't investigated furthur because of impending
Vyatta product release. Plan to revisit this week.