[PATCH/RFC 4/4] irq-pio: add read/write multiple support

[PATCH/RFC 4/4] irq-pio: add read/write multiple support

[Albert Lee]

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

Patch 4/4: add read/write multiple support

Changes:
    - add ATA_CMD_READ_MULTI, etc. to ata.h
    - add multi_count to ata_device and initialize it with device 
identify data
    - merge ata_prot_to_cmd() with ata_dev_set_protocol()
    - ata_pio_complete(): change the wait condition from (ATA_BUSY | 
ATA_DRQ) to ATA_BUSY
    - add ata_pio_sectors() to support r/w multiple commands
    - ata_pio_block(): add ata_altstatus(ap) to prevent reading device 
status before it is valid


For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com>


[-- Attachment #2: idpio4.diff --]
[-- Type: text/plain, Size: 6509 bytes --]

--- irq-pio/include/linux/ata.h	2005-09-30 10:00:30.000000000 +0800
+++ id8/include/linux/ata.h	2005-10-03 13:54:52.000000000 +0800
@@ -128,6 +128,10 @@ enum {
 	ATA_CMD_PIO_READ_EXT	= 0x24,
 	ATA_CMD_PIO_WRITE	= 0x30,
 	ATA_CMD_PIO_WRITE_EXT	= 0x34,
+	ATA_CMD_READ_MULTI	= 0xC4,
+	ATA_CMD_READ_MULTI_EXT	= 0x29,
+	ATA_CMD_WRITE_MULTI	= 0xC5,
+	ATA_CMD_WRITE_MULTI_EXT	= 0x39,
 	ATA_CMD_SET_FEATURES	= 0xEF,
 	ATA_CMD_PACKET		= 0xA0,
 	ATA_CMD_VERIFY		= 0x40,
@@ -270,6 +274,14 @@ static inline int is_atapi_taskfile(stru
 	       (tf->protocol == ATA_PROT_ATAPI_DMA);
 }
 
+static inline int is_multi_taskfile(struct ata_taskfile *tf)
+{
+	return (tf->command == ATA_CMD_READ_MULTI) ||
+	       (tf->command == ATA_CMD_WRITE_MULTI) ||
+	       (tf->command == ATA_CMD_READ_MULTI_EXT) ||
+	       (tf->command == ATA_CMD_WRITE_MULTI_EXT);
+}
+
 static inline int ata_ok(u8 status)
 {
 	return ((status & (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ | ATA_ERR))
--- id6/include/linux/libata.h	2005-10-03 10:18:29.000000000 +0800
+++ id8/include/linux/libata.h	2005-10-03 13:56:29.000000000 +0800
@@ -285,6 +285,8 @@ struct ata_device {
 	u8			xfer_protocol;	/* taskfile xfer protocol */
 	u8			read_cmd;	/* opcode to use on read */
 	u8			write_cmd;	/* opcode to use on write */
+	unsigned int		multi_count;	/* sectors count for
+						   READ/WRITE MULTIPLE */
 };
 
 struct ata_port {
--- id7/drivers/scsi/libata-core.c	2005-10-03 12:39:03.000000000 +0800
+++ id8/drivers/scsi/libata-core.c	2005-10-03 18:33:26.000000000 +0800
@@ -616,49 +616,6 @@ void ata_tf_from_fis(u8 *fis, struct ata
 }
 
 /**
- *	ata_prot_to_cmd - determine which read/write opcodes to use
- *	@protocol: ATA_PROT_xxx taskfile protocol
- *	@lba48: true is lba48 is present
- *
- *	Given necessary input, determine which read/write commands
- *	to use to transfer data.
- *
- *	LOCKING:
- *	None.
- */
-static int ata_prot_to_cmd(int protocol, int lba48)
-{
-	int rcmd = 0, wcmd = 0;
-
-	switch (protocol) {
-	case ATA_PROT_PIO:
-		if (lba48) {
-			rcmd = ATA_CMD_PIO_READ_EXT;
-			wcmd = ATA_CMD_PIO_WRITE_EXT;
-		} else {
-			rcmd = ATA_CMD_PIO_READ;
-			wcmd = ATA_CMD_PIO_WRITE;
-		}
-		break;
-
-	case ATA_PROT_DMA:
-		if (lba48) {
-			rcmd = ATA_CMD_READ_EXT;
-			wcmd = ATA_CMD_WRITE_EXT;
-		} else {
-			rcmd = ATA_CMD_READ;
-			wcmd = ATA_CMD_WRITE;
-		}
-		break;
-
-	default:
-		return -1;
-	}
-
-	return rcmd | (wcmd << 8);
-}
-
-/**
  *	ata_dev_set_protocol - set taskfile protocol and r/w commands
  *	@dev: device to examine and configure
  *
@@ -675,19 +632,42 @@ static void ata_dev_set_protocol(struct 
 {
 	int pio = (dev->flags & ATA_DFLAG_PIO);
 	int lba48 = (dev->flags & ATA_DFLAG_LBA48);
-	int proto, cmd;
+	int rcmd, wcmd;
 
-	if (pio)
-		proto = dev->xfer_protocol = ATA_PROT_PIO;
-	else
-		proto = dev->xfer_protocol = ATA_PROT_DMA;
+	if (pio) {
+		dev->xfer_protocol = ATA_PROT_PIO;
 
-	cmd = ata_prot_to_cmd(proto, lba48);
-	if (cmd < 0)
-		BUG();
+		if (dev->multi_count) {
+			if (lba48) {
+				rcmd = ATA_CMD_READ_MULTI_EXT;
+				wcmd = ATA_CMD_WRITE_MULTI_EXT;
+			} else {
+				rcmd = ATA_CMD_READ_MULTI;
+				wcmd = ATA_CMD_WRITE_MULTI;
+			}
+		} else {
+			if (lba48) {
+				rcmd = ATA_CMD_PIO_READ_EXT;
+				wcmd = ATA_CMD_PIO_WRITE_EXT;
+			} else {
+				rcmd = ATA_CMD_PIO_READ;
+				wcmd = ATA_CMD_PIO_WRITE;
+			}
+		}
+	} else {
+		dev->xfer_protocol = ATA_PROT_DMA;
+
+		if (lba48) {
+			rcmd = ATA_CMD_READ_EXT;
+			wcmd = ATA_CMD_WRITE_EXT;
+		} else {
+			rcmd = ATA_CMD_READ;
+			wcmd = ATA_CMD_WRITE;
+		}
+	}
 
-	dev->read_cmd = cmd & 0xff;
-	dev->write_cmd = (cmd >> 8) & 0xff;
+	dev->read_cmd = rcmd;
+	dev->write_cmd = wcmd;
 }
 
 static const char * xfer_mode_str[] = {
@@ -1269,6 +1249,12 @@ retry:
 			dev->n_sectors = ata_id_u32(dev->id, 60);
 		}
 
+		if (dev->id[59] & 0x100) {
+			dev->multi_count = dev->id[59] & 0xff;
+			DPRINTK("ata%u: dev %u multi count %u\n",
+				ap->id, device, dev->multi_count);
+		}
+
 		ap->host->max_cmd_len = 16;
 
 		/* print device info to dmesg */
@@ -2483,13 +2469,13 @@ static int ata_pio_complete (struct ata_
 	 * we enter, BSY will be cleared in a chk-status or two.  If not,
 	 * the drive is probably seeking or something.  Snooze for a couple
 	 * msecs, then chk-status again.  If still busy, fall back to
-	 * HSM_ST_POLL state.
+	 * HSM_ST_LAST_POLL state.
 	 */
-	drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 10);
-	if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
+	drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
+	if (drv_stat & ATA_BUSY) {
 		msleep(2);
-		drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 10);
-		if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
+		drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
+		if (drv_stat & ATA_BUSY) {
 			ap->hsm_task_state = HSM_ST_LAST_POLL;
 			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
 			return 1;
@@ -2697,6 +2683,32 @@ static void ata_pio_sector(struct ata_qu
 }
 
 /**
+ *	ata_pio_sectors - Transfer one or many 512-byte sectors.
+ *	@qc: Command on going
+ *
+ *	Transfer one or many ATA_SECT_SIZE of data from/to the 
+ *	ATA device for the DRQ request.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
+static void ata_pio_sectors(struct ata_queued_cmd *qc)
+{
+	if (is_multi_taskfile(&qc->tf)) {
+		/* READ/WRITE MULTIPLE */
+		unsigned int nsect;
+
+		assert(qc->dev->multi_count);
+
+		nsect = min(qc->nsect - qc->cursect, qc->dev->multi_count);
+		while (nsect--)
+			ata_pio_sector(qc);
+	} else
+		ata_pio_sector(qc);
+}
+
+/**
  *	atapi_send_cdb - Write CDB bytes to hardware
  *	@ap: Port to which ATAPI device is attached.
  *	@qc: Taskfile currently active
@@ -2791,11 +2803,11 @@ static int ata_pio_first_block(struct at
 		 * send first data block.
 		 */
 
-		/* ata_pio_sector() might change the state to HSM_ST_LAST.
-		 * so, the state is changed here before ata_pio_sector().
+		/* ata_pio_sectors() might change the state to HSM_ST_LAST.
+		 * so, the state is changed here before ata_pio_sectors().
 		 */
 		ap->hsm_task_state = HSM_ST;
-		ata_pio_sector(qc);
+		ata_pio_sectors(qc);
 		ata_altstatus(ap); /* flush */
 	} else
 		/* send CDB */
@@ -2995,8 +3007,10 @@ static void ata_pio_block(struct ata_por
 			return;
 		}
 
-		ata_pio_sector(qc);
+		ata_pio_sectors(qc);
 	}
+
+	ata_altstatus(ap); /* flush */
 }
 
 static void ata_pio_error(struct ata_port *ap)
@@ -3932,7 +3946,7 @@ fsm_start:
 				goto fsm_start;
 			}
 
