[PATCH 1/4] libata sata_qstor fix oops on rmmod

[PATCH 1/4] libata sata_qstor fix oops on rmmod

[Mark Lord]

sata_qstor fix oops on rmmod.

sata_qstor likes to disable the chip on module unload,
so it provides a libata "host_stop" method to do this.
But in recent kernels, this routine is now called too late,
after the PCI mmio resources have already been released.
Which produces an oops.

This really needs to be fixed higher up.
For an interim workaround, we disable this function for now.

This belongs in 2.6.24.

Signed-off-by:  Mark Lord <mlord@pobox.com>
---

--- old/drivers/ata/sata_qstor.c	2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/ata/sata_qstor.c	2007-11-06 22:32:34.000000000 -0500
@@ -523,10 +523,20 @@
 
 static void qs_host_stop(struct ata_host *host)
 {
+#if 0
+	/*
+	 * This code used to work, but now segfaults
+	 * because we get called *after* our mmio resources
+	 * have already been unmapped.  That's probably a bug
+	 * in an upper layer somewhere, so this code has been
+	 * disabled (but left in place) until it gets tracked
+	 * down properly.
+	 */
 	void __iomem *mmio_base = qs_mmio_base(host);
 
 	writeb(0, mmio_base + QS_HCT_CTRL); /* disable host interrupts */
 	writeb(QS_CNFG3_GSRST, mmio_base + QS_HCF_CNFG3); /* global reset */
+#endif
 }
 
 static void qs_host_init(struct ata_host *host, unsigned int chip_id)

[PATCH 2/4] libata sata_qstor nuke idle state

[Mark Lord]

sata_qstor nuke idle state.

We're really only ever in one of two hardware states:  packet, or mmio.
Get rid of unnecessary "qs_state_idle" state.

This belongs in 2.6.24.

Signed-off-by:  Mark Lord <mlord@pobox.com>
---

--- old/drivers/ata/sata_qstor.c	2007-11-07 07:41:28.000000000 -0500
+++ linux/drivers/ata/sata_qstor.c	2007-11-07 08:06:29.000000000 -0500
@@ -103,7 +103,7 @@
 	QS_DMA_BOUNDARY		= ~0UL
 };
 
