[PATCH #upstream-fixes] sata_sil: fix spurious IRQ handling

[PATCH #upstream-fixes] sata_sil: fix spurious IRQ handling

[Tejun Heo]

Interestingly, sata_sil raises spurious interrupts if it's coupled
with Sil SATA_PATA bridge.  Currently, sata_sil interrupt handler is
strict about spurious interrupts and freezes the port when it occurs.
This patch makes it more forgiving.

* On SATA PHY event interrupt, serror value is checked to see whether
  it really is PHYRDY CHG event.  If not, SATA PHY event interrupt is
  ignored.

* If ATA interrupt occurs while no command is in progress, it's
  cleared and ignored.

This fixes bugzilla bug 9505.

  http://bugzilla.kernel.org/show_bug.cgi?id=9505

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/sata_sil.c |   29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Index: work/drivers/ata/sata_sil.c
===================================================================
--- work.orig/drivers/ata/sata_sil.c
+++ work/drivers/ata/sata_sil.c
@@ -390,23 +390,28 @@ static void sil_host_intr(struct ata_por
 		sil_scr_read(ap, SCR_ERROR, &serror);
 		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.
+		/* Sometimes spurious interrupts occur, double check
+		 * it's PHYRDY CHG.
 		 */
-		if (!(ap->pflags & ATA_PFLAG_FROZEN)) {
-			ata_ehi_hotplugged(&ap->link.eh_info);
-			ap->link.eh_info.serror |= serror;
+		if (serror & SERR_PHYRDY_CHG) {
+			/* 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->pflags & ATA_PFLAG_FROZEN)) {
+				ata_ehi_hotplugged(&ap->link.eh_info);
+				ap->link.eh_info.serror |= serror;
+			}
+			goto freeze;
 		}
 
-		goto freeze;
+		if (!(bmdma2 & SIL_DMA_COMPLETE))
+			return;
 	}
 
-	if (unlikely(!qc))
-		goto freeze;
-
-	if (unlikely(qc->tf.flags & ATA_TFLAG_POLLING)) {
+	if (unlikely(!qc || (qc->tf.flags & ATA_TFLAG_POLLING))) {
 		/* this sometimes happens, just clear IRQ */
 		ata_chk_status(ap);
 		return;

[PATCH #upstream-fixes] libata: clear link->eh_info.serror from ata_std_postreset()

[Tejun Heo]

link->eh_info.serror is used to cache SError for controllers which
need it cleared from interrupt handler to clear IRQ.  It also should
be cleared after reset just like SError itself.

Make ata_std_postreset() clear link->eh_info.serror too and update
sata_sil such that it doesn't care about bookkeeping the value.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |    1 +
 drivers/ata/sata_sil.c    |   11 +----------
 2 files changed, 2 insertions(+), 10 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -3923,6 +3923,7 @@ void ata_std_postreset(struct ata_link *
 	/* clear SError */
 	if (sata_scr_read(link, SCR_ERROR, &serror) == 0)
 		sata_scr_write(link, SCR_ERROR, serror);
+	link->eh_info.serror = 0;
 
 	/* is double-select really necessary? */
 	if (classes[0] != ATA_DEV_NONE)
Index: work/drivers/ata/sata_sil.c
===================================================================
--- work.orig/drivers/ata/sata_sil.c
+++ work/drivers/ata/sata_sil.c
@@ -394,16 +394,7 @@ static void sil_host_intr(struct ata_por
 		 * it's PHYRDY CHG.
 		 */
 		if (serror & SERR_PHYRDY_CHG) {
-			/* 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->pflags & ATA_PFLAG_FROZEN)) {
-				ata_ehi_hotplugged(&ap->link.eh_info);
-				ap->link.eh_info.serror |= serror;
-			}
+			ap->link.eh_info.serror |= serror;
 			goto freeze;
 		}
 

Re: [PATCH #upstream-fixes] libata: clear link->eh_info.serror from ata_std_postreset()

[Jeff Garzik]

Tejun Heo wrote:
> link->eh_info.serror is used to cache SError for controllers which
> need it cleared from interrupt handler to clear IRQ.  It also should
> be cleared after reset just like SError itself.
> 
> Make ata_std_postreset() clear link->eh_info.serror too and update
> sata_sil such that it doesn't care about bookkeeping the value.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-core.c |    1 +
>  drivers/ata/sata_sil.c    |   11 +----------
>  2 files changed, 2 insertions(+), 10 deletions(-)
> 
> Index: work/drivers/ata/libata-core.c
> ===================================================================
> --- work.orig/drivers/ata/libata-core.c
> +++ work/drivers/ata/libata-core.c
> @@ -3923,6 +3923,7 @@ void ata_std_postreset(struct ata_link *
>  	/* clear SError */
>  	if (sata_scr_read(link, SCR_ERROR, &serror) == 0)
>  		sata_scr_write(link, SCR_ERROR, serror);
> +	link->eh_info.serror = 0;

IMO it would make more sense to record the state of the hardware 
following sata_scr_write() than simply zeroing the cache value.

Just a gut feeling... it seems like having a manufactured value (zero) 
rather than the last known from-the-hardware value could lead to 
inconsistencies.

Comments?

	Jeff

Re: [PATCH #upstream-fixes] libata: clear link->eh_info.serror from ata_std_postreset()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> link->eh_info.serror is used to cache SError for controllers which
>> need it cleared from interrupt handler to clear IRQ.  It also should
>> be cleared after reset just like SError itself.
>>
>> Make ata_std_postreset() clear link->eh_info.serror too and update
>> sata_sil such that it doesn't care about bookkeeping the value.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>>  drivers/ata/libata-core.c |    1 +
>>  drivers/ata/sata_sil.c    |   11 +----------
>>  2 files changed, 2 insertions(+), 10 deletions(-)
>>
>> Index: work/drivers/ata/libata-core.c
>> ===================================================================
>> --- work.orig/drivers/ata/libata-core.c
>> +++ work/drivers/ata/libata-core.c
>> @@ -3923,6 +3923,7 @@ void ata_std_postreset(struct ata_link *
>>      /* clear SError */
>>      if (sata_scr_read(link, SCR_ERROR, &serror) == 0)
>>          sata_scr_write(link, SCR_ERROR, serror);
>> +    link->eh_info.serror = 0;
> 
> IMO it would make more sense to record the state of the hardware
> following sata_scr_write() than simply zeroing the cache value.
> 
> Just a gut feeling... it seems like having a manufactured value (zero)
> rather than the last known from-the-hardware value could lead to
> inconsistencies.
> 
> Comments?

link->eh_info.serror is always used as complement to the hardware SError
value.  It's a temporary storage to dump/accumulate SError value when
for whatever reason the hardware can't hold the value.
link->eh_info.serror and the hardware SError value are always OR'd
before being used, so no reason to dup the value.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes] libata: clear link->eh_info.serror from ata_std_postreset()

[Jeff Garzik]

Tejun Heo wrote:
> link->eh_info.serror is used to cache SError for controllers which
> need it cleared from interrupt handler to clear IRQ.  It also should
> be cleared after reset just like SError itself.
> 
> Make ata_std_postreset() clear link->eh_info.serror too and update
> sata_sil such that it doesn't care about bookkeeping the value.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-core.c |    1 +
>  drivers/ata/sata_sil.c    |   11 +----------
>  2 files changed, 2 insertions(+), 10 deletions(-)

applied #upstream-fixes

Re: [PATCH #upstream-fixes] sata_sil: fix spurious IRQ handling

[Jeff Garzik]

Tejun Heo wrote:
> Interestingly, sata_sil raises spurious interrupts if it's coupled
> with Sil SATA_PATA bridge.  Currently, sata_sil interrupt handler is
> strict about spurious interrupts and freezes the port when it occurs.
> This patch makes it more forgiving.
> 
> * On SATA PHY event interrupt, serror value is checked to see whether
>   it really is PHYRDY CHG event.  If not, SATA PHY event interrupt is
>   ignored.
> 
> * If ATA interrupt occurs while no command is in progress, it's
>   cleared and ignored.
> 
> This fixes bugzilla bug 9505.
> 
>   http://bugzilla.kernel.org/show_bug.cgi?id=9505
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied #upstream-fixes