Skip to contentSkip to conversation

: bug fixing and updates

PATCH In review Bryan Wu 2 participants 7 messages Apr 25, 2008 libata

lore
Share & tools
lore
author
#1 lore raw

PATCH 1/3 : decrease count first.

From: Roel Kluin <[email protected]>

When count reaches 0 the postfix decrement still subtracts (to -1), so bfin_reset_controller() returns as if the busy flag was cleared while it was not.

---
 drivers/ata/pata_bf54x.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/ata/pata_bf54x.c +1 -1
index 0a5ad98..f334ba5 100644
@@ -1417,7 +1417,7 @@ static int bfin_reset_controller(struct ata_host *host)
 	count = 10000000;
 	do {
 		status = read_atapi_register(base, ATA_REG_STATUS);
-	} while (count-- && (status & ATA_BUSY));
+	} while (--count && (status & ATA_BUSY));
 
 	/* Enable only ATAPI Device interrupt */
 	ATAPI_SET_INT_MASK(base, 1);
Signature · 1 line
1.5.5
author
#2 lore raw

PATCH 2/3 : Fix bug - Implement bfin ata interrupt handler to avoid "irq 68 nobody cared"

In reply to Bryan Wu

From: Sonic Zhang <[email protected]>

Return IRQ_HANDLED when bfin ata device is busy.
http://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?action=TrackerItemEdit&tracker_item_id=3513

---
 drivers/ata/pata_bf54x.c |  125 ++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 121 insertions(+), 4 deletions(-)
drivers/ata/pata_bf54x.c +121 -4
index f334ba5..4142894 100644
@@ -1272,8 +1272,8 @@ static void bfin_freeze(struct ata_port *ap)
 
 void bfin_thaw(struct ata_port *ap)
 {
+	dev_dbg(ap->dev, "in atapi dma thaw\n");
 	bfin_check_status(ap);
-	bfin_irq_clear(ap);
 	bfin_irq_on(ap);
 }
 
@@ -1339,13 +1339,131 @@ static int bfin_port_start(struct ata_port *ap)
 	return 0;
 }
 
