Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Robert Hancock <hancockr@shaw.ca>]

Alan Cox wrote:
>> I guess that bit doesn't really make any difference with remotely modern 
>> drives, then.. Could we make that ata_id_has_dword_io check always 
>> return true if ata_id_is_ata returns true and only check word 48 if not?
> 
> I suspect (but need to dig further) that ata_id_has_dword_io should only
> be called by pata_legacy.
> 
>> I saw Willy Tarreau's patch from February for this, I agree that we 
>> should likely use a separate data_xfer method for 32-bit transfer (or if 
>> enough controllers should support 32-bit, then just make it be the 
>> default and make a separate 16-bit only function for those that don't), 
>> rather than punting the decision to the user with hdparm.
> 
> Definitely.
> 
>> You mentioned in the thread for Willy's patch that "some
>> controllers have quirky rules for 32bit xfers" - any details anywhere?
> 
> There are two main ones
> 
> - Some controllers only support 32bit I/O for a multiple of 32bit values
> [sometimes 'unless the fifo is disabled']. I'd have to go back over the
> docs but I think the AMD may be one of those
> - Some controllers (VLB generally) require a magic sequence before the
> transfer. You'll see that in the pata_legacy bits.

Here's my first cut at it. Compile tested only. This sets most controllers
to use 32-bit PIO except for those which could potentially be on a real ISA
or other 16-bit bus. It's a bit non-obvious what to do with some of the
drivers, so input is welcome.

This implementation doesn't check the ata_id_has_dword_io at all, since it
would only make a difference on controllers where we don't really want to
use it anyway.

It seems like regardless of whether we do 32-bit by default or not the 32-bit
data_xfer function should be added to libata core as we have several drivers
which duplicate the same code currently..

Signed-off-by: Robert Hancock <hancockr@shaw.ca>	

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 304fdc6..acb65a9 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -673,13 +673,14 @@ static inline void ata_tf_to_host(struct ata_port *ap,
 }
 
 /**
- *	ata_sff_data_xfer - Transfer data by PIO
+ *	ata_sff_data_xfer_16bit - Transfer data by PIO
  *	@dev: device to target
  *	@buf: data buffer
  *	@buflen: buffer length
  *	@rw: read/write
  *
- *	Transfer data from/to the device data register by PIO.
+ *	Transfer data from/to the device data register by PIO using only
+ *	16-bit transfers.
  *
  *	LOCKING:
  *	Inherited from caller.
@@ -687,7 +688,7 @@ static inline void ata_tf_to_host(struct ata_port *ap,
  *	RETURNS:
  *	Bytes consumed.
  */
-unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
+unsigned int ata_sff_data_xfer_16bit(struct ata_device *dev, unsigned char *buf,
 			       unsigned int buflen, int rw)
 {
 	struct ata_port *ap = dev->link->ap;
@@ -719,6 +720,51 @@ unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
 }
 
 /**
+ *	ata_sff_data_xfer - Transfer data by PIO
+ *	@dev: device to target
+ *	@buf: data buffer
+ *	@buflen: buffer length
+ *	@rw: read/write
+ *
+ *	Transfer data from/to the device data register by PIO.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	Bytes consumed.
+ */
+unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
+			       unsigned int buflen, int rw)
+{
+	struct ata_port *ap = dev->link->ap;
+	void __iomem *data_addr = ap->ioaddr.data_addr;
+	unsigned int words = buflen >> 2;
+	unsigned int slop = buflen & 3;
+
+	/* Transfer multiple of 4 bytes */
+	if (rw == READ)
+		ioread32_rep(data_addr, buf, words);
+	else
+		iowrite32_rep(data_addr, buf, words);
+
+	/* Transfer trailing 1 byte, if any. */
+	if (unlikely(slop)) {
+		__le32 pad = 0;
+		if (rw == READ) {
+			pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
+			memcpy(buf + buflen - slop, &pad, slop);
+		} else {
+			memcpy(&pad, buf + buflen - slop, slop);
+			iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
+		}
+		buflen += 4 - slop;
+	}
+
+	return buflen;
+}
+
+/**
  *	ata_sff_data_xfer_noirq - Transfer data by PIO
  *	@dev: device to target
  *	@buf: data buffer
@@ -748,6 +794,36 @@ unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev, unsigned char *buf,
 }
 
 /**
+ *	ata_sff_data_xfer_noirq_16bit - Transfer data by PIO
+ *	@dev: device to target
+ *	@buf: data buffer
+ *	@buflen: buffer length
+ *	@rw: read/write
+ *
+ *	Transfer data from/to the device data register by PIO. Do the
+ *	transfer with interrupts disabled using only 16-bit transfers.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	Bytes consumed.
+ */
+unsigned int ata_sff_data_xfer_noirq_16bit(struct ata_device *dev,
+				     unsigned char *buf,
+				     unsigned int buflen, int rw)
+{
+	unsigned long flags;
+	unsigned int consumed;
+
+	local_irq_save(flags);
+	consumed = ata_sff_data_xfer_16bit(dev, buf, buflen, rw);
+	local_irq_restore(flags);
+
+	return consumed;
+}
+
+/**
  *	ata_pio_sector - Transfer a sector of data.
  *	@qc: Command on going
  *
@@ -2827,7 +2903,9 @@ EXPORT_SYMBOL_GPL(ata_sff_tf_load);
 EXPORT_SYMBOL_GPL(ata_sff_tf_read);
 EXPORT_SYMBOL_GPL(ata_sff_exec_command);
 EXPORT_SYMBOL_GPL(ata_sff_data_xfer);
+EXPORT_SYMBOL_GPL(ata_sff_data_xfer_16bit);
 EXPORT_SYMBOL_GPL(ata_sff_data_xfer_noirq);
+EXPORT_SYMBOL_GPL(ata_sff_data_xfer_noirq_16bit);
 EXPORT_SYMBOL_GPL(ata_sff_irq_on);
 EXPORT_SYMBOL_GPL(ata_sff_irq_clear);
 EXPORT_SYMBOL_GPL(ata_sff_hsm_move);
diff --git a/drivers/ata/pata_at32.c b/drivers/ata/pata_at32.c
index 82fb6e2..aa90b19 100644
--- a/drivers/ata/pata_at32.c
+++ b/drivers/ata/pata_at32.c
@@ -173,6 +173,7 @@ static struct scsi_host_template at32_sht = {
 static struct ata_port_operations at32_port_ops = {
 	.inherits		= &ata_sff_port_ops,
 	.cable_detect		= ata_cable_40wire,
+	.sff_data_xfer		= ata_sff_data_xfer_16bit,
 	.set_piomode		= pata_at32_set_piomode,
 };
 
diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
index cf9e984..a1f09ca 100644
--- a/drivers/ata/pata_icside.c
+++ b/drivers/ata/pata_icside.c
@@ -336,7 +336,7 @@ static struct ata_port_operations pata_icside_port_ops = {
 	.inherits		= &ata_sff_port_ops,
 	/* no need to build any PRD tables for DMA */
 	.qc_prep		= ata_noop_qc_prep,