-			ata_pio_sector(qc);
+			ata_pio_sectors(qc);
 
 			if (ap->hsm_task_state == HSM_ST_LAST &&
 			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {

Re: [PATCH/RFC 4/4] irq-pio: add read/write multiple support

[Mark Lord]

Albert Lee wrote:
> Patch 4/4: add read/write multiple support
> 

One "fine point" when programming the R/W MULT commands
is to remember that each register access is very costly
in terms of processor time.

So whenever an access can be done with 28-bit LBA,
it should not use the EXT versions of the instructions,
even though the device may be LBA48 capable.

The various drivers I have written for Linux and other OSs
that deal with this always logic equivalen to this somewhere:

if (lba_sector < (1<<28) && sector_count <= 256) {
     use LBA28
} else if (device is lba48 capable) {
     use LBA48
} else {
     error
}

For SATA drives, this is unlikely to make any difference,
but for PATA it can reduce the CPU overhead of issuing
a command by 40% or so --> saves three PCI bus transactions,
which amounts to about 1-4us real-time, depending on the
exact hardware in question.

I'm really looking forward to having PIO in libata,
so that we can support compactflash cards and the like.
My Delkin 32-bit Cardbus Adapter driver (currently in 2.4.x IDE)
will be one of the first new pure PIO drivers in libata..

Cheers
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

Re: [PATCH/RFC 4/4] irq-pio: add read/write multiple support

[Jeff Garzik]

Mark Lord wrote:
> Albert Lee wrote:
> 
>> Patch 4/4: add read/write multiple support
>>
> 
> One "fine point" when programming the R/W MULT commands
> is to remember that each register access is very costly
> in terms of processor time.
> 
> So whenever an access can be done with 28-bit LBA,
> it should not use the EXT versions of the instructions,
> even though the device may be LBA48 capable.
> 
> The various drivers I have written for Linux and other OSs
> that deal with this always logic equivalen to this somewhere:
> 
> if (lba_sector < (1<<28) && sector_count <= 256) {
>     use LBA28
> } else if (device is lba48 capable) {
>     use LBA48
> } else {
>     error
> }

Alan lists this on his PATA should-do list, too.

Since this is legacy hardware, I am a bit skeptical about doing this -- 
and also 32-bit PIO, which is in the same category -- because this 
increases the amount of code we have to deal with, for the same basic 
function.  For rarely-hit situations, I don't mind paying some 
performance penalty if it reduces long term maintenance.  I am willing 
to push some ancient platforms in the "slow but working" category, to 
keep the code small and easier to review/debug/maintain.

That said, it should be simple enough to push the logic into 
libata-scsi.c, where it translates a SCSI READ/WRITE command into an ATA 
read/write command.  Patches welcome...


> For SATA drives, this is unlikely to make any difference,
> but for PATA it can reduce the CPU overhead of issuing
> a command by 40% or so --> saves three PCI bus transactions,
> which amounts to about 1-4us real-time, depending on the
> exact hardware in question.

When running devices in PIO-only mode, the PCI bus transactions are very 
noticeable even with SATA.

Also, though this may be stating the obvious, all this discussion is 
irrelevant for FIS-based chips, since the control flow is entirely 
different there.


> I'm really looking forward to having PIO in libata,
> so that we can support compactflash cards and the like.
> My Delkin 32-bit Cardbus Adapter driver (currently in 2.4.x IDE)
> will be one of the first new pure PIO drivers in libata..

Cool :)

	Jeff

Re: [PATCH/RFC 4/4] irq-pio: add read/write multiple support

[Jeff Garzik]

Albert Lee wrote:
> Patch 4/4: add read/write multiple support
> 
> Changes:
>    - add ATA_CMD_READ_MULTI, etc. to ata.h
>    - add multi_count to ata_device and initialize it with device 
> identify data
>    - merge ata_prot_to_cmd() with ata_dev_set_protocol()
>    - ata_pio_complete(): change the wait condition from (ATA_BUSY | 
> ATA_DRQ) to ATA_BUSY
>    - add ata_pio_sectors() to support r/w multiple commands
>    - ata_pio_block(): add ata_altstatus(ap) to prevent reading device 
> status before it is valid
> 
> 
> For your review, thanks.
> 
> Albert
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> 
> 
> ------------------------------------------------------------------------
> 
> --- id7/drivers/scsi/libata-core.c	2005-10-03 12:39:03.000000000 +0800
> +++ id8/drivers/scsi/libata-core.c	2005-10-03 18:33:26.000000000 +0800
> @@ -616,49 +616,6 @@ void ata_tf_from_fis(u8 *fis, struct ata
>  }
>  
>  /**
> - *	ata_prot_to_cmd - determine which read/write opcodes to use
> - *	@protocol: ATA_PROT_xxx taskfile protocol
> - *	@lba48: true is lba48 is present
> - *
> - *	Given necessary input, determine which read/write commands
> - *	to use to transfer data.
> - *
> - *	LOCKING:
> - *	None.
> - */
> -static int ata_prot_to_cmd(int protocol, int lba48)
> -{
> -	int rcmd = 0, wcmd = 0;
> -
> -	switch (protocol) {
> -	case ATA_PROT_PIO:
> -		if (lba48) {
> -			rcmd = ATA_CMD_PIO_READ_EXT;
> -			wcmd = ATA_CMD_PIO_WRITE_EXT;
> -		} else {
> -			rcmd = ATA_CMD_PIO_READ;
> -			wcmd = ATA_CMD_PIO_WRITE;
> -		}
> -		break;
> -
> -	case ATA_PROT_DMA:
> -		if (lba48) {
> -			rcmd = ATA_CMD_READ_EXT;
> -			wcmd = ATA_CMD_WRITE_EXT;
> -		} else {
> -			rcmd = ATA_CMD_READ;
> -			wcmd = ATA_CMD_WRITE;
> -		}
> -		break;
> -
> -	default:
> -		return -1;
> -	}
> -
> -	return rcmd | (wcmd << 8);
> -}
> -
> -/**
>   *	ata_dev_set_protocol - set taskfile protocol and r/w commands
>   *	@dev: device to examine and configure
>   *
> @@ -675,19 +632,42 @@ static void ata_dev_set_protocol(struct 
>  {
>  	int pio = (dev->flags & ATA_DFLAG_PIO);
>  	int lba48 = (dev->flags & ATA_DFLAG_LBA48);
> -	int proto, cmd;
> +	int rcmd, wcmd;
>  
> -	if (pio)
> -		proto = dev->xfer_protocol = ATA_PROT_PIO;
> -	else
> -		proto = dev->xfer_protocol = ATA_PROT_DMA;
> +	if (pio) {
> +		dev->xfer_protocol = ATA_PROT_PIO;
>  
> -	cmd = ata_prot_to_cmd(proto, lba48);
> -	if (cmd < 0)
> -		BUG();
> +		if (dev->multi_count) {
> +			if (lba48) {
> +				rcmd = ATA_CMD_READ_MULTI_EXT;
> +				wcmd = ATA_CMD_WRITE_MULTI_EXT;
> +			} else {
> +				rcmd = ATA_CMD_READ_MULTI;
> +				wcmd = ATA_CMD_WRITE_MULTI;
> +			}
> +		} else {
> +			if (lba48) {
> +				rcmd = ATA_CMD_PIO_READ_EXT;
> +				wcmd = ATA_CMD_PIO_WRITE_EXT;
> +			} else {
> +				rcmd = ATA_CMD_PIO_READ;
> +				wcmd = ATA_CMD_PIO_WRITE;
> +			}
> +		}
> +	} else {
> +		dev->xfer_protocol = ATA_PROT_DMA;
> +
> +		if (lba48) {
> +			rcmd = ATA_CMD_READ_EXT;
> +			wcmd = ATA_CMD_WRITE_EXT;
> +		} else {
> +			rcmd = ATA_CMD_READ;
> +			wcmd = ATA_CMD_WRITE;
> +		}
> +	}
>  
> -	dev->read_cmd = cmd & 0xff;
> -	dev->write_cmd = (cmd >> 8) & 0xff;
> +	dev->read_cmd = rcmd;
> +	dev->write_cmd = wcmd;
>  }
>  
>  static const char * xfer_mode_str[] = {

I rather liked having ata_prot_to_cmd() separate, but your version is 
OK.  No objection, just a twitch :)


> @@ -2483,13 +2469,13 @@ static int ata_pio_complete (struct ata_
>  	 * we enter, BSY will be cleared in a chk-status or two.  If not,
>  	 * the drive is probably seeking or something.  Snooze for a couple
>  	 * msecs, then chk-status again.  If still busy, fall back to
> -	 * HSM_ST_POLL state.
> +	 * HSM_ST_LAST_POLL state.
>  	 */
> -	drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 10);
> -	if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
> +	drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
> +	if (drv_stat & ATA_BUSY) {
>  		msleep(2);
> -		drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 10);
> -		if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
> +		drv_stat = ata_busy_wait(ap, ATA_BUSY, 10);
> +		if (drv_stat & ATA_BUSY) {
>  			ap->hsm_task_state = HSM_ST_LAST_POLL;
>  			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
>  			return 1;

Eventually we should make sure that DRQ is cleared, at the end of the 
transfer.  Does other code make this check?

Ideally, in _all_ cases where look at the status register, we should 
check the DRDY and DF bits as well.  That's why in drivers/ide/ you find

	check_bits = ATA_BSY | ATA_DRQ | ATA_DF | ATA_DRDY
	ok_bits = ATA_DRDY
	if ((Status & check_bits) == ok_bits)	# BUS IDLE
		OK
	else
		error

Patch is otherwise OK.

Not applied, since patches #2 and #3 were not applied.

Re: [PATCH/RFC 4/4] irq-pio: add read/write multiple support

[Albert Lee]

Jeff,

> Mark Lord wrote:
>
>> Albert Lee wrote:
>>
>>> Patch 4/4: add read/write multiple support
>>>
>>
>> One "fine point" when programming the R/W MULT commands
>> is to remember that each register access is very costly
>> in terms of processor time.
>>
>> So whenever an access can be done with 28-bit LBA,
>> it should not use the EXT versions of the instructions,
>> even though the device may be LBA48 capable.
>>
>> The various drivers I have written for Linux and other OSs
>> that deal with this always logic equivalen to this somewhere:
>>
>> if (lba_sector < (1<<28) && sector_count <= 256) {
>>     use LBA28
>> } else if (device is lba48 capable) {
>>     use LBA48
>> } else {
>>     error
>> }
>
>
> Alan lists this on his PATA should-do list, too.
>
> Since this is legacy hardware, I am a bit skeptical about doing this 
> -- and also 32-bit PIO, which is in the same category -- because this 
> increases the amount of code we have to deal with, for the same basic 
> function.  For rarely-hit situations, I don't mind paying some 
> performance penalty if it reduces long term maintenance.  I am willing 
> to push some ancient platforms in the "slow but working" category, to 
> keep the code small and easier to review/debug/maintain.
>
> That said, it should be simple enough to push the logic into 
> libata-scsi.c, where it translates a SCSI READ/WRITE command into an 
> ATA read/write command.  Patches welcome...
>
I would like to work on the LBA28/LBA48 translation. Please withdraw the 
r/w multiple patch 4/4 for the moment since it might conflict with it.
Thanks.

Albert

Re: [PATCH/RFC 4/4] irq-pio: add read/write multiple support

[Jeff Garzik]

Albert Lee wrote:
> I would like to work on the LBA28/LBA48 translation. Please withdraw the 
> r/w multiple patch 4/4 for the moment since it might conflict with it.
> Thanks.

Now that your C/H/S support work is merged into the 'upstream' branch 
(queued for 2.6.15), the LBA28/LBA48 translation work is probably 
easier.  Your C/H/S patches really did a good job of cleaning up 
ata_scsi_rw_xlat().

	Jeff

[PATCH/RFC 0/4] libata: more irq driven pio follow-up patches

[Albert Lee]

Dear all,

More irq driven PIO follow-up patches (revised).

Changes:
  1. merge the ata_dataout_task workqueue with the ata_pio_task workqueue
  2. add read/write multiple support

patch 1/4: misc fixes
   - ata_pio_block(): add ata_altstatus(ap) to prevent reading device status before it is valid
   - remove the unnecessary HSM_ST_IDLE state from ata_pio_task()
   - raise BUG() when unknown state is found in ata_pio_task()

patch 2/4: merge the ata_dataout_task workqueue with the ata_pio_task workqueue
   - remove ap->dataout_task from struct ata_port
   - let ata_pio_task() handle the HSM_ST_FIRST state.
   - rename ata_dataout_task() to ata_pio_first_block()

patch 3/4: eliminate unnecessary queuing in ata_pio_first_block()
   - change the return value of ata_pio_complete()
   - add return value to ata_pio_first_block()
   - rename the variable "qc_completed" to "has_next" in ata_pio_task()
   - use "has_next" to eliminate unnecessary queuing in ata_pio_first_block()

patch 4/4: add read/write multiple support
   - add is_multi_taskfile() to ata.h
   - initialize ata_device->multi_count with device identify data
   - use ata_pio_sectors() to support r/w multiple commands

Patch against the libata-dev irq-pio branch (cd8200e6d4f9f05e6ea48f7c000be890337396ac).

For your review and advice, thanks.

Albert

[PATCH/RFC 1/4] irq-pio: misc fixes

[Albert Lee]

Patch 1/4: misc fixes

Changes:
   - ata_pio_block(): add ata_altstatus(ap) to prevent reading device status before it is valid
   - remove the unnecessary HSM_ST_IDLE state from ata_pio_task()
   - raise BUG() when unknown state is found in ata_pio_task()

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com> 

============

--- linux-ori/drivers/scsi/libata-core.c	2005-11-01 17:22:41.000000000 +0800
+++ id1/drivers/scsi/libata-core.c	2005-11-01 18:45:16.000000000 +0800
@@ -3224,6 +3224,8 @@ static void ata_pio_block(struct ata_por
 
 		ata_pio_sector(qc);
 	}
+
+	ata_altstatus(ap); /* flush */
 }
 
 static void ata_pio_error(struct ata_port *ap)