+inline unsigned int bfin_ata_host_intr(struct ata_port *ap,
+				   struct ata_queued_cmd *qc)
+{
+	struct ata_eh_info *ehi = &ap->eh_info;
+	u8 status, host_stat = 0;
+
+	VPRINTK("ata%u: protocol %d task_state %d\n",
+		ap->print_id, qc->tf.protocol, ap->hsm_task_state);
+
+	/* Check whether we are expecting interrupt in this state */
+	switch (ap->hsm_task_state) {
+	case HSM_ST_FIRST:
+		/* Some pre-ATAPI-4 devices assert INTRQ
+		 * at this state when ready to receive CDB.
+		 */
+
+		/* Check the ATA_DFLAG_CDB_INTR flag is enough here.
+		 * The flag was turned on only for atapi devices.
+		 * No need to check is_atapi_taskfile(&qc->tf) again.
+		 */
+		if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
+			goto idle_irq;
+		break;
+	case HSM_ST_LAST:
+		if (qc->tf.protocol == ATA_PROT_DMA ||
+		    qc->tf.protocol == ATAPI_PROT_DMA) {
+			/* check status of DMA engine */
+			host_stat = ap->ops->bmdma_status(ap);
+			VPRINTK("ata%u: host_stat 0x%X\n",
+				ap->print_id, host_stat);
+
+			/* if it's not our irq... */
+			if (!(host_stat & ATA_DMA_INTR))
+				goto idle_irq;
+
+			/* before we do anything else, clear DMA-Start bit */
+			ap->ops->bmdma_stop(qc);
+
+			if (unlikely(host_stat & ATA_DMA_ERR)) {
+				/* error when transfering data to/from memory */
+				qc->err_mask |= AC_ERR_HOST_BUS;
+				ap->hsm_task_state = HSM_ST_ERR;
+			}
+		}
+		break;
+	case HSM_ST:
+		break;
+	default:
+		goto idle_irq;
+	}
+
+	/* check altstatus */
+	status = ap->ops->sff_check_altstatus(ap);
+	if (status & ATA_BUSY)
+		goto busy_ata;
+
+	/* check main status, clearing INTRQ */
+	status = ap->ops->sff_check_status(ap);
+	if (unlikely(status & ATA_BUSY))
+		goto busy_ata;
+
+	/* ack bmdma irq events */
+	ap->ops->sff_irq_clear(ap);
+
+	ata_sff_hsm_move(ap, qc, status, 0);
+
+	if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
+				       qc->tf.protocol == ATAPI_PROT_DMA))
+		ata_ehi_push_desc(ehi, "BMDMA stat 0x%x", host_stat);
+
+busy_ata:
+	return 1;	/* irq handled */
+
+idle_irq:
+	ap->stats.idle_irq++;
+
+#ifdef ATA_IRQ_TRAP
+	if ((ap->stats.idle_irq % 1000) == 0) {
+		ap->ops->irq_ack(ap, 0); /* debug trap */
+		ata_port_printk(ap, KERN_WARNING, "irq trap\n");
+		return 1;
+	}
+#endif
+	return 0;	/* irq not handled */
+}
+
+irqreturn_t bfin_ata_interrupt(int irq, void *dev_instance)
+{
+	struct ata_host *host = dev_instance;
+	unsigned int i;
+	unsigned int handled = 0;
+	unsigned long flags;
+
+	/* TODO: make _irqsave conditional on x86 PCI IDE legacy mode */
+	spin_lock_irqsave(&host->lock, flags);
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap;
+
+		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)) &&
+			    (qc->flags & ATA_QCFLAG_ACTIVE))
+				handled |= bfin_ata_host_intr(ap, qc);
+
+		}
+	}
+
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	return IRQ_RETVAL(handled);
+}
+
+
 static struct scsi_host_template bfin_sht = {
 	ATA_BASE_SHT(DRV_NAME),
 	.sg_tablesize		= SG_NONE,
 	.dma_boundary		= ATA_DMA_BOUNDARY,
 };
 
