[PATCH 0/2] sata_vsc regression fix and vsc_sata_interrupt_cleanup

[PATCH 0/2] sata_vsc regression fix and vsc_sata_interrupt_cleanup

[Dan Williams]

Tejun,

The following patches clean up vsc_sata_interrupt according to your recommendations in the thread "sata_vsc with iq3124h regression in 2.6.20".

Dan Williams (2):
      sata_vsc: factor the error and normal intr paths into separate routines
      sata_vsc: clean up vsc_sata_interrupt

* Based on Linus' latest git tree (commit: e696268a73f7f59a333624f9abf8ffc9412ee64c)
* Tested with an iq3124h PCI-X add-in card on an iop13xx platform
* Interrupt based hotplug is also enabled by these patches

Regards,

Dan

[PATCH 1/2] sata_vsc: factor the error and normal intr paths into separate routines

[Dan Williams]

Separate sata_vsc interrupt handling into a normal (per-port) path and an
error path with the addition of vsc_port_intr and vsc_error_intr
respectively.  The error path handles interrupt based
hotplug events which requires the definition of vsc_freeze and vsc_thaw.

Note: vsc_port_intr has a workaround for unexpected interrupts that occur
during an identify command.

Cc: Jeremy Higdon <jeremy@sgi.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

 drivers/ata/sata_vsc.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/sata_vsc.c b/drivers/ata/sata_vsc.c
index 2fd037b..cc73797 100644
--- a/drivers/ata/sata_vsc.c
+++ b/drivers/ata/sata_vsc.c
@@ -96,6 +96,7 @@ enum {
 			      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),
+	VSC_SATA_SERR_INVALID_FIS	= (1 << 25),
 };
 
 #define is_vsc_sata_int_err(port_idx, int_status) \
@@ -119,6 +120,26 @@ static void vsc_sata_scr_write (struct ata_port *ap, unsigned int sc_reg,
 }
 
 
+static void vsc_freeze(struct ata_port *ap)
+{
+	void __iomem *mask_addr;
+
+	mask_addr = ap->host->iomap[VSC_MMIO_BAR] +
+		VSC_SATA_INT_MASK_OFFSET + ap->port_no;
+
+	writeb(0, mask_addr);
+}
+
+static void vsc_thaw(struct ata_port *ap)
+{
+	void __iomem *mask_addr;
+
+	mask_addr = ap->host->iomap[VSC_MMIO_BAR] +
+		VSC_SATA_INT_MASK_OFFSET + ap->port_no;
+
+	writeb(0xff, mask_addr);
+}
+
 static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
 {
 	void __iomem *mask_addr;
@@ -203,6 +224,61 @@ static void vsc_sata_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
         }
 }
 
