diff mbox

[11/12] libata: use IRQ expecting

Message ID 4C2DFAB0.3020703@kernel.org
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Tejun Heo July 2, 2010, 2:41 p.m. UTC 
Hello, Jeff.

So, something like the following.  This should be applied on top of
the two previous libata patches.  The amount of code in the hot path
is very small.  Compared to the cpu actually taking an interrupt and
accessing hardware, it should be negligible, and this will give us
working and acceptably performing systems in the presence of most
types of IRQ problems.

Thanks.

Subject: [PATCH] libata: use IRQ expecting

Legacy ATA is very susceptible to IRQ delivery problems in both
directions - lost and spurious interrupts.  In traditional PATA, the
IRQ line is ultimately out of the controller and driver's control.

Even relatively new SATA controllers share this problem as many still
emulate the traditional IDE interface which doesn't have reliable way
to indicate interrupt pending state and there also is an issue
regarding the interpretation of nIEN on both sides of the cable.

Controllers with native interface have fewer problems compared to the
ones which use SFF but they still are affected by IRQ misrouting or
broken MSI implementations.

IRQ delivery problems on ATA are particularly nasty because it
commonly hosts installation and/or booting.

Most of these problems can be worked around by using the new IRQ
expecting mechanism without adding any noticeable overhead.  In ATA,
almost all operations are initiated by the host and the controller
signals progress or completion using IRQ.  IRQ expecting can easily be
added in libata core and applied to all libata drivers.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-core.c |   55 ++++++++++++++++++++++++++++++----------------
 drivers/ata/libata-eh.c   |    4 ++-
 drivers/ata/libata-sff.c  |   37 +++++++++++++++---------------
 include/linux/libata.h    |    2 +
 4 files changed, 60 insertions(+), 38 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tejun Heo July 2, 2010, 2:53 p.m. UTC | #1 
On 07/02/2010 04:41 PM, Tejun Heo wrote:
> Hello, Jeff.
> 
> So, something like the following.  This should be applied on top of
> the two previous libata patches.  The amount of code in the hot path
> is very small.  Compared to the cpu actually taking an interrupt and
> accessing hardware, it should be negligible, and this will give us
> working and acceptably performing systems in the presence of most
> types of IRQ problems.

The whole tree is available in the following branch.

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git libata-irq-expect

Thanks.
Tejun Heo July 10, 2010, 10:06 a.m. UTC | #2 
On 07/02/2010 04:41 PM, Tejun Heo wrote:
> Hello, Jeff.
> 
> So, something like the following.  This should be applied on top of
> the two previous libata patches.  The amount of code in the hot path
> is very small.  Compared to the cpu actually taking an interrupt and
> accessing hardware, it should be negligible, and this will give us
> working and acceptably performing systems in the presence of most
> types of IRQ problems.

Ping.
Jeff Garzik July 14, 2010, 7:58 a.m. UTC | #3 
On 07/10/2010 06:06 AM, Tejun Heo wrote:
> On 07/02/2010 04:41 PM, Tejun Heo wrote:
>> Hello, Jeff.
>>
>> So, something like the following.  This should be applied on top of
>> the two previous libata patches.  The amount of code in the hot path
>> is very small.  Compared to the cpu actually taking an interrupt and
>> accessing hardware, it should be negligible, and this will give us
>> working and acceptably performing systems in the presence of most
>> types of IRQ problems.
>
> Ping.

Give me another day or two.  I'm working up an alternate patch for 
demonstration (still use the irq-expecting api, but differently).

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo July 14, 2010, 9:26 a.m. UTC | #4 
On 07/14/2010 09:58 AM, Jeff Garzik wrote:
> Give me another day or two.  I'm working up an alternate patch for
> demonstration (still use the irq-expecting api, but differently).

Sure thing.