-static const struct ata_port_operations bfin_pata_ops = {
+static struct ata_port_operations bfin_pata_ops = {
 	.inherits		= &ata_sff_port_ops,
 
 	.set_piomode		= bfin_set_piomode,
@@ -1370,7 +1488,6 @@ static const struct ata_port_operations bfin_pata_ops = {
 	.thaw			= bfin_thaw,
 	.softreset		= bfin_softreset,
 	.postreset		= bfin_postreset,
-	.post_internal_cmd	= bfin_bmdma_stop,
 
 	.sff_irq_clear		= bfin_irq_clear,
 	.sff_irq_on		= bfin_irq_on,
@@ -1507,7 +1624,7 @@ static int __devinit bfin_atapi_probe(struct platform_device *pdev)
 	}
 
 	if (ata_host_activate(host, platform_get_irq(pdev, 0),
-		ata_sff_interrupt, IRQF_SHARED, &bfin_sht) != 0) {
+		bfin_ata_interrupt, IRQF_SHARED, &bfin_sht) != 0) {
 		peripheral_free_list(atapi_io_port);
 		dev_err(&pdev->dev, "Fail to attach ATAPI device\n");
 		return -ENODEV;
Signature · 1 line
1.5.5
#3 lore raw

Re: [PATCH 2/3] [libata/BF54x]: Fix bug - Implement bfin ata interrupt handler to avoid "irq 68 nobody cared"

Quoted 10 lines · Bryan Wu

From: Sonic Zhang <[email protected]>

Return IRQ_HANDLED when bfin ata device is busy.
http://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?action=TrackerItemEdit&tracker_item_id=3513

---
 drivers/ata/pata_bf54x.c |  125 ++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 121 insertions(+), 4 deletions(-)

AFAICS the two new functions you add should be (a) marked 'static' and (b) not marked 'inline'.

We avoid inline these days, preferring to let the compiler make the best choice for that platform/CPU.

Jeff

author
#4 lore raw

Re: [PATCH 2/3] [libata/BF54x]: Fix bug - Implement bfin ata interrupt handler to avoid "irq 68 nobody cared"

Quoted 13 lines · Jeff Garzik
Quoted 4 lines · Bryan Wu

From: Sonic Zhang <[email protected]>

Return IRQ_HANDLED when bfin ata device is busy.

http://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?action=TrackerItemEdit&tracker_item_id=3513

Quoted 5 lines
---
 drivers/ata/pata_bf54x.c |  125

++++++++++++++++++++++++++++++++++++++++++++--

Quoted 9 lines
Quoted 2 lines
 1 files changed, 121 insertions(+), 4 deletions(-)

AFAICS the two new functions you add should be (a) marked 'static' and (b)
not marked 'inline'.

We avoid inline these days, preferring to let the compiler make the best
choice for that platform/CPU.

No problem, I will fix it.

-Bryan

author
#5 lore raw

PATCH 3/3 : Fix compling failure bug - change variable's name according to API update

In reply to Bryan Wu

From: Sonic Zhang <[email protected]>

---
 drivers/ata/pata_bf54x.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
drivers/ata/pata_bf54x.c +2 -3
index 4142894..94b818e 100644
@@ -1342,7 +1342,7 @@ static int bfin_port_start(struct ata_port *ap)
 inline unsigned int bfin_ata_host_intr(struct ata_port *ap,
 				   struct ata_queued_cmd *qc)
 {
-	struct ata_eh_info *ehi = &ap->eh_info;
+	struct ata_eh_info *ehi = &ap->link.eh_info;
 	u8 status, host_stat = 0;
 
 	VPRINTK("ata%u: protocol %d task_state %d\n",
@@ -1443,11 +1443,10 @@ irqreturn_t bfin_ata_interrupt(int irq, void *dev_instance)
 		    !(ap->flags & ATA_FLAG_DISABLED)) {
 			struct ata_queued_cmd *qc;
 
-			qc = ata_qc_from_tag(ap, ap->active_tag);
+			qc = ata_qc_from_tag(ap, ap->link.active_tag);
 			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
 			    (qc->flags & ATA_QCFLAG_ACTIVE))
 				handled |= bfin_ata_host_intr(ap, qc);
-
 		}
 	}
 
Signature · 1 line
1.5.5
#6 lore raw

Re: [PATCH 3/3] [libata/BF54x]: Fix compling failure bug - change variable's name according to API update

Quoted 22 lines · Bryan Wu

From: Sonic Zhang <[email protected]>

---
 drivers/ata/pata_bf54x.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
drivers/ata/pata_bf54x.c +1 -1
index 4142894..94b818e 100644
@@ -1342,7 +1342,7 @@ static int bfin_port_start(struct ata_port *ap)
 inline unsigned int bfin_ata_host_intr(struct ata_port *ap,
 				   struct ata_queued_cmd *qc)
 {
-	struct ata_eh_info *ehi = &ap->eh_info;
+	struct ata_eh_info *ehi = &ap->link.eh_info;
 	u8 status, host_stat = 0;
 
 	VPRINTK("ata%u: protocol %d task_state %d\n",

drivers/ata/pata_bf54x.c irqreturn_t bfin_ata_interrupt(int irq, void *dev_instance) · line 1449

Quoted 8 lines
drivers/ata/pata_bf54x.c +1 -1
@@ -1443,11 +1443,10 @@ irqreturn_t bfin_ata_interrupt(int irq, void *dev_instance)
 		    !(ap->flags & ATA_FLAG_DISABLED)) {
 			struct ata_queued_cmd *qc;
 
-			qc = ata_qc_from_tag(ap, ap->active_tag);
+			qc = ata_qc_from_tag(ap, ap->link.active_tag);
 			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
 			    (qc->flags & ATA_QCFLAG_ACTIVE))
 				handled |= bfin_ata_host_intr(ap, qc);

you need to combine this patch with the first patch, to form a single logical whole.

Build fixes should always be folded into the patch that introduces that problem (unless the patch is already upstream, of course). Otherwise, you potentially introduce git-bisect breakage.

Jeff