@@ -3251,9 +3253,6 @@ fsm_start:
 	qc_completed = 0;
 
 	switch (ap->hsm_task_state) {
-	case HSM_ST_IDLE:
-		return;
-
 	case HSM_ST:
 		ata_pio_block(ap);
 		break;
@@ -3271,6 +3270,10 @@ fsm_start:
 	case HSM_ST_ERR:
 		ata_pio_error(ap);
 		return;
+
+	default:
+		BUG();
+		return;
 	}
 
 	if (timeout)

[PATCH/RFC 2/4] irq-pio: merge the ata_dataout_task workqueue with ata_pio_task workqueue

[Albert Lee]

Patch 2/4:
  merge the ata_dataout_task workqueue with ata_pio_task workqueue

Changes:
   - remove ap->dataout_task from struct ata_port
   - let ata_pio_task() handle the HSM_ST_FIRST state.
   - rename ata_dataout_task() to ata_pio_first_block()
   - replace the ata_dataout_task workqueue with ata_pio_task workqueue

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

========

--- id1/include/linux/libata.h	2005-11-01 17:39:14.000000000 +0800
+++ id2/include/linux/libata.h	2005-11-01 17:57:00.000000000 +0800
@@ -334,8 +334,6 @@ struct ata_port {
 	struct ata_host_stats	stats;
 	struct ata_host_set	*host_set;
 
-	struct work_struct	dataout_task;
-
 	struct work_struct	pio_task;
 	unsigned int		hsm_task_state;
 	unsigned long		pio_task_timeout;
--- id1/drivers/scsi/libata-core.c	2005-11-01 18:45:16.000000000 +0800
+++ id2/drivers/scsi/libata-core.c	2005-11-01 18:45:35.000000000 +0800
@@ -2963,8 +2963,8 @@ static void atapi_send_cdb(struct ata_po
 }
 
 /**
- *	ata_dataout_task - Write first data block to hardware
- *	@_data: Port to which ATA/ATAPI device is attached.
+ *	ata_pio_first_block - Write first data block to hardware
+ *	@ap: Port to which ATA/ATAPI device is attached.
  *
  *	When device has indicated its readiness to accept
  *	the data, this function sends out the CDB or 
@@ -2977,9 +2977,8 @@ static void atapi_send_cdb(struct ata_po
  *	Kernel thread context (may sleep)
  */
 
-static void ata_dataout_task(void *_data)
+static void ata_pio_first_block(struct ata_port *ap)
 {
-	struct ata_port *ap = _data;
 	struct ata_queued_cmd *qc;
 	u8 status;
 	unsigned long flags;
@@ -3253,6 +3252,10 @@ fsm_start:
 	qc_completed = 0;
 
 	switch (ap->hsm_task_state) {
+	case HSM_ST_FIRST:
+		ata_pio_first_block(ap);
+		return;
+
 	case HSM_ST:
 		ata_pio_block(ap);
 		break;
@@ -3703,10 +3706,10 @@ int ata_qc_issue_prot(struct ata_queued_
 		if (qc->tf.flags & ATA_TFLAG_WRITE) {
 			/* PIO data out protocol */
 			ap->hsm_task_state = HSM_ST_FIRST;
-			queue_work(ata_wq, &ap->dataout_task);
+			queue_work(ata_wq, &ap->pio_task);
 
 			/* always send first data block using
-			 * the ata_dataout_task() codepath.
+			 * the ata_pio_task() codepath.
 			 */
 		} else {
 			/* PIO data in protocol */
@@ -3733,7 +3736,7 @@ int ata_qc_issue_prot(struct ata_queued_
 		/* send cdb by polling if no cdb interrupt */
 		if ((!(qc->dev->flags & ATA_DFLAG_CDB_INTR)) ||
 		    (qc->tf.flags & ATA_TFLAG_POLLING))
-			queue_work(ata_wq, &ap->dataout_task);
+			queue_work(ata_wq, &ap->pio_task);
 		break;
 
 	case ATA_PROT_ATAPI_DMA:
@@ -3745,7 +3748,7 @@ int ata_qc_issue_prot(struct ata_queued_
 
 		/* send cdb by polling if no cdb interrupt */
 		if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
-			queue_work(ata_wq, &ap->dataout_task);
+			queue_work(ata_wq, &ap->pio_task);
 		break;
 
 	default:
@@ -4325,7 +4328,6 @@ static void ata_host_init(struct ata_por
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
 
-	INIT_WORK(&ap->dataout_task, ata_dataout_task, ap);
 	INIT_WORK(&ap->pio_task, ata_pio_task, ap);
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++)

[PATCH/RFC 3/4] irq-pio: eliminate unnecessary queuing in ata_pio_first_block()

[Albert Lee]

patch 3/4: 
  eliminate unnecessary queuing in ata_pio_first_block()

Changes:
   - change the return value of ata_pio_complete() 0 <-> 1
   - add return value for ata_pio_first_block()
   - rename variable "qc_completed" to "has_next" in ata_pio_task()
   - use has_next to eliminate unnecessary queuing in ata_pio_first_block()

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

==========

--- id2/drivers/scsi/libata-core.c	2005-11-01 18:45:35.000000000 +0800
+++ id3/drivers/scsi/libata-core.c	2005-11-01 18:45:50.000000000 +0800
@@ -2697,7 +2697,8 @@ static unsigned long ata_pio_poll(struct
  *	None.  (executing in kernel thread context)
  *
  *	RETURNS:
- *	Non-zero if qc completed, zero otherwise.
+ *	Zero if qc completed.
+ *	Non-zero if has next.
  */
 
 static int ata_pio_complete (struct ata_port *ap)
@@ -2719,14 +2720,14 @@ static int ata_pio_complete (struct ata_
 		if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
 			ap->hsm_task_state = HSM_ST_LAST_POLL;
 			ap->pio_task_timeout = jiffies + ATA_TMOUT_PIO;
-			return 0;
+			return 1;
 		}
 	}
 
 	drv_stat = ata_wait_idle(ap);
 	if (!ata_ok(drv_stat)) {
 		ap->hsm_task_state = HSM_ST_ERR;
-		return 0;
+		return 1;
 	}
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
@@ -2738,7 +2739,7 @@ static int ata_pio_complete (struct ata_
 
 	/* another command may start at this point */
 
-	return 1;
+	return 0;
 }
 
 
@@ -2975,27 +2976,42 @@ static void atapi_send_cdb(struct ata_po
  *
  *	LOCKING:
  *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	Zero if irq handler takes over
+ *	Non-zero if has next (polling).
  */
 
-static void ata_pio_first_block(struct ata_port *ap)
+static int ata_pio_first_block(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc;
 	u8 status;
 	unsigned long flags;
+	int has_next;
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
 	assert(qc != NULL);
 	assert(qc->flags & ATA_QCFLAG_ACTIVE);
 
+	/* if polling, we will stay in the work queue after sending the data.
+	 * otherwise, interrupt handler takes over after sending the data.
+	 */
+	has_next = (qc->tf.flags & ATA_TFLAG_POLLING);
+
 	/* sleep-wait for BSY to clear */
 	DPRINTK("busy wait\n");
-	if (ata_busy_sleep(ap, ATA_TMOUT_DATAOUT_QUICK, ATA_TMOUT_DATAOUT))
+	if (ata_busy_sleep(ap, ATA_TMOUT_DATAOUT_QUICK, ATA_TMOUT_DATAOUT)) {
+		ap->hsm_task_state = HSM_ST_TMOUT;
 		goto err_out;
+	}
 
 	/* make sure DRQ is set */
 	status = ata_chk_status(ap);
-	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)
+	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
+		/* device status error */
+		ap->hsm_task_state = HSM_ST_ERR;
 		goto err_out;
+	}
 
 	/* Send the CDB (atapi) or the first data block (ata pio out).
 	 * During the state transition, interrupt handler shouldn't
@@ -3019,18 +3035,15 @@ static void ata_pio_first_block(struct a
 		/* send CDB */
 		atapi_send_cdb(ap, qc);
 
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
 	/* if polling, ata_pio_task() handles the rest.
 	 * otherwise, interrupt handler takes over from here.
 	 */
-	if (qc->tf.flags & ATA_TFLAG_POLLING)
-		queue_work(ata_wq, &ap->pio_task);
-
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	return;
+	return has_next;
 
 err_out:
-	ata_pio_error(ap);
+	return 1; /* has next */
 }
 
 /**
@@ -3245,23 +3258,23 @@ static void ata_pio_task(void *_data)
 {
 	struct ata_port *ap = _data;
 	unsigned long timeout;
-	int qc_completed;
+	int has_next;
 
 fsm_start:
 	timeout = 0;
-	qc_completed = 0;
+	has_next = 1;
 
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
-		ata_pio_first_block(ap);
-		return;
+		has_next = ata_pio_first_block(ap);
+		break;
 
 	case HSM_ST:
 		ata_pio_block(ap);
 		break;
 
 	case HSM_ST_LAST:
-		qc_completed = ata_pio_complete(ap);
+		has_next = ata_pio_complete(ap);
 		break;
 
 	case HSM_ST_POLL:
@@ -3281,7 +3294,7 @@ fsm_start:
 
 	if (timeout)
 		queue_delayed_work(ata_wq, &ap->pio_task, timeout);
-	else if (!qc_completed)
+	else if (has_next)
 		goto fsm_start;
 }
 

[PATCH/RFC 4/4] irq-pio: add read/write multiple support

[Albert Lee]

patch 4/4: 
  add read/write multiple support

Changes:
   - add is_multi_taskfile() to ata.h
   - initialize ata_device->multi_count with device identify data
   - use ata_pio_sectors() to support r/w multiple commands

For your review, thanks.

Albert
Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

========

--- linux-ori/include/linux/ata.h	2005-11-01 17:22:45.000000000 +0800
+++ id4/include/linux/ata.h	2005-11-01 18:27:08.000000000 +0800
@@ -293,6 +293,14 @@ static inline int is_atapi_taskfile(cons
 	       (tf->protocol == ATA_PROT_ATAPI_DMA);
 }
 
+static inline int is_multi_taskfile(struct ata_taskfile *tf)
+{
+	return (tf->command == ATA_CMD_READ_MULTI) ||
+	       (tf->command == ATA_CMD_WRITE_MULTI) ||
+	       (tf->command == ATA_CMD_READ_MULTI_EXT) ||
+	       (tf->command == ATA_CMD_WRITE_MULTI_EXT);
+}
+
 static inline int ata_ok(u8 status)
 {
 	return ((status & (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ | ATA_ERR))
--- id3/drivers/scsi/libata-core.c	2005-11-01 18:45:50.000000000 +0800
+++ id4/drivers/scsi/libata-core.c	2005-11-01 18:36:08.000000000 +0800
@@ -1261,6 +1261,12 @@ retry:
 
 		}
 
+		if (dev->id[59] & 0x100) {
+			dev->multi_count = dev->id[59] & 0xff;
+			DPRINTK("ata%u: dev %u multi count %u\n",
+				ap->id, device, dev->multi_count);
+		}
+
 		ap->host->max_cmd_len = 16;
 	}
 
@@ -2711,7 +2717,7 @@ static int ata_pio_complete (struct ata_
 	 * we enter, BSY will be cleared in a chk-status or two.  If not,
 	 * the drive is probably seeking or something.  Snooze for a couple
 	 * msecs, then chk-status again.  If still busy, fall back to
-	 * HSM_ST_POLL state.
+	 * HSM_ST_LAST_POLL state.
 	 */
 	drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 10);
 	if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
@@ -2928,6 +2934,32 @@ static void ata_pio_sector(struct ata_qu
 }
 
 /**
+ *	ata_pio_sectors - Transfer one or many 512-byte sectors.
+ *	@qc: Command on going
+ *
+ *	Transfer one or many ATA_SECT_SIZE of data from/to the 
+ *	ATA device for the DRQ request.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+
+static void ata_pio_sectors(struct ata_queued_cmd *qc)
+{
+	if (is_multi_taskfile(&qc->tf)) {
+		/* READ/WRITE MULTIPLE */
+		unsigned int nsect;
+
+		assert(qc->dev->multi_count);
+
+		nsect = min(qc->nsect - qc->cursect, qc->dev->multi_count);
+		while (nsect--)
+			ata_pio_sector(qc);
+	} else
+		ata_pio_sector(qc);
+}
+
+/**
  *	atapi_send_cdb - Write CDB bytes to hardware
  *	@ap: Port to which ATAPI device is attached.
  *	@qc: Taskfile currently active
@@ -3025,11 +3057,11 @@ static int ata_pio_first_block(struct at
 		 * send first data block.
 		 */
 
-		/* ata_pio_sector() might change the state to HSM_ST_LAST.
-		 * so, the state is changed here before ata_pio_sector().
+		/* ata_pio_sectors() might change the state to HSM_ST_LAST.
+		 * so, the state is changed here before ata_pio_sectors().
 		 */
 		ap->hsm_task_state = HSM_ST;
-		ata_pio_sector(qc);
+		ata_pio_sectors(qc);
 		ata_altstatus(ap); /* flush */
 	} else
 		/* send CDB */
@@ -3234,7 +3266,7 @@ static void ata_pio_block(struct ata_por
 			return;
 		}
 
-		ata_pio_sector(qc);
+		ata_pio_sectors(qc);
 	}
 
 	ata_altstatus(ap); /* flush */
@@ -4120,7 +4152,7 @@ fsm_start:
 				goto fsm_start;
 			}
 
-			ata_pio_sector(qc);
+			ata_pio_sectors(qc);
 
 			if (ap->hsm_task_state == HSM_ST_LAST &&
 			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {

Re: [PATCH/RFC 1/4] irq-pio: misc fixes

[Jeff Garzik]

applied 1-4