[PATCH 1/2] libata EH: freeze port before aborting commands

[PATCH 1/2] libata EH: freeze port before aborting commands

[Jeff Garzik]

commit 9c2fd3d1955572d3d1ee596b5f81c9768dde896e
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Apr 7 19:13:15 2009 -0400

    [libata] EH: freeze port before aborting commands
    
    Call the ->freeze() hook before aborting qc's, because some hardware
    requires special handling prior to accessing the taskfile registers
    (for diagnosis/analysis/reset).  Most notably, hardware may wish to
    disable the DMA engine or interrupts in the ->freeze() hook.
    
    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

---
Alan, we should create ata_sff_bmdma_freeze() and use that to turn off
the PCI IDE BMDMA engine in ->freeze(), too, I think...


diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 0183131..b79cbd6 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -999,7 +999,9 @@ static void __ata_port_freeze(struct ata_port *ap)
  *	ata_port_freeze - abort & freeze port
  *	@ap: ATA port to freeze
  *
- *	Abort and freeze @ap.
+ *	Abort and freeze @ap.  The freeze operation must be called
+ *	first, because some hardware requires special operations
+ *	before the taskfile registers are accessible.
  *
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
@@ -1013,8 +1015,8 @@ int ata_port_freeze(struct ata_port *ap)
 
 	WARN_ON(!ap->ops->error_handler);
 
-	nr_aborted = ata_port_abort(ap);
 	__ata_port_freeze(ap);
+	nr_aborted = ata_port_abort(ap);
 
 	return nr_aborted;
 }

[PATCH 2/2] sata_sil: disable DMA engine in ->freeze

[Jeff Garzik]

Dustin, please see if these two patches solve your problem?


commit a6a2da59f2a4ba834a6e20e2d2a4d030bb9371ea
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Apr 7 19:18:32 2009 -0400

    [libata] sata_sil: disable DMA engine in sil_freeze()
    
    We must disable the DMA engine before accessing taskfile registers.
    
    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index e67ce8e..896975f 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -565,6 +565,19 @@ static void sil_freeze(struct ata_port *ap)
 	tmp |= SIL_MASK_IDE0_INT << ap->port_no;
 	writel(tmp, mmio_base + SIL_SYSCFG);
 	readl(mmio_base + SIL_SYSCFG);	/* flush */
+
+	/* Ensure DMA_ENABLE is off.
+	 *
+	 * This is because the controller will not give us access to the
+	 * taskfile registers while a DMA is in progress
+	 */
+	iowrite8(ioread8(ap->ioaddr.bmdma_addr) & ~SIL_DMA_ENABLE,
+		 ap->ioaddr.bmdma_addr);
+
+	/* According to ata_bmdma_stop, an HDMA transition requires
+	 * on PIO cycle. But we can't read a taskfile register.
+	 */
+	ioread8(ap->ioaddr.bmdma_addr);
 }
 
 static void sil_thaw(struct ata_port *ap)

Re: [PATCH 2/2] sata_sil: disable DMA engine in ->freeze

[Sagar Borikar]

This patch works for our platform.

Thanks
Sagar

On Wed, Apr 8, 2009 at 4:53 AM, Jeff Garzik <jeff@garzik.org> wrote:
>
> Dustin, please see if these two patches solve your problem?
>
>
> commit a6a2da59f2a4ba834a6e20e2d2a4d030bb9371ea
> Author: Jeff Garzik <jeff@garzik.org>
> Date:   Tue Apr 7 19:18:32 2009 -0400
>
>    [libata] sata_sil: disable DMA engine in sil_freeze()
>
>    We must disable the DMA engine before accessing taskfile registers.
>
>    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
>
> diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
> index e67ce8e..896975f 100644
> --- a/drivers/ata/sata_sil.c
> +++ b/drivers/ata/sata_sil.c
> @@ -565,6 +565,19 @@ static void sil_freeze(struct ata_port *ap)
>        tmp |= SIL_MASK_IDE0_INT << ap->port_no;
>        writel(tmp, mmio_base + SIL_SYSCFG);
>        readl(mmio_base + SIL_SYSCFG);  /* flush */
> +
> +       /* Ensure DMA_ENABLE is off.
> +        *
> +        * This is because the controller will not give us access to the
> +        * taskfile registers while a DMA is in progress
> +        */
> +       iowrite8(ioread8(ap->ioaddr.bmdma_addr) & ~SIL_DMA_ENABLE,
> +                ap->ioaddr.bmdma_addr);
> +
> +       /* According to ata_bmdma_stop, an HDMA transition requires
> +        * on PIO cycle. But we can't read a taskfile register.
> +        */
> +       ioread8(ap->ioaddr.bmdma_addr);
>  }
>
>  static void sil_thaw(struct ata_port *ap)
>

