Necessary evil to get sata_vsc to initialize with Intel iq3124h hba

PATCH In review Dan Williams 3 participants 6 messages Dec 14, 2005 – Jan 11, 2006

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Jeremy Higdon

2005-12-18 09:19 UTC #1 lore raw

Quoted 4 lines · Dan Williams

* libata does not care about error interrupts, so handle them locally
* the interrupts that are ignored only appear to happen at init time
Signed-off-by Dan Williams <[email protected]>

Okay, I finally had some time to look at this. A few comments . . . .

Quoted 18 lines

---

 drivers/scsi/sata_vsc.c |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)

applies-to: 90d8277c8775f3c09655df3b48c37fe07504d162 710b992dc354832bb854b9fb05c8fc73c15664d8

drivers/scsi/sata_vsc.c +3 -0

index cf94e01..7dcc486 100644
@@ -81,6 +81,19 @@
 /* Port stride */
 #define VSC_SATA_PORT_OFFSET		0x200
 
+/* Error interrupt status bit offsets */
+#define VSC_SATA_INT_ERROR_E_OFFSET	2
+#define VSC_SATA_INT_ERROR_P_OFFSET	4

drivers/scsi/sata_vsc.c line 96

Quoted 10 lines

drivers/scsi/sata_vsc.c +10 -0

+#define VSC_SATA_INT_ERROR_T_OFFSET	5
+#define VSC_SATA_INT_ERROR_M_OFFSET	1
+#define is_vsc_sata_int_err(port_idx, int_status) \
+	 (int_status & ((1 << (VSC_SATA_INT_ERROR_E_OFFSET + (8 * port_idx))) | \
+		        (1 << (VSC_SATA_INT_ERROR_P_OFFSET + (8 * port_idx))) | \
+		        (1 << (VSC_SATA_INT_ERROR_T_OFFSET + (8 * port_idx))) | \
+		        (1 << (VSC_SATA_INT_ERROR_M_OFFSET + (8 * port_idx)))   \
+		       )\
+ 	 )
+

I just want to be sure I'm getting this right.
Here you're specifying an interest in the "Data Integrity", "Unrecognized FIS Reception", "FIFO" errors, and "PHY went from Not-Ready to Ready" bits (text is using names in VSC7174 manual). Are these the four bits that have caused you trouble, and you haven't had problems with the "CRC Error" and "RERR Received", and "PHY Ready Change-Of-State" bits?

Quoted 5 lines

