2008-12-08 17:50:37

by Andreas Bombe

[permalink] [raw]
Subject: [PATCH] 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(-)

diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
index 4b1d4ac..42e4d71 100644
--- a/drivers/block/amiflop.c
+++ b/drivers/block/amiflop.c
@@ -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);
--
1.5.6.5


2008-12-09 08:31:25

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

On Mon, 8 Dec 2008, Andreas Bombe wrote:
> 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.

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

> --- a/drivers/block/amiflop.c
> +++ b/drivers/block/amiflop.c
> @@ -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?

> +
> 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

> 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

2008-12-09 08:53:48

by Joerg Dorchain

[permalink] [raw]
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

On Mon, Dec 08, 2008 at 04:59:38PM +0000, Andreas Bombe wrote:
> 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


Attachments:
(No filename) (753.00 B)
signature.asc (197.00 B)
Digital signature
Download all attachments

2008-12-09 18:34:24

by Andreas Bombe

[permalink] [raw]
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

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

On Tue, Dec 09, 2008 at 09:31:11AM +0100, Geert Uytterhoeven wrote:
> 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.

> > --- a/drivers/block/amiflop.c
> > +++ b/drivers/block/amiflop.c
> > @@ -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?

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.

2008-12-10 01:11:29

by Andreas Bombe

[permalink] [raw]
Subject: [PATCH v2] 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.

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]>
---

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(-)

diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
index 4b1d4ac..8df436f 100644
--- a/drivers/block/amiflop.c
+++ b/drivers/block/amiflop.c
@@ -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);
--
1.5.6.5

2008-12-10 01:14:51

by Andreas Bombe

[permalink] [raw]
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

On Tue, Dec 09, 2008 at 09:26:08AM +0100, Joerg Dorchain wrote:
> On Mon, Dec 08, 2008 at 04:59:38PM +0000, Andreas Bombe wrote:
> > 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.

2008-12-10 08:16:51

by Joerg Dorchain

[permalink] [raw]
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

On Wed, Dec 10, 2008 at 01:48:10AM +0100, Andreas Bombe wrote:
> 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.

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


Attachments:
(No filename) (1.23 kB)
signature.asc (197.00 B)
Digital signature
Download all attachments

2008-12-13 15:48:35

by Kolbjørn Barmen

[permalink] [raw]
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

On Tue, 9 Dec 2008, Geert Uytterhoeven wrote:

> > 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

2008-12-13 21:31:00

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

On Sat, 13 Dec 2008, Kolbjørn Barmen wrote:
> On Tue, 9 Dec 2008, Geert Uytterhoeven wrote:
> > > 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)? ;-)

> 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

2008-12-14 16:21:30

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2] amiflop: get rid of sleep_on calls

On Wed, 10 Dec 2008, Andreas Bombe wrote:
> 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]>
> ---
>
> 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

2008-12-16 20:25:54

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

On Tue, 9 Dec 2008, Joerg Dorchain wrote:
> On Mon, Dec 08, 2008 at 04:59:38PM +0000, Andreas Bombe wrote:
> > 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.

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

2008-12-23 14:22:58

by Andreas Bombe

[permalink] [raw]
Subject: Re: [PATCH v2] amiflop: get rid of sleep_on calls

On Sun, Dec 14, 2008 at 05:21:17PM +0100, Geert Uytterhoeven wrote:
> On Wed, 10 Dec 2008, Andreas Bombe wrote:
> > 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.

2008-12-31 16:30:21

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2] amiflop: get rid of sleep_on calls

On Tue, 23 Dec 2008, Andreas Bombe wrote:
> On Sun, Dec 14, 2008 at 05:21:17PM +0100, Geert Uytterhoeven wrote:
> > On Wed, 10 Dec 2008, Andreas Bombe wrote:
> > > 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.

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


diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
index 8df436f..e42f168 100644
--- a/drivers/block/amiflop.c
+++ b/drivers/block/amiflop.c
@@ -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

2008-12-17 08:00:37

by Joerg Dorchain

[permalink] [raw]
Subject: Re: [PATCH] amiflop: get rid of sleep_on calls

On Tue, Dec 16, 2008 at 09:23:00PM +0100, Geert Uytterhoeven wrote:
> >
> > 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


Attachments:
(No filename) (229.00 B)
signature.asc (197.00 B)
Digital signature
Download all attachments