[PATCH] sata_sil: update device hotplug handling

[PATCH] sata_sil: update device hotplug handling

[Tejun Heo]

Some flavors of 3112 cannot mask SATA_IRQ reliably and ends up
scheduling hotplug event during hardreset.  This patch makes sata_sil
simply use SError hotplug event handling in libata core layer.

This patch also makes sil_interrupt() clear interrupt before freezing.
Without this, some 3112 controllers cause interrupt storms.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/sata_sil.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

3338e8a25e5245a3aad7ad0d4d79a44faa918eb2
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 0898cbe..00906ab 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -343,12 +343,8 @@ static void sil_host_intr(struct ata_por
 	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
 	u8 status;
 
-	if (unlikely(bmdma2 & SIL_DMA_SATA_IRQ)) {
-		ata_ehi_hotplugged(&ap->eh_info);
-		goto freeze;
-	}
-
-	if (unlikely(!qc || qc->tf.ctl & ATA_NIEN))
+	if (unlikely((bmdma2 & SIL_DMA_SATA_IRQ) ||
+		     (!qc || qc->tf.ctl & ATA_NIEN)))
 		goto freeze;
 
 	/* Check whether we are expecting interrupt in this state */
@@ -399,6 +395,7 @@ static void sil_host_intr(struct ata_por
  err_hsm:
 	qc->err_mask |= AC_ERR_HSM;
  freeze:
+	ata_bmdma_irq_clear(ap);
 	ata_port_freeze(ap);
 }
 
-- 
1.3.2

Re: [PATCH] sata_sil: update device hotplug handling

[Jeff Garzik]

Tejun Heo wrote:
> Some flavors of 3112 cannot mask SATA_IRQ reliably and ends up
> scheduling hotplug event during hardreset.

Can you give more detail?  This sounds like a software bug?  I don't see 
SError clearing the pre-patched code path, the lack of which may create 
the conditions you describe.

	Jeff

Re: [PATCH] sata_sil: update device hotplug handling

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Some flavors of 3112 cannot mask SATA_IRQ reliably and ends up
>> scheduling hotplug event during hardreset.
> 
> Can you give more detail?  This sounds like a software bug?  I don't see 
> SError clearing the pre-patched code path, the lack of which may create 
> the conditions you describe.

This is from a sil3112 PCMCIA card.

# lspci -nvvv -s 07:00
0000:07:00.0 0180: 1095:3112 (rev 02)
         Subsystem: 1095:3112
         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- 
ParErr- Stepping- SERR- FastB2B-
         Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium 
 >TAbort- <TAbort- <MAbort- >SERR- <PERR-
         Latency: 64
         Interrupt: pin A routed to IRQ 10
         Region 0: I/O ports at 2410 [size=8]
         Region 1: I/O ports at 2420 [size=4]
         Region 2: I/O ports at 2418 [size=8]
         Region 3: I/O ports at 2424 [size=4]
         Region 4: I/O ports at 2400 [size=16]
         Region 5: Memory at 34000000 (32-bit, non-prefetchable) [size=512]
         Expansion ROM at 30000000 [disabled] [size=512K]
         Capabilities: [60] Power Management version 2
                 Flags: PMEClk- DSI+ D1+ D2+ AuxCurrent=0mA 
PME(D0-,D1-,D2-,D3hot-,D3cold-)
                 Status: D0 PME-Enable- DSel=0 DScale=2 PME-

I have sil3114 and sil3112 PCI cards and both don't need SError clearing 
to stop raising SATA IRQs.  Masking & acking are enough.  They never 
raise the interrupt again until unmasked.  Which is the correct behavior 
considering the following description about SATA IRQs in the datasheet.

"N – This bit enables an interrupt upon the assertion of the N bit in 
the DIAG field of the SError register"

On this PCMCIA card, SATA IRQ mask doesn't seem to work.  Hardreset 
causes SATA IRQs and thus rescheduling of hotplug event - eventually 
libata gives up on it.  It seems like this PCMCIA controller continues 
to raise SATA IRQ as long as corresponding SError bits are set.  Calling 
ata_bmdma_irq_clear() makes it slow and gives cpu more time to proceed.

Hmmm... On further testing, clearing SError does a much better job.  The 
controller still raises spurious interrupts but only a few times.  Will 
re-spin the patch w/ SError clearing.

-- 
tejun

[PATCH] sata_sil: update device hotplug handling, take #2

[Tejun Heo]

SIEN on some 3112 controllers doesn't mask SATA IRQ properly.  IRQ
stays asserted even after SIEN is masked and IRQ is acked.  Also, even
while frozen, any SATA PHY event including hardreset raises SATA IRQ.
Clearing SError seems to be the only way to deassert SATA IRQ.

This patch makes sil_host_intr() clear SError on SATA IRQs and ignore
SATA IRQs reported while frozen so that hardreset doesn't trigger
hotplug event (which ends up hardresetting again).

In such cases, the port still gets re-frozen to minimize the danger of
screaming interrupts.  This results in one nil EH repeat on
controllers with broken SIEN but other than that does no harm.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

Here's the revised version.  The PCMCIA 3112 controller simply cannot
mask SATA IRQs w/ SIEN.  I've tested it w/ discrete 3112 and 3114
cards and the PCMCIA one.  All work fine with this patch applied.

 drivers/scsi/sata_sil.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

590d11f6b5303ea1b695e255d69eb7008e298913
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index a7e99a1..ec30a5c 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -344,7 +344,25 @@ static void sil_host_intr(struct ata_por
 	u8 status;
 
 	if (unlikely(bmdma2 & SIL_DMA_SATA_IRQ)) {
-		ata_ehi_hotplugged(&ap->eh_info);
+		u32 serror;
+
+		/* SIEN doesn't mask SATA IRQs on some 3112s.  Those
+		 * controllers continue to assert IRQ as long as
+		 * SError bits are pending.  Clear SError immediately.
+		 */
+		serror = sil_scr_read(ap, SCR_ERROR);
+		sil_scr_write(ap, SCR_ERROR, serror);
+
+		/* Trigger hotplug and accumulate SError only if the
+		 * port isn't already frozen.  Otherwise, PHY events
+		 * during hardreset makes controllers with broken SIEN
+		 * repeat probing needlessly.
+		 */
+		if (!(ap->flags & ATA_FLAG_FROZEN)) {
+			ata_ehi_hotplugged(&ap->eh_info);
+			ap->eh_info.serror |= serror;
+		}
+
 		goto freeze;
 	}
 
-- 
1.3.2

Re: [PATCH] sata_sil: update device hotplug handling, take #2

[Jeff Garzik]

Tejun Heo wrote:
> SIEN on some 3112 controllers doesn't mask SATA IRQ properly.  IRQ
> stays asserted even after SIEN is masked and IRQ is acked.  Also, even
> while frozen, any SATA PHY event including hardreset raises SATA IRQ.
> Clearing SError seems to be the only way to deassert SATA IRQ.
> 
> This patch makes sil_host_intr() clear SError on SATA IRQs and ignore
> SATA IRQs reported while frozen so that hardreset doesn't trigger
> hotplug event (which ends up hardresetting again).
> 
> In such cases, the port still gets re-frozen to minimize the danger of
> screaming interrupts.  This results in one nil EH repeat on
> controllers with broken SIEN but other than that does no harm.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
> Here's the revised version.  The PCMCIA 3112 controller simply cannot
> mask SATA IRQs w/ SIEN.  I've tested it w/ discrete 3112 and 3114
> cards and the PCMCIA one.  All work fine with this patch applied.

Applied.

Note that it is helpful to read the FreeBSD 311x driver, to see what 
error handling they use.  It seems like they reset the chip much more 
frequently than we do.

	Jeff