amiflop: get rid of sleep_on calls

PATCH Superseded Andreas Bombe 4 participants 14 messages Dec 8, 2008 – Dec 31, 2008 Block layer

Share & tools

Andreas Bombe

author

2008-12-08 17:50 UTC #0 lore raw

PATCH 1/1 amiflop: get rid of sleep_on calls

Apart from sleep_on() calls that could be easily converted to wait_event() and completion calls amiflop also used a flag in ms_delay() and ms_isr() as a custom mutex for ms_delay() without a need for explicit unlocking. I converted that to a standard mutex semaphore.

The replacement for the unconditional sleep_on() in fd_motor_on() is a complete_all() together with a INIT_COMPLETION() before the mod_timer() call. It appears to me that fd_motor_on() might be called concurrently and fd_select() does not guarantee mutual exclusivity in the case the same drive gets selected again.

Signed-off-by Andreas Bombe <[email protected]>

---

Note: I haven't tested these changes beyond correct compilation.

The complete_all() with INIT_COMPLETION() was because I'm not sure about whether fd_motor_on() might run concurrently. Better safe than sorry.

 drivers/block/amiflop.c |   40 ++++++++++++++++------------------------
 1 files changed, 16 insertions(+), 24 deletions(-)

drivers/block/amiflop.c +16 ~~-24~~

index 4b1d4ac..42e4d71 100644
@@ -156,7 +156,7 @@ static volatile int fdc_busy = -1;
 static volatile int fdc_nested;
 static DECLARE_WAIT_QUEUE_HEAD(fdc_wait);
  
-static DECLARE_WAIT_QUEUE_HEAD(motor_wait);
+static DECLARE_COMPLETION(motor_on_completion);
 
 static volatile int selected = -1;	/* currently selected drive */