-typedef enum { qs_state_idle, qs_state_pkt, qs_state_mmio } qs_state_t;
+typedef enum { qs_state_mmio, qs_state_pkt } qs_state_t;
 
 struct qs_port_priv {
 	u8			*pkt;
@@ -222,7 +222,9 @@
 static inline void qs_enter_reg_mode(struct ata_port *ap)
 {
 	u8 __iomem *chan = qs_mmio_base(ap->host) + (ap->port_no * 0x4000);
+	struct qs_port_priv *pp = ap->private_data;
 
+	pp->state = qs_state_mmio;
 	writeb(QS_CTR0_REG, chan + QS_CCT_CTR0);
 	readb(chan + QS_CCT_CTR0);        /* flush */
 }
@@ -238,19 +240,12 @@
 
 static void qs_phy_reset(struct ata_port *ap)
 {
-	struct qs_port_priv *pp = ap->private_data;
-
-	pp->state = qs_state_idle;
 	qs_reset_channel_logic(ap);
 	sata_phy_reset(ap);
 }
 
 static void qs_eng_timeout(struct ata_port *ap)
 {
-	struct qs_port_priv *pp = ap->private_data;
-
-	if (pp->state != qs_state_idle) /* healthy paranoia */
-		pp->state = qs_state_mmio;
 	qs_reset_channel_logic(ap);
 	ata_eng_timeout(ap);
 }
@@ -409,7 +404,6 @@
 					switch (sHST) {
 					case 0: /* successful CPB */
 					case 3: /* device error */
-						pp->state = qs_state_idle;
 						qs_enter_reg_mode(qc->ap);
 						qc->err_mask |= ac_err_mask(sDST);
 						ata_qc_complete(qc);
@@ -448,7 +442,6 @@
 					ap->print_id, qc->tf.protocol, status);
 
 				/* complete taskfile transaction */
-				pp->state = qs_state_idle;
 				qc->err_mask |= ac_err_mask(status);
 				ata_qc_complete(qc);
 				handled = 1;
@@ -504,7 +497,6 @@
 	rc = ata_port_start(ap);
 	if (rc)
 		return rc;
-	qs_enter_reg_mode(ap);
 	pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
 	if (!pp)
 		return -ENOMEM;
@@ -515,6 +507,7 @@
 	memset(pp->pkt, 0, QS_PKT_BYTES);
 	ap->private_data = pp;
 
+	qs_enter_reg_mode(ap);
 	addr = (u64)pp->pkt_dma;
 	writel((u32) addr,        chan + QS_CCF_CPBA);
 	writel((u32)(addr >> 32), chan + QS_CCF_CPBA + 4);

Re: [PATCH 2/4] libata sata_qstor nuke idle state

[Jeff Garzik]

On Wed, Nov 07, 2007 at 10:52:55AM -0500, Mark Lord wrote:
> sata_qstor nuke idle state.
> 
> We're really only ever in one of two hardware states:  packet, or mmio.
> Get rid of unnecessary "qs_state_idle" state.
> 
> This belongs in 2.6.24.
> 
> Signed-off-by:  Mark Lord <mlord@pobox.com>

applied patches 2-4

Please put "this belongs in 2.6.24" comment behind the "---" separator,
as described in Documentation/SubmittingPatches, so that Linus's patch
merging tools automatically snip such comments.

	Jeff

[PATCH 3/4] libata sata_qstor workaround for spurious interrupts

[Mark Lord]

sata_qstor workaround for spurious interrupts.

The qstor hardware generates spurious interrupts from time to time when switching
in and out of packet mode.  These eventually result in the IRQ being disabled,
which kills other devices sharing this IRQ with us.

This workaround isn't perfect, but it's about the best we can do for this hardware.
Spurious interrupts will still happen, but won't be logged as such,
and therefore won't cause the IRQ to be inadvertently disabled.

This belongs in 2.6.24.

Signed-off-by:  Mark Lord <mlord@pobox.com>
---

--- old/drivers/ata/sata_qstor.c	2007-11-07 09:12:32.000000000 -0500
+++ linux/drivers/ata/sata_qstor.c	2007-11-07 09:17:41.000000000 -0500
@@ -425,24 +425,27 @@
 		if (ap &&
 		    !(ap->flags & ATA_FLAG_DISABLED)) {
 			struct ata_queued_cmd *qc;
-			struct qs_port_priv *pp = ap->private_data;
-			if (!pp || pp->state != qs_state_mmio)
-				continue;
+			struct qs_port_priv *pp;
 			qc = ata_qc_from_tag(ap, ap->link.active_tag);
-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
-
-				/* check main status, clearing INTRQ */
-				u8 status = ata_check_status(ap);
-				if ((status & ATA_BUSY))
-					continue;
-				DPRINTK("ata%u: protocol %d (dev_stat 0x%X)\n",
-					ap->print_id, qc->tf.protocol, status);
-
-				/* complete taskfile transaction */
-				qc->err_mask |= ac_err_mask(status);
-				ata_qc_complete(qc);
+			if (!qc || !(qc->flags & ATA_QCFLAG_ACTIVE)) {
+				/*
+				 * The qstor hardware generates spurious
+				 * interrupts from time to time when switching
+				 * in and out of packet mode.
+				 * There's no obvious way to know if we're
+				 * here now due to that, so just ack the irq
+				 * and pretend we knew it was ours.. (ugh).
+				 * This does not affect packet mode.
+				 */
+				ata_check_status(ap);
 				handled = 1;
+				continue;
 			}
+			pp = ap->private_data;
+			if (!pp || pp->state != qs_state_mmio)
+				continue;
+			if (!(qc->tf.flags & ATA_TFLAG_POLLING))
+				handled |= ata_host_intr(ap, qc);
 		}
 	}
 	return handled;
