Skip to contentSkip to conversation

falconide: remove needless ST-DMA locking

RFC PATCH In review Bartlomiej Zolnierkiewicz 3 participants 6 messages Oct 17, 2008 – Nov 16, 2008 libata

lore
Share & tools
lore

Bartlomiej Zolnierkiewicz

author
#0 lore raw

RFC PATCH 1/1 falconide: remove needless ST-DMA locking

While working on ide_do_request() improvements I stumbled upon mismatched ide_get_lock() / ide_release_lock() calls.

[ It seems to be known issue:
http://marc.info/?l=linux-m68k&m=121423752829622&w=2 ]

I may be completely wrong but how's about the following patch?

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [RFC PATCH] falconide: remove needless ST-DMA locking

According to comments in arch/m68k/atari/stdma.c IDE controller on Atari Falcon doesn't use ST-DMA chip itself and no locking is needed (both ST-DMA and IDE IRQ handlers are registered with IRQF_SHARED).

* Remove code for locking ST-DMA from <asm-m68k/ide.h> and falconide.c.

* Remove ide_{get,release}_lock() from ide_do_request() (it was broken anyway).

* Remove #ifndef IDE_ARCH_LOCK handling from <linux/ide.h>.

This patch should fix "ide_release_lock: bug" errors reported by Stephen R Marenka.

Cc Geert Uytterhoeven <[email protected]>
Cc Michael Schmitz <[email protected]>
Cc Stephen R Marenka <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
against Linus' tree

 drivers/ide/ide-io.c           |   12 +-----------
 drivers/ide/legacy/falconide.c |   11 -----------
 include/asm-m68k/ide.h         |   31 -------------------------------
 include/linux/ide.h            |    6 ------
 4 files changed, 1 insertion(+), 59 deletions(-)
drivers/ide/ide-io.c +1 -11
Index: b/drivers/ide/ide-io.c
===================================================================
@@ -967,9 +967,6 @@ static void ide_do_request (ide_hwgroup_
 	ide_startstop_t	startstop;
 	int             loops = 0;
 
-	/* for atari only: POSSIBLY BROKEN HERE(?) */
-	ide_get_lock(ide_intr, hwgroup);
-
 	/* caller must own ide_lock */
 	BUG_ON(!irqs_disabled());
 

@@ -1008,15 +1005,8 @@ static void ide_do_request (ide_hwgroup_
 				mod_timer(&hwgroup->timer, sleep);
 				/* we purposely leave hwgroup->busy==1
 				 * while sleeping */
-			} else {
-				/* Ugly, but how can we sleep for the lock
-				 * otherwise? perhaps from tq_disk?
-				 */
-
-				/* for atari only */
-				ide_release_lock();
+			} else
 				hwgroup->busy = 0;
-			}
 
 			/* no more work for this hwgroup (for now) */
 			return;
drivers/ide/legacy/falconide.c +0 -11
Index: b/drivers/ide/legacy/falconide.c
===================================================================
@@ -35,14 +35,6 @@
 
 #define ATA_HD_CONTROL	0x39
 
-    /*
-     *  falconide_intr_lock is used to obtain access to the IDE interrupt,
-     *  which is shared between several drivers.
-     */
-
-int falconide_intr_lock;
-EXPORT_SYMBOL(falconide_intr_lock);
-
 static void falconide_input_data(ide_drive_t *drive, struct request *rq,
 				 void *buf, unsigned int len)
 {

@@ -133,10 +125,7 @@ static int __init falconide_init(void)
 		goto err;
 	}
 
-	ide_get_lock(NULL, NULL);
 	rc = ide_host_register(host, &falconide_port_info, hws);
-	ide_release_lock();
-
 	if (rc)
 		goto err_free;
include/asm-m68k/ide.h +0 -31
Index: b/include/asm-m68k/ide.h
===================================================================
@@ -101,37 +101,6 @@
 #define M68K_IDE_SWAPW  (MACH_IS_Q40 || MACH_IS_ATARI)
 #endif
 
-#ifdef CONFIG_BLK_DEV_FALCON_IDE
-#define IDE_ARCH_LOCK
-
-extern int falconide_intr_lock;
-
-static __inline__ void ide_release_lock (void)
-{
-	if (MACH_IS_ATARI) {
-		if (falconide_intr_lock == 0) {
-			printk("ide_release_lock: bug\n");
-			return;
-		}
-		falconide_intr_lock = 0;
-		stdma_release();
-	}
-}
-
-static __inline__ void
-ide_get_lock(irq_handler_t handler, void *data)
-{
-	if (MACH_IS_ATARI) {
-		if (falconide_intr_lock == 0) {
-			if (in_interrupt() > 0)
-				panic( "Falcon IDE hasn't ST-DMA lock in interrupt" );
-			stdma_lock(handler, data);
-			falconide_intr_lock = 1;
-		}
-	}
-}
-#endif /* CONFIG_BLK_DEV_FALCON_IDE */
-
 #define IDE_ARCH_ACK_INTR
 #define ide_ack_intr(hwif)	((hwif)->ack_intr ? (hwif)->ack_intr(hwif) : 1)
include/linux/ide.h +0 -6
Index: b/include/linux/ide.h
===================================================================
@@ -241,12 +241,6 @@ static inline int __ide_default_irq(unsi
 # define ide_ack_intr(hwif) (1)
 #endif
 
-/* Currently only Atari needs it */
-#ifndef IDE_ARCH_LOCK
-# define ide_release_lock()			do {} while (0)
-# define ide_get_lock(hdlr, data)		do {} while (0)
-#endif /* IDE_ARCH_LOCK */
-
 /*
  * Now for the data we need to maintain per-drive:  ide_drive_t
  */
#1 lore raw

Hi Bartlomiej,

Quoted 5 lines

While working on ide_do_request() improvements I stumbled upon mismatched ide_get_lock() / ide_release_lock() calls.

[ It seems to be known issue:
http://marc.info/?l=linux-m68k&m=121423752829622&w=2 ]

It is a known issue, and I submitted a simple fix to Geert a month or so ago. It involves moving the ide_get_lock call inside the request loop instead of doing it once at the start of the function.

Quoted 1 line

I may be completely wrong but how's about the following patch?

You are, indeed .

Quoted 6 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [RFC PATCH] falconide: remove needless ST-DMA locking

According to comments in arch/m68k/atari/stdma.c IDE controller on Atari Falcon doesn't use ST-DMA chip itself and no locking is needed (both ST-DMA and IDE IRQ handlers are registered with IRQF_SHARED).

The IDE controller does not use the DMA chip, but it does use the same
interrupt vector as floppy controller, SCSI controller and DMA do. SCSI, as well as floppy do shart DMA. From what I recall, it is not possible to figure out if the interrupt was a DMA complete from the DMA chip state. That's basically whi we need to reserve DMA and interrupt for exclusive use of a single driver a a time.

Quoted 8 lines

* Remove code for locking ST-DMA from <asm-m68k/ide.h> and falconide.c.

* Remove ide_{get,release}_lock() from ide_do_request() (it was broken anyway).

* Remove #ifndef IDE_ARCH_LOCK handling from <linux/ide.h>.

Quoted 7 lines

This patch should fix "ide_release_lock: bug" errors reported by Stephen R Marenka.

Cc Geert Uytterhoeven <[email protected]>
Cc Michael Schmitz <[email protected]>
Cc Stephen R Marenka <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Objected: Michael Schmitz <[email protected]>

Do not apply, please.

Michael

#2 lore raw
Quoted 9 lines · Michael Schmitz
Quoted 5 lines

While working on ide_do_request() improvements I stumbled upon mismatched ide_get_lock() / ide_release_lock() calls.

[ It seems to be known issue:
http://marc.info/?l=linux-m68k&m=121423752829622&w=2 ]

It is a known issue, and I submitted a simple fix to Geert a month or so ago. It involves moving the ide_get_lock call inside the request loop instead of doing it once at the start of the function.

See http://marc.info/?l=linux-ide&m=121473433631934&w=2

In response to this patch, I wondered:

Quoted 8 lines
Quoted 8 lines

If hwgroup->busy serves a similar purpose to falconide_intr_lock, what about
moving the setting/clearing of hwgroup->busy into ide_{get,release}_lock()
(and possibly renaming ide_{get,release}_lock() to e.g. ide_hwgroup_{set,clear}_busy())?

What about the other places where hwgroup->busy is set/cleared?

And Michael responsed:

Quoted 8 lines

Uh - that's where it gets a bit sticky again. hwgroup->busy is set and cleared quite a lot 'preemptively' all over ide-io.c, f.e. in timeout handling. I'm not sure
whether this would just reintroduce the bug message.

The lock must be held as long as there are any interrupts to be expected from IDE. If the hwgroup->busy semantics reflects just that, it's worth a try.

Bart, can you shed a light on the hwgroup->busy semantics?

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

author
#3 lore raw
Quoted 31 lines · Geert Uytterhoeven
Quoted 9 lines · Michael Schmitz
Quoted 5 lines

While working on ide_do_request() improvements I stumbled upon mismatched ide_get_lock() / ide_release_lock() calls.

[ It seems to be known issue:
http://marc.info/?l=linux-m68k&m=121423752829622&w=2 ]

It is a known issue, and I submitted a simple fix to Geert a month or so ago. It involves moving the ide_get_lock call inside the request loop instead of doing it once at the start of the function.

See http://marc.info/?l=linux-ide&m=121473433631934&w=2

In response to this patch, I wondered:

Quoted 8 lines
Quoted 8 lines

If hwgroup->busy serves a similar purpose to falconide_intr_lock, what about
moving the setting/clearing of hwgroup->busy into ide_{get,release}_lock()
(and possibly renaming ide_{get,release}_lock() to e.g. ide_hwgroup_{set,clear}_busy())?

What about the other places where hwgroup->busy is set/cleared?

And Michael responsed:

Quoted 5 lines

Uh - that's where it gets a bit sticky again. hwgroup->busy is set and cleared quite a lot 'preemptively' all over ide-io.c, f.e. in timeout handling. I'm not sure
whether this would just reintroduce the bug message.

Quoted 5 lines
Quoted 3 lines

The lock must be held as long as there are any interrupts to be expected from IDE. If the hwgroup->busy semantics reflects just that, it's worth a try.

Bart, can you shed a light on the hwgroup->busy semantics?

hwgroup->busy means that hwgroup is busy ;-)

It protects against any access to the underlying hardware so it is also used when device is accessed in polling mode (without waiting for IRQ). However your proposal still sounds fine. We can treat hwgroup->busy as "waiting for IRQ" flag on Falcon and it would be correct (we will just hold the lock needlessly sometimes but we're already doing exactly that).

[ The one thing that we have to watch out is not to leak IDE core
specific things to <asm/ide.h> and host/platform specific ones to IDE core so some new abstraction level may be needed for handling ST-DMA lock itself (->[un]lock_host methods in struct ide_port_ops or something along the lines)... ]

Thanks,
Bart

author
#4 lore raw
Quoted 47 lines · Bartlomiej Zolnierkiewicz
Quoted 31 lines · Geert Uytterhoeven
Quoted 9 lines · Michael Schmitz
Quoted 5 lines

While working on ide_do_request() improvements I stumbled upon mismatched ide_get_lock() / ide_release_lock() calls.

[ It seems to be known issue:
http://marc.info/?l=linux-m68k&m=121423752829622&w=2 ]

It is a known issue, and I submitted a simple fix to Geert a month or so ago. It involves moving the ide_get_lock call inside the request loop instead of doing it once at the start of the function.

See http://marc.info/?l=linux-ide&m=121473433631934&w=2

In response to this patch, I wondered:

Quoted 8 lines
Quoted 8 lines

If hwgroup->busy serves a similar purpose to falconide_intr_lock, what about
moving the setting/clearing of hwgroup->busy into ide_{get,release}_lock()
(and possibly renaming ide_{get,release}_lock() to e.g. ide_hwgroup_{set,clear}_busy())?

What about the other places where hwgroup->busy is set/cleared?

And Michael responsed:

Quoted 5 lines

Uh - that's where it gets a bit sticky again. hwgroup->busy is set and cleared quite a lot 'preemptively' all over ide-io.c, f.e. in timeout handling. I'm not sure
whether this would just reintroduce the bug message.

Quoted 5 lines
Quoted 3 lines

The lock must be held as long as there are any interrupts to be expected from IDE. If the hwgroup->busy semantics reflects just that, it's worth a try.

Bart, can you shed a light on the hwgroup->busy semantics?

hwgroup->busy means that hwgroup is busy ;-)

It protects against any access to the underlying hardware so it is also used when device is accessed in polling mode (without waiting for IRQ). However your proposal still sounds fine. We can treat hwgroup->busy as "waiting for IRQ" flag on Falcon and it would be correct (we will just hold the lock needlessly sometimes but we're already doing exactly that).

Quoted 5 lines

[ The one thing that we have to watch out is not to leak IDE core
specific things to <asm/ide.h> and host/platform specific ones to IDE core so some new abstraction level may be needed for handling ST-DMA lock itself (->[un]lock_host methods in struct ide_port_ops or something along the lines)... ]

Since there was no follow-ups on this I've just applied Michael's patch for now (& will push it to Linus for .28)...

#5 lore raw
Quoted 48 lines · Bartlomiej Zolnierkiewicz
Quoted 47 lines · Bartlomiej Zolnierkiewicz
Quoted 31 lines · Geert Uytterhoeven
Quoted 9 lines · Michael Schmitz
Quoted 5 lines

While working on ide_do_request() improvements I stumbled upon mismatched ide_get_lock() / ide_release_lock() calls.

[ It seems to be known issue:
http://marc.info/?l=linux-m68k&m=121423752829622&w=2 ]

It is a known issue, and I submitted a simple fix to Geert a month or so ago. It involves moving the ide_get_lock call inside the request loop instead of doing it once at the start of the function.

See http://marc.info/?l=linux-ide&m=121473433631934&w=2

In response to this patch, I wondered:

Quoted 8 lines
Quoted 8 lines

If hwgroup->busy serves a similar purpose to falconide_intr_lock, what about
moving the setting/clearing of hwgroup->busy into ide_{get,release}_lock()
(and possibly renaming ide_{get,release}_lock() to e.g. ide_hwgroup_{set,clear}_busy())?

What about the other places where hwgroup->busy is set/cleared?

And Michael responsed:

Quoted 5 lines

Uh - that's where it gets a bit sticky again. hwgroup->busy is set and cleared quite a lot 'preemptively' all over ide-io.c, f.e. in timeout handling. I'm not sure
whether this would just reintroduce the bug message.

Quoted 5 lines
Quoted 3 lines

The lock must be held as long as there are any interrupts to be expected from IDE. If the hwgroup->busy semantics reflects just that, it's worth a try.

Bart, can you shed a light on the hwgroup->busy semantics?

hwgroup->busy means that hwgroup is busy ;-)

It protects against any access to the underlying hardware so it is also used when device is accessed in polling mode (without waiting for IRQ). However your proposal still sounds fine. We can treat hwgroup->busy as "waiting for IRQ" flag on Falcon and it would be correct (we will just hold the lock needlessly sometimes but we're already doing exactly that).

Quoted 8 lines
Quoted 5 lines

[ The one thing that we have to watch out is not to leak IDE core
specific things to <asm/ide.h> and host/platform specific ones to IDE core so some new abstraction level may be needed for handling ST-DMA lock itself (->[un]lock_host methods in struct ide_port_ops or something along the lines)... ]

Since there was no follow-ups on this I've just applied Michael's patch for now (& will push it to Linus for .28)...

Thank you very much!

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