@@ -184,8 +184,7 @@ static unsigned char mfmencode[16]={
 static unsigned char mfmdecode[128];
 
 /* floppy internal millisecond timer stuff */
-static volatile int ms_busy = -1;
-static DECLARE_WAIT_QUEUE_HEAD(ms_wait);
+static DECLARE_COMPLETION(ms_wait_completion);
 #define MS_TICKS ((amiga_eclock+50)/1000)
 
 /*

@@ -211,8 +210,7 @@ static int fd_device[4] = { 0, 0, 0, 0 };
 
 static irqreturn_t ms_isr(int irq, void *dummy)
 {
-	ms_busy = -1;
-	wake_up(&ms_wait);
+	complete(&ms_wait_completion);
 	return IRQ_HANDLED;
 }

@@ -220,19 +218,17 @@ static irqreturn_t ms_isr(int irq, void *dummy)
    A more generic routine would do a schedule a la timer.device */
 static void ms_delay(int ms)
 {
-	unsigned long flags;
 	int ticks;
+	static DECLARE_MUTEX(mutex);
+
 	if (ms > 0) {
-		local_irq_save(flags);
-		while (ms_busy == 0)
-			sleep_on(&ms_wait);
-		ms_busy = 0;
-		local_irq_restore(flags);
+		down(&mutex);
 		ticks = MS_TICKS*ms-1;
 		ciaa.tblo=ticks%256;
 		ciaa.tbhi=ticks/256;
 		ciaa.crb=0x19; /*count eclock, force load, one-shoot, start */
-		sleep_on(&ms_wait);
+		wait_for_completion(&ms_wait_completion);
+		up(&mutex);
 	}
 }

@@ -254,8 +250,7 @@ static void get_fdc(int drive)
 	printk("get_fdc: drive %d  fdc_busy %d  fdc_nested %d\n",drive,fdc_busy,fdc_nested);
 #endif
 	local_irq_save(flags);
-	while (!try_fdc(drive))
-		sleep_on(&fdc_wait);
+	wait_event(fdc_wait, try_fdc(drive));
 	fdc_busy = drive;
 	fdc_nested++;
 	local_irq_restore(flags);

@@ -330,7 +325,7 @@ static void fd_deselect (int drive)
 static void motor_on_callback(unsigned long nr)
 {
 	if (!(ciaa.pra & DSKRDY) || --on_attempts == 0) {
-		wake_up (&motor_wait);
+		complete_all(&motor_on_completion);
 	} else {
 		motor_on_timer.expires = jiffies + HZ/10;
 		add_timer(&motor_on_timer);

@@ -347,11 +342,12 @@ static int fd_motor_on(int nr)
 		unit[nr].motor = 1;
 		fd_select(nr);
 
+		INIT_COMPLETION(motor_on_completion);
 		motor_on_timer.data = nr;
 		mod_timer(&motor_on_timer, jiffies + HZ/2);
 
 		on_attempts = 10;
-		sleep_on (&motor_wait);
+		wait_for_completion(&motor_on_completion);
 		fd_deselect(nr);
 	}

@@ -582,8 +578,7 @@ static void raw_read(int drive)
 {
 	drive&=3;
 	get_fdc(drive);
-	while (block_flag)
-		sleep_on(&wait_fd_block);
+	wait_event(wait_fd_block, !block_flag);
 	fd_select(drive);
 	/* setup adkcon bits correctly */
 	custom.adkcon = ADK_MSBSYNC;

@@ -598,8 +593,7 @@ static void raw_read(int drive)
 
 	block_flag = 1;
 
-	while (block_flag)
-		sleep_on (&wait_fd_block);
+	wait_event(wait_fd_block, !block_flag);
 
 	custom.dsklen = 0;
 	fd_deselect(drive);

@@ -616,8 +610,7 @@ static int raw_write(int drive)
 		rel_fdc();
 		return 0;
 	}
-	while (block_flag)
-		sleep_on(&wait_fd_block);
+	wait_event(wait_fd_block, !block_flag);
 	fd_select(drive);
 	/* clear adkcon bits */
 	custom.adkcon = ADK_PRECOMP1|ADK_PRECOMP0|ADK_WORDSYNC|ADK_MSBSYNC;

@@ -1294,8 +1287,7 @@ static int non_int_flush_track (unsigned long nr)
 			writepending = 0;
 			return 0;
 		}
-		while (block_flag == 2)
-			sleep_on (&wait_fd_block);
+		wait_event(wait_fd_block, block_flag != 2);
 	}
 	else {
 		local_irq_restore(flags);

Signature · 1 line

1.5.6.5

Geert Uytterhoeven

2008-12-09 08:31 UTC #1 lore raw

Quoted 11 lines · Andreas Bombe

Apart from sleep_on() calls that could be easily converted to wait_event() and completion calls amiflop also used a flag in ms_delay() and ms_isr() as a custom mutex for ms_delay() without a need for explicit unlocking. I converted that to a standard mutex semaphore.
The replacement for the unconditional sleep_on() in fd_motor_on() is a complete_all() together with a INIT_COMPLETION() before the mod_timer() call. It appears to me that fd_motor_on() might be called concurrently and fd_select() does not guarantee mutual exclusivity in the case the same drive gets selected again.

Quoted 7 lines

Signed-off-by Andreas Bombe <[email protected]>
---
Note: I haven't tested these changes beyond correct compilation.
The complete_all() with INIT_COMPLETION() was because I'm not sure about whether fd_motor_on() might run concurrently. Better safe than sorry.

I'm afraid my Amiga floppy drive doesn't work anymore... Any other testers available?

drivers/block/amiflop.c static irqreturn_t ms_isr(int irq, void *dummy) · line 222

Quoted 9 lines

drivers/block/amiflop.c +1 -1

@@ -220,19 +218,17 @@ static irqreturn_t ms_isr(int irq, void *dummy)
    A more generic routine would do a schedule a la timer.device */
 static void ms_delay(int ms)
 {
-	unsigned long flags;
 	int ticks;
+	static DECLARE_MUTEX(mutex);

               ^^^^^^^^^^^^^

Shouldn't this be a real mutex (declared using DEFINE_MUTEX(mutex)) instead of a semaphore used as a mutex?

drivers/block/amiflop.c static irqreturn_t ms_isr(int irq, void *dummy) · line 225

Quoted 8 lines

drivers/block/amiflop.c +2 -5

+
 	if (ms > 0) {
-		local_irq_save(flags);
-		while (ms_busy == 0)
-			sleep_on(&ms_wait);
-		ms_busy = 0;
-		local_irq_restore(flags);
+		down(&mutex);

                ^^^^
		mutex_lock

drivers/block/amiflop.c static irqreturn_t ms_isr(int irq, void *dummy) · line 231

Quoted 7 lines

drivers/block/amiflop.c +2 -1

 		ticks = MS_TICKS*ms-1;
 		ciaa.tblo=ticks%256;
 		ciaa.tbhi=ticks/256;
 		ciaa.crb=0x19; /*count eclock, force load, one-shoot, start */
-		sleep_on(&ms_wait);
+		wait_for_completion(&ms_wait_completion);
+		up(&mutex);

mutex_unlock

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Andreas Bombe

author

2008-12-09 18:34 UTC #2 lore raw

[Resend, vger appears not to like other smtp server]

Quoted 2 lines · Geert Uytterhoeven

I'm afraid my Amiga floppy drive doesn't work anymore... Any other testers available?

I still have a long unused Amiga around. However, it has not been graced with Linux yet, has no network device, the harddisk wasn't sounding very happy the last time I ran it those many years ago and that probably didn't improve. So I might be able to test it myself, but it would take time to set up.

Quoted 10 lines

drivers/block/amiflop.c static irqreturn_t ms_isr(int irq, void *dummy) · line 222

Quoted 9 lines

drivers/block/amiflop.c +1 -1

@@ -220,19 +218,17 @@ static irqreturn_t ms_isr(int irq, void *dummy)
    A more generic routine would do a schedule a la timer.device */
 static void ms_delay(int ms)
 {
-	unsigned long flags;
 	int ticks;
+	static DECLARE_MUTEX(mutex);

               ^^^^^^^^^^^^^

Quoted 2 lines

Shouldn't this be a real mutex (declared using DEFINE_MUTEX(mutex)) instead of a semaphore used as a mutex?

Yes, it should be. I came back to Linux kernel development only in the recent months and totally missed these kind of locks. I'll prepare a new patch later.

Andreas Bombe

author

2008-12-10 01:11 UTC #3 lore raw

PATCH v2 1/1 amiflop: get rid of sleep_on calls

In reply to Geert Uytterhoeven

Signed-off-by Andreas Bombe <[email protected]>

---

This version changes the struct semaphore used as mutex into a struct mutex. Still only compile tested.

I just tried out my Amiga and it appears to be in better shape than expected. So I might get to actually test it when I find the time. My Amiga has no Linux installation so I need to whip up a ramdisk or something else sufficient to test it.

 drivers/block/amiflop.c |   40 ++++++++++++++++------------------------
 1 files changed, 16 insertions(+), 24 deletions(-)

drivers/block/amiflop.c +16 ~~-24~~

index 4b1d4ac..8df436f 100644
@@ -156,7 +156,7 @@ static volatile int fdc_busy = -1;
 static volatile int fdc_nested;
 static DECLARE_WAIT_QUEUE_HEAD(fdc_wait);
  
-static DECLARE_WAIT_QUEUE_HEAD(motor_wait);
+static DECLARE_COMPLETION(motor_on_completion);
 
 static volatile int selected = -1;	/* currently selected drive */

@@ -184,8 +184,7 @@ static unsigned char mfmencode[16]={
 static unsigned char mfmdecode[128];
 
 /* floppy internal millisecond timer stuff */
-static volatile int ms_busy = -1;
-static DECLARE_WAIT_QUEUE_HEAD(ms_wait);
+static DECLARE_COMPLETION(ms_wait_completion);
 #define MS_TICKS ((amiga_eclock+50)/1000)
 
 /*

@@ -211,8 +210,7 @@ static int fd_device[4] = { 0, 0, 0, 0 };
 
 static irqreturn_t ms_isr(int irq, void *dummy)
 {
-	ms_busy = -1;
-	wake_up(&ms_wait);
+	complete(&ms_wait_completion);
 	return IRQ_HANDLED;
 }

@@ -220,19 +218,17 @@ static irqreturn_t ms_isr(int irq, void *dummy)
    A more generic routine would do a schedule a la timer.device */
 static void ms_delay(int ms)
 {
-	unsigned long flags;
 	int ticks;
+	static DEFINE_MUTEX(mutex);
+
 	if (ms > 0) {
-		local_irq_save(flags);
-		while (ms_busy == 0)
-			sleep_on(&ms_wait);
-		ms_busy = 0;
-		local_irq_restore(flags);
+		mutex_lock(&mutex);
 		ticks = MS_TICKS*ms-1;
 		ciaa.tblo=ticks%256;
 		ciaa.tbhi=ticks/256;
 		ciaa.crb=0x19; /*count eclock, force load, one-shoot, start */
-		sleep_on(&ms_wait);
+		wait_for_completion(&ms_wait_completion);
+		mutex_unlock(&mutex);
 	}
 }

@@ -254,8 +250,7 @@ static void get_fdc(int drive)
 	printk("get_fdc: drive %d  fdc_busy %d  fdc_nested %d\n",drive,fdc_busy,fdc_nested);
 #endif
 	local_irq_save(flags);
-	while (!try_fdc(drive))
-		sleep_on(&fdc_wait);
+	wait_event(fdc_wait, try_fdc(drive));
 	fdc_busy = drive;
 	fdc_nested++;
 	local_irq_restore(flags);

@@ -330,7 +325,7 @@ static void fd_deselect (int drive)
 static void motor_on_callback(unsigned long nr)
 {
 	if (!(ciaa.pra & DSKRDY) || --on_attempts == 0) {
-		wake_up (&motor_wait);
+		complete_all(&motor_on_completion);
 	} else {
 		motor_on_timer.expires = jiffies + HZ/10;
 		add_timer(&motor_on_timer);

@@ -347,11 +342,12 @@ static int fd_motor_on(int nr)
 		unit[nr].motor = 1;
 		fd_select(nr);
 
+		INIT_COMPLETION(motor_on_completion);
 		motor_on_timer.data = nr;
 		mod_timer(&motor_on_timer, jiffies + HZ/2);
 
 		on_attempts = 10;
-		sleep_on (&motor_wait);
+		wait_for_completion(&motor_on_completion);
 		fd_deselect(nr);
 	}

@@ -582,8 +578,7 @@ static void raw_read(int drive)
 {
 	drive&=3;
 	get_fdc(drive);
-	while (block_flag)
-		sleep_on(&wait_fd_block);
+	wait_event(wait_fd_block, !block_flag);
 	fd_select(drive);
 	/* setup adkcon bits correctly */
 	custom.adkcon = ADK_MSBSYNC;

@@ -598,8 +593,7 @@ static void raw_read(int drive)
 
 	block_flag = 1;
 
-	while (block_flag)
-		sleep_on (&wait_fd_block);
+	wait_event(wait_fd_block, !block_flag);
 
 	custom.dsklen = 0;
 	fd_deselect(drive);

@@ -616,8 +610,7 @@ static int raw_write(int drive)
 		rel_fdc();
 		return 0;
 	}
-	while (block_flag)
-		sleep_on(&wait_fd_block);
+	wait_event(wait_fd_block, !block_flag);
 	fd_select(drive);
 	/* clear adkcon bits */
 	custom.adkcon = ADK_PRECOMP1|ADK_PRECOMP0|ADK_WORDSYNC|ADK_MSBSYNC;

@@ -1294,8 +1287,7 @@ static int non_int_flush_track (unsigned long nr)
 			writepending = 0;
 			return 0;
 		}
-		while (block_flag == 2)
-			sleep_on (&wait_fd_block);
+		wait_event(wait_fd_block, block_flag != 2);
 	}
 	else {
 		local_irq_restore(flags);

Signature · 1 line

1.5.6.5

Geert Uytterhoeven

2008-12-14 16:21 UTC #4 lore raw

Quoted 14 lines · Andreas Bombe

Apart from sleep_on() calls that could be easily converted to wait_event() and completion calls amiflop also used a flag in ms_delay() and ms_isr() as a custom mutex for ms_delay() without a need for explicit unlocking. I converted that to a standard mutex.
The replacement for the unconditional sleep_on() in fd_motor_on() is a complete_all() together with a INIT_COMPLETION() before the mod_timer() call. It appears to me that fd_motor_on() might be called concurrently and fd_select() does not guarantee mutual exclusivity in the case the same drive gets selected again.
Signed-off-by Andreas Bombe <[email protected]>
---

Quoted 7 lines

This version changes the struct semaphore used as mutex into a struct mutex. Still only compile tested.
I just tried out my Amiga and it appears to be in better shape than expected. So I might get to actually test it when I find the time. My Amiga has no Linux installation so I need to whip up a ramdisk or something else sufficient to test it.

I think the good old ELF ramdisk at ftp://ftp.uni-erlangen.de/pub/unix/Linux/680x0/v2.0/filesys/filesys-ELF-2.0.x-1400K-2.gz should be sufficient to give it a try...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Andreas Bombe

author

2008-12-23 14:22 UTC #5 lore raw

Quoted 9 lines · Geert Uytterhoeven

Quoted 4 lines · Andreas Bombe
I just tried out my Amiga and it appears to be in better shape than expected. So I might get to actually test it when I find the time. My Amiga has no Linux installation so I need to whip up a ramdisk or something else sufficient to test it.
I think the good old ELF ramdisk at ftp://ftp.uni-erlangen.de/pub/unix/Linux/680x0/v2.0/filesys/filesys-ELF-2.0.x-1400K-2.gz should be sufficient to give it a try...

Not that easy with just 8 Megs of Fast RAM. Besides, I couldn't gunzip the file to look at the image, gzip gives a format error.

Now, I finally got to test it with a very simply self-made initrd. So far I only did some read tests in a single thread (dd to /dev/null at various offsets). It didn't crash or hang, but I noticed that the floppy motor is never shut off (I'm not sure about the motor, at least the floppy LED stays on). However, that is not a regression, I tested the mainline code and it shows the same problem.

Geert Uytterhoeven

2008-12-31 16:30 UTC #6 lore raw

Quoted 14 lines · Andreas Bombe

Quoted 9 lines · Geert Uytterhoeven
Quoted 4 lines · Andreas Bombe
I just tried out my Amiga and it appears to be in better shape than expected. So I might get to actually test it when I find the time. My Amiga has no Linux installation so I need to whip up a ramdisk or something else sufficient to test it.
I think the good old ELF ramdisk at ftp://ftp.uni-erlangen.de/pub/unix/Linux/680x0/v2.0/filesys/filesys-ELF-2.0.x-1400K-2.gz should be sufficient to give it a try...
Not that easy with just 8 Megs of Fast RAM. Besides, I couldn't gunzip the file to look at the image, gzip gives a format error.

Quoted 6 lines

Now, I finally got to test it with a very simply self-made initrd. So far I only did some read tests in a single thread (dd to /dev/null at various offsets). It didn't crash or hang, but I noticed that the floppy motor is never shut off (I'm not sure about the motor, at least the floppy LED stays on). However, that is not a regression, I tested the mainline code and it shows the same problem.

Indeed, the floppy motor keeps on running.

I added some debug code and got this:

	floppy_off: nr = 0x40000000
	floppy_off: mod motor_off_timer 0 jiffies + 3*HZ
	fd_motor_off: drive = 0xc0000000

drivers/block/amiflop.c +5 -5

index 8df436f..e42f168 100644
@@ -336,6 +336,7 @@ static int fd_motor_on(int nr)
 {
 	nr &= 3;
 
+pr_err("%s: del motor_off_timer %d\n", __func__, nr);
 	del_timer(motor_off_timer + nr);
 
 	if (!unit[nr].motor) {

@@ -368,16 +369,13 @@ static int fd_motor_on(int nr)
 static void fd_motor_off(unsigned long drive)
 {
 	long calledfromint;
-#ifdef MODULE
-	long decusecount;
-
-	decusecount = drive & 0x40000000;
-#endif
+pr_err("%s: drive = 0x%lx\n", __func__, drive);
 	calledfromint = drive & 0x80000000;
 	drive&=3;
 	if (calledfromint && !try_fdc(drive)) {
 		/* We would be blocked in an interrupt, so try again later */
 		motor_off_timer[drive].expires = jiffies + 1;
+pr_err("%s: add motor_off_timer %ld jiffies + 1\n", __func__, drive);
 		add_timer(motor_off_timer + drive);
 		return;
 	}

@@ -391,9 +389,11 @@ static void floppy_off (unsigned int nr)
 {
 	int drive;
 
+pr_err("%s: nr = 0x%x\n", __func__, nr);
 	drive = nr & 3;
 	/* called this way it is always from interrupt */
 	motor_off_timer[drive].data = nr | 0x80000000;
+pr_err("%s: mod motor_off_timer %d jiffies + 3*HZ\n", __func__, drive);
 	mod_timer(motor_off_timer + drive, jiffies + 3*HZ);
 }

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Kolbjørn Barmen

2008-12-13 15:48 UTC #7 lore raw

In reply to Geert Uytterhoeven

Quoted 5 lines · Geert Uytterhoeven

Quoted 2 lines
The complete_all() with INIT_COMPLETION() was because I'm not sure about whether fd_motor_on() might run concurrently. Better safe than sorry.
I'm afraid my Amiga floppy drive doesn't work anymore... Any other testers available?

I have some, even enough external ones to chain up to the max of 4 floppy drives (floppy raid anyone? :)). What needs to be tested? I havent updated kernel for quite some time (still running 2.6.22), should I now pull sources from git or what?

-- kolla

Geert Uytterhoeven

2008-12-13 21:30 UTC #8 lore raw

Quoted 9 lines · Kolbjørn Barmen

Quoted 5 lines · Geert Uytterhoeven
Quoted 2 lines
The complete_all() with INIT_COMPLETION() was because I'm not sure about whether fd_motor_on() might run concurrently. Better safe than sorry.
I'm afraid my Amiga floppy drive doesn't work anymore... Any other testers available?
I have some, even enough external ones to chain up to the max of 4 floppy drives (floppy raid anyone? :)). What needs to be tested?

That the floppy drive still works(tm)? ;-)

Quoted 2 lines

I havent updated kernel for quite some time (still running 2.6.22), should I now pull sources from git or what?

I haven't applied Andreas' patch yet, so you should pull the sources from git and apply his patch.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Joerg Dorchain

2008-12-09 08:53 UTC #9 lore raw

In reply to Andreas Bombe

Quoted 5 lines · Andreas Bombe

The replacement for the unconditional sleep_on() in fd_motor_on() is a complete_all() together with a INIT_COMPLETION() before the mod_timer() call. It appears to me that fd_motor_on() might be called concurrently and fd_select() does not guarantee mutual exclusivity in the case the same drive gets selected again.

Selecting the same drive repeatly does not matter. The selected drive is the one the next command or transfer applies to.

It is possible to select one drive, start the motor, select the next one, start the motor, as long as as trasnfer is tried until the drive is up to speed (either by signal or the timeout)

At first glance, the patch looks ok.

Bye,

Joerg

Andreas Bombe

author

2008-12-10 01:14 UTC #10 lore raw

Quoted 9 lines · Joerg Dorchain

Quoted 5 lines · Andreas Bombe
The replacement for the unconditional sleep_on() in fd_motor_on() is a complete_all() together with a INIT_COMPLETION() before the mod_timer() call. It appears to me that fd_motor_on() might be called concurrently and fd_select() does not guarantee mutual exclusivity in the case the same drive gets selected again.
Selecting the same drive repeatly does not matter. The selected drive is the one the next command or transfer applies to.

I think we're not talking about the same problem. If I were to use complete() together with wait_for_completion() there would be a problem if fd_motor_on() can get as far as wait_for_completion() while a previous completion is yet uncompleted. This can not happen for different drives, as the fd_select() would block. If it could happen for the same drive, the complete() would allow only one task to continue. The complete_all() takes care of that.

If requests are serialized for a drive so that there won't ever be two running at the same time for certain (thinking about it, it's probable), I could make it a simple complete(). It's hardly worth the risk, however.

Joerg Dorchain

2008-12-10 08:16 UTC #11 lore raw

Quoted 8 lines · Andreas Bombe

I think we're not talking about the same problem. If I were to use complete() together with wait_for_completion() there would be a problem if fd_motor_on() can get as far as wait_for_completion() while a previous completion is yet uncompleted. This can not happen for different drives, as the fd_select() would block. If it could happen for the same drive, the complete() would allow only one task to continue. The complete_all() takes care of that.

Quoted 4 lines

If requests are serialized for a drive so that there won't ever be two running at the same time for certain (thinking about it, it's probable), I could make it a simple complete(). It's hardly worth the risk, however.

IIRC (have not touched the driver for quite some time) at a certain stage all requests for a specific drive are serialized. The amiga floppy drives read and write whole tracks only, so follow-up acesses are likely to be on the same track and then served from the buffer of decoded blocks. I you do wild track jumping, it is slower than the original AmigaOS-driver.

But as I am not sure, and floppy drives are slow anyway, the complete_all() won't give any notable performance penalty, I guess.

Joerg

Geert Uytterhoeven

2008-12-16 20:23 UTC #12 lore raw

In reply to Joerg Dorchain

Quoted 7 lines · Joerg Dorchain

Quoted 5 lines · Andreas Bombe
The replacement for the unconditional sleep_on() in fd_motor_on() is a complete_all() together with a INIT_COMPLETION() before the mod_timer() call. It appears to me that fd_motor_on() might be called concurrently and fd_select() does not guarantee mutual exclusivity in the case the same drive gets selected again.

Quoted 8 lines

Selecting the same drive repeatly does not matter. The selected drive is the one the next command or transfer applies to.
It is possible to select one drive, start the motor, select the next one, start the motor, as long as as trasnfer is tried until the drive is up to speed (either by signal or the timeout)
At first glance, the patch looks ok.

Does that mean I can add your Acked-by?

Anyway, I've applied it to my git repository (to be pushed to kernel.org soon).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Joerg Dorchain

2008-12-17 08:00 UTC #13 lore raw

Quoted 4 lines · Geert Uytterhoeven

Quoted 2 lines
At first glance, the patch looks ok.
Does that mean I can add your Acked-by?

Yes. Although real world testing would be nice, though.

Bye,

Joerg