[PATCH 0/1] m68k/atari - ide: do not register interrupt if host->get_lock is set

[PATCH 0/1] m68k/atari - ide: do not register interrupt if host->get_lock is set

[Michael Schmitz]

Geert,

as hinted in my earlier patch series, this one bit me while debugging the
SCSI/IDE deadlock on my Falcon. 

On Falcon, registering the IDE interrupt handler is taken care of by
registering stdma_int() in the arch setup code, then passing the relevant
IDE interrupt handler as argument to stdma_lock(). This ensures that the IDE
inthandler is only called if IDE has acquired the ST-DMA lock, and avoids
the IDE handler stepping on the SCSI handlers' toes if the ST-DMA is locked
by SCSI. 

At some point in the past, the IDE interrupt was registered unconditionally
even on m68k, resulting in both stdma_int() and ide_interrupt() being
registered, again opening up the potential of IDE taking interrupts meant to
be handled by SCSI (or floppy). This might result in deadlocking the SCSI
driver if IDE releases the ST-DMA lock in error. 

The next patch changes the IDE core to only register the IDE handler if
host->get_lock is not set - AFAIK m68k is the sole user of host->get_lock
so we can rely on IDE interrupts being handled through the multiplexer
whenever host->get_lock happens to be set. 

Cheers,

	Michael

[PATCH] m68k/atari - ide: do not register interrupt if host->get_lock is set

[Michael Schmitz]

On m68k, host->get_lock may be used to both lock and register the
interrupt that the IDE host shares with other device drivers. Registering
the IDE interrupt handler in ide-probe.c results in duplicating the
interrupt registered, and may result in IDE accepting interrupts even
when another driver has locked the interrupt hardware.

Signed-off-by: Michael Schmitz <schmitz@debian.org>
---
 drivers/ide/ide-probe.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
index 2a744a9..ae691da 100644
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -853,8 +853,9 @@ static int init_irq (ide_hwif_t *hwif)
 	if (irq_handler == NULL)
 		irq_handler = ide_intr;
 