Thanks.
Jeff Garzik July 27, 2010, 5:37 p.m. UTC | #5 
On 07/02/2010 10:41 AM, Tejun Heo wrote:
> Hello, Jeff.
>
> So, something like the following.  This should be applied on top of
> the two previous libata patches.  The amount of code in the hot path
> is very small.  Compared to the cpu actually taking an interrupt and
> accessing hardware, it should be negligible, and this will give us
> working and acceptably performing systems in the presence of most
> types of IRQ problems.
>
> Thanks.
>
> Subject: [PATCH] libata: use IRQ expecting
>
> Legacy ATA is very susceptible to IRQ delivery problems in both
> directions - lost and spurious interrupts.  In traditional PATA, the
> IRQ line is ultimately out of the controller and driver's control.
>
> Even relatively new SATA controllers share this problem as many still
> emulate the traditional IDE interface which doesn't have reliable way
> to indicate interrupt pending state and there also is an issue
> regarding the interpretation of nIEN on both sides of the cable.
>
> Controllers with native interface have fewer problems compared to the
> ones which use SFF but they still are affected by IRQ misrouting or
> broken MSI implementations.
>
> IRQ delivery problems on ATA are particularly nasty because it
> commonly hosts installation and/or booting.
>
> Most of these problems can be worked around by using the new IRQ
> expecting mechanism without adding any noticeable overhead.  In ATA,
> almost all operations are initiated by the host and the controller
> signals progress or completion using IRQ.  IRQ expecting can easily be
> added in libata core and applied to all libata drivers.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> ---
>   drivers/ata/libata-core.c |   55 ++++++++++++++++++++++++++++++----------------
>   drivers/ata/libata-eh.c   |    4 ++-
>   drivers/ata/libata-sff.c  |   37 +++++++++++++++---------------
>   include/linux/libata.h    |    2 +
>   4 files changed, 60 insertions(+), 38 deletions(-)

I suppose the few cycles it costs are worth it...

Assuming this new version (from July 2) is tested,

Acked-by: Jeff Garzik <jgarzik@redhat.com>


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -4958,22 +4958,7 @@  static void ata_verify_xfer(struct ata_q
 	dev->flags &= ~ATA_DFLAG_DUBIOUS_XFER;
 }

