Skip to contentSkip to conversation

libata: misc updates in preparation of PMP support

PATCH In review Tejun Heo 2 participants 27 messages Jul 1, 2007 – Jul 13, 2007

Subsystems (2)
libata libata SATA promise TX2/TX4 controller driver
lore
Share & tools
lore
author
#4 lore raw

PATCH 2/12 libata: implement AC_ERR_NCQ

In reply to Tejun Heo

When an NCQ command fails, all commands in flight are aborted and the offending one is reported using log page 10h. Depending on controller characteristics and LLD implementation, all commands may appear as having a device error due to shared TF status making it hard to determine what's actually going on.

This patch adds AC_ERR_NCQ, marks the command reported by log page 10h with it and print extra "<F>" after the error report for the command to help distinguishing the offending command.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-eh.c |    7 ++++---
 include/linux/libata.h  |    3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)
drivers/ata/libata-eh.c +4 -3
index 744665a..8d8961f 100644
@@ -1130,7 +1130,7 @@ static void ata_eh_analyze_ncq_error(struct ata_port *ap)
 	/* we've got the perpetrator, condemn it */
 	qc = __ata_qc_from_tag(ap, tag);
 	memcpy(&qc->result_tf, &tf, sizeof(tf));
-	qc->err_mask |= AC_ERR_DEV;
+	qc->err_mask |= AC_ERR_DEV | AC_ERR_NCQ;
 	ehc->i.err_mask &= ~AC_ERR_DEV;
 }
 

@@ -1551,7 +1551,7 @@ static void ata_eh_report(struct ata_port *ap)
 			"cmd %02x/%02x:%02x:%02x:%02x:%02x/%02x:%02x:%02x:%02x:%02x/%02x "
 			"tag %d cdb 0x%x data %u %s\n         "
 			"res %02x/%02x:%02x:%02x:%02x:%02x/%02x:%02x:%02x:%02x:%02x/%02x "
-			"Emask 0x%x (%s)\n",
+			"Emask 0x%x (%s)%s\n",
 			cmd->command, cmd->feature, cmd->nsect,
 			cmd->lbal, cmd->lbam, cmd->lbah,
 			cmd->hob_feature, cmd->hob_nsect,

@@ -1562,7 +1562,8 @@ static void ata_eh_report(struct ata_port *ap)
 			res->lbal, res->lbam, res->lbah,
 			res->hob_feature, res->hob_nsect,
 			res->hob_lbal, res->hob_lbam, res->hob_lbah,
-			res->device, qc->err_mask, ata_err_string(qc->err_mask));
+			res->device, qc->err_mask, ata_err_string(qc->err_mask),
+			qc->err_mask & AC_ERR_NCQ ? " <F>" : "");
 	}
 }
include/linux/libata.h +2 -1
index 8d3e391..c2cad0f 100644
@@ -322,7 +322,8 @@ enum ata_completion_errors {
 	AC_ERR_SYSTEM		= (1 << 6), /* system error */
 	AC_ERR_INVALID		= (1 << 7), /* invalid argument */
 	AC_ERR_OTHER		= (1 << 8), /* unknown */
-	AC_ERR_NODEV_HINT	= (1 << 9), /* polling device detection hint */
+	AC_ERR_NCQ		= (1 << 9), /* marker for offending NCQ qc */
+	AC_ERR_NODEV_HINT	= (1 << 10), /* polling device detection hint */
 };
 
 /* forward declarations */
Signature · 1 line
1.5.0.3
author
#9 lore raw

PATCH 4/12 ahci: make NO_NCQ handling more consistent

In reply to Tejun Heo

ahci_save_initial_config() is responsible for reading, screening the host CAP register and storing the modified result into hpriv->cap for the rest of the driver. Move ATA_FLAG_NO_NCQ handling into ahci_save_initial_config(). It's more consistent this way and the rest of the driver can always refer to hpriv->cap to determine configured capability.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/ahci.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
drivers/ata/ahci.c +8 -2
index 0e001ad..0555db5 100644
@@ -527,13 +527,19 @@ static void ahci_save_initial_config(struct pci_dev *pdev,
 	hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
 	hpriv->saved_port_map = port_map = readl(mmio + HOST_PORTS_IMPL);
 
-	/* some chips lie about 64bit support */
+	/* some chips lie about their capabilities, bust them */
 	if ((cap & HOST_CAP_64) && (pi->flags & AHCI_FLAG_32BIT_ONLY)) {
 		dev_printk(KERN_INFO, &pdev->dev,
 			   "controller can't do 64bit DMA, forcing 32bit\n");
 		cap &= ~HOST_CAP_64;
 	}
 
+	if ((cap & HOST_CAP_NCQ) && (pi->flags & AHCI_FLAG_NO_NCQ)) {
+		dev_printk(KERN_INFO, &pdev->dev,
+			   "controller can't do NCQ, turning off CAP_NCQ\n");
+		cap &= ~HOST_CAP_NCQ;
+	}
+
 	/* fixup zero port_map */
 	if (!port_map) {
 		port_map = (1 << ahci_nr_ports(cap)) - 1;

@@ -1752,7 +1758,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	ahci_save_initial_config(pdev, &pi, hpriv);
 
 	/* prepare host */
-	if (!(pi.flags & AHCI_FLAG_NO_NCQ) && (hpriv->cap & HOST_CAP_NCQ))
+	if (hpriv->cap & HOST_CAP_NCQ)
 		pi.flags |= ATA_FLAG_NCQ;
 
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, fls(hpriv->port_map));
Signature · 1 line
1.5.0.3
author
#1 lore raw

PATCH 1/12 libata: update EH report formatting

In reply to Tejun Heo

Update formatting for LLD provided error descriptions such that...

* No default () around it
* Each element is separated by <> not ,

This change makes each element responsible for putting a space after itself instead of before - e.g. "<elem> " not ", elem". This is more conventional and allows more flexible combination of messages.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/ahci.c          |    8 ++++----
 drivers/ata/libata-core.c   |    2 +-
 drivers/ata/libata-eh.c     |    4 ++--
 drivers/ata/sata_inic162x.c |    2 +-
 drivers/ata/sata_nv.c       |   19 ++++++++++---------
 drivers/ata/sata_promise.c  |    2 +-
 drivers/ata/sata_sil24.c    |   10 +++++-----
 7 files changed, 24 insertions(+), 23 deletions(-)
drivers/ata/ahci.c +4 -4
index b14e7ef..ff0b70d 100644
@@ -1197,7 +1197,7 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 	ahci_scr_write(ap, SCR_ERROR, serror);
 
 	/* analyze @irq_stat */
-	ata_ehi_push_desc(ehi, "irq_stat 0x%08x", irq_stat);
+	ata_ehi_push_desc(ehi, "irq_stat 0x%08x ", irq_stat);
 
 	/* some controllers set IRQ_IF_ERR on device errors, ignore it */
 	if (ap->flags & AHCI_FLAG_IGN_IRQ_IF_ERR)

@@ -1217,12 +1217,12 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 	if (irq_stat & PORT_IRQ_IF_ERR) {
 		err_mask |= AC_ERR_ATA_BUS;
 		action |= ATA_EH_SOFTRESET;
-		ata_ehi_push_desc(ehi, ", interface fatal error");
+		ata_ehi_push_desc(ehi, "<interface fatal error> ");
 	}
 
 	if (irq_stat & (PORT_IRQ_CONNECT | PORT_IRQ_PHYRDY)) {
 		ata_ehi_hotplugged(ehi);
-		ata_ehi_push_desc(ehi, ", %s", irq_stat & PORT_IRQ_CONNECT ?
+		ata_ehi_push_desc(ehi, "<%s> ", irq_stat & PORT_IRQ_CONNECT ?
 			"connection status changed" : "PHY RDY changed");
 	}
 

@@ -1231,7 +1231,7 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 
 		err_mask |= AC_ERR_HSM;
 		action |= ATA_EH_SOFTRESET;
-		ata_ehi_push_desc(ehi, ", unknown FIS %08x %08x %08x %08x",
+		ata_ehi_push_desc(ehi, "<unknown FIS %08x %08x %08x %08x> ",
 				  unk[0], unk[1], unk[2], unk[3]);
 	}
drivers/ata/libata-core.c +1 -1
index e5364af..8570975 100644
@@ -5581,7 +5581,7 @@ inline unsigned int ata_host_intr (struct ata_port *ap,
 
 	if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
 				       qc->tf.protocol == ATA_PROT_ATAPI_DMA))
-		ata_ehi_push_desc(ehi, "BMDMA stat 0x%x", host_stat);
+		ata_ehi_push_desc(ehi, "BMDMA stat 0x%x ", host_stat);
 
 	return 1;	/* irq handled */
drivers/ata/libata-eh.c +2 -2
index 9ee0a8c..744665a 100644
@@ -1524,14 +1524,14 @@ static void ata_eh_report(struct ata_port *ap)
 			       ehc->i.err_mask, ap->sactive, ehc->i.serror,
 			       ehc->i.action, frozen);
 		if (desc)
-			ata_dev_printk(ehc->i.dev, KERN_ERR, "(%s)\n", desc);
+			ata_dev_printk(ehc->i.dev, KERN_ERR, "%s\n", desc);
 	} else {
 		ata_port_printk(ap, KERN_ERR, "exception Emask 0x%x "
 				"SAct 0x%x SErr 0x%x action 0x%x%s\n",
 				ehc->i.err_mask, ap->sactive, ehc->i.serror,
 				ehc->i.action, frozen);
 		if (desc)
-			ata_port_printk(ap, KERN_ERR, "(%s)\n", desc);
+			ata_port_printk(ap, KERN_ERR, "%s\n", desc);
 	}
 
 	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
drivers/ata/sata_inic162x.c +1 -1
index e8525d7..4f1fc28 100644
@@ -310,7 +310,7 @@ static void inic_host_intr(struct ata_port *ap)
 	}
 
 	/* error */