-	.sff_data_xfer		= ata_sff_data_xfer_noirq,
+	.sff_data_xfer		= ata_sff_data_xfer_noirq_16bit,
 	.bmdma_setup		= pata_icside_bmdma_setup,
 	.bmdma_start		= pata_icside_bmdma_start,
 	.bmdma_stop		= pata_icside_bmdma_stop,
diff --git a/drivers/ata/pata_isapnp.c b/drivers/ata/pata_isapnp.c
index 6a111ba..6f38f76 100644
--- a/drivers/ata/pata_isapnp.c
+++ b/drivers/ata/pata_isapnp.c
@@ -26,6 +26,7 @@ static struct scsi_host_template isapnp_sht = {
 static struct ata_port_operations isapnp_port_ops = {
 	.inherits	= &ata_sff_port_ops,
 	.cable_detect	= ata_cable_40wire,
+	.sff_data_xfer	= ata_sff_data_xfer_16bit,
 };
 
 /**
diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
index bc037ff..d919934 100644
--- a/drivers/ata/pata_legacy.c
+++ b/drivers/ata/pata_legacy.c
@@ -226,12 +226,12 @@ static const struct ata_port_operations legacy_base_port_ops = {
 
 static struct ata_port_operations simple_port_ops = {
 	.inherits	= &legacy_base_port_ops,
-	.sff_data_xfer	= ata_sff_data_xfer_noirq,
+	.sff_data_xfer	= ata_sff_data_xfer_noirq_16bit,
 };
 
 static struct ata_port_operations legacy_port_ops = {
 	.inherits	= &legacy_base_port_ops,
-	.sff_data_xfer	= ata_sff_data_xfer_noirq,
+	.sff_data_xfer	= ata_sff_data_xfer_noirq_16bit,
 	.set_mode	= legacy_set_mode,
 };
 
@@ -286,39 +286,18 @@ static void pdc20230_set_piomode(struct ata_port *ap, struct ata_device *adev)
 static unsigned int pdc_data_xfer_vlb(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw)
 {
-	if (ata_id_has_dword_io(dev->id)) {
-		struct ata_port *ap = dev->link->ap;
-		int slop = buflen & 3;
-		unsigned long flags;
+	unsigned long flags;
 
-		local_irq_save(flags);
+	local_irq_save(flags);
 
-		/* Perform the 32bit I/O synchronization sequence */
-		ioread8(ap->ioaddr.nsect_addr);
-		ioread8(ap->ioaddr.nsect_addr);
-		ioread8(ap->ioaddr.nsect_addr);
+	/* Perform the 32bit I/O synchronization sequence */
+	ioread8(ap->ioaddr.nsect_addr);
+	ioread8(ap->ioaddr.nsect_addr);
+	ioread8(ap->ioaddr.nsect_addr);
 
-		/* Now the data */
-		if (rw == READ)
-			ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-		else
-			iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-
-		if (unlikely(slop)) {
-			__le32 pad;
-			if (rw == READ) {
-				pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
-				memcpy(buf + buflen - slop, &pad, slop);
-			} else {
-				memcpy(&pad, buf + buflen - slop, slop);
-				iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
-			}
-			buflen += 4 - slop;
-		}
-		local_irq_restore(flags);
-	} else
-		buflen = ata_sff_data_xfer_noirq(dev, buf, buflen, rw);
+	buflen = ata_sff_data_xfer(dev, buf, buflen, rw);
 
