Re: [patch 05/25] ata: add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61

[Zoltan Boszormenyi <zboszor@dunaweb.hu>]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

Hi,

Zoltan Boszormenyi írta:
> I will test the code without this locking. Can you give me a better idea
> besides beating both my disks with separate requests? Say, bonnie++
> and simultaneous hdparm -tT on both?

I tested the driver without locking with the above test, it survived nicely.
Patch is attached which deletes locking, enables swncq by default and
a correction to my previous readability cleanup.

Best regards,
Zoltán Böszörményi


[-- Attachment #2: sata_nv.c-nolock-swncq-enabled-cleanup.patch --]
[-- Type: text/x-patch, Size: 2290 bytes --]

--- linux-2.6.23-rc3-mm1/drivers/ata/sata_nv.c.committed	2007-09-21 08:32:04.000000000 +0200
+++ linux-2.6.23-rc3-mm1/drivers/ata/sata_nv.c	2007-09-21 09:27:49.000000000 +0200
@@ -279,7 +279,6 @@
 
 	unsigned int	last_issue_tag;
 
-	spinlock_t	lock;
 	/* fifo circular queue to store deferral command */
 	struct defer_queue defer_queue;
 
@@ -637,7 +636,7 @@
 MODULE_VERSION(DRV_VERSION);
 
 static int adma_enabled = 1;
-static int swncq_enabled;
+static int swncq_enabled = 1;
 
 static void nv_adma_register_mode(struct ata_port *ap)
 {
@@ -1965,7 +1964,6 @@
 	pp->sactive_block = ap->ioaddr.scr_addr + 4 * SCR_ACTIVE;
 	pp->irq_block = mmio + NV_INT_STATUS_MCP55 + ap->port_no * 2;
 	pp->tag_block = mmio + NV_NCQ_REG_MCP55 + ap->port_no * 2;
-	spin_lock_init(&pp->lock);
 
 	return 0;
 }
@@ -2051,18 +2049,15 @@
 {
 	struct ata_port *ap = qc->ap;
 	struct nv_swncq_port_priv *pp = ap->private_data;
-	unsigned long flags;
 
 	if (qc->tf.protocol != ATA_PROT_NCQ)
 		return ata_qc_issue_prot(qc);
 
 	DPRINTK("Enter\n");
-	spin_lock_irqsave(&pp->lock, flags);
 	if (!pp->qc_active)
 		nv_swncq_issue_atacmd(ap, qc);
 	else
 		nv_swncq_qc_to_dq(ap, qc);	/* add qc to defer queue */
-	spin_unlock_irqrestore(&pp->lock, flags);
 	return 0;
 }
 
@@ -2265,7 +2260,6 @@
 		return;
 	}
 
-	spin_lock(&pp->lock);
 	if (fis & NV_SWNCQ_IRQ_BACKOUT) {
 		/* If the IRQ is backout, driver must issue
 		 * the new command again some time later.
@@ -2290,8 +2284,7 @@
 		 */
 		pp->dhfis_bits |= (0x1 << pp->last_issue_tag);
 		pp->ncq_flags |= ncq_saw_d2h;
-		if ((pp->ncq_flags & ncq_saw_sdb) ||
-		    (pp->ncq_flags & ncq_saw_backout)) {
+		if (pp->ncq_flags & (ncq_saw_sdb | ncq_saw_backout)) {
 			ata_ehi_push_desc(ehi, "illegal fis transaction");
 			ehi->err_mask |= AC_ERR_HSM;
 			ehi->action |= ATA_EH_HARDRESET;
@@ -2302,7 +2295,7 @@
 		    !(pp->ncq_flags & ncq_saw_dmas)) {
 			ata_stat = ap->ops->check_status(ap);
 			if (ata_stat & ATA_BUSY)
-				goto irq_exit;
+				return;
 
 			if (pp->defer_queue.defer_bits) {
 				DPRINTK("send next command\n");
@@ -2321,13 +2314,11 @@
 		rc = nv_swncq_dmafis(ap);
 	}
 
-irq_exit:
-	spin_unlock(&pp->lock);
 	return;
+
 irq_error:
 	ata_ehi_push_desc(ehi, "fis:0x%x", fis);
 	ata_port_freeze(ap);
-	spin_unlock(&pp->lock);
 	return;
 }