-	ata_ehi_push_desc(ehi, "irq_stat=0x%x", irq_stat);
+	ata_ehi_push_desc(ehi, "irq_stat=0x%x ", irq_stat);
 
 	if (irq_stat & (PIRQ_OFFLINE | PIRQ_ONLINE)) {
 		ata_ehi_hotplugged(ehi);
drivers/ata/sata_nv.c +10 -9
index adfa693..e9701c7 100644
@@ -714,19 +714,20 @@ static int nv_adma_check_cpb(struct ata_port *ap, int cpb_num, int force_err)
 		int freeze = 0;
 
 		ata_ehi_clear_desc(ehi);
-		ata_ehi_push_desc(ehi, "CPB resp_flags 0x%x", flags );
+		ata_ehi_push_desc(ehi, "<CPB resp_flags 0x%x", flags );
 		if (flags & NV_CPB_RESP_ATA_ERR) {
-			ata_ehi_push_desc(ehi, ": ATA error");
+			ata_ehi_push_desc(ehi, ": ATA error> ");
 			ehi->err_mask |= AC_ERR_DEV;
 		} else if (flags & NV_CPB_RESP_CMD_ERR) {
-			ata_ehi_push_desc(ehi, ": CMD error");
+			ata_ehi_push_desc(ehi, ": CMD error> ");
 			ehi->err_mask |= AC_ERR_DEV;
 		} else if (flags & NV_CPB_RESP_CPB_ERR) {
-			ata_ehi_push_desc(ehi, ": CPB error");
+			ata_ehi_push_desc(ehi, ": CPB error> ");
 			ehi->err_mask |= AC_ERR_SYSTEM;
 			freeze = 1;
 		} else {
 			/* notifier error, but no error in CPB flags? */
+			ata_ehi_push_desc(ehi, ": unknown> ");
 			ehi->err_mask |= AC_ERR_OTHER;
 			freeze = 1;
 		}

@@ -853,19 +854,19 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
 				struct ata_eh_info *ehi = &ap->eh_info;
 
 				ata_ehi_clear_desc(ehi);
-				ata_ehi_push_desc(ehi, "ADMA status 0x%08x", status );
+				ata_ehi_push_desc(ehi, "<ADMA status 0x%08x", status );
 				if (status & NV_ADMA_STAT_TIMEOUT) {
 					ehi->err_mask |= AC_ERR_SYSTEM;
-					ata_ehi_push_desc(ehi, ": timeout");
+					ata_ehi_push_desc(ehi, ": timeout> ");
 				} else if (status & NV_ADMA_STAT_HOTPLUG) {
 					ata_ehi_hotplugged(ehi);
-					ata_ehi_push_desc(ehi, ": hotplug");
+					ata_ehi_push_desc(ehi, ": hotplug> ");
 				} else if (status & NV_ADMA_STAT_HOTUNPLUG) {
 					ata_ehi_hotplugged(ehi);
-					ata_ehi_push_desc(ehi, ": hot unplug");
+					ata_ehi_push_desc(ehi, ": hot unplug> ");
 				} else if (status & NV_ADMA_STAT_SERROR) {
 					/* let libata analyze SError and figure out the cause */
-					ata_ehi_push_desc(ehi, ": SError");
+					ata_ehi_push_desc(ehi, ": SError> ");
 				}
 				ata_port_freeze(ap);
 				continue;
drivers/ata/sata_promise.c +1 -1
index 7fcc4fe..ff3d5c8 100644
@@ -634,7 +634,7 @@ static void pdc_error_intr(struct ata_port *ap, struct ata_queued_cmd *qc,
 	unsigned int ac_err_mask = 0;
 
 	ata_ehi_clear_desc(ehi);
-	ata_ehi_push_desc(ehi, "port_status 0x%08x", port_status);
+	ata_ehi_push_desc(ehi, "port_status 0x%08x ", port_status);
 	port_status &= err_mask;
 
 	if (port_status & PDC_DRIVE_ERR)
drivers/ata/sata_sil24.c +5 -5
index 0ddfae9..23650f3 100644
@@ -765,11 +765,11 @@ static void sil24_error_intr(struct ata_port *ap)
 	/* first, analyze and record host port events */
 	ata_ehi_clear_desc(ehi);
 
-	ata_ehi_push_desc(ehi, "irq_stat 0x%08x", irq_stat);
+	ata_ehi_push_desc(ehi, "irq_stat 0x%08x ", irq_stat);
 
 	if (irq_stat & (PORT_IRQ_PHYRDY_CHG | PORT_IRQ_DEV_XCHG)) {
 		ata_ehi_hotplugged(ehi);
-		ata_ehi_push_desc(ehi, ", %s",
+		ata_ehi_push_desc(ehi, "<%s> ",
 			       irq_stat & PORT_IRQ_PHYRDY_CHG ?
 			       "PHY RDY changed" : "device exchanged");
 		freeze = 1;

@@ -778,7 +778,7 @@ static void sil24_error_intr(struct ata_port *ap)
 	if (irq_stat & PORT_IRQ_UNK_FIS) {
 		ehi->err_mask |= AC_ERR_HSM;
 		ehi->action |= ATA_EH_SOFTRESET;
-		ata_ehi_push_desc(ehi , ", unknown FIS");
+		ata_ehi_push_desc(ehi, "<unknown FIS> ");
 		freeze = 1;
 	}
 

@@ -797,11 +797,11 @@ static void sil24_error_intr(struct ata_port *ap)
 		if (ci && ci->desc) {
 			err_mask |= ci->err_mask;
 			action |= ci->action;
-			ata_ehi_push_desc(ehi, ", %s", ci->desc);
+			ata_ehi_push_desc(ehi, "<%s> ", ci->desc);
 		} else {
 			err_mask |= AC_ERR_OTHER;
 			action |= ATA_EH_SOFTRESET;
-			ata_ehi_push_desc(ehi, ", unknown command error %d",
+			ata_ehi_push_desc(ehi, "<unknown command error %d> ",
 					  cerr);
 		}
Signature · 1 line
1.5.0.3
author
#7 lore raw

PATCH 3/12 libata: make ->scr_read/write callbacks return error code

In reply to Tejun Heo

Convert ->scr_read/write callbacks to return error code to better indicate failure. This will help handling of SCR_NOTIFICATION.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/ahci.c          |   23 ++++++++------
 drivers/ata/libata-core.c   |   21 ++++++------
 drivers/ata/sata_inic162x.c |   16 +++++-----
 drivers/ata/sata_mv.c       |   72 ++++++++++++++++++++++++++++--------------
 drivers/ata/sata_nv.c       |   16 +++++----
 drivers/ata/sata_promise.c  |   25 +++++++++------
 drivers/ata/sata_qstor.c    |   16 +++++----
 drivers/ata/sata_sil.c      |   25 +++++++++-----
 drivers/ata/sata_sil24.c    |   17 ++++++---
 drivers/ata/sata_sis.c      |   22 +++++++------
 drivers/ata/sata_svw.c      |   13 ++++---
 drivers/ata/sata_uli.c      |   16 +++++----
 drivers/ata/sata_via.c      |   27 +++++++++-------
 drivers/ata/sata_vsc.c      |   13 ++++---
 include/linux/libata.h      |    5 +--
 15 files changed, 191 insertions(+), 136 deletions(-)
drivers/ata/ahci.c +13 -10
index ff0b70d..0e001ad 100644
@@ -213,8 +213,8 @@ struct ahci_port_priv {
 	unsigned int		ncq_saw_sdb:1;
 };
 
-static u32 ahci_scr_read (struct ata_port *ap, unsigned int sc_reg);
-static void ahci_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static int ahci_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val);
+static int ahci_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val);
 static int ahci_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
 static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc);
 static void ahci_irq_clear(struct ata_port *ap);

@@ -593,7 +593,7 @@ static void ahci_restore_initial_config(struct ata_host *host)
 	(void) readl(mmio + HOST_PORTS_IMPL);	/* flush */
 }
 
-static u32 ahci_scr_read (struct ata_port *ap, unsigned int sc_reg_in)
+static int ahci_scr_read(struct ata_port *ap, unsigned int sc_reg_in, u32 *val)
 {
 	unsigned int sc_reg;
 

@@ -603,15 +603,15 @@ static u32 ahci_scr_read (struct ata_port *ap, unsigned int sc_reg_in)
 	case SCR_ERROR:		sc_reg = 2; break;
 	case SCR_ACTIVE:	sc_reg = 3; break;
 	default:
-		return 0xffffffffU;
+		return -EINVAL;
 	}
 
-	return readl(ap->ioaddr.scr_addr + (sc_reg * 4));
+	*val = readl(ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
 
 
-static void ahci_scr_write (struct ata_port *ap, unsigned int sc_reg_in,
-			       u32 val)
+static int ahci_scr_write(struct ata_port *ap, unsigned int sc_reg_in, u32 val)
 {
 	unsigned int sc_reg;
 

@@ -621,10 +621,11 @@ static void ahci_scr_write (struct ata_port *ap, unsigned int sc_reg_in,
 	case SCR_ERROR:		sc_reg = 2; break;
 	case SCR_ACTIVE:	sc_reg = 3; break;
 	default:
-		return;
+		return -EINVAL;
 	}
 
 	writel(val, ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
 
 static void ahci_start_engine(struct ata_port *ap)

@@ -1061,6 +1062,7 @@ static int ahci_hardreset(struct ata_port *ap, unsigned int *class,
 static int ahci_vt8251_hardreset(struct ata_port *ap, unsigned int *class,
 				 unsigned long deadline)
 {
+	u32 serror;
 	int rc;
 
 	DPRINTK("ENTER\n");

@@ -1071,7 +1073,8 @@ static int ahci_vt8251_hardreset(struct ata_port *ap, unsigned int *class,
 				 deadline);
 
 	/* vt8251 needs SError cleared for the port to operate */
-	ahci_scr_write(ap, SCR_ERROR, ahci_scr_read(ap, SCR_ERROR));
+	ahci_scr_read(ap, SCR_ERROR, &serror);
+	ahci_scr_write(ap, SCR_ERROR, serror);
 
 	ahci_start_engine(ap);
 

@@ -1193,7 +1196,7 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 	ata_ehi_clear_desc(ehi);
 
 	/* AHCI needs SError cleared; otherwise, it might lock up */
-	serror = ahci_scr_read(ap, SCR_ERROR);
+	ahci_scr_read(ap, SCR_ERROR, &serror);
 	ahci_scr_write(ap, SCR_ERROR, serror);
 
 	/* analyze @irq_stat */
drivers/ata/libata-core.c +10 -11
index 8570975..147235c 100644
@@ -5678,10 +5678,8 @@ int sata_scr_valid(struct ata_port *ap)
  */
 int sata_scr_read(struct ata_port *ap, int reg, u32 *val)
 {
-	if (sata_scr_valid(ap)) {
-		*val = ap->ops->scr_read(ap, reg);
-		return 0;
-	}
+	if (sata_scr_valid(ap))
+		return ap->ops->scr_read(ap, reg, val);
 	return -EOPNOTSUPP;
 }
 

@@ -5703,10 +5701,8 @@ int sata_scr_read(struct ata_port *ap, int reg, u32 *val)
  */
 int sata_scr_write(struct ata_port *ap, int reg, u32 val)
 {
-	if (sata_scr_valid(ap)) {
-		ap->ops->scr_write(ap, reg, val);
-		return 0;
-	}
+	if (sata_scr_valid(ap))
+		return ap->ops->scr_write(ap, reg, val);
 	return -EOPNOTSUPP;
 }
 

@@ -5727,10 +5723,13 @@ int sata_scr_write(struct ata_port *ap, int reg, u32 val)
  */
 int sata_scr_write_flush(struct ata_port *ap, int reg, u32 val)
 {
+	int rc;
+
 	if (sata_scr_valid(ap)) {
-		ap->ops->scr_write(ap, reg, val);
-		ap->ops->scr_read(ap, reg);
-		return 0;
+		rc = ap->ops->scr_write(ap, reg, val);
+		if (rc == 0)
+			rc = ap->ops->scr_read(ap, reg, &val);
+		return rc;
 	}
 	return -EOPNOTSUPP;
 }
drivers/ata/sata_inic162x.c +8 -8
index 4f1fc28..2ad5690 100644
@@ -190,34 +190,34 @@ static void inic_reset_port(void __iomem *port_base)
 	writew(ctl, idma_ctl);
 }
 
-static u32 inic_scr_read(struct ata_port *ap, unsigned sc_reg)
+static int inic_scr_read(struct ata_port *ap, unsigned sc_reg, u32 *val)
 {
 	void __iomem *scr_addr = ap->ioaddr.scr_addr;
 	void __iomem *addr;
-	u32 val;
 
 	if (unlikely(sc_reg >= ARRAY_SIZE(scr_map)))
-		return 0xffffffffU;
+		return -EINVAL;
 
 	addr = scr_addr + scr_map[sc_reg] * 4;
-	val = readl(scr_addr + scr_map[sc_reg] * 4);
+	*val = readl(scr_addr + scr_map[sc_reg] * 4);
 
 	/* this controller has stuck DIAG.N, ignore it */
 	if (sc_reg == SCR_ERROR)
-		val &= ~SERR_PHYRDY_CHG;
-	return val;
+		*val &= ~SERR_PHYRDY_CHG;
+	return 0;
 }
 
-static void inic_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val)
+static int inic_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val)
 {
 	void __iomem *scr_addr = ap->ioaddr.scr_addr;
 	void __iomem *addr;
 
 	if (unlikely(sc_reg >= ARRAY_SIZE(scr_map)))
-		return;
+		return -EINVAL;
 
 	addr = scr_addr + scr_map[sc_reg] * 4;
 	writel(val, scr_addr + scr_map[sc_reg] * 4);
+	return 0;
 }
 
 /*
drivers/ata/sata_mv.c +48 -24

124 lines not on this page · raw message

drivers/ata/sata_nv.c +9 -7
index e9701c7..9790f3c 100644
@@ -236,8 +236,8 @@ static void nv_ck804_host_stop(struct ata_host *host);
 static irqreturn_t nv_generic_interrupt(int irq, void *dev_instance);
 static irqreturn_t nv_nf2_interrupt(int irq, void *dev_instance);
 static irqreturn_t nv_ck804_interrupt(int irq, void *dev_instance);
-static u32 nv_scr_read (struct ata_port *ap, unsigned int sc_reg);
-static void nv_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static int nv_scr_read (struct ata_port *ap, unsigned int sc_reg, u32 *val);
+static int nv_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 
 static void nv_nf2_freeze(struct ata_port *ap);
 static void nv_nf2_thaw(struct ata_port *ap);

@@ -1391,20 +1391,22 @@ static irqreturn_t nv_ck804_interrupt(int irq, void *dev_instance)
 	return ret;
 }
 
-static u32 nv_scr_read (struct ata_port *ap, unsigned int sc_reg)
+static int nv_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return 0xffffffffU;
+		return -EINVAL;
 
-	return ioread32(ap->ioaddr.scr_addr + (sc_reg * 4));
+	*val = ioread32(ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
 
-static void nv_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val)
+static int nv_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return;
+		return -EINVAL;
 
 	iowrite32(val, ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
 
 static void nv_nf2_freeze(struct ata_port *ap)
drivers/ata/sata_promise.c +15 -10
index ff3d5c8..c710059 100644
@@ -130,8 +130,8 @@ struct pdc_port_priv {
 	dma_addr_t		pkt_dma;
 };
 
-static u32 pdc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg);
-static void pdc_sata_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static int pdc_sata_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val);
+static int pdc_sata_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val);
 static int pdc_ata_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
 static int pdc_common_port_start(struct ata_port *ap);
 static int pdc_sata_port_start(struct ata_port *ap);

@@ -431,20 +431,21 @@ static int pdc_sata_cable_detect(struct ata_port *ap)
 	return ATA_CBL_SATA;
 }
 
-static u32 pdc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
+static int pdc_sata_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return 0xffffffffU;
-	return readl(ap->ioaddr.scr_addr + (sc_reg * 4));
+		return -EINVAL;
+	*val = readl(ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
 
 
-static void pdc_sata_scr_write (struct ata_port *ap, unsigned int sc_reg,
-			       u32 val)
+static int pdc_sata_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return;
+		return -EINVAL;
 	writel(val, ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
 
 static void pdc_atapi_pkt(struct ata_queued_cmd *qc)

@@ -647,8 +648,12 @@ static void pdc_error_intr(struct ata_port *ap, struct ata_queued_cmd *qc,
 			   | PDC_PCI_SYS_ERR | PDC1_PCI_PARITY_ERR))
 		ac_err_mask |= AC_ERR_HOST_BUS;
 
-	if (sata_scr_valid(ap))
-		ehi->serror |= pdc_sata_scr_read(ap, SCR_ERROR);
+	if (sata_scr_valid(ap)) {
+		u32 serror;
+
+		pdc_sata_scr_read(ap, SCR_ERROR, &serror);
+		ehi->serror |= serror;
+	}
 
 	qc->err_mask |= ac_err_mask;
drivers/ata/sata_qstor.c +9 -7
index 6688ccb..13348a6 100644
@@ -111,8 +111,8 @@ struct qs_port_priv {
 	qs_state_t		state;
 };
 
-static u32 qs_scr_read (struct ata_port *ap, unsigned int sc_reg);
-static void qs_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static int qs_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val);
+static int qs_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val);
 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);

@@ -255,18 +255,20 @@ static void qs_eng_timeout(struct ata_port *ap)
 	ata_eng_timeout(ap);
 }
 
-static u32 qs_scr_read (struct ata_port *ap, unsigned int sc_reg)
+static int qs_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return ~0U;
-	return readl(ap->ioaddr.scr_addr + (sc_reg * 8));
+		return -EINVAL;
+	*val = readl(ap->ioaddr.scr_addr + (sc_reg * 8));
+	return 0;
 }
 
-static void qs_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val)
+static int qs_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return;
+		return -EINVAL;
 	writel(val, ap->ioaddr.scr_addr + (sc_reg * 8));
+	return 0;
 }
 
 static unsigned int qs_fill_sg(struct ata_queued_cmd *qc)
drivers/ata/sata_sil.c +16 -9
index c3f0a86..94a528f 100644
@@ -115,8 +115,8 @@ static int sil_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
 static int sil_pci_device_resume(struct pci_dev *pdev);
 #endif
 static void sil_dev_config(struct ata_device *dev);
-static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
-static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static int sil_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val);
+static int sil_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val);
 static int sil_set_mode (struct ata_port *ap, struct ata_device **r_failed);
 static void sil_freeze(struct ata_port *ap);
 static void sil_thaw(struct ata_port *ap);

@@ -350,19 +350,26 @@ static inline void __iomem *sil_scr_addr(struct ata_port *ap, unsigned int sc_re
 	return NULL;
 }
 
-static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg)
+static int sil_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
 	void __iomem *mmio = sil_scr_addr(ap, sc_reg);
-	if (mmio)
-		return readl(mmio);
-	return 0xffffffffU;
+
+	if (mmio) {
+		*val = readl(mmio);
+		return 0;
+	}
+	return -EINVAL;
 }
 
-static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val)
+static int sil_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	void __iomem *mmio = sil_scr_addr(ap, sc_reg);
-	if (mmio)
+
+	if (mmio) {
 		writel(val, mmio);
+		return 0;
+	}
+	return -EINVAL;
 }
 
 static void sil_host_intr(struct ata_port *ap, u32 bmdma2)

@@ -378,7 +385,7 @@ static void sil_host_intr(struct ata_port *ap, u32 bmdma2)
 		 * controllers continue to assert IRQ as long as
 		 * SError bits are pending.  Clear SError immediately.
 		 */
-		serror = sil_scr_read(ap, SCR_ERROR);
+		sil_scr_read(ap, SCR_ERROR, &serror);
 		sil_scr_write(ap, SCR_ERROR, serror);
 
 		/* Trigger hotplug and accumulate SError only if the
drivers/ata/sata_sil24.c +11 -6
index 23650f3..07489a0 100644
@@ -326,8 +326,8 @@ struct sil24_port_priv {
 
 static void sil24_dev_config(struct ata_device *dev);
 static u8 sil24_check_status(struct ata_port *ap);
-static u32 sil24_scr_read(struct ata_port *ap, unsigned sc_reg);
-static void sil24_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val);
+static int sil24_scr_read(struct ata_port *ap, unsigned sc_reg, u32 *val);
+static int sil24_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val);
 static void sil24_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
 static void sil24_qc_prep(struct ata_queued_cmd *qc);
 static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc);

@@ -488,25 +488,30 @@ static int sil24_scr_map[] = {
 	[SCR_ACTIVE]	= 3,
 };
 
-static u32 sil24_scr_read(struct ata_port *ap, unsigned sc_reg)
+static int sil24_scr_read(struct ata_port *ap, unsigned sc_reg, u32 *val)
 {
 	void __iomem *scr_addr = ap->ioaddr.scr_addr;
+
 	if (sc_reg < ARRAY_SIZE(sil24_scr_map)) {
 		void __iomem *addr;
 		addr = scr_addr + sil24_scr_map[sc_reg] * 4;
-		return readl(scr_addr + sil24_scr_map[sc_reg] * 4);
+		*val = readl(scr_addr + sil24_scr_map[sc_reg] * 4);
+		return 0;
 	}
-	return 0xffffffffU;
+	return -EINVAL;
 }
 
-static void sil24_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val)
+static int sil24_scr_write(struct ata_port *ap, unsigned sc_reg, u32 val)
 {
 	void __iomem *scr_addr = ap->ioaddr.scr_addr;
+
 	if (sc_reg < ARRAY_SIZE(sil24_scr_map)) {
 		void __iomem *addr;
 		addr = scr_addr + sil24_scr_map[sc_reg] * 4;
 		writel(val, scr_addr + sil24_scr_map[sc_reg] * 4);
+		return 0;
 	}
+	return -EINVAL;
 }
 
 static void sil24_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
drivers/ata/sata_sis.c +12 -10
index 221099d..b1d5e23 100644
@@ -64,8 +64,8 @@ enum {
 };
 
 static int sis_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
-static u32 sis_scr_read (struct ata_port *ap, unsigned int sc_reg);
-static void sis_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static int sis_scr_read (struct ata_port *ap, unsigned int sc_reg, u32 *val);
+static int sis_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 
 static const struct pci_device_id sis_pci_tbl[] = {
 	{ PCI_VDEVICE(SI, 0x0180), sis_180 },		/* SiS 964/180 */

@@ -208,36 +208,37 @@ static void sis_scr_cfg_write (struct ata_port *ap, unsigned int sc_reg, u32 val
 		pci_write_config_dword(pdev, cfg_addr+0x10, val);
 }
 
-static u32 sis_scr_read (struct ata_port *ap, unsigned int sc_reg)
+static int sis_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
-	u32 val, val2 = 0;
 	u8 pmr;
 
 	if (sc_reg > SCR_CONTROL)
-		return 0xffffffffU;
+		return -EINVAL;
 
 	if (ap->flags & SIS_FLAG_CFGSCR)
 		return sis_scr_cfg_read(ap, sc_reg);
 
 	pci_read_config_byte(pdev, SIS_PMR, &pmr);
 
-	val = ioread32(ap->ioaddr.scr_addr + (sc_reg * 4));
+	*val = ioread32(ap->ioaddr.scr_addr + (sc_reg * 4));
 
 	if ((pdev->device == 0x0182) || (pdev->device == 0x0183) || (pdev->device == 0x1182) ||
 	    (pdev->device == 0x1183) || (pmr & SIS_PMR_COMBINED))
-		val2 = ioread32(ap->ioaddr.scr_addr + (sc_reg * 4) + 0x10);
+		*val |= ioread32(ap->ioaddr.scr_addr + (sc_reg * 4) + 0x10);
+
+	*val &= 0xfffffffb;
 
-	return (val | val2) &  0xfffffffb;
+	return 0;
 }
 
-static void sis_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val)
+static int sis_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
 	u8 pmr;
 
 	if (sc_reg > SCR_CONTROL)
-		return;
+		return -EINVAL;
 
 	pci_read_config_byte(pdev, SIS_PMR, &pmr);
 

@@ -249,6 +250,7 @@ static void sis_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val)
 		    (pdev->device == 0x1183) || (pmr & SIS_PMR_COMBINED))
 			iowrite32(val, ap->ioaddr.scr_addr + (sc_reg * 4)+0x10);
 	}