Re: [PATCH 2/2] sata_sil: disable DMA engine in ->freeze

[Jeff Garzik]

Sagar Borikar wrote:
> This patch works for our platform.

Did you try both patches together?  Both are required for the solution.

	Jeff

Re: [PATCH 2/2] sata_sil: disable DMA engine in ->freeze

[Sagar Borikar]

Yes!

Thanks again

Sagar

On Wed, Apr 8, 2009 at 10:47 AM, Jeff Garzik <jeff@garzik.org> wrote:
> Sagar Borikar wrote:
>>
>> This patch works for our platform.
>
> Did you try both patches together?  Both are required for the solution.
>
>        Jeff
>
>
>
>

Re: [PATCH 2/2] sata_sil: disable DMA engine in ->freeze

[Dustin Harrison]

Jeff Garzik wrote:
> Sagar Borikar wrote:
>> This patch works for our platform.
>
> Did you try both patches together?  Both are required for the solution.
>
Thanks for updating the patch Jeff.  I see you spotted the reason why I 
didn't put the code into sil_freeze.  I tested your patch and it 
prevented the kernel panic.  I now get the following output, which seems 
to be correct to me.

WARNING: at drivers/ata/libata-core.c:5209 ata_qc_complete()
Call Trace:
[<80107ed0>] dump_stack+0x8/0x34
[<802f92e0>] ata_qc_complete+0x58/0x114
[<80303aec>] ata_port_abort+0xc4/0x104
[<80306870>] sil_interrupt+0x340/0x384
[<80146cc0>] handle_IRQ_event+0x54/0xb8
[<80146dc0>] __do_IRQ+0x9c/0x11c
[<80102670>] ll_sutuspci_irq+0xe8/0x158
[<8010161c>] do_extended_irq+0xa0/0x104
[<80102b94>] ret_from_irq+0x0/0x4
[<8029c4fc>] both_aligned+0x1c/0x64
[<8014a9a4>] file_read_actor+0xac/0x154
[<8014a564>] do_generic_mapping_read+0x210/0x5a4
[<8014ce98>] generic_file_aio_read+0x180/0x1d4
[<8016cba4>] do_sync_read+0xc4/0x128
[<8016ccbc>] vfs_read+0xb4/0x154
[<8016d02c>] sys_read+0x54/0x98
[<8010a620>] stack_done+0x20/0x3c

ata1.00: exception Emask 0x10 SAct 0x0 SErr 0x10000 action 0x2 frozen
ata1.00: cmd c8/00:f8:08:e3:06/00:00:00:00:00/e0 tag 0 cdb 0x0 data 
126976 in
         res d0/d0:d0:d0:d0:d0/00:00:00:00:00/d0 Emask 0x12 (ATA bus error)
ata1: hard resetting port
ata1: SATA link down (SStatus 0 SControl 310)
ata1: failed to recover some devices, retrying in 5 secs
ata1: hard resetting port
ata1: SATA link down (SStatus 0 SControl 310)
ata1.00: limiting speed to UDMA/100:PIO3
ata1: failed to recover some devices, retrying in 5 secs
ata1: hard resetting port
ata1: SATA link down (SStatus 0 SControl 310)
ata1.00: disabled
sd 0:0:0:0: [sda] Result: hostbyte=0x00 driverbyte=0x08
sd 0:0:0:0: [sda] Sense Key : 0xb [current] [descriptor]
Descriptor sense data with sense descriptors (in hex):
        72 0b 47 00 00 00 00 0c 00 0a 80 00 00 00 00 00
        00 d0 d0 d0
sd 0:0:0:0: [sda] ASC=0x47 ASCQ=0x0
end_request: I/O error, dev sda, sector 451336
Buffer I/O error on device sda, logical block 56417
Buffer I/O error on device sda, logical block 56418
Buffer I/O error on device sda, logical block 56419
Buffer I/O error on device sda, logical block 56420
Buffer I/O error on device sda, logical block 56421
Buffer I/O error on device sda, logical block 56422
Buffer I/O error on device sda, logical block 56423
Buffer I/O error on device sda, logical block 56424
Buffer I/O error on device sda, logical block 56425
Buffer I/O error on device sda, logical block 56426
sd 0:0:0:0: rejecting I/O to offline device
sd 0:0:0:0: [sda] Result: hostbyte=0x01 driverbyte=0x00
end_request: I/O error, dev sda, sector 451584
ata1: EH complete
ata1.00: detaching (SCSI 0:0:0:0)
dd: /dev/sda: Input/output error
# sd 0:0:0:0: [sda] Synchronizing SCSI cache
sd 0:0:0:0: [sda] Result: hostbyte=0x04 driverbyte=0x00
sd 0:0:0:0: [sda] Stopping disk
sd 0:0:0:0: [sda] START_STOP FAILED
sd 0:0:0:0: [sda] Result: hostbyte=0x04 driverbyte=0x00