@@ -452,12 +455,13 @@
 {
 	struct ata_host *host = dev_instance;
 	unsigned int handled = 0;
+	unsigned long flags;
 
 	VPRINTK("ENTER\n");
 
-	spin_lock(&host->lock);
+	spin_lock_irqsave(&host->lock, flags);
 	handled  = qs_intr_pkt(host) | qs_intr_mmio(host);
-	spin_unlock(&host->lock);
+	spin_unlock_irqrestore(&host->lock, flags);
 
 	VPRINTK("EXIT\n");
 

[PATCH 4/4] libata sata_qstor conversion to new error handling (EH).

[Mark Lord]

sata_qstor conversion to new error handling (EH).

Convert sata_qstor to use the newer libata EH mechanisms.
Based on earlier work by Jeff Garzik.

This belongs in 2.6.24.

Signed-off-by:  Mark Lord <mlord@pobox.com>
---

--- old/drivers/ata/sata_qstor.c	2007-11-07 09:17:41.000000000 -0500
+++ linux/drivers/ata/sata_qstor.c	2007-11-07 10:28:08.000000000 -0500
@@ -116,14 +116,15 @@
 static int qs_ata_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
 static int qs_port_start(struct ata_port *ap);
 static void qs_host_stop(struct ata_host *host);
-static void qs_phy_reset(struct ata_port *ap);
 static void qs_qc_prep(struct ata_queued_cmd *qc);
 static unsigned int qs_qc_issue(struct ata_queued_cmd *qc);
 static int qs_check_atapi_dma(struct ata_queued_cmd *qc);
 static void qs_bmdma_stop(struct ata_queued_cmd *qc);
 static u8 qs_bmdma_status(struct ata_port *ap);
 static void qs_irq_clear(struct ata_port *ap);
-static void qs_eng_timeout(struct ata_port *ap);
+static void qs_freeze(struct ata_port *ap);
+static void qs_thaw(struct ata_port *ap);
+static void qs_error_handler(struct ata_port *ap);
 
 static struct scsi_host_template qs_ata_sht = {
 	.module			= THIS_MODULE,
@@ -150,11 +151,12 @@
 	.check_atapi_dma	= qs_check_atapi_dma,
 	.exec_command		= ata_exec_command,
 	.dev_select		= ata_std_dev_select,
-	.phy_reset		= qs_phy_reset,
 	.qc_prep		= qs_qc_prep,
 	.qc_issue		= qs_qc_issue,
 	.data_xfer		= ata_data_xfer,
-	.eng_timeout		= qs_eng_timeout,
+	.freeze			= qs_freeze,
+	.thaw			= qs_thaw,
+	.error_handler		= qs_error_handler,
 	.irq_clear		= qs_irq_clear,
 	.irq_on			= ata_irq_on,
 	.scr_read		= qs_scr_read,
@@ -169,8 +171,6 @@
 	/* board_2068_idx */
 	{
 		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SATA_RESET |
-				  //FIXME ATA_FLAG_SRST |
 				  ATA_FLAG_MMIO | ATA_FLAG_PIO_POLLING,
 		.pio_mask	= 0x10, /* pio4 */
 		.udma_mask	= ATA_UDMA6,
@@ -235,16 +235,28 @@
 	qs_enter_reg_mode(ap);
 }
 
-static void qs_phy_reset(struct ata_port *ap)
+static void qs_freeze(struct ata_port *ap)
 {
-	qs_reset_channel_logic(ap);
-	sata_phy_reset(ap);
+	u8 __iomem *mmio_base = qs_mmio_base(ap->host);
+
+	writeb(0, mmio_base + QS_HCT_CTRL); /* disable host interrupts */
+	qs_enter_reg_mode(ap);
 }
 
-static void qs_eng_timeout(struct ata_port *ap)
+static void qs_thaw(struct ata_port *ap)
 {
+	u8 __iomem *mmio_base = qs_mmio_base(ap->host);
+
+	qs_enter_reg_mode(ap);
+	writeb(1, mmio_base + QS_HCT_CTRL); /* enable host interrupts */
+}
+
+static int qs_prereset(struct ata_link *link, unsigned long deadline)
+{
+	struct ata_port *ap = link->ap;
+
 	qs_reset_channel_logic(ap);
-	ata_eng_timeout(ap);
+	return ata_std_prereset(link, deadline);
 }
 
 static int qs_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
@@ -255,6 +267,13 @@
 	return 0;
 }
 
+static void qs_error_handler(struct ata_port *ap)
+{
+	qs_enter_reg_mode(ap);
+	ata_do_eh(ap, qs_prereset, ata_std_softreset, NULL,
+		  ata_std_postreset);
+}
+
 static int qs_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	if (sc_reg > SCR_CONTROL)
@@ -353,7 +372,6 @@
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
-
 		pp->state = qs_state_pkt;
 		qs_packet_start(qc);
 		return 0;
@@ -370,6 +388,26 @@
 	return ata_qc_issue_prot(qc);
 }
 