drivers/scsi/sata_vsc.c +0 -0

 static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
 {

@@ -193,13 +206,28 @@ static irqreturn_t vsc_sata_interrupt (i
 			struct ata_port *ap;

drivers/scsi/sata_vsc.c static irqreturn_t vsc_sata_interrupt (i · line 217

Quoted 10 lines

drivers/scsi/sata_vsc.c +9 -0

 			ap = host_set->ports[i];
+
+			if (is_vsc_sata_int_err(i, int_status)) {
+				u32 err_status;
+				printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
+				err_status = ap ? vsc_sata_scr_read(ap, SCR_ERROR) : 0;
+				vsc_sata_scr_write(ap, SCR_ERROR, err_status);
+				handled++;
+			}
+

This is short circuiting the normal LIBATA interrupt handling, right? Later, if LIBATA becomes interested in these bits, they'll already be cleared.

Also, if ap actually is NULL, the vsc_sata_scr_write() call will fail when it tries to dereference ap.

Quoted 4 lines

drivers/scsi/sata_vsc.c +0 -0

 			if (ap && !(ap->flags &
 				    (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
 				struct ata_queued_cmd *qc;

drivers/scsi/sata_vsc.c static irqreturn_t vsc_sata_interrupt (i · line 230

Quoted 10 lines

drivers/scsi/sata_vsc.c +7 -1

 				qc = ata_qc_from_tag(ap, ap->active_tag);
-				if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+				if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
 					handled += ata_host_intr(ap, qc);
+				} else {
+					printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
+					ata_chk_status(ap);
+					handled++;
+				}
+

I don't think I fully understand the problem you're trying to solve, but the gist seems to be that you're taking endless interrupts even when ATA_NIEN is set. So you add the dummy ata_chk_status call to clear interrupts that the libata core isn't interested in. But ata_chk_status doesn't actually clear it unless you first write to the SCR_ERROR register above?

FWIW, on my Vitesse chip, the int_status and SCR_ERROR bits that are set and when this code is called this code are 0x83 and 0x50002, respectively. But not clearing them doesn't seem to cause a problem . . . this must be different behavior in your chip (presumably the Intel branch).

Can you try this redo of your patch? I get this printed with it:

vsc_sata_interrupt: clearing interrupt,status 8300; sata err status 50002

jeremy

drivers/scsi/sata_vsc.c +31 -1

@@ -81,6 +81,19 @@
 /* Port stride */
 #define VSC_SATA_PORT_OFFSET		0x200
 
+/* Error interrupt status bit offsets */
+#define VSC_SATA_INT_ERROR_E_OFFSET	2
+#define VSC_SATA_INT_ERROR_P_OFFSET	4
+#define VSC_SATA_INT_ERROR_T_OFFSET	5
+#define VSC_SATA_INT_ERROR_M_OFFSET	1
+#define is_vsc_sata_int_err(port_idx, int_status) \
+	 (int_status & ((1 << (VSC_SATA_INT_ERROR_E_OFFSET + (8 * port_idx))) | \
+		        (1 << (VSC_SATA_INT_ERROR_P_OFFSET + (8 * port_idx))) | \
+		        (1 << (VSC_SATA_INT_ERROR_T_OFFSET + (8 * port_idx))) | \
+		        (1 << (VSC_SATA_INT_ERROR_M_OFFSET + (8 * port_idx)))   \
+		       )\
+ 	 )
+
 
 static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
 {

@@ -206,8 +219,25 @@
 				struct ata_queued_cmd *qc;
 
 				qc = ata_qc_from_tag(ap, ap->active_tag);
-				if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+				if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
 					handled += ata_host_intr(ap, qc);
+				} else if (is_vsc_sata_int_err(i, int_status)) {
+					/*
+					 * On some chips, there will be an endless
+					 * interrupt loop if the SCR_ERROR interrupts
+					 * are not cleared.
+					 */
+					u32 err_status;
+					err_status = vsc_sata_scr_read(ap, SCR_ERROR);
+					printk(KERN_DEBUG "%s: clearing interrupt,"
+					       "status %x; sata err status %x\n",
+					       __FUNCTION__,
+					       int_status, err_status);
+					vsc_sata_scr_write(ap, SCR_ERROR, err_status);
+					/* Clear interrupt status */
+					ata_chk_status(ap);
+					handled++;
+				}
 			}
 		}
 	}

Dan Williams

2005-12-27 19:27 UTC #2 lore raw

Quoted 35 lines · Jeremy Higdon

Quoted 4 lines · Dan Williams
* libata does not care about error interrupts, so handle them locally
* the interrupts that are ignored only appear to happen at init time
Signed-off-by Dan Williams <[email protected]>
Okay, I finally had some time to look at this. A few comments . . . .
Quoted 27 lines
---
 drivers/scsi/sata_vsc.c |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)
applies-to: 90d8277c8775f3c09655df3b48c37fe07504d162 710b992dc354832bb854b9fb05c8fc73c15664d8
drivers/scsi/sata_vsc.c +12 -0
index cf94e01..7dcc486 100644
@@ -81,6 +81,19 @@
 /* Port stride */
 #define VSC_SATA_PORT_OFFSET         0x200
 
+/* Error interrupt status bit offsets */
+#define VSC_SATA_INT_ERROR_E_OFFSET  2
+#define VSC_SATA_INT_ERROR_P_OFFSET  4
+#define VSC_SATA_INT_ERROR_T_OFFSET  5
+#define VSC_SATA_INT_ERROR_M_OFFSET  1
+#define is_vsc_sata_int_err(port_idx, int_status) \
+      (int_status & ((1 << (VSC_SATA_INT_ERROR_E_OFFSET + (8 * port_idx))) | \
+                     (1 << (VSC_SATA_INT_ERROR_P_OFFSET + (8 * port_idx))) | \
+                     (1 << (VSC_SATA_INT_ERROR_T_OFFSET + (8 * port_idx))) | \
+                     (1 << (VSC_SATA_INT_ERROR_M_OFFSET + (8 * port_idx)))   \
+                    )\
+      )

Quoted 8 lines

drivers/scsi/sata_vsc.c line 96
Quoted 1 line
drivers/scsi/sata_vsc.c +1 -0
+
I just want to be sure I'm getting this right.
Here you're specifying an interest in the "Data Integrity", "Unrecognized FIS Reception", "FIFO" errors, and "PHY went from Not-Ready to Ready" bits (text is using names in VSC7174 manual). Are these the four bits that have caused you trouble, and you haven't had problems with the "CRC Error" and "RERR Received", and "PHY Ready Change-Of-State" bits?

In the Intel 31244 manual (pg 222
http://www.intel.com/design/storage/manuals/27360306.pdf) it classifies some errors as diagnostic. My initial thought was to just check for and clear non-diagnostic errors, but after working with your patch I discovered this was not sufficient.

Quoted 5 lines

drivers/scsi/sata_vsc.c +0 -0

 static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
 {

@@ -193,13 +206,28 @@ static irqreturn_t vsc_sata_interrupt (i
                      struct ata_port *ap;

Quoted 14 lines

drivers/scsi/sata_vsc.c static irqreturn_t vsc_sata_interrupt (i · line 217

Quoted 10 lines

drivers/scsi/sata_vsc.c +9 -0

                      ap = host_set->ports[i];
+
+                     if (is_vsc_sata_int_err(i, int_status)) {
+                             u32 err_status;
+                             printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
+                             err_status = ap ? vsc_sata_scr_read(ap, SCR_ERROR) : 0;
+                             vsc_sata_scr_write(ap, SCR_ERROR, err_status);
+                             handled++;
+                     }
+

This is short circuiting the normal LIBATA interrupt handling, right? Later, if LIBATA becomes interested in these bits, they'll already be cleared.

See below, it turns out that clobbering these bits is not necessary, but it would negatively impact performance to leave them set.

Quoted 2 lines

Also, if ap actually is NULL, the vsc_sata_scr_write() call will fail when it tries to dereference ap.

Ok.

Quoted 16 lines

Quoted 5 lines

drivers/scsi/sata_vsc.c +0 -0

                      if (ap && !(ap->flags &
                                  (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
                              struct ata_queued_cmd *qc;

drivers/scsi/sata_vsc.c static irqreturn_t vsc_sata_interrupt (i · line 230

Quoted 10 lines

drivers/scsi/sata_vsc.c +7 -1

                              qc = ata_qc_from_tag(ap, ap->active_tag);
-                             if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+                             if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
                                      handled += ata_host_intr(ap, qc);
+                             } else {
+                                     printk(KERN_DEBUG "%s: ignoring interrupt(s)\n", __FUNCTION__);
+                                     ata_chk_status(ap);
+                                     handled++;
+                             }
+

Quoted 3 lines

I don't think I fully understand the problem you're trying to solve, but the gist seems to be that you're taking endless interrupts even when ATA_NIEN is set.

Yes...

Quoted 4 lines

So you add the dummy ata_chk_status call to clear
interrupts that the libata core isn't interested in. But ata_chk_status doesn't actually clear it unless you first write to the SCR_ERROR register above?

Actually I added the dummy ata_chk_status because just clearing the SCR_ERROR bits did not stop the stream of interrupts. Upon further investigation it appears that clearing the SCR_ERRORS is not _required_ and all that is necessary is the ata_chk_status() call. however not clearing the SCR_ERROR results in the error interrupt recurring everytime the disk is accessed.

For example:
vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002
vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002 Adding 136512k swap on /dev/sda1. Priority:-1 extents:1 across:136512k
vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002
vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002 Adding 125456k swap on /dev/sdb1. Priority:-2 extents:1 across:125456k
vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002
vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002 Adding 136512k swap on /dev/sdc1. Priority:-3 extents:1 across:136512k

Quoted 4 lines

FWIW, on my Vitesse chip, the int_status and SCR_ERROR bits that are set and when this code is called this code are 0x83 and 0x50002, respectively. But not clearing them doesn't seem to cause a problem . . . this must be different behavior in your chip (presumably the Intel branch).

Yes, with your patch I get the same output, perhaps the Vitesse part does not tie these bits to the error interrupt, or they are masked by default?

I like your new organization of the code. I went ahead and revised the patch further to make is_vsc_sata_int_err() less expensive, and better document the need for the extra error handling code. I also updated the Kconfig entry to make it obvious that the Intel part can use the same driver. My apologies for attaching the patch I am still working on the best way to get list traffic to a sane MTA on my linux box at work.

Dan

drivers/scsi/Kconfig +2 -2

index 20dd85a..5bde03b 100644
@@ -586,10 +586,10 @@ config SCSI_SATA_VIA
 	  If unsure, say N.
 
 config SCSI_SATA_VITESSE
-	tristate "VITESSE VSC-7174 SATA support"
+	tristate "VITESSE VSC-7174 / INTEL 31244 SATA support"
 	depends on SCSI_SATA && PCI
 	help
-	  This option enables support for Vitesse VSC7174 Serial ATA.
+	  This option enables support for Vitesse VSC7174 and Intel 31244 Serial ATA.
 
 	  If unsure, say N.

drivers/scsi/libata-scsi.c +4 -1

index 2282c04..72ddba9 100644
@@ -2044,7 +2044,7 @@ static int atapi_qc_complete(struct ata_
 	else {
 		u8 *scsicmd = cmd->cmnd;
 
-		if ((scsicmd[0] == INQUIRY) && ((scsicmd[1] & 0x03) == 0)) {
+		if (scsicmd[0] == INQUIRY) {
 			u8 *buf = NULL;
 			unsigned int buflen;

@@ -2058,6 +2058,9 @@ static int atapi_qc_complete(struct ata_
 	 * device.  2) Ensure response data format / ATAPI information
 	 * are always correct.
 	 */
+	/* FIXME: do we ever override EVPD pages and the like, with
+	 * this code?
+	 */
 			if (buf[2] == 0) {
 				buf[2] = 0x5;
 				buf[3] = 0x32;

drivers/scsi/sata_vsc.c +37 -1

index fcfa486..149ed83 100644
@@ -81,6 +81,22 @@
 /* Port stride */
 #define VSC_SATA_PORT_OFFSET		0x200
 
+/* Error interrupt status bit offsets */
+#define VSC_SATA_INT_ERROR_CRC		(1 << 6)
+#define VSC_SATA_INT_ERROR_T		(1 << 5)
+#define VSC_SATA_INT_ERROR_P		(1 << 4)
+#define VSC_SATA_INT_ERROR_R		(1 << 3)
+#define VSC_SATA_INT_ERROR_E		(1 << 2)
+#define VSC_SATA_INT_ERROR_M		(1 << 1)
+#define VSC_SATA_INT_PHY_CHANGE	(1 << 0)
+#define VSC_SATA_INT_ERROR (VSC_SATA_INT_ERROR_CRC  + VSC_SATA_INT_ERROR_T + \
+			     VSC_SATA_INT_ERROR_P    + VSC_SATA_INT_ERROR_R + \
+			     VSC_SATA_INT_ERROR_E    + VSC_SATA_INT_ERROR_M + \
+			     VSC_SATA_INT_PHY_CHANGE)
+
+#define is_vsc_sata_int_err(port_idx, int_status) \
+	 (int_status & (VSC_SATA_INT_ERROR << (8 * port_idx)))
+
 
 static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
 {

@@ -206,8 +222,28 @@ static irqreturn_t vsc_sata_interrupt (i
 				struct ata_queued_cmd *qc;
 
 				qc = ata_qc_from_tag(ap, ap->active_tag);
-				if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+				if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
 					handled += ata_host_intr(ap, qc);
+				} else if (is_vsc_sata_int_err(i, int_status)) {
+					/*
+					 * On some chips (i.e. Intel 31244), an error 
+					 * interrupt will sneak in at initialization
+					 * time (phy state changes).  Clearing the SCR
+					 * error register is not required, but it prevents
+					 * the phy state change interrupts from recurring 
+					 * later.
+					 */
+					u32 err_status;
+					err_status = vsc_sata_scr_read(ap, SCR_ERROR);
+					printk(KERN_DEBUG "%s: clearing interrupt, "
+					       "status %x; sata err status %x\n",
+					       __FUNCTION__,
+					       int_status, err_status);
+					vsc_sata_scr_write(ap, SCR_ERROR, err_status);
+					/* Clear interrupt status */
+					ata_chk_status(ap);
+					handled++;
+				}
 			}
 		}
 	}

Jeremy Higdon

2006-01-05 07:57 UTC #3 lore raw

Quoted 30 lines · Dan Williams

Quoted 4 lines · Jeremy Higdon
I don't think I fully understand the problem you're trying to solve, but the gist seems to be that you're taking endless interrupts even when ATA_NIEN is set.
Yes...
Quoted 4 lines
So you add the dummy ata_chk_status call to clear
interrupts that the libata core isn't interested in. But ata_chk_status doesn't actually clear it unless you first write to the SCR_ERROR register above?
Actually I added the dummy ata_chk_status because just clearing the SCR_ERROR bits did not stop the stream of interrupts. Upon further investigation it appears that clearing the SCR_ERRORS is not _required_ and all that is necessary is the ata_chk_status() call. however not clearing the SCR_ERROR results in the error interrupt recurring everytime the disk is accessed.
For example:
vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002
vsc_sata_interrupt: clearing interrupt, status 30383; sata err status 50002 Adding 136512k swap on /dev/sda1. Priority:-1 extents:1 across:136512k
vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002
vsc_sata_interrupt: clearing interrupt, status 38303; sata err status 50002 Adding 125456k swap on /dev/sdb1. Priority:-2 extents:1 across:125456k
vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002
vsc_sata_interrupt: clearing interrupt, status 830303; sata err status 50002 Adding 136512k swap on /dev/sdc1. Priority:-3 extents:1 across:136512k

Quoted 8 lines

Quoted 4 lines
FWIW, on my Vitesse chip, the int_status and SCR_ERROR bits that are set and when this code is called this code are 0x83 and 0x50002, respectively. But not clearing them doesn't seem to cause a problem . . . this must be different behavior in your chip (presumably the Intel branch).
Yes, with your patch I get the same output, perhaps the Vitesse part does not tie these bits to the error interrupt, or they are masked by default?

Dan, please trim trailing whitespace from your patch, if you would. Other than that, your patch to sata_vsc.c and Kconfig looks fine to me. I'm not sure about the patch to libata-scsi.c, but jgarzik can review that one.

I checked both the Intel and Vitesse documentation, and they both indicate that the error interrupts are masked by default. Perhaps your PROM/BIOS software is enabling them. Ours does not, and the sata_vsc driver doesn't either. Perhaps it should -- though since we're just ignoring them, that doesn't make much sense. Maybe we should explicitly mask them off? Or explicitly enable them, rather than leaving the chip in whatever state the PROM/BIOS leaves it. Perhaps this patch is all you really need. Can you try it?

drivers/scsi/sata_vsc.c +2 -2

@@ -108,9 +108,9 @@
 		VSC_SATA_INT_MASK_OFFSET + ap->port_no;
 	mask = readb(mask_addr);
 	if (ctl & ATA_NIEN)
-		mask |= 0x80;
+		mask = 0x80;
 	else
-		mask &= 0x7F;
+		mask = 0;
 	writeb(mask, mask_addr);
 }

jeremy

Dan Williams

2006-01-09 23:30 UTC #4 lore raw

Quoted 17 lines

Dan, please trim trailing whitespace from your patch, if you would. Other than that, your patch to sata_vsc.c and Kconfig looks fine to me. I'm not sure about the patch to libata-scsi.c, but jgarzik can review that one.
I checked both the Intel and Vitesse documentation, and they both indicate that the error interrupts are masked by default. Perhaps your PROM/BIOS software is enabling them. Ours does not, and the sata_vsc driver doesn't either. Perhaps it should -- though since we're just ignoring them, that doesn't make much sense. Maybe we should explicitly mask them off? Or explicitly enable them, rather than leaving the chip in whatever state the PROM/BIOS leaves it. Perhaps this patch is all you really need. Can you try it?
drivers/scsi/sata_vsc.c 2006-01-04 23:55:16.000000000 -0800 renamed from drivers/scsi/sata_vsc.c 2006-01-02 19:21:10.000000000 -0800 +0 -0
@@ -108,9 +108,9 @@
                VSC_SATA_INT_MASK_OFFSET + ap->port_no;

drivers/scsi/sata_vsc.c 2006-01-04 23:55:16.000000000 -0800 line 116

Quoted 10 lines

drivers/scsi/sata_vsc.c 2006-01-04 23:55:16.000000000 -0800 renamed from drivers/scsi/sata_vsc.c 2006-01-02 19:21:10.000000000 -0800 +2 -2
        mask = readb(mask_addr);
        if (ctl & ATA_NIEN)
-               mask |= 0x80;
+               mask = 0x80;
        else
-               mask &= 0x7F;
+               mask = 0;
        writeb(mask, mask_addr);
 }
 

Surprisingly, I still get the endless error interrupts with this patch applied.

I integrated this patch with the previous explicit clear patch, and added debug prints to see when vsc_intr_mask_update is called relative to the interrupt handler. Below is the debug output, and attached is the integrated patch with the interrupt mask changes (trimmed for white space against 2.6.15).

Dan

libata version 1.20 loaded.
sata_vsc 0000:00:01.0: version 1.1
ata1: SATA max UDMA/133 cmd 0xEE0A0200 ctl 0xEE0A0229 bmdma 0xEE0A0270 irq 25
ata2: SATA max UDMA/133 cmd 0xEE0A0400 ctl 0xEE0A0429 bmdma 0xEE0A0470 irq 25
ata3: SATA max UDMA/133 cmd 0xEE0A0600 ctl 0xEE0A0629 bmdma 0xEE0A0670 irq 25
ata4: SATA max UDMA/133 cmd 0xEE0A0800 ctl 0xEE0A0829 bmdma 0xEE0A0870 irq 25 entering vsc_intr_mask_update
exiting vsc_intr_mask_update
vsc_sata_interrupt: clearing interrupt, status 83; sata err status 50002
ata1: dev 0 cfg 49:2f00 82:74eb 83:7f63 84:4003 85:74e9 86:3c43 87:4003 88:007f
ata1: dev 0 ATA-6, max UDMA/133, 145226112 sectors: LBA48
ata1: dev 0 configured for UDMA/133
scsi0 : sata_vsc
entering vsc_intr_mask_update
exiting vsc_intr_mask_update
vsc_sata_interrupt: clearing interrupt, status 8300; sata err status 50002
ata2: dev 0 cfg 49:2f00 82:74eb 83:7f63 84:4003 85:74e9 86:3c43 87:4003 88:007f
ata2: dev 0 ATA-6, max UDMA/133, 145226112 sectors: LBA48
ata2: dev 0 configured for UDMA/133
scsi1 : sata_vsc
entering vsc_intr_mask_update
exiting vsc_intr_mask_update
vsc_sata_interrupt: clearing interrupt, status 830000; sata err status 50002
ata3: dev 0 cfg 49:2f00 82:74eb 83:7f63 84:4003 85:74e9 86:3c43 87:4003 88:007f
ata3: dev 0 ATA-6, max UDMA/133, 145226112 sectors: LBA48
ata3: dev 0 configured for UDMA/133
scsi2 : sata_vsc
ata4: no device found (phy stat 00000000)
scsi3 : sata_vsc

 Vendor: ATA       Model: WDC WD740GD-00FL  Rev: 27.0
 Type:   Direct-Access                      ANSI SCSI revision: 05
 Vendor: ATA       Model: WDC WD740GD-00FL  Rev: 27.0
 Type:   Direct-Access                      ANSI SCSI revision: 05
 Vendor: ATA       Model: WDC WD740GD-00FL  Rev: 27.0
 Type:   Direct-Access                      ANSI SCSI revision: 05

[PATCH] Clear Intel 31244 error interrupts in the sata_vsc driver
* This patch represents the result of the following thread:
http://marc.theaimsgroup.com/?t=113459121300006&r=1&w=2

Signed-off-by Dan Williams <[email protected]>

drivers/scsi/sata_vsc.c +41 -5

@@ -81,6 +81,22 @@
 /* Port stride */
 #define VSC_SATA_PORT_OFFSET		0x200
 
+/* Error interrupt status bit offsets */
+#define VSC_SATA_INT_ERROR_CRC		(1 << 6)
+#define VSC_SATA_INT_ERROR_T		(1 << 5)
+#define VSC_SATA_INT_ERROR_P		(1 << 4)
+#define VSC_SATA_INT_ERROR_R		(1 << 3)
+#define VSC_SATA_INT_ERROR_E		(1 << 2)
+#define VSC_SATA_INT_ERROR_M		(1 << 1)
+#define VSC_SATA_INT_PHY_CHANGE	(1 << 0)
+#define VSC_SATA_INT_ERROR (VSC_SATA_INT_ERROR_CRC  + VSC_SATA_INT_ERROR_T + \
+			     VSC_SATA_INT_ERROR_P    + VSC_SATA_INT_ERROR_R + \
+			     VSC_SATA_INT_ERROR_E    + VSC_SATA_INT_ERROR_M + \
+			     VSC_SATA_INT_PHY_CHANGE)
+
+#define is_vsc_sata_int_err(port_idx, int_status) \
+	 (int_status & (VSC_SATA_INT_ERROR << (8 * port_idx)))
+
 
 static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
 {

@@ -102,15 +118,15 @@
 static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
 {
 	void __iomem *mask_addr;
-	u8 mask;
-
+	u8 mask;
+
 	mask_addr = ap->host_set->mmio_base +
 		VSC_SATA_INT_MASK_OFFSET + ap->port_no;
 	mask = readb(mask_addr);
 	if (ctl & ATA_NIEN)
-		mask |= 0x80;
+		mask = 0x80;
 	else
-		mask &= 0x7F;
+		mask = 0;
 	writeb(mask, mask_addr);
 }

@@ -206,8 +222,28 @@
 				struct ata_queued_cmd *qc;
 
 				qc = ata_qc_from_tag(ap, ap->active_tag);
-				if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+				if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
 					handled += ata_host_intr(ap, qc);
+				} else if (is_vsc_sata_int_err(i, int_status)) {
+					/*
+					 * On some chips (i.e. Intel 31244), an error
+					 * interrupt will sneak in at initialization
+					 * time (phy state changes).  Clearing the SCR
+					 * error register is not required, but it prevents
+					 * the phy state change interrupts from recurring
+					 * later.
+					 */
+					u32 err_status;
+					err_status = vsc_sata_scr_read(ap, SCR_ERROR);
+					printk(KERN_DEBUG "%s: clearing interrupt, "
+					       "status %x; sata err status %x\n",
+					       __FUNCTION__,
+					       int_status, err_status);
+					vsc_sata_scr_write(ap, SCR_ERROR, err_status);
+					/* Clear interrupt status */
+					ata_chk_status(ap);
+					handled++;
+				}
 			}
 		}
 	}

drivers/scsi/Kconfig +2 -2

@@ -586,10 +586,10 @@
 	  If unsure, say N.
 
 config SCSI_SATA_VITESSE
-	tristate "VITESSE VSC-7174 SATA support"
+	tristate "VITESSE VSC-7174 / INTEL 31244 SATA support"
 	depends on SCSI_SATA && PCI
 	help
-	  This option enables support for Vitesse VSC7174 Serial ATA.
+	  This option enables support for Vitesse VSC7174 and Intel 31244 Serial ATA.
 
 	  If unsure, say N.

Jeremy Higdon

2006-01-11 09:39 UTC #5 lore raw

Quoted 2 lines · Dan Williams

Surprisingly, I still get the endless error interrupts with this patch applied.

Do you get the endless interrupts even after the mask is cleared? I'm thinking that maybe you're getting them before that point, in which case, we could move the clearing earlier.

If you get error interrupts even after the mask is cleared, then that's very strange.

jeremy

Quoted 7 lines

I integrated this patch with the previous explicit clear patch, and added debug prints to see when vsc_intr_mask_update is called relative to the interrupt handler. Below is the debug output, and attached is the integrated patch with the interrupt mask changes (trimmed for white space against 2.6.15).
Dan