-	if (request_irq(hwif->irq, irq_handler, sa, hwif->name, hwif))
-		goto out_up;
+	if (!host->get_lock)
+		if (request_irq(hwif->irq, irq_handler, sa, hwif->name, hwif))
+			goto out_up;
 
 #if !defined(__mc68000__)
 	printk(KERN_INFO "%s at 0x%03lx-0x%03lx,0x%03lx on irq %d", hwif->name,
-- 
1.7.0.4

Re: [PATCH] m68k/atari - ide: do not register interrupt if host->get_lock is set

[Geert Uytterhoeven]

On Tue, Jan 28, 2014 at 9:07 AM, Michael Schmitz <schmitzmic@gmail.com> wrote:
> --- a/drivers/ide/ide-probe.c
> +++ b/drivers/ide/ide-probe.c
> @@ -853,8 +853,9 @@ static int init_irq (ide_hwif_t *hwif)
>         if (irq_handler == NULL)
>                 irq_handler = ide_intr;
>
> -       if (request_irq(hwif->irq, irq_handler, sa, hwif->name, hwif))
> -               goto out_up;
> +       if (!host->get_lock)
> +               if (request_irq(hwif->irq, irq_handler, sa, hwif->name, hwif))
> +                       goto out_up;

Don't you need a similar check for free_irq()?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] m68k/atari - ide: do not register interrupt if host->get_lock is set

[Michael Schmitz]

Geert,

On Fri, Jan 31, 2014 at 4:03 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Jan 28, 2014 at 9:07 AM, Michael Schmitz <schmitzmic@gmail.com> wrote:
>> --- a/drivers/ide/ide-probe.c
>> +++ b/drivers/ide/ide-probe.c
>> @@ -853,8 +853,9 @@ static int init_irq (ide_hwif_t *hwif)
>>         if (irq_handler == NULL)
>>                 irq_handler = ide_intr;
>>
>> -       if (request_irq(hwif->irq, irq_handler, sa, hwif->name, hwif))
>> -               goto out_up;
>> +       if (!host->get_lock)
>> +               if (request_irq(hwif->irq, irq_handler, sa, hwif->name, hwif))
>> +                       goto out_up;
>
> Don't you need a similar check for free_irq()?

Absolutely. Thanks for spotting this. I'll send the fixed version to linux-ide.

Cheers,

  Michael

[PATCH] m68k/atari - ide: do not register interrupt if host->get_lock is set

[Michael Schmitz]

On m68k, host->get_lock is used to both lock and register the interrupt
that the IDE host shares with other device drivers. Registering the
IDE interrupt handler in ide-probe.c results in duplicating the
interrupt registered (once via host->get lock, and also via init_irq()),
and may result in IDE accepting interrupts even when another driver has
locked the interrupt hardware. This opens the whole locking scheme up
to races.

host->get_lock is set on m68k only, so other drivers' behaviour is not
changed.

Signed-off-by: Michael Schmitz <schmitz@debian.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: linux-ide@vger.kernel.org
---
 drivers/ide/ide-probe.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
index 2a744a9..a3d3b17 100644
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -853,8 +853,9 @@ static int init_irq (ide_hwif_t *hwif)
 	if (irq_handler == NULL)
 		irq_handler = ide_intr;
 
-	if (request_irq(hwif->irq, irq_handler, sa, hwif->name, hwif))
-		goto out_up;
+	if (!host->get_lock)
+		if (request_irq(hwif->irq, irq_handler, sa, hwif->name, hwif))
+			goto out_up;
 
 #if !defined(__mc68000__)
 	printk(KERN_INFO "%s at 0x%03lx-0x%03lx,0x%03lx on irq %d", hwif->name,
@@ -1533,7 +1534,8 @@ static void ide_unregister(ide_hwif_t *hwif)
 
 	ide_proc_unregister_port(hwif);
 
-	free_irq(hwif->irq, hwif);
+	if (!hwif->host->get_lock)
+		free_irq(hwif->irq, hwif);
 
 	device_unregister(hwif->portdev);
 	device_unregister(&hwif->gendev);
-- 
1.7.0.4

Re: [PATCH] m68k/atari - ide: do not register interrupt if host->get_lock is set

[David Miller]

From: Michael Schmitz <schmitzmic@gmail.com>
Date: Sat,  1 Feb 2014 13:48:13 +1300

> On m68k, host->get_lock is used to both lock and register the interrupt
> that the IDE host shares with other device drivers. Registering the
> IDE interrupt handler in ide-probe.c results in duplicating the
> interrupt registered (once via host->get lock, and also via init_irq()),
> and may result in IDE accepting interrupts even when another driver has
> locked the interrupt hardware. This opens the whole locking scheme up
> to races.
> 
> host->get_lock is set on m68k only, so other drivers' behaviour is not
> changed.
> 
> Signed-off-by: Michael Schmitz <schmitz@debian.org>

It's a bit kludgy, but minimal and correct.

Applied, thank you.

Re: [PATCH] m68k/atari - ide: do not register interrupt if host->get_lock is set

[Michael Schmitz]

Thanks Dave,

>> On m68k, host->get_lock is used to both lock and register the 
>> interrupt
>> that the IDE host shares with other device drivers. Registering the
>> IDE interrupt handler in ide-probe.c results in duplicating the
>> interrupt registered (once via host->get lock, and also via 
>> init_irq()),
>> and may result in IDE accepting interrupts even when another driver 
>> has
>> locked the interrupt hardware. This opens the whole locking scheme up
>> to races.
>>
>> host->get_lock is set on m68k only, so other drivers' behaviour is not
>> changed.
>>
>> Signed-off-by: Michael Schmitz <schmitz@debian.org>
>
> It's a bit kludgy, but minimal and correct.

Would you have preferred to use a host flag instead?

> Applied, thank you.

Thanks again,

	Michael

Re: [PATCH] m68k/atari - ide: do not register interrupt if host->get_lock is set

[David Miller]

From: Michael Schmitz <schmitzmic@gmail.com>
Date: Thu, 6 Mar 2014 19:47:06 +1300

> Thanks Dave,
> 
>>> On m68k, host->get_lock is used to both lock and register the
>>> interrupt
>>> that the IDE host shares with other device drivers. Registering the
>>> IDE interrupt handler in ide-probe.c results in duplicating the
>>> interrupt registered (once via host->get lock, and also via
>>> init_irq()),
>>> and may result in IDE accepting interrupts even when another driver
>>> has
>>> locked the interrupt hardware. This opens the whole locking scheme up
>>> to races.
>>>
>>> host->get_lock is set on m68k only, so other drivers' behaviour is not
>>> changed.
>>>
>>> Signed-off-by: Michael Schmitz <schmitz@debian.org>
>>
>> It's a bit kludgy, but minimal and correct.
> 
> Would you have preferred to use a host flag instead?

I looked into that, we are out of host flags.  We'd either need to expand
the flags value to 64-bits or add another u32.

That's overkill for this.