+static void vsc_error_intr(struct ata_port *ap)
+{
+	u32 irq_stat;
+	struct ata_eh_info *ehi = &ap->eh_info;
+	int freeze = 0;
+
+	/* clear the error interrupt */
+	irq_stat = vsc_sata_scr_read(ap, SCR_ERROR);
+	vsc_sata_scr_write(ap, SCR_ERROR, irq_stat);
+
+	/* first, analyze and record host port events */
+	ata_ehi_clear_desc(ehi);
+
+	ata_ehi_push_desc(ehi, "irq_stat 0x%08x", irq_stat);
+
+	if (irq_stat & SERR_PHYRDY_CHG) {
+		ata_ehi_hotplugged(ehi);
+		ata_ehi_push_desc(ehi, ", PHY RDY changed");
+		freeze = 1;
+	}
+
+	if (irq_stat & VSC_SATA_SERR_INVALID_FIS) {
+		ehi->err_mask |= AC_ERR_HSM;
+		ehi->action |= ATA_EH_SOFTRESET;
+		ata_ehi_push_desc(ehi , ", unknown FIS");
+		freeze = 1;
+	}
+
+	/* freeze or abort */
+	if (freeze)
+		ata_port_freeze(ap);
+	else
+		ata_port_abort(ap);
+
+}
+
+static void vsc_port_intr(u8 port_status, struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc;
+
+	if (unlikely(port_status & VSC_SATA_INT_ERROR)) {
+		vsc_error_intr(ap);
+		return;
+	}
+
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	if (qc) {
+		if(likely(!(qc->tf.flags & ATA_TFLAG_POLLING)))
+			ata_host_intr(ap, qc);
+		else if (qc->tf.command == ATA_CMD_ID_ATA) {
+			/* handle quirky interrupts during identify */
+			ata_chk_status(ap);
+		}
+	}
+}
 
 /*
  * vsc_sata_interrupt

[PATCH 2/2] sata_vsc: clean up vsc_sata_interrupt

[Dan Williams]

1/ Clean up vsc_sata_interrupt and have it call vsc_port_intr to handle the
interrupt.
2/ Enable all interrupts in vsc_intr_mask_update to detect errors and
hotplug events
3/ Hook up vsc_freeze and vsc_thaw in vsc_sata_ops
4/ Remove the now unnecessary is_vsc_sata_int_err

Tested with an iq3124h PCI-X add-in card on an iop13xx.

Cc: Jeremy Higdon <jeremy@sgi.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

 drivers/ata/sata_vsc.c |   74 +++++++++++++++---------------------------------
 1 files changed, 23 insertions(+), 51 deletions(-)

diff --git a/drivers/ata/sata_vsc.c b/drivers/ata/sata_vsc.c
index cc73797..48d8675 100644
--- a/drivers/ata/sata_vsc.c
+++ b/drivers/ata/sata_vsc.c
@@ -99,10 +99,6 @@ enum {
 	VSC_SATA_SERR_INVALID_FIS	= (1 << 25),
 };
 
-#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)
 {
 	if (sc_reg > SCR_CONTROL)
@@ -149,9 +145,9 @@ static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
 		VSC_SATA_INT_MASK_OFFSET + ap->port_no;
 	mask = readb(mask_addr);
 	if (ctl & ATA_NIEN)
-		mask |= 0x80;
+		mask = 0xff;
 	else
-		mask &= 0x7F;
+		mask = 0x0;
 	writeb(mask, mask_addr);
 }
 
@@ -290,59 +286,35 @@ static irqreturn_t vsc_sata_interrupt (int irq, void *dev_instance)
 	struct ata_host *host = dev_instance;
 	unsigned int i;
 	unsigned int handled = 0;
-	u32 int_status;
+	u32 status;
 
-	spin_lock(&host->lock);
+	status = readl(host->iomap[VSC_MMIO_BAR] + VSC_SATA_INT_STAT_OFFSET);
 
-	int_status = readl(host->iomap[VSC_MMIO_BAR] +
-			   VSC_SATA_INT_STAT_OFFSET);
-
-	for (i = 0; i < host->n_ports; i++) {
-		if (int_status & ((u32) 0xFF << (8 * i))) {
-			struct ata_port *ap;
+	if (status == 0xffffffff) {
+		printk(KERN_ERR DRV_NAME ": IRQ status == 0xffffffff, "
+			"PCI fault or device removal?\n");
+		goto out;
+	} else if (!status)
+		goto out;
 
-			ap = host->ports[i];
+	spin_lock(&host->lock);
 
-			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++;
-			}
+	for (i = 0; i < host->n_ports; i++) {
+		u8 port_status = (status >> (8 * i)) & 0xff;
+		if (port_status) {
+			struct ata_port *ap = host->ports[i];
 
 			if (ap && !(ap->flags & ATA_FLAG_DISABLED)) {
-				struct ata_queued_cmd *qc;
-
-				qc = ata_qc_from_tag(ap, ap->active_tag);
-				if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
-					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++;
-				}
-			}
+				vsc_port_intr(port_status, ap);
+				handled++;
+			} else
+				printk(KERN_ERR DRV_NAME
+					": interrupt from disabled port %d\n", i);
 		}
 	}
 
 	spin_unlock(&host->lock);
-
+out:
 	return IRQ_RETVAL(handled);
 }
 
@@ -380,8 +352,8 @@ static const struct ata_port_operations vsc_sata_ops = {
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
 	.data_xfer		= ata_data_xfer,
-	.freeze			= ata_bmdma_freeze,
-	.thaw			= ata_bmdma_thaw,
+	.freeze			= vsc_freeze,
+	.thaw			= vsc_thaw,
 	.error_handler		= ata_bmdma_error_handler,
 	.post_internal_cmd	= ata_bmdma_post_internal_cmd,
 	.irq_handler		= vsc_sata_interrupt,

Re: [PATCH 1/2] sata_vsc: factor the error and normal intr paths into separate routines

[Tejun Heo]

Hello,

On Tue, Feb 20, 2007 at 06:21:27PM -0700, Dan Williams wrote:
> @@ -119,6 +120,26 @@ static void vsc_sata_scr_write (struct ata_port *ap, unsigned int sc_reg,
>  }
>  
>  
> +static void vsc_freeze(struct ata_port *ap)
> +{
> +	void __iomem *mask_addr;
> +
> +	mask_addr = ap->host->iomap[VSC_MMIO_BAR] +
> +		VSC_SATA_INT_MASK_OFFSET + ap->port_no;
> +
> +	writeb(0, mask_addr);
> +}
> +
> +static void vsc_thaw(struct ata_port *ap)
> +{
> +	void __iomem *mask_addr;
> +
> +	mask_addr = ap->host->iomap[VSC_MMIO_BAR] +
> +		VSC_SATA_INT_MASK_OFFSET + ap->port_no;
> +
> +	writeb(0xff, mask_addr);
> +}
> +

I think it's better to define them in the same patch they are used.

>  static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
>  {
>  	void __iomem *mask_addr;
> @@ -203,6 +224,61 @@ static void vsc_sata_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
>          }
>  }
>  
> +static void vsc_error_intr(struct ata_port *ap)
> +{
> +	u32 irq_stat;
> +	struct ata_eh_info *ehi = &ap->eh_info;
> +	int freeze = 0;
> +
> +	/* clear the error interrupt */
> +	irq_stat = vsc_sata_scr_read(ap, SCR_ERROR);
> +	vsc_sata_scr_write(ap, SCR_ERROR, irq_stat);
> +
> +	/* first, analyze and record host port events */
> +	ata_ehi_clear_desc(ehi);
> +
> +	ata_ehi_push_desc(ehi, "irq_stat 0x%08x", irq_stat);
> +
> +	if (irq_stat & SERR_PHYRDY_CHG) {
> +		ata_ehi_hotplugged(ehi);
> +		ata_ehi_push_desc(ehi, ", PHY RDY changed");
> +		freeze = 1;
> +	}
> +
> +	if (irq_stat & VSC_SATA_SERR_INVALID_FIS) {
> +		ehi->err_mask |= AC_ERR_HSM;
> +		ehi->action |= ATA_EH_SOFTRESET;
> +		ata_ehi_push_desc(ehi , ", unknown FIS");
> +		freeze = 1;
> +	}
> +
> +	/* freeze or abort */
> +	if (freeze)
> +		ata_port_freeze(ap);
> +	else
> +		ata_port_abort(ap);
> +
> +}