+	local_irq_restore(flags);
 	return buflen;
 }
 
@@ -733,33 +712,6 @@ static unsigned int qdi_qc_issue(struct ata_queued_cmd *qc)
 	return ata_sff_qc_issue(qc);
 }
 
-static unsigned int vlb32_data_xfer(struct ata_device *adev, unsigned char *buf,
-					unsigned int buflen, int rw)
-{
-	struct ata_port *ap = adev->link->ap;
-	int slop = buflen & 3;
-
-	if (ata_id_has_dword_io(adev->id)) {
-		if (rw == WRITE)
-			iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-		else
-			ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-
-		if (unlikely(slop)) {
-			__le32 pad;
-			if (rw == WRITE) {
-				memcpy(&pad, buf + buflen - slop, slop);
-				iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
-			} else {
-				pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
-				memcpy(buf + buflen - slop, &pad, slop);
-			}
-		}
-		return (buflen + 3) & ~3;
-	} else
-		return ata_sff_data_xfer(adev, buf, buflen, rw);
-}
-
 static int qdi_port(struct platform_device *dev,
 			struct legacy_probe *lp, struct legacy_data *ld)
 {
@@ -773,19 +725,19 @@ static struct ata_port_operations qdi6500_port_ops = {
 	.inherits	= &legacy_base_port_ops,
 	.set_piomode	= qdi6500_set_piomode,
 	.qc_issue	= qdi_qc_issue,
-	.sff_data_xfer	= vlb32_data_xfer,
+	.sff_data_xfer	= ata_sff_data_xfer,
 };
 
 static struct ata_port_operations qdi6580_port_ops = {
 	.inherits	= &legacy_base_port_ops,
 	.set_piomode	= qdi6580_set_piomode,
-	.sff_data_xfer	= vlb32_data_xfer,
+	.sff_data_xfer	= ata_sff_data_xfer,
 };
 
 static struct ata_port_operations qdi6580dp_port_ops = {
 	.inherits	= &legacy_base_port_ops,
 	.set_piomode	= qdi6580dp_set_piomode,
-	.sff_data_xfer	= vlb32_data_xfer,
+	.sff_data_xfer	= ata_sff_data_xfer,
 };
 
 static DEFINE_SPINLOCK(winbond_lock);
@@ -856,7 +808,7 @@ static int winbond_port(struct platform_device *dev,
 static struct ata_port_operations winbond_port_ops = {
 	.inherits	= &legacy_base_port_ops,
 	.set_piomode	= winbond_set_piomode,
-	.sff_data_xfer	= vlb32_data_xfer,
+	.sff_data_xfer	= ata_sff_data_xfer,
 };
 
 static struct legacy_controller controllers[] = {
diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index a9e8273..0ab2c8e 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -265,6 +265,7 @@ static struct ata_port_operations mpc52xx_ata_port_ops = {
 	.cable_detect		= ata_cable_40wire,
 	.set_piomode		= mpc52xx_ata_set_piomode,
 	.post_internal_cmd	= ATA_OP_NULL,
+	.sff_data_xfer		= ata_sff_data_xfer_16bit,
 };
 
 static int __devinit
diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c
index 41b4361..8cb4923 100644
--- a/drivers/ata/pata_pcmcia.c
+++ b/drivers/ata/pata_pcmcia.c
@@ -133,7 +133,7 @@ static struct scsi_host_template pcmcia_sht = {
 
 static struct ata_port_operations pcmcia_port_ops = {
 	.inherits	= &ata_sff_port_ops,
-	.sff_data_xfer	= ata_sff_data_xfer_noirq,
+	.sff_data_xfer	= ata_sff_data_xfer_noirq_16bit,
 	.cable_detect	= ata_cable_40wire,
 	.set_mode	= pcmcia_set_mode,
 };
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 8f65ad6..3310eb0 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -52,7 +52,7 @@ static struct scsi_host_template pata_platform_sht = {
 
 static struct ata_port_operations pata_platform_port_ops = {
 	.inherits		= &ata_sff_port_ops,
-	.sff_data_xfer		= ata_sff_data_xfer_noirq,
+	.sff_data_xfer		= ata_sff_data_xfer_noirq_16bit,
 	.cable_detect		= ata_cable_unknown,
 	.set_mode		= pata_platform_set_mode,
 	.port_start		= ATA_OP_NULL,
diff --git a/drivers/ata/pata_qdi.c b/drivers/ata/pata_qdi.c
index 63b7a1c..36cc778 100644
--- a/drivers/ata/pata_qdi.c
+++ b/drivers/ata/pata_qdi.c
@@ -124,35 +124,6 @@ static unsigned int qdi_qc_issue(struct ata_queued_cmd *qc)
 	return ata_sff_qc_issue(qc);
 }
 
-static unsigned int qdi_data_xfer(struct ata_device *dev, unsigned char *buf,
-				  unsigned int buflen, int rw)
-{
-	if (ata_id_has_dword_io(dev->id)) {
-		struct ata_port *ap = dev->link->ap;
-		int slop = buflen & 3;
-
-		if (rw == READ)
-			ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-		else
-			iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-
-		if (unlikely(slop)) {
-			__le32 pad;
-			if (rw == READ) {
-				pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
-				memcpy(buf + buflen - slop, &pad, slop);
-			} else {
-				memcpy(&pad, buf + buflen - slop, slop);
-				iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
-			}
-			buflen += 4 - slop;
-		}
-	} else
-		buflen = ata_sff_data_xfer(dev, buf, buflen, rw);
-
-	return buflen;
-}
-
 static struct scsi_host_template qdi_sht = {
 	ATA_PIO_SHT(DRV_NAME),
 };
@@ -160,7 +131,6 @@ static struct scsi_host_template qdi_sht = {
 static struct ata_port_operations qdi6500_port_ops = {
 	.inherits	= &ata_sff_port_ops,
 	.qc_issue	= qdi_qc_issue,
-	.sff_data_xfer	= qdi_data_xfer,
 	.cable_detect	= ata_cable_40wire,
 	.set_piomode	= qdi6500_set_piomode,
 };
diff --git a/drivers/ata/pata_winbond.c b/drivers/ata/pata_winbond.c
index a7606b0..56f5f8e 100644
--- a/drivers/ata/pata_winbond.c
+++ b/drivers/ata/pata_winbond.c
@@ -92,42 +92,12 @@ static void winbond_set_piomode(struct ata_port *ap, struct ata_device *adev)
 }
 
 
-static unsigned int winbond_data_xfer(struct ata_device *dev,
-			unsigned char *buf, unsigned int buflen, int rw)
-{
-	struct ata_port *ap = dev->link->ap;
-	int slop = buflen & 3;
-
-	if (ata_id_has_dword_io(dev->id)) {
-		if (rw == READ)
-			ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-		else
-			iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-
-		if (unlikely(slop)) {
-			__le32 pad;
-			if (rw == READ) {
-				pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
-				memcpy(buf + buflen - slop, &pad, slop);
-			} else {
-				memcpy(&pad, buf + buflen - slop, slop);
-				iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
-			}
-			buflen += 4 - slop;
-		}
-	} else
-		buflen = ata_sff_data_xfer(dev, buf, buflen, rw);
-
-	return buflen;
-}
-
 static struct scsi_host_template winbond_sht = {
 	ATA_PIO_SHT(DRV_NAME),
 };
 
 static struct ata_port_operations winbond_port_ops = {
 	.inherits	= &ata_sff_port_ops,
-	.sff_data_xfer	= winbond_data_xfer,
 	.cable_detect	= ata_cable_40wire,
 	.set_piomode	= winbond_set_piomode,
 };
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 06b8033..643328a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1481,8 +1481,12 @@ extern void ata_sff_exec_command(struct ata_port *ap,
 				 const struct ata_taskfile *tf);
 extern unsigned int ata_sff_data_xfer(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw);
+extern unsigned int ata_sff_data_xfer_16bit(struct ata_device *dev,
+			unsigned char *buf, unsigned int buflen, int rw);
 extern unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw);
+extern unsigned int ata_sff_data_xfer_noirq_16bit(struct ata_device *dev,
+			unsigned char *buf, unsigned int buflen, int rw);
 extern u8 ata_sff_irq_on(struct ata_port *ap);
 extern void ata_sff_irq_clear(struct ata_port *ap);
 extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,