-/**
- *	ata_qc_complete - Complete an active ATA command
- *	@qc: Command to complete
- *
- *	Indicate to the mid and upper layers that an ATA command has
- *	completed, with either an ok or not-ok status.
- *
- *	Refrain from calling this function multiple times when
- *	successfully completing multiple NCQ commands.
- *	ata_qc_complete_multiple() should be used instead, which will
- *	properly update IRQ expect state.
- *
- *	LOCKING:
- *	spin_lock_irqsave(host lock)
- */
-void ata_qc_complete(struct ata_queued_cmd *qc)
+static void ata_qc_complete_raw(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;

@@ -5052,6 +5037,27 @@  void ata_qc_complete(struct ata_queued_c
 }

 /**
+ *	ata_qc_complete - Complete an active ATA command
+ *	@qc: Command to complete
+ *
+ *	Indicate to the mid and upper layers that an ATA command has
+ *	completed, with either an ok or not-ok status.
+ *
+ *	Refrain from calling this function multiple times when
+ *	successfully completing multiple NCQ commands.
+ *	ata_qc_complete_multiple() should be used instead, which will
+ *	properly update IRQ expect state.
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host lock)
+ */
+void ata_qc_complete(struct ata_queued_cmd *qc)
+{
+	unexpect_irq(qc->ap->irq_expect, false);
+	ata_qc_complete_raw(qc);
+}
+
+/**
  *	ata_qc_complete_multiple - Complete multiple qcs successfully
  *	@ap: port in question
  *	@qc_active: new qc_active mask
@@ -5076,6 +5082,8 @@  int ata_qc_complete_multiple(struct ata_
 	int nr_done = 0;
 	u32 done_mask;

+	unexpect_irq(ap->irq_expect, false);
+
 	done_mask = ap->qc_active ^ qc_active;

 	if (unlikely(done_mask & qc_active)) {
@@ -5090,12 +5098,15 @@  int ata_qc_complete_multiple(struct ata_

 		qc = ata_qc_from_tag(ap, tag);
 		if (qc) {
-			ata_qc_complete(qc);
+			ata_qc_complete_raw(qc);
 			nr_done++;
 		}
 		done_mask &= ~(1 << tag);
 	}

+	if (ap->qc_active)
+		expect_irq(ap->irq_expect);
+
 	return nr_done;
 }

@@ -5162,6 +5173,7 @@  void ata_qc_issue(struct ata_queued_cmd
 	qc->err_mask |= ap->ops->qc_issue(qc);
 	if (unlikely(qc->err_mask))
 		goto err;
+	expect_irq(ap->irq_expect);
 	return;

 sg_err:
@@ -6194,8 +6206,13 @@  int ata_host_activate(struct ata_host *h
 	if (rc)
 		return rc;

-	for (i = 0; i < host->n_ports; i++)
-		ata_port_desc(host->ports[i], "irq %d", irq);
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		if (!ata_port_is_dummy(ap))
+			ap->irq_expect = init_irq_expect(irq, host);
+		ata_port_desc(ap, "irq %d%s", irq, ap->irq_expect ? "+" : "");
+	}

 	rc = ata_host_register(host, sht);
 	/* if failed, just free the IRQ and leave ports alone */
Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -619,8 +619,10 @@  void ata_scsi_error(struct Scsi_Host *ho
 		 * handler doesn't diddle with those qcs.  This must
 		 * be done atomically w.r.t. setting QCFLAG_FAILED.
 		 */
-		if (nr_timedout)
+		if (nr_timedout) {
+			unexpect_irq(ap->irq_expect, true);
 			__ata_port_freeze(ap);
+		}

 		spin_unlock_irqrestore(ap->lock, flags);

Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -751,6 +751,8 @@  struct ata_port {
 	struct ata_host		*host;
 	struct device 		*dev;

+	struct irq_expect	*irq_expect;	/* for irq expecting */
+
 	struct delayed_work	hotplug_task;
 	struct work_struct	scsi_rescan_task;

Index: work/drivers/ata/libata-sff.c
===================================================================
--- work.orig/drivers/ata/libata-sff.c
+++ work/drivers/ata/libata-sff.c
@@ -2388,7 +2388,8 @@  int ata_pci_sff_activate_host(struct ata
 	struct device *dev = host->dev;
 	struct pci_dev *pdev = to_pci_dev(dev);
 	const char *drv_name = dev_driver_string(host->dev);
-	int legacy_mode = 0, rc;
+	struct ata_port *ap[2] = { host->ports[0], host->ports[1] };
+	int legacy_mode = 0, i, rc;

 	rc = ata_host_start(host);
 	if (rc)
@@ -2422,29 +2423,29 @@  int ata_pci_sff_activate_host(struct ata
 		if (rc)
 			goto out;

-		ata_port_desc(host->ports[0], "irq %d", pdev->irq);
-		ata_port_desc(host->ports[1], "irq %d", pdev->irq);
+		for (i = 0; i < 2; i++) {
+			if (!ata_port_is_dummy(ap[i]))
+				ap[i]->irq_expect =
+					init_irq_expect(pdev->irq, host);
+			ata_port_desc(ap[i], "irq %d%s",
+				      pdev->irq, ap[i]->irq_expect ? "+" : "");
+		}
 	} else if (legacy_mode) {
-		if (!ata_port_is_dummy(host->ports[0])) {
-			rc = devm_request_irq(dev, ATA_PRIMARY_IRQ(pdev),
-					      irq_handler, IRQF_SHARED,
-					      drv_name, host);
-			if (rc)
-				goto out;
+		unsigned int irqs[2] = { ATA_PRIMARY_IRQ(pdev),
+					 ATA_SECONDARY_IRQ(pdev) };

-			ata_port_desc(host->ports[0], "irq %d",
-				      ATA_PRIMARY_IRQ(pdev));
-		}
+		for (i = 0; i < 2; i++) {
+			if (ata_port_is_dummy(ap[i]))
+				continue;

-		if (!ata_port_is_dummy(host->ports[1])) {
-			rc = devm_request_irq(dev, ATA_SECONDARY_IRQ(pdev),
-					      irq_handler, IRQF_SHARED,
-					      drv_name, host);
+			rc = devm_request_irq(dev, irqs[i], irq_handler,
+					      IRQF_SHARED, drv_name, host);
 			if (rc)
 				goto out;

-			ata_port_desc(host->ports[1], "irq %d",
-				      ATA_SECONDARY_IRQ(pdev));
+			ap[i]->irq_expect = init_irq_expect(irqs[i], host);
+			ata_port_desc(ap[i], "irq %d%s",
+				      irqs[i], ap[i]->irq_expect ? "+" : "");
 		}
 	}