+static void qs_do_or_die(struct ata_queued_cmd *qc, u8 status)
+{
+	qc->err_mask |= ac_err_mask(status);
+
+	if (!qc->err_mask) {
+		ata_qc_complete(qc);
+	} else {
+		struct ata_port    *ap  = qc->ap;
+		struct ata_eh_info *ehi = &ap->link.eh_info;
+
+		ata_ehi_clear_desc(ehi);
+		ata_ehi_push_desc(ehi, "status 0x%02X", status);
+
+		if (qc->err_mask == AC_ERR_DEV)
+			ata_port_abort(ap);
+		else
+			ata_port_freeze(ap);
+	}
+}
+
 static inline unsigned int qs_intr_pkt(struct ata_host *host)
 {
 	unsigned int handled = 0;
@@ -402,8 +440,7 @@
 					case 0: /* successful CPB */
 					case 3: /* device error */
 						qs_enter_reg_mode(qc->ap);
-						qc->err_mask |= ac_err_mask(sDST);
-						ata_qc_complete(qc);
+						qs_do_or_die(qc, sDST);
 						break;
 					default:
 						break;

Re: [PATCH 4/4] libata sata_qstor conversion to new error handling (EH).

[Jeff Garzik]

On Wed, Nov 07, 2007 at 10:54:15AM -0500, Mark Lord wrote:
> sata_qstor conversion to new error handling (EH).
> 
> Convert sata_qstor to use the newer libata EH mechanisms.
> Based on earlier work by Jeff Garzik.
> 
> This belongs in 2.6.24.
> 
> Signed-off-by:  Mark Lord <mlord@pobox.com>

Did you test this with both device probe ("it works") and at least
one device error?

	Jeff

Re: [PATCH 4/4] libata sata_qstor conversion to new error handling (EH).

[Mark Lord]

Jeff Garzik wrote:
> On Wed, Nov 07, 2007 at 10:54:15AM -0500, Mark Lord wrote:
>> sata_qstor conversion to new error handling (EH).
>>
>> Convert sata_qstor to use the newer libata EH mechanisms.
>> Based on earlier work by Jeff Garzik.
>>
>> This belongs in 2.6.24.
>>
>> Signed-off-by:  Mark Lord <mlord@pobox.com>
> 
> Did you test this with both device probe ("it works") and at least
> one device error?

Yes, all four patches tested and working here.
I think there's two of us on the planet using that board with Linux. :)

The EH still isn't perfect -- never recovers from stuck-DRQ,
and SG_IO WRITEs don't work.  That's really no different from
the existing 2.6.24 version.  Regular I/O looks fine.

And this way it stops spamming the syslogs with the new WARN_ON()
calls in libata every time it noticed we didn't have the new EH hooks.

Re: [PATCH 4/4] libata sata_qstor conversion to new error handling (EH).

[Mark Lord]

Mark Lord wrote:
..
> and SG_IO WRITEs don't work.  That's really no different from
> the existing 2.6.24 version.  Regular I/O looks fine.
...

I should qualify that:  SG_IO *PIO* WRITEs don't work.
SG_IO PIO READs do work, and SG_IO DMA R/W both work.

Cheers

Re: [PATCH 4/4] libata sata_qstor conversion to new error handling (EH).

[Jeff Garzik]

On Wed, Nov 07, 2007 at 06:36:13PM -0500, Mark Lord wrote:
> Mark Lord wrote:
> ..
> >and SG_IO WRITEs don't work.  That's really no different from
> >the existing 2.6.24 version.  Regular I/O looks fine.
> ...
> 
> I should qualify that:  SG_IO *PIO* WRITEs don't work.
> SG_IO PIO READs do work, and SG_IO DMA R/W both work.

multi-sector or single-sector PIO?

	Jeff

Re: [PATCH 4/4] libata sata_qstor conversion to new error handling (EH).

[Mark Lord]

Jeff Garzik wrote:
> On Wed, Nov 07, 2007 at 06:36:13PM -0500, Mark Lord wrote:
>> Mark Lord wrote:
>> ..
>>> and SG_IO WRITEs don't work.  That's really no different from
>>> the existing 2.6.24 version.  Regular I/O looks fine.
>> ...
>>
>> I should qualify that:  SG_IO *PIO* WRITEs don't work.
>> SG_IO PIO READs do work, and SG_IO DMA R/W both work.
> 
> multi-sector or single-sector PIO?
..

I was issuing single-sector PIO writes for one sector.
Something gets confused somewhere by that.

It really didn't seem worth tracking down further,
as regular kernel initiated I/O is fine (DMA though),
and this chip doesn't have much of a user/testing base.

I suppose it has to do with the initial DRQ interrupt
from a PIO WRITE --> the chip designers actually never
tested anything other than READ and IDENTIFY in register
PIO mode, so it could even be a chip issue.

The full-fledged qstor driver uses the chip's packet mode
for *everything*, including "PIO" R/W, so this is actually
the first time that direct PIO has even been attempted.
No surprise that it might fail.

If the chip were more commonly available/used, then I would
probably update sata_qstor to use packet mode for PIO as well.
But that's more effort than it's worth, really.

Cheers

Re: [PATCH 1/4] libata sata_qstor fix oops on rmmod

[Mark Lord]

Mark Lord wrote:
>
>This belongs in 2.6.24. 

Note that without these four patches, sata_qstor is not currently functional in 2.6.24.

Re: [PATCH 1/4] libata sata_qstor fix oops on rmmod

[Tejun Heo]

Mark Lord wrote:
> sata_qstor fix oops on rmmod.
> 
> sata_qstor likes to disable the chip on module unload,
> so it provides a libata "host_stop" method to do this.
> But in recent kernels, this routine is now called too late,
> after the PCI mmio resources have already been released.
> Which produces an oops.
> 
> This really needs to be fixed higher up.
> For an interim workaround, we disable this function for now.

Ah.. you're right.  port and host stop should be called before any host
resource is torn down.  I'll submit a patch soon.  Thanks.

-- 
tejun

Re: [PATCH 1/4] libata sata_qstor fix oops on rmmod

[Mark Lord]

Mark Lord wrote:
> sata_qstor fix oops on rmmod.
> 
> sata_qstor likes to disable the chip on module unload,
> so it provides a libata "host_stop" method to do this.
> But in recent kernels, this routine is now called too late,
> after the PCI mmio resources have already been released.
> Which produces an oops.
> 
> This really needs to be fixed higher up.
> For an interim workaround, we disable this function for now.
> 
> This belongs in 2.6.24.
> 
> Signed-off-by:  Mark Lord <mlord@pobox.com>
> ---
> 
> --- old/drivers/ata/sata_qstor.c    2007-10-12 12:43:44.000000000 -0400
> +++ linux/drivers/ata/sata_qstor.c    2007-11-06 22:32:34.000000000 -0500
> @@ -523,10 +523,20 @@
..

Tejun Heo wrote:
>[PATCH #upstream-fixes] libata: port and host should be stopped before hardware resources are released
..

Once Tejun's fix is applied, this particular sata_qstor patch [1/4]
is no longer necessary and should be dropped.

-ml