+	return 0;
 }
 
 static int sis_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
drivers/ata/sata_svw.c +7 -6
index 4fd4f10..d8191d8 100644
@@ -103,20 +103,21 @@ static int k2_sata_check_atapi_dma(struct ata_queued_cmd *qc)
 	return 0;
 }
 
-static u32 k2_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
+static int k2_sata_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return 0xffffffffU;
-	return readl(ap->ioaddr.scr_addr + (sc_reg * 4));
+		return -EINVAL;
+	*val = readl(ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
 
 
-static void k2_sata_scr_write (struct ata_port *ap, unsigned int sc_reg,
-			       u32 val)
+static int k2_sata_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return;
+		return -EINVAL;
 	writel(val, ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
drivers/ata/sata_uli.c +9 -7
index 6815de7..957c747 100644
@@ -57,8 +57,8 @@ struct uli_priv {
 };
 
 static int uli_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
-static u32 uli_scr_read (struct ata_port *ap, unsigned int sc_reg);
-static void uli_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static int uli_scr_read (struct ata_port *ap, unsigned int sc_reg, u32 *val);
+static int uli_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 
 static const struct pci_device_id uli_pci_tbl[] = {
 	{ PCI_VDEVICE(AL, 0x5289), uli_5289 },

@@ -164,20 +164,22 @@ static void uli_scr_cfg_write (struct ata_port *ap, unsigned int scr, u32 val)
 	pci_write_config_dword(pdev, cfg_addr, val);
 }
 
-static u32 uli_scr_read (struct ata_port *ap, unsigned int sc_reg)
+static int uli_scr_read (struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return 0xffffffffU;
+		return -EINVAL;
 
-	return uli_scr_cfg_read(ap, sc_reg);
+	*val = uli_scr_cfg_read(ap, sc_reg);
+	return 0;
 }
 
-static void uli_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val)
+static int uli_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	if (sc_reg > SCR_CONTROL)	//SCR_CONTROL=2, SCR_ERROR=1, SCR_STATUS=0
-		return;
+		return -EINVAL;
 
 	uli_scr_cfg_write(ap, sc_reg, val);
+	return 0;
 }
 
 static int uli_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