Standard SError values are analyzed by standard EH.  Please take a
look at libata-eh.c:ata_eh_analyze_serror().  Bit 25 is currently
ignored by the standard analyze code as it's in DIAG area which
indicates non-fatal issues.  If the bit doesn't carry any special
meaning for vsc, you can simply trust the standard code.

	serror = read_serror();
	clear_serror();

	ehi->serror |= serror;

	if (serror & FREEZE_MASK)
		ata_port_freeze(ap);
	else
		ata_port_abort(ap);

If Bit 25 needs special handling for vsc but all other bits are
standard, you can do something like the following.

	serror = read_serror();
	clear_serror();

	if (serror & INVALID_FIS) {
		/* comment that this bit is defined differently */
		do whatever necessary;
		serror &= ~INVALID_FIS;
	}

	ehi->serror |= serror;

	if (serror & FREEZE_MASK)
		ata_port_freeze(ap);
	else
		ata_port_abort(ap);

This should enable all the standard SError analysis including hotplug
detection.  If SError is defined in such a way that only those two
bits are meaningful, the current code should be fine.

Note that if you can mute IRQ (freeze the port) without clearing
SError, you don't need to do it in the IRQ handler, the standard EH
code will take care of it.

> +static void vsc_port_intr(u8 port_status, struct ata_port *ap)
> +{
> +	struct ata_queued_cmd *qc;
> +
> +	if (unlikely(port_status & VSC_SATA_INT_ERROR)) {
> +		vsc_error_intr(ap);
> +		return;
> +	}
> +
> +	qc = ata_qc_from_tag(ap, ap->active_tag);
> +	if (qc) {
> +		if(likely(!(qc->tf.flags & ATA_TFLAG_POLLING)))
> +			ata_host_intr(ap, qc);

if( -> if (

> +		else if (qc->tf.command == ATA_CMD_ID_ATA) {
> +			/* handle quirky interrupts during identify */
> +			ata_chk_status(ap);
> +		}
> +	}
> +}

Is it only for identify?  Not for all polling commands?  As vsc has
reliable interrupt reporting, I think it will be safer to clear IRQ
always if the irq is raised by the controller.

Thanks.

-- 
tejun

Re: [PATCH 2/2] sata_vsc: clean up vsc_sata_interrupt

[Tejun Heo]

Hello,

On Tue, Feb 20, 2007 at 06:21:32PM -0700, Dan Williams wrote:
> @@ -149,9 +145,9 @@ static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
>  		VSC_SATA_INT_MASK_OFFSET + ap->port_no;
>  	mask = readb(mask_addr);
>  	if (ctl & ATA_NIEN)
> -		mask |= 0x80;
> +		mask = 0xff;
>  	else
> -		mask &= 0x7F;
> +		mask = 0x0;
>  	writeb(mask, mask_addr);

Wouldn't it be better to listen for SATA error interrupt
(e.g. hotplug) even during polling commands?

> @@ -290,59 +286,35 @@ static irqreturn_t vsc_sata_interrupt (int irq, void *dev_instance)
>  	struct ata_host *host = dev_instance;
>  	unsigned int i;
>  	unsigned int handled = 0;
> -	u32 int_status;
> +	u32 status;
>  
> -	spin_lock(&host->lock);
> +	status = readl(host->iomap[VSC_MMIO_BAR] + VSC_SATA_INT_STAT_OFFSET);
>  
> -	int_status = readl(host->iomap[VSC_MMIO_BAR] +
> -			   VSC_SATA_INT_STAT_OFFSET);
> -
> -	for (i = 0; i < host->n_ports; i++) {
> -		if (int_status & ((u32) 0xFF << (8 * i))) {
> -			struct ata_port *ap;
> +	if (status == 0xffffffff) {
> +		printk(KERN_ERR DRV_NAME ": IRQ status == 0xffffffff, "
> +			"PCI fault or device removal?\n");
> +		goto out;
> +	} else if (!status)
> +		goto out;

I think unlikely() here will be helpful.  e.g.

	if (unlikely(status == 0 || status == 0xffffffff)) {
		/* blah blah */
		goto out;
	}

Thanks.

-- 
tejun