Re: [PATCH 2/2] sata_sil: disable DMA engine in ->freeze

[Jeff Garzik]

Dustin Harrison wrote:
> Jeff Garzik wrote:
>> Sagar Borikar wrote:
>>> This patch works for our platform.
>>
>> Did you try both patches together?  Both are required for the solution.
>>
> Thanks for updating the patch Jeff.  I see you spotted the reason why I 
> didn't put the code into sil_freeze.  I tested your patch and it 
> prevented the kernel panic.  I now get the following output, which seems 
> to be correct to me.
> 
> WARNING: at drivers/ata/libata-core.c:5209 ata_qc_complete()

hum, my patch would indeed trigger this WARN_ON_ONCE() in ata_qc_complete():

         if (ap->ops->error_handler) {
                 struct ata_device *dev = qc->dev;
                 struct ata_eh_info *ehi = &dev->link->eh_info;

                 WARN_ON_ONCE(ap->pflags & ATA_PFLAG_FROZEN);

Tejun, was that WARN_ON() originally added to detect spurious callers? 
Or, completions after the EH started?

AFAICS, libata still owns the qc's at this point, so it should not be a 
problem to complete them when the port is frozen.

	Jeff




> Call Trace:
> [<80107ed0>] dump_stack+0x8/0x34
> [<802f92e0>] ata_qc_complete+0x58/0x114
> [<80303aec>] ata_port_abort+0xc4/0x104
> [<80306870>] sil_interrupt+0x340/0x384
> [<80146cc0>] handle_IRQ_event+0x54/0xb8
> [<80146dc0>] __do_IRQ+0x9c/0x11c
> [<80102670>] ll_sutuspci_irq+0xe8/0x158
> [<8010161c>] do_extended_irq+0xa0/0x104
> [<80102b94>] ret_from_irq+0x0/0x4
> [<8029c4fc>] both_aligned+0x1c/0x64
> [<8014a9a4>] file_read_actor+0xac/0x154
> [<8014a564>] do_generic_mapping_read+0x210/0x5a4
> [<8014ce98>] generic_file_aio_read+0x180/0x1d4
> [<8016cba4>] do_sync_read+0xc4/0x128
> [<8016ccbc>] vfs_read+0xb4/0x154
> [<8016d02c>] sys_read+0x54/0x98
> [<8010a620>] stack_done+0x20/0x3c

Re: [PATCH 2/2] sata_sil: disable DMA engine in ->freeze

[Tejun Heo]

Hello,

Jeff Garzik wrote:
>> Thanks for updating the patch Jeff.  I see you spotted the reason why
>> I didn't put the code into sil_freeze.  I tested your patch and it
>> prevented the kernel panic.  I now get the following output, which
>> seems to be correct to me.
>>
>> WARNING: at drivers/ata/libata-core.c:5209 ata_qc_complete()
> 
> hum, my patch would indeed trigger this WARN_ON_ONCE() in
> ata_qc_complete():
> 
>         if (ap->ops->error_handler) {
>                 struct ata_device *dev = qc->dev;
>                 struct ata_eh_info *ehi = &dev->link->eh_info;
> 
>                 WARN_ON_ONCE(ap->pflags & ATA_PFLAG_FROZEN);
> 
> Tejun, was that WARN_ON() originally added to detect spurious callers?
> Or, completions after the EH started?
> 
> AFAICS, libata still owns the qc's at this point, so it should not be a
> problem to complete them when the port is frozen.

Ah... that one.  The WARN_ON_ONCE() was added because I was worried
that LLD interrupt handler might get activated after the port is
frozen and try to complete the commands which now belong to EH.  Given
that ata_qc_from_tag() returns NULL for any commands which get already
marked failed, it's a bit paranoid.  Well, I was a bit paranoid when
adding new EH the first time, so...

Removing it should be fine at this point, I think, but I'm away from
my toys so testing is a bit difficult, so please test the path which
triggered the WARN_ON_ONCE() works fine (it should) before removing
it.

Thanks.

-- 
tejun