drivers/ata/sata_via.c +15 -12
index e816965..d39d68d 100644
@@ -72,8 +72,8 @@ enum {
 };
 
 static int svia_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
-static u32 svia_scr_read (struct ata_port *ap, unsigned int sc_reg);
-static void svia_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static int svia_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val);
+static int svia_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val);
 static void svia_noop_freeze(struct ata_port *ap);
 static void vt6420_error_handler(struct ata_port *ap);
 static int vt6421_pata_cable_detect(struct ata_port *ap);

@@ -249,18 +249,20 @@ MODULE_LICENSE("GPL");
 MODULE_DEVICE_TABLE(pci, svia_pci_tbl);
 MODULE_VERSION(DRV_VERSION);
 
-static u32 svia_scr_read (struct ata_port *ap, unsigned int sc_reg)
+static int svia_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return 0xffffffffU;
-	return ioread32(ap->ioaddr.scr_addr + (4 * sc_reg));
+		return -EINVAL;
+	*val = ioread32(ap->ioaddr.scr_addr + (4 * sc_reg));
+	return 0;
 }
 
-static void svia_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val)
+static int svia_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return;
+		return -EINVAL;
 	iowrite32(val, ap->ioaddr.scr_addr + (4 * sc_reg));
+	return 0;
 }
 
 static void svia_noop_freeze(struct ata_port *ap)

@@ -305,18 +307,19 @@ static int vt6420_prereset(struct ata_port *ap, unsigned long deadline)
 
 	/* Resume phy.  This is the old SATA resume sequence */
 	svia_scr_write(ap, SCR_CONTROL, 0x300);
-	svia_scr_read(ap, SCR_CONTROL); /* flush */
+	svia_scr_read(ap, SCR_CONTROL, &scontrol); /* flush */
 
 	/* wait for phy to become ready, if necessary */
 	do {
 		msleep(200);
-		if ((svia_scr_read(ap, SCR_STATUS) & 0xf) != 1)
+		svia_scr_read(ap, SCR_STATUS, &sstatus);
+		if ((sstatus & 0xf) != 1)
 			break;
 	} while (time_before(jiffies, timeout));
 
 	/* open code sata_print_link_status() */
-	sstatus = svia_scr_read(ap, SCR_STATUS);
-	scontrol = svia_scr_read(ap, SCR_CONTROL);
+	svia_scr_read(ap, SCR_STATUS, &sstatus);
+	svia_scr_read(ap, SCR_CONTROL, &scontrol);
 
 	online = (sstatus & 0xf) == 0x3;
 

@@ -325,7 +328,7 @@ static int vt6420_prereset(struct ata_port *ap, unsigned long deadline)
 			online ? "up" : "down", sstatus, scontrol);
 
 	/* SStatus is read one more time */
-	svia_scr_read(ap, SCR_STATUS);
+	svia_scr_read(ap, SCR_STATUS, &sstatus);
 
 	if (!online) {
 		/* tell EH to bail */
drivers/ata/sata_vsc.c +7 -6
index 8133017..8b7a9c6 100644
@@ -98,20 +98,21 @@ enum {
 			      VSC_SATA_INT_PHY_CHANGE),
 };
 
-static u32 vsc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg)
+static int vsc_sata_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return 0xffffffffU;
-	return readl(ap->ioaddr.scr_addr + (sc_reg * 4));
+		return -EINVAL;
+	*val = readl(ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
 
 
-static void vsc_sata_scr_write (struct ata_port *ap, unsigned int sc_reg,
-			       u32 val)
+static int vsc_sata_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
 {
 	if (sc_reg > SCR_CONTROL)
-		return;
+		return -EINVAL;
 	writel(val, ap->ioaddr.scr_addr + (sc_reg * 4));
+	return 0;
 }
include/linux/libata.h +2 -3
index c2cad0f..bc91ca0 100644
@@ -620,9 +620,8 @@ struct ata_port_operations {
 	u8 (*irq_on) (struct ata_port *);
 	u8 (*irq_ack) (struct ata_port *ap, unsigned int chk_drq);
 
-	u32 (*scr_read) (struct ata_port *ap, unsigned int sc_reg);
-	void (*scr_write) (struct ata_port *ap, unsigned int sc_reg,
-			   u32 val);
+	int (*scr_read) (struct ata_port *ap, unsigned int sc_reg, u32 *val);
+	int (*scr_write) (struct ata_port *ap, unsigned int sc_reg, u32 val);
 
 	int (*port_suspend) (struct ata_port *ap, pm_message_t mesg);
 	int (*port_resume) (struct ata_port *ap);
Signature · 1 line
1.5.0.3

Tejun Heo

author
#0 lore raw

PATCH 1/4 libata: misc updates in preparation of PMP support

Hello,

This patchset contains the following 12 patches updating various aspects of libata in preparation of PMP support.

#01 [PATCH] libata: update EH report formatting
#02 [PATCH] libata: implement AC_ERR_NCQ
#03 [PATCH] libata: make ->scr_read/write callbacks return error code #04 [PATCH] ahci: make NO_NCQ handling more consistent
#05 [PATCH] ahci: implement SCR_NOTIFICATION r/w
#06 [PATCH] libata: improve SATA PHY speed down logic
#07 [PATCH] libata: quickly trigger SATA SPD down after debouncing failed #08 [PATCH] libata: improve SCSI scan failure handling
#09 [PATCH] libata: reorganize ata_ehi_hotplugged()
#10 [PATCH] libata: clear HOTPLUG flag after a reset
#11 [PATCH] libata: schedule probing after SError access failure during autopsy #12 [PATCH] libata: implement EH fast drain

#01-03 are mostly trivial. #04-05 are minor updates to ahci. #06-#12 improves EH behaviors including SATA speed down, hotplug handling and fast draining if EH is pending. Please refer to individual commit messages for more info.

This patchset is on top of

libata-dev#upstream (4e1ae96828f6cee7b89ab8ca474c150fe211afd8)

Thanks.

--
tejun

author
#15 lore raw

PATCH 6/12 libata: improve SATA PHY speed down logic

sata_down_spd_limit() first reads the current SPD from SStatus and limit the speed to the lower one of one below the current limit or one below the current SPD in SStatus. SPD may not be accessible or valid when SPD down is requested making sata_down_spd_limit() fail when it's most needed.

This patch makes the current SPD cached after each successful reset and forces GEN I speed (1.5Gbps) if neither of SStatus or the cached value is valid, so sata_down_spd_limit() is now guaranteed to lower the speed limit if lower speed is available.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |   29 ++++++++++++++++++++++-------
 drivers/ata/libata-eh.c   |    6 ++++++
 include/linux/libata.h    |    1 +
 3 files changed, 29 insertions(+), 7 deletions(-)
drivers/ata/libata-core.c +22 -7
index 147235c..49fe30a 100644
@@ -2413,21 +2413,35 @@ int sata_down_spd_limit(struct ata_port *ap)
 	u32 sstatus, spd, mask;
 	int rc, highbit;
 
+	if (!sata_scr_valid(ap))
+		return -EOPNOTSUPP;
+
+	/* If SCR can be read, use it to determine the current SPD.
+	 * If not, use cached value in ap->sata_spd.
+	 */
 	rc = sata_scr_read(ap, SCR_STATUS, &sstatus);
-	if (rc)
-		return rc;
+	if (rc == 0)
+		spd = (sstatus >> 4) & 0xf;
+	else
+		spd = ap->sata_spd;
 
 	mask = ap->sata_spd_limit;
 	if (mask <= 1)
 		return -EINVAL;
+
+	/* unconditionally mask off the highest bit */
 	highbit = fls(mask) - 1;
 	mask &= ~(1 << highbit);
 
-	spd = (sstatus >> 4) & 0xf;
-	if (spd <= 1)
-		return -EINVAL;
-	spd--;
-	mask &= (1 << spd) - 1;
+	/* Mask off all speeds higher than or equal to the current
+	 * one.  Force 1.5Gbps if current SPD is not available.
+	 */
+	if (spd > 1)
+		mask &= (1 << (spd - 1)) - 1;
+	else
+		mask &= 1;
+
+	/* were we already at the bottom? */
 	if (!mask)
 		return -EINVAL;
 

@@ -5941,6 +5955,7 @@ void ata_dev_init(struct ata_device *dev)
 
 	/* SATA spd limit is bound to the first device */
 	ap->sata_spd_limit = ap->hw_sata_spd_limit;
+	ap->sata_spd = 0;
 
 	/* High bits of dev->flags are used to record warm plug
 	 * requests which occur asynchronously.  Synchronize using
drivers/ata/libata-eh.c +6 -0
index 8d8961f..d18c475 100644
@@ -1734,12 +1734,18 @@ static int ata_eh_reset(struct ata_port *ap, int classify,
 	}
 
 	if (rc == 0) {
+		u32 sstatus;
+
 		/* After the reset, the device state is PIO 0 and the
 		 * controller state is undefined.  Record the mode.
 		 */
 		for (i = 0; i < ATA_MAX_DEVICES; i++)
 			ap->device[i].pio_mode = XFER_PIO_0;
 
+		/* record current link speed */
+		if (sata_scr_read(ap, SCR_STATUS, &sstatus) == 0)
+			ap->sata_spd = (sstatus >> 4) & 0xf;
+
 		if (postreset)
 			postreset(ap, classes);
include/linux/libata.h +1 -0
index bc91ca0..05670b5 100644
@@ -531,6 +531,7 @@ struct ata_port {
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
 	unsigned int		hw_sata_spd_limit;
 	unsigned int		sata_spd_limit;	/* SATA PHY speed limit */
+	unsigned int		sata_spd;	/* current SATA PHY speed */
 
 	/* record runtime error info, protected by host lock */
 	struct ata_eh_info	eh_info;
Signature · 1 line
1.5.0.3
author
#11 lore raw

PATCH 5/12 ahci: implement SCR_NOTIFICATION r/w

In reply to Tejun Heo

Make ahci_scr_read/write() handle SCR_NOTIFICATION if the controller supports it. Also, print "sntf" in the cap line if supported.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/ahci.c |   68 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 37 insertions(+), 31 deletions(-)
drivers/ata/ahci.c +37 -31
index 0555db5..2e4c9ad 100644
@@ -98,6 +98,7 @@ enum {
 	HOST_CAP_SSC		= (1 << 14), /* Slumber capable */
 	HOST_CAP_CLO		= (1 << 24), /* Command List Override support */
 	HOST_CAP_SSS		= (1 << 27), /* Staggered Spin-up */
+	HOST_CAP_SNTF		= (1 << 29), /* SNotification register */
 	HOST_CAP_NCQ		= (1 << 30), /* Native Command Queueing */
 	HOST_CAP_64		= (1 << 31), /* PCI DAC (64-bit DMA) support */
 

@@ -112,11 +113,11 @@ enum {
 	PORT_TFDATA		= 0x20,	/* taskfile data */
 	PORT_SIG		= 0x24,	/* device TF signature */
 	PORT_CMD_ISSUE		= 0x38, /* command issue */
-	PORT_SCR		= 0x28, /* SATA phy register block */
 	PORT_SCR_STAT		= 0x28, /* SATA phy register: SStatus */
 	PORT_SCR_CTL		= 0x2c, /* SATA phy register: SControl */
 	PORT_SCR_ERR		= 0x30, /* SATA phy register: SError */
 	PORT_SCR_ACT		= 0x34, /* SATA phy register: SActive */
+	PORT_SCR_NTF		= 0x3c, /* SATA phy register: SNotification */
 
 	/* PORT_IRQ_{STAT,MASK} bits */
 	PORT_IRQ_COLD_PRES	= (1 << 31), /* cold presence detect */

@@ -599,39 +600,45 @@ static void ahci_restore_initial_config(struct ata_host *host)
 	(void) readl(mmio + HOST_PORTS_IMPL);	/* flush */
 }
 
-static int ahci_scr_read(struct ata_port *ap, unsigned int sc_reg_in, u32 *val)
+static unsigned ahci_scr_offset(struct ata_port *ap, unsigned int sc_reg)
 {
-	unsigned int sc_reg;
-
-	switch (sc_reg_in) {
-	case SCR_STATUS:	sc_reg = 0; break;
-	case SCR_CONTROL:	sc_reg = 1; break;
-	case SCR_ERROR:		sc_reg = 2; break;
-	case SCR_ACTIVE:	sc_reg = 3; break;
-	default:
-		return -EINVAL;
-	}
+	static const int offset[] = {
+		[SCR_STATUS]		= PORT_SCR_STAT,
+		[SCR_CONTROL]		= PORT_SCR_CTL,
+		[SCR_ERROR]		= PORT_SCR_ERR,
+		[SCR_ACTIVE]		= PORT_SCR_ACT,
+		[SCR_NOTIFICATION]	= PORT_SCR_NTF,
+	};
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 
-	*val = readl(ap->ioaddr.scr_addr + (sc_reg * 4));
+	if (sc_reg < ARRAY_SIZE(offset) &&
+	    (sc_reg != SCR_NOTIFICATION || (hpriv->cap & HOST_CAP_SNTF)))
+		return offset[sc_reg];
 	return 0;
 }
 
-
-static int ahci_scr_write(struct ata_port *ap, unsigned int sc_reg_in, u32 val)
+static int ahci_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val)
 {
-	unsigned int sc_reg;
-
-	switch (sc_reg_in) {
-	case SCR_STATUS:	sc_reg = 0; break;
-	case SCR_CONTROL:	sc_reg = 1; break;
-	case SCR_ERROR:		sc_reg = 2; break;
-	case SCR_ACTIVE:	sc_reg = 3; break;
-	default:
-		return -EINVAL;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	int offset = ahci_scr_offset(ap, sc_reg);
+
+	if (offset) {
+		*val = readl(port_mmio + offset);
+		return 0;
 	}
+	return -EINVAL;
+}
 
-	writel(val, ap->ioaddr.scr_addr + (sc_reg * 4));
-	return 0;
+static int ahci_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	int offset = ahci_scr_offset(ap, sc_reg);
+
+	if (offset) {
+		writel(val, port_mmio + offset);
+		return 0;
+	}
+	return -EINVAL;
 }
 
 static void ahci_start_engine(struct ata_port *ap)

@@ -1698,12 +1705,13 @@ static void ahci_print_info(struct ata_host *host)
 
 	dev_printk(KERN_INFO, &pdev->dev,
 		"flags: "
-	       	"%s%s%s%s%s%s"
-	       	"%s%s%s%s%s%s%s\n"
+		"%s%s%s%s%s%s%s"
+		"%s%s%s%s%s%s%s\n"
 	       	,
 
 		cap & (1 << 31) ? "64bit " : "",
 		cap & (1 << 30) ? "ncq " : "",
+		cap & (1 << 29) ? "sntf " : "",
 		cap & (1 << 28) ? "ilck " : "",
 		cap & (1 << 27) ? "stag " : "",
 		cap & (1 << 26) ? "pm " : "",

@@ -1772,10 +1780,8 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		void __iomem *port_mmio = ahci_port_base(ap);
 
 		/* standard SATA port setup */
-		if (hpriv->port_map & (1 << i)) {
+		if (hpriv->port_map & (1 << i))
 			ap->ioaddr.cmd_addr = port_mmio;
-			ap->ioaddr.scr_addr = port_mmio + PORT_SCR;
-		}
 
 		/* disabled/not-implemented port */
 		else
Signature · 1 line
1.5.0.3
author
#16 lore raw

PATCH 7/12 libata: quickly trigger SATA SPD down after debouncing failed

In reply to Tejun Heo

Debouncing failure is a good indicator of basic link problem. Use -EPIPE to indicate debouncing failure and make ata_eh_reset() invoke sata_down_spd_limit() if the error occurs during reset.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |    6 ++++--
 drivers/ata/libata-eh.c   |    2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)
drivers/ata/libata-core.c +4 -2
index 49fe30a..c0a2b6c 100644
@@ -3294,9 +3294,11 @@ int sata_phy_debounce(struct ata_port *ap, const unsigned long *params,
 		last = cur;
 		last_jiffies = jiffies;
 
-		/* check deadline */
+		/* Check deadline.  If debouncing failed, return
+		 * -EPIPE to tell upper layer to lower link speed.
+		 */
 		if (time_after(jiffies, deadline))
-			return -EBUSY;
+			return -EPIPE;
 	}
 }
drivers/ata/libata-eh.c +1 -1
index d18c475..1c50fae 100644
@@ -1725,7 +1725,7 @@ static int ata_eh_reset(struct ata_port *ap, int classify,
 			schedule_timeout_uninterruptible(delta);
 		}
 
-		if (reset == hardreset &&
+		if (rc == -EPIPE ||
 		    try == ARRAY_SIZE(ata_eh_reset_timeouts) - 1)
 			sata_down_spd_limit(ap);
 		if (hardreset)
Signature · 1 line
1.5.0.3
author
#17 lore raw

PATCH 8/12 libata: improve SCSI scan failure handling

In reply to Tejun Heo

SCSI scan may fail due to memory allocation failure even if EH is not in progress. Due to use of GFP_ATOMIC in SCSI scan path, allocation failure isn't too rare especially while probing multiple devices at once which is the case when a bunch of devices are connected to PMP.

This patch moves SCSI scan failure detetion logic from ata_scsi_hotplug() to ata_scsi_scan_host() and implement synchronous scan behavior. The synchronous path sleeps briefly and repeats SCSI scan if some devices aren't attached properly. It contains robust retry loop to minimize the chance of device misdetection during boot and falls back to async retry if everything fails.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |    2 +-
 drivers/ata/libata-scsi.c |   63 +++++++++++++++++++++++++++++++++-----------
 drivers/ata/libata.h      |    2 +-
 3 files changed, 49 insertions(+), 18 deletions(-)
drivers/ata/libata-core.c +1 -1
index c0a2b6c..f776048 100644
@@ -6399,7 +6399,7 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-		ata_scsi_scan_host(ap);
+		ata_scsi_scan_host(ap, 1);
 	}
 
 	return 0;
drivers/ata/libata-scsi.c +47 -16

90 lines not on this page · raw message

drivers/ata/libata.h +1 -1
index ba17fc5..48836b2 100644
@@ -112,7 +112,7 @@ static inline int ata_acpi_on_devcfg(struct ata_device *adev) { return 0; }
 /* libata-scsi.c */
 extern int ata_scsi_add_hosts(struct ata_host *host,
 			      struct scsi_host_template *sht);
-extern void ata_scsi_scan_host(struct ata_port *ap);
+extern void ata_scsi_scan_host(struct ata_port *ap, int sync);
 extern int ata_scsi_offline_dev(struct ata_device *dev);
 extern void ata_scsi_hotplug(struct work_struct *work);
 extern unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf,
Signature · 1 line
1.5.0.3
author
#18 lore raw

PATCH 9/12 libata: reorganize ata_ehi_hotplugged()

In reply to Tejun Heo

__ata_ehi_hotplugged() now has no users. Regorganize ata_ehi_hotplugged() such that a new function ata_ehi_schedule_probe() deals with scheduling probing. ata_ehi_hotplugged() calls it and additionally marks hotplug specific flags. ata_ehi_schedule_probe() will be used laster.

Signed-off-by Tejun Heo <[email protected]>
---
 include/linux/libata.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
include/linux/libata.h +4 -3
index 05670b5..09af7a4 100644
@@ -921,16 +921,17 @@ extern void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
 	(ehi)->desc_len = 0; \
 } while (0)
 
-static inline void __ata_ehi_hotplugged(struct ata_eh_info *ehi)
+static inline void ata_ehi_schedule_probe(struct ata_eh_info *ehi)
 {
-	ehi->flags |= ATA_EHI_HOTPLUGGED | ATA_EHI_RESUME_LINK;
+	ehi->flags |= ATA_EHI_RESUME_LINK;
 	ehi->action |= ATA_EH_SOFTRESET;
 	ehi->probe_mask |= (1 << ATA_MAX_DEVICES) - 1;
 }
 
 static inline void ata_ehi_hotplugged(struct ata_eh_info *ehi)
 {
-	__ata_ehi_hotplugged(ehi);
+	ata_ehi_schedule_probe(ehi);
+	ehi->flags |= ATA_EHI_HOTPLUGGED;
 	ehi->err_mask |= AC_ERR_ATA_BUS;
 }
Signature · 1 line
1.5.0.3
author
#19 lore raw

PATCH 10/12 libata: clear HOTPLUG flag after a reset

In reply to Tejun Heo

ATA_EHI_HOTPLUGGED is a hint for reset functions indicating the the port might have gone through hotplug/unplug just before entering EH. Reset functions modify their behaviors a bit to handle the situation better - e.g. using longer debouncing delay.

Currently, once HOTPLUG is set, it isn't cleared till the end of EH. This is unnecessary and makes EH take longer. Clear the HOTPLUGGED flag after a reset try (successful or not).

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-eh.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)
drivers/ata/libata-eh.c +10 -5
index 1c50fae..6518108 100644
@@ -1649,7 +1649,7 @@ static int ata_eh_reset(struct ata_port *ap, int classify,
 			} else
 				ata_port_printk(ap, KERN_ERR,
 					"prereset failed (errno=%d)\n", rc);
-			return rc;
+			goto out;
 		}
 	}
 

@@ -1662,7 +1662,8 @@ static int ata_eh_reset(struct ata_port *ap, int classify,
 		/* prereset told us not to reset, bang classes and return */
 		for (i = 0; i < ATA_MAX_DEVICES; i++)
 			classes[i] = ATA_DEV_NONE;
-		return 0;
+		rc = 0;
+		goto out;
 	}
 
 	/* did prereset() screw up?  if so, fix up to avoid oopsing */

@@ -1698,7 +1699,8 @@ static int ata_eh_reset(struct ata_port *ap, int classify,
 			ata_port_printk(ap, KERN_ERR,
 					"follow-up softreset required "
 					"but no softreset avaliable\n");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto out;
 		}
 
 		ata_eh_about_to_do(ap, NULL, ATA_EH_RESET_MASK);

@@ -1708,7 +1710,8 @@ static int ata_eh_reset(struct ata_port *ap, int classify,
 		    classes[0] == ATA_DEV_UNKNOWN) {
 			ata_port_printk(ap, KERN_ERR,
 					"classification failed\n");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto out;
 		}
 	}
 

@@ -1753,7 +1756,9 @@ static int ata_eh_reset(struct ata_port *ap, int classify,
 		ata_eh_done(ap, NULL, ehc->i.action & ATA_EH_RESET_MASK);
 		ehc->i.action |= ATA_EH_REVALIDATE;
 	}
-
+ out:
+	/* clear hotplug flag */
+	ehc->i.flags &= ~ATA_EHI_HOTPLUGGED;
 	return rc;
 }
Signature · 1 line
1.5.0.3
author
#24 lore raw

PATCH 12/12 libata: implement EH fast drain

In reply to Tejun Heo

In most cases, when EH is scheduled, all in-flight commands are aborted causing EH to kick in immediately. However, in some cases (especially with PMP), it's unclear which commands are affected by the error condition and although aborting all in-flight commands work, it isn't optimal and may cause unnecessary disruption. On the other hand, waiting for in-flight commands to drain themselves can take up to 30seconds.

This patch implements EH fast drain to handle such situations. It gives in-flight commands some time to finish up but doesn't wait for too long. After EH is scheduled, fast drain timer is started and if no other completion occurs in ATA_EH_FASTDRAIN_INTERVAL all in-flight commands are aborted. If any completion occurred in the interval, the port is given another interval to finish up itself.

Currently ATA_EH_FASTDRAIN_INTERVAL is 3 secs which should be enough for finishing up most commands.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-core.c |    3 +
 drivers/ata/libata-eh.c   |   99 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/ata/libata.h      |    1 +
 include/linux/libata.h    |    3 +
 4 files changed, 104 insertions(+), 2 deletions(-)
drivers/ata/libata-core.c +3 -0
index f776048..49c9ea5 100644
@@ -6023,6 +6023,9 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 	INIT_WORK(&ap->scsi_rescan_task, ata_scsi_dev_rescan);
 	INIT_LIST_HEAD(&ap->eh_done_q);
 	init_waitqueue_head(&ap->eh_wait_q);
+	init_timer_deferrable(&ap->fastdrain_timer);
+	ap->fastdrain_timer.function = ata_eh_fastdrain_timerfn;
+	ap->fastdrain_timer.data = (unsigned long)ap;
 
 	ap->cbl = ATA_CBL_NONE;
drivers/ata/libata-eh.c +97 -2

135 lines not on this page · raw message

drivers/ata/libata.h +1 -0
index 48836b2..564cd23 100644
@@ -151,6 +151,7 @@ extern int ata_bus_probe(struct ata_port *ap);
 extern enum scsi_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd);
 extern void ata_scsi_error(struct Scsi_Host *host);
 extern void ata_port_wait_eh(struct ata_port *ap);
+extern void ata_eh_fastdrain_timerfn(unsigned long arg);
 extern void ata_qc_schedule_eh(struct ata_queued_cmd *qc);
 
 /* libata-sff.c */
include/linux/libata.h +3 -0
index 09af7a4..1424e80 100644
@@ -565,6 +565,9 @@ struct ata_port {
 	pm_message_t		pm_mesg;
 	int			*pm_result;
 
+	struct timer_list	fastdrain_timer;
+	unsigned long		fastdrain_cnt;
+
 	void			*private_data;
 
 #ifdef CONFIG_ATA_ACPI
Signature · 1 line
1.5.0.3
author
#21 lore raw

PATCH 11/12 libata: schedule probing after SError access failure during autopsy

In reply to Tejun Heo

If SError isn't accessible, EH can't tell whether hotplug has happened or not. Report SError read failure with AC_ERR_OTHER and schedule probing with hardreset. This will be mainly useful for PMPs.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-eh.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
drivers/ata/libata-eh.c +5 -1
index 6518108..fe4be8a 100644
@@ -1413,8 +1413,12 @@ static void ata_eh_autopsy(struct ata_port *ap)
 	if (rc == 0) {
 		ehc->i.serror |= serror;
 		ata_eh_analyze_serror(ap);
-	} else if (rc != -EOPNOTSUPP)
+	} else if (rc != -EOPNOTSUPP) {
+		/* SError read failed, force hardreset and probing */
+		ata_ehi_schedule_probe(&ehc->i);
 		ehc->i.action |= ATA_EH_HARDRESET;
+		ehc->i.err_mask |= AC_ERR_OTHER;
+	}
 
 	/* analyze NCQ failure */
 	ata_eh_analyze_ncq_error(ap);
Signature · 1 line
1.5.0.3
#2 lore raw

Re: [PATCH 01/12] libata: update EH report formatting

In reply to Tejun Heo
Quoted 10 lines · Tejun Heo

Update formatting for LLD provided error descriptions such that...

* No default () around it
* Each element is separated by <> not ,

This change makes each element responsible for putting a space after itself instead of before - e.g. "<elem> " not ", elem". This is more conventional and allows more flexible combination of messages.

Signed-off-by Tejun Heo <[email protected]>

either way, it is messy.

I guarantee that programmers are not going to remember always to add a "magic space" after certain error descriptions. It is far, far better for the upper layer to help out a bit, and insert a space and/or comma for the LLDD. After all, the LLDD might not be the only one printing error messages at that time; the upper layer might also wish to do so in the same dmesg line.

#5 lore raw

Re: [PATCH 02/12] libata: implement AC_ERR_NCQ

In reply to Tejun Heo

include/linux/libata.h enum ata_completion_errors { · line 326

Quoted 8 lines · Tejun Heo

+++ b/include/linux/libata.h

include/linux/libata.h +2 -1
@@ -322,7 +322,8 @@ enum ata_completion_errors {
 	AC_ERR_SYSTEM		= (1 << 6), /* system error */
 	AC_ERR_INVALID		= (1 << 7), /* invalid argument */
 	AC_ERR_OTHER		= (1 << 8), /* unknown */
-	AC_ERR_NODEV_HINT	= (1 << 9), /* polling device detection hint */
+	AC_ERR_NCQ		= (1 << 9), /* marker for offending NCQ qc */
+	AC_ERR_NODEV_HINT	= (1 << 10), /* polling device detection hint */

patch is OK, except for the above very-minor flaw: to remind, do not re-arrange bit numbers in the same patch you are adding bits. it makes the patch more difficult to read. Just add the new bit at the end.

If you wish to re-arrange the bits, do it in a separate patch (though I see no need for re-arranging)

#8 lore raw

Re: [PATCH 03/12] libata: make ->scr_read/write callbacks return error code

In reply to Tejun Heo
Quoted 4 lines · Tejun Heo

Convert ->scr_read/write callbacks to return error code to better indicate failure. This will help handling of SCR_NOTIFICATION.

Signed-off-by Tejun Heo <[email protected]>

ACK

but I do not look forward to merging this with #mv-eh :) [my problem, not yours]

#10 lore raw

Re: [PATCH 04/12] ahci: make NO_NCQ handling more consistent

In reply to Tejun Heo
Quoted 23 lines · Tejun Heo

ahci_save_initial_config() is responsible for reading, screening the host CAP register and storing the modified result into hpriv->cap for the rest of the driver. Move ATA_FLAG_NO_NCQ handling into ahci_save_initial_config(). It's more consistent this way and the rest of the driver can always refer to hpriv->cap to determine configured capability.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/ahci.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
drivers/ata/ahci.c +1 -1
index 0e001ad..0555db5 100644
@@ -527,13 +527,19 @@ static void ahci_save_initial_config(struct pci_dev *pdev,
 	hpriv->saved_cap = cap = readl(mmio + HOST_CAP);
 	hpriv->saved_port_map = port_map = readl(mmio + HOST_PORTS_IMPL);
 
-	/* some chips lie about 64bit support */
+	/* some chips lie about their capabilities, bust them */
 	if ((cap & HOST_CAP_64) && (pi->flags & AHCI_FLAG_32BIT_ONLY)) {

drivers/ata/ahci.c static void ahci_save_initial_config(struct pci_dev *pdev, · line 541

Quoted 10 lines
drivers/ata/ahci.c +5 -0
 		dev_printk(KERN_INFO, &pdev->dev,
 			   "controller can't do 64bit DMA, forcing 32bit\n");
 		cap &= ~HOST_CAP_64;
 	}
 
+	if ((cap & HOST_CAP_NCQ) && (pi->flags & AHCI_FLAG_NO_NCQ)) {
+		dev_printk(KERN_INFO, &pdev->dev,
+			   "controller can't do NCQ, turning off CAP_NCQ\n");
+		cap &= ~HOST_CAP_NCQ;
+	}

ACK, even though I dislike the manipulation of cap.

Your patch did not start this manipulation, as illustrated by AHCI_FLAG_32BIT_ONLY handling, but it is nonetheless a loss of information we may later come to regret. I predict the creation of 'original_cap' sometime in the distant future :)

#12 lore raw

Re: [PATCH 05/12] ahci: implement SCR_NOTIFICATION r/w

In reply to Tejun Heo

drivers/ata/ahci.c static void ahci_print_info(struct ata_host *host) · line 1714

Quoted 13 lines · Tejun Heo
drivers/ata/ahci.c +3 -2
@@ -1698,12 +1705,13 @@ static void ahci_print_info(struct ata_host *host)
 
 	dev_printk(KERN_INFO, &pdev->dev,
 		"flags: "
-	       	"%s%s%s%s%s%s"
-	       	"%s%s%s%s%s%s%s\n"
+		"%s%s%s%s%s%s%s"
+		"%s%s%s%s%s%s%s\n"
 	       	,
 
 		cap & (1 << 31) ? "64bit " : "",
 		cap & (1 << 30) ? "ncq " : "",
+		cap & (1 << 29) ? "sntf " : "",

ACK, although it is not clear to me why both "%s%s%s" lines are changed.
whitespace addition, perhaps?

#20 lore raw

Re: [PATCH 10/12] libata: clear HOTPLUG flag after a reset

In reply to Tejun Heo
Quoted 5 lines · Tejun Heo

ATA_EHI_HOTPLUGGED is a hint for reset functions indicating the the port might have gone through hotplug/unplug just before entering EH. Reset functions modify their behaviors a bit to handle the situation better - e.g. using longer debouncing delay.

Quoted 8 lines

Currently, once HOTPLUG is set, it isn't cleared till the end of EH. This is unnecessary and makes EH take longer. Clear the HOTPLUGGED flag after a reset try (successful or not).

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-eh.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

ACK patches 6-10

#22 lore raw

Re: [PATCH 11/12] libata: schedule probing after SError access failure during autopsy

In reply to Tejun Heo
Quoted 8 lines · Tejun Heo

If SError isn't accessible, EH can't tell whether hotplug has happened or not. Report SError read failure with AC_ERR_OTHER and schedule probing with hardreset. This will be mainly useful for PMPs.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-eh.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

What if the LLDD told us hotplug happened?

Even though sata_promise has SError, it should be noted that hotplug under sata_promise is not signalled through normal means, but by a special vendor-specific register

#25 lore raw

Re: [PATCH 12/12] libata: implement EH fast drain

In reply to Tejun Heo
Quoted 15 lines · Tejun Heo

In most cases, when EH is scheduled, all in-flight commands are aborted causing EH to kick in immediately. However, in some cases (especially with PMP), it's unclear which commands are affected by the error condition and although aborting all in-flight commands work, it isn't optimal and may cause unnecessary disruption. On the other hand, waiting for in-flight commands to drain themselves can take up to 30seconds.

This patch implements EH fast drain to handle such situations. It gives in-flight commands some time to finish up but doesn't wait for too long. After EH is scheduled, fast drain timer is started and if no other completion occurs in ATA_EH_FASTDRAIN_INTERVAL all in-flight commands are aborted. If any completion occurred in the interval, the port is given another interval to finish up itself.

Quoted 4 lines

Currently ATA_EH_FASTDRAIN_INTERVAL is 3 secs which should be enough for finishing up most commands.

Signed-off-by Tejun Heo <[email protected]>

what type of errors are we talking about? device errors can be handled on a more granular basis than other errors.

author
#6 lore raw

Re: [PATCH 02/12] libata: implement AC_ERR_NCQ

In reply to Jeff Garzik
Quoted 16 lines · Jeff Garzik
Quoted 9 lines · Tejun Heo

+++ b/include/linux/libata.h

include/linux/libata.h +2 -1
@@ -322,7 +322,8 @@ enum ata_completion_errors {
     AC_ERR_SYSTEM        = (1 << 6), /* system error */
     AC_ERR_INVALID        = (1 << 7), /* invalid argument */
     AC_ERR_OTHER        = (1 << 8), /* unknown */
-    AC_ERR_NODEV_HINT    = (1 << 9), /* polling device detection hint */
+    AC_ERR_NCQ        = (1 << 9), /* marker for offending NCQ qc */
+    AC_ERR_NODEV_HINT    = (1 << 10), /* polling device detection

hint */

patch is OK, except for the above very-minor flaw: to remind, do not re-arrange bit numbers in the same patch you are adding bits. it makes the patch more difficult to read. Just add the new bit at the end.

Quoted 2 lines

If you wish to re-arrange the bits, do it in a separate patch (though I see no need for re-arranging)

Indeed, I didn't mean to rearrange them. Probably just slipped through while editing. Will fix.

Signature · 1 line
tejun
author
#13 lore raw

Re: [PATCH 05/12] ahci: implement SCR_NOTIFICATION r/w

In reply to Jeff Garzik
Quoted 12 lines · Jeff Garzik
Quoted 11 lines · Tejun Heo
drivers/ata/ahci.c +2 -2
@@ -1698,12 +1705,13 @@ static void ahci_print_info(struct ata_host
 *host)
 
     dev_printk(KERN_INFO, &pdev->dev,
         "flags: "
-               "%s%s%s%s%s%s"
-               "%s%s%s%s%s%s%s\n"
+        "%s%s%s%s%s%s%s"
+        "%s%s%s%s%s%s%s\n"
                ,
Quoted 6 lines

drivers/ata/ahci.c static void ahci_print_info(struct ata_host *host) · line 1714

Quoted 3 lines
drivers/ata/ahci.c +1 -0
         cap & (1 << 31) ? "64bit " : "",
         cap & (1 << 30) ? "ncq " : "",
+        cap & (1 << 29) ? "sntf " : "",

ACK, although it is not clear to me why both "%s%s%s" lines are changed.
whitespace addition, perhaps?

No, another slip through. The patch originally contained ncq(x) pmp(x) kind of notation to indicate disabled features then I dropped it as it was an overkill. The formatting change is left over. Sorry about that.

Signature · 1 line
tejun
author
#3 lore raw

Re: [PATCH 01/12] libata: update EH report formatting

In reply to Jeff Garzik
Quoted 12 lines · Jeff Garzik
Quoted 10 lines · Tejun Heo

Update formatting for LLD provided error descriptions such that...

* No default () around it
* Each element is separated by <> not ,

This change makes each element responsible for putting a space after itself instead of before - e.g. "<elem> " not ", elem". This is more conventional and allows more flexible combination of messages.

Signed-off-by Tejun Heo <[email protected]>
Quoted 8 lines

either way, it is messy.

I guarantee that programmers are not going to remember always to add a "magic space" after certain error descriptions. It is far, far better for the upper layer to help out a bit, and insert a space and/or comma for the LLDD. After all, the LLDD might not be the only one printing error messages at that time; the upper layer might also wish to do so in the same dmesg line.

Okay then, I guess it's __ata_ehi_push_desc()/ata_ehi_push_desc() time, where the prefixed one is the raw one and the latter one is with automatic "<> " added. Will fix.

Signature · 1 line
tejun
author
#23 lore raw

Re: [PATCH 11/12] libata: schedule probing after SError access failure during autopsy

In reply to Jeff Garzik
Quoted 10 lines · Jeff Garzik
Quoted 8 lines · Tejun Heo

If SError isn't accessible, EH can't tell whether hotplug has happened or not. Report SError read failure with AC_ERR_OTHER and schedule probing with hardreset. This will be mainly useful for PMPs.

Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/libata-eh.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
Quoted 5 lines

What if the LLDD told us hotplug happened?

Even though sata_promise has SError, it should be noted that hotplug under sata_promise is not signalled through normal means, but by a special vendor-specific register

Those two are independent and doesn't interfere with each other. This is just safeguard against PMP SCR read failures which might or might not imply phy event. ie. it's just another source of hotplug event.

Signature · 1 line
tejun
author
#26 lore raw

Re: [PATCH 12/12] libata: implement EH fast drain

In reply to Jeff Garzik
Quoted 16 lines · Jeff Garzik
Quoted 15 lines · Tejun Heo

In most cases, when EH is scheduled, all in-flight commands are aborted causing EH to kick in immediately. However, in some cases (especially with PMP), it's unclear which commands are affected by the error condition and although aborting all in-flight commands work, it isn't optimal and may cause unnecessary disruption. On the other hand, waiting for in-flight commands to drain themselves can take up to 30seconds.

This patch implements EH fast drain to handle such situations. It gives in-flight commands some time to finish up but doesn't wait for too long. After EH is scheduled, fast drain timer is started and if no other completion occurs in ATA_EH_FASTDRAIN_INTERVAL all in-flight commands are aborted. If any completion occurred in the interval, the port is given another interval to finish up itself.

Quoted 7 lines
Quoted 4 lines

Currently ATA_EH_FASTDRAIN_INTERVAL is 3 secs which should be enough for finishing up most commands.

Signed-off-by Tejun Heo <[email protected]>

what type of errors are we talking about? device errors can be handled on a more granular basis than other errors.

On standalone (non-PMP) port, this doesn't matter because in-flight commands are always aborted when error occurs. Fast drain helps when PMP is involved. For example, after certain event, PMP can notify host that something is off via SDB notify but until EH kicks in and performs autopsy, there's no way to tell to which port the event happened.

If there were in-flight commands to the failed link and/or PMP chip went for lunch after the event (it often does), EH is forced to wait for timeout even though it knows something is off. Fast drain helps in such situations - ie. when EH knows something wrong is going on and it's very likely that commands which aren't completing are going to timeout.

Signature · 1 line
tejun
author
#14 lore raw

Re: [PATCH 05/12] ahci: implement SCR_NOTIFICATION r/w

In reply to Jeff Garzik
Quoted 12 lines · Jeff Garzik
Quoted 11 lines · Tejun Heo
drivers/ata/ahci.c +2 -2
@@ -1698,12 +1705,13 @@ static void ahci_print_info(struct ata_host
 *host)
 
     dev_printk(KERN_INFO, &pdev->dev,
         "flags: "
-               "%s%s%s%s%s%s"
-               "%s%s%s%s%s%s%s\n"
+        "%s%s%s%s%s%s%s"
+        "%s%s%s%s%s%s%s\n"
                ,
Quoted 6 lines

drivers/ata/ahci.c static void ahci_print_info(struct ata_host *host) · line 1714

Quoted 3 lines
drivers/ata/ahci.c +1 -0
         cap & (1 << 31) ? "64bit " : "",
         cap & (1 << 30) ? "ncq " : "",
+        cap & (1 << 29) ? "sntf " : "",

ACK, although it is not clear to me why both "%s%s%s" lines are changed.
whitespace addition, perhaps?

Actually, it was minor cleanup converting eight spaces into a tab. I'll note it in the commit message.

Signature · 1 line
tejun