Skip to contentSkip to conversation

libata: interrupt driven pio

RFC PATCH In review Albert Lee 7 participants 77 messages Sep 9, 2005 – Oct 12, 2005

Subsystems (2)
lore
Share & tools
lore

Albert Lee

author
#0 lore raw

RFC PATCH 1/1 libata: interrupt driven pio

Dear all,

The interrupt driven PIO draft patch is ready for your review.

Changes:
- add PIO_ST_FIRST for the state before sending ATAPI CDB or sending "ATA PIO data out" first data block.
- remove the ATA_FLAG_NOINTR flag since the interrupt handler is now aware of the states
- modify ata_pio_sector() and atapi_pio_bytes() to work in the interrupt context
- modify the ata_host_intr() to handle PIO interrupts
- modify ata_qc_issue_prot() to initialize states
- atapi_packet_task() changed to handle "ATA PIO data out" first data block
- support the pre-ATA4 ATAPI device which raise interrupt when ready to receive CDB

Patch against 2.6.13 (80ac2912f846c01d702774bb6aa7100ec71e88b9).

For your review and comment, thanks.

Albert

include/linux/ata.h +3 -0
@@ -181,6 +181,7 @@ enum {
 	ATA_TFLAG_ISADDR	= (1 << 1), /* enable r/w to nsect/lba regs */
 	ATA_TFLAG_DEVICE	= (1 << 2), /* enable r/w to device reg */
 	ATA_TFLAG_WRITE		= (1 << 3), /* data dir: host->dev==1 (write) */
+	ATA_TFLAG_POLLING	= (1 << 4), /* set nIEN to 1 and use polling */
 };
 
 enum ata_tf_protocols {
@@ -250,6 +251,8 @@ struct ata_taskfile {
 	  ((u64) (id)[(n) + 1] << 16) |	\
 	  ((u64) (id)[(n) + 0]) )
 
+#define atapi_id_cdb_intr(id)	(((id)[0] & 0x60) == 0x20)
+
 static inline int atapi_cdb_len(u16 *dev_id)
 {
 	u16 tmp = dev_id[0] & 0x3;
include/linux/libata.h +1 -2
@@ -116,8 +116,6 @@ enum {
 	ATA_FLAG_MMIO		= (1 << 6), /* use MMIO, not PIO */
 	ATA_FLAG_SATA_RESET	= (1 << 7), /* use COMRESET */
 	ATA_FLAG_PIO_DMA	= (1 << 8), /* PIO cmds via DMA */
-	ATA_FLAG_NOINTR		= (1 << 9), /* FIXME: Remove this once
-					     * proper HSM is in place. */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */
@@ -166,6 +164,7 @@ enum pio_task_states {
 	PIO_ST_LAST,
 	PIO_ST_LAST_POLL,
 	PIO_ST_ERR,
+	PIO_ST_FIRST,
 };
 
 /* forward declarations */
drivers/scsi/libata-core.c +234 -61

412 lines not on this page · raw message

#1 lore raw
Quoted 17 lines · Albert Lee

Dear all,

The interrupt driven PIO draft patch is ready for your review.

Changes:
- add PIO_ST_FIRST for the state before sending ATAPI CDB or sending "ATA PIO data out" first data block.
- remove the ATA_FLAG_NOINTR flag since the interrupt handler is now aware of the states
- modify ata_pio_sector() and atapi_pio_bytes() to work in the interrupt context
- modify the ata_host_intr() to handle PIO interrupts
- modify ata_qc_issue_prot() to initialize states
- atapi_packet_task() changed to handle "ATA PIO data out" first data block
- support the pre-ATA4 ATAPI device which raise interrupt when ready to receive CDB

Quoted 1 line

Patch against 2.6.13 (80ac2912f846c01d702774bb6aa7100ec71e88b9).

Very nice, thanks for working on this.

Comments:

1) With this work, PIO_ST_xxx is no longer an appropriate name. I suggest s/PIO_ST_/HSM_ST_/

2) ditto with s/pio_task_state/hsm_task_state/

3) Don't bother with in_atomic(), since we mix contexts in libata. Causes more trouble than its worth. Just use *map_atomic()

4) prefer 'ata_id_cdb_intr' to 'atapi_...'

5) please don't put braces around single-line C statements:

+ if (status & ATA_BUSY) {
+ goto idle_irq;
+ }

6) the following locking is questionable. AFAICS this should be resolved elsewhere, and if you have to take the lock here, you already have problems

+ /* PIO data out protocol.
+ * send first data block.
+ */
+ ata_pio_sector(qc);

- /* PIO commands are handled by polling */
+ spin_lock_irqsave(&ap->host_set->lock, flags);
ap->pio_task_state = PIO_ST;
- queue_work(ata_wq, &ap->pio_task);
+ spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+ /* interrupt handler takes over from here */

ie. why queue the task at all, if the intr handler takes over?

7) Overall, looks pretty good at first glance. This patch will require much testing before global deployment. Once you have a final patch, we'll let it simmer in libata-dev.git for a while, allowing -mm users plenty of time for testing. Possibly a 2.6.16 version target.

#2 lore raw
Quoted 20 lines · Jeff Garzik
Quoted 18 lines · Albert Lee

Dear all,

The interrupt driven PIO draft patch is ready for your review.

Changes:
- add PIO_ST_FIRST for the state before sending ATAPI CDB or sending "ATA PIO data out" first data block.
- remove the ATA_FLAG_NOINTR flag since the interrupt handler is now aware of the states
- modify ata_pio_sector() and atapi_pio_bytes() to work in the interrupt context
- modify the ata_host_intr() to handle PIO interrupts
- modify ata_qc_issue_prot() to initialize states
- atapi_packet_task() changed to handle "ATA PIO data out" first data block
- support the pre-ATA4 ATAPI device which raise interrupt when ready to receive CDB

Patch against 2.6.13 (80ac2912f846c01d702774bb6aa7100ec71e88b9).

...

Quoted 4 lines

6) the following locking is questionable. AFAICS this should be resolved elsewhere, and if you have to take the lock here, you already have problems

To protect this instance against being preempted on MP? Not that large of an issue at present, but suspect that Ingo's work may affect in the near future. I agree that this may be wrong location.

What mechanism (elsewhere) currently protects on irq handler entry?

Quoted 14 lines
drivers/scsi/libata-core.c +8 -2
+		/* PIO data out protocol.
+		 * send first data block.
+		 */
+		ata_pio_sector(qc);
 
-		/* PIO commands are handled by polling */
+		spin_lock_irqsave(&ap->host_set->lock, flags);
 		ap->pio_task_state = PIO_ST;
-		queue_work(ata_wq, &ap->pio_task);
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+		/* interrupt handler takes over from here */
Quoted 8 lines

ie. why queue the task at all, if the intr handler takes over?

7) Overall, looks pretty good at first glance. This patch will require much testing before global deployment. Once you have a final patch, we'll let it simmer in libata-dev.git for a while, allowing -mm users plenty of time for testing. Possibly a 2.6.16 version target.

Way to go Albert!

++doug

author
#3 lore raw

cover libata: interrupt driven pio (revised)

In reply to Jeff Garzik

Dear all,

The revised interrupt driven PIO draft patch is ready for your review.

Patch 1/3: interrupt driven pio patch for libata-core

Patch 2/3: s/PIO_ST_/HSM_ST_/ and s/pio_task_state/hsm_task_state/.

Patch 3/3: interrupt driven pio patch for LLD

Patch for 2.6.14-rc1 (044a500e46742d39d22f1781cfb64ba93b463e39).
Tested ok on x86 with Promise PDC20275, Seagate ST380011A and LITEON 52x CD-ROM drive.

For your review and advice, thanks.

Albert

author
#4 lore raw

PATCH 1/3 libata: interrupt driven pio for libata-core

Patch 1/3: interrupt driven pio for libata-core

Changes
- add PIO_ST_FIRST for the state before sending ATAPI CDB or sending "ATA PIO data out" first data block.
- add ATA_TFLAG_POLLING and ATA_DFLAG_CDB_INTR flags
- remove the ATA_FLAG_NOINTR flag since the interrupt handler is now aware of the states
- modify ata_pio_sector() and atapi_pio_bytes() to work in the interrupt context
- modify the ata_host_intr() to handle PIO interrupts
- modify ata_qc_issue_prot() to initialize states
- atapi_packet_task() changed to handle "ATA PIO data out" first data block
- support the pre-ATA4 ATAPI device which raise interrupt when ready to receive CDB

For your review, thanks.

Albert

include/linux/ata.h +3 -0
@@ -181,6 +181,7 @@ enum {
 	ATA_TFLAG_ISADDR	= (1 << 1), /* enable r/w to nsect/lba regs */
 	ATA_TFLAG_DEVICE	= (1 << 2), /* enable r/w to device reg */
 	ATA_TFLAG_WRITE		= (1 << 3), /* data dir: host->dev==1 (write) */
+	ATA_TFLAG_POLLING	= (1 << 4), /* set nIEN to 1 and use polling */
 };
 
 enum ata_tf_protocols {
@@ -250,6 +251,8 @@ struct ata_taskfile {
 	  ((u64) (id)[(n) + 1] << 16) |	\
 	  ((u64) (id)[(n) + 0]) )
 
+#define ata_id_cdb_intr(id)	(((id)[0] & 0x60) == 0x20)
+
 static inline int atapi_cdb_len(u16 *dev_id)
 {
 	u16 tmp = dev_id[0] & 0x3;
include/linux/libata.h +2 -2
@@ -97,6 +97,7 @@ enum {
 	ATA_DFLAG_LBA48		= (1 << 0), /* device supports LBA48 */
 	ATA_DFLAG_PIO		= (1 << 1), /* device currently in PIO mode */
 	ATA_DFLAG_LOCK_SECTORS	= (1 << 2), /* don't adjust max_sectors */
+	ATA_DFLAG_CDB_INTR	= (1 << 3), /* device asserts INTRQ when ready for CDB */
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
@@ -115,8 +116,6 @@ enum {
 	ATA_FLAG_MMIO		= (1 << 6), /* use MMIO, not PIO */
 	ATA_FLAG_SATA_RESET	= (1 << 7), /* use COMRESET */
 	ATA_FLAG_PIO_DMA	= (1 << 8), /* PIO cmds via DMA */
-	ATA_FLAG_NOINTR		= (1 << 9), /* FIXME: Remove this once
-					     * proper HSM is in place. */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */
@@ -165,6 +164,7 @@ enum pio_task_states {
 	PIO_ST_LAST,
 	PIO_ST_LAST_POLL,
 	PIO_ST_ERR,
+	PIO_ST_FIRST,
 };
 
 /* forward declarations */
drivers/scsi/libata-core.c +271 -70

484 lines not on this page · raw message

author
#5 lore raw

Re: [PATCH/RFC 1/3] libata: interrupt driven pio for libata-core

Quoted 33 lines

Patch 1/3: interrupt driven pio for libata-core

Changes
- add PIO_ST_FIRST for the state before sending ATAPI CDB or sending "ATA PIO data out" first data block.
- add ATA_TFLAG_POLLING and ATA_DFLAG_CDB_INTR flags
- remove the ATA_FLAG_NOINTR flag since the interrupt handler is now aware of the states
- modify ata_pio_sector() and atapi_pio_bytes() to work in the interrupt context
- modify the ata_host_intr() to handle PIO interrupts
- modify ata_qc_issue_prot() to initialize states
- atapi_packet_task() changed to handle "ATA PIO data out" first data block
- support the pre-ATA4 ATAPI device which raise interrupt when ready to receive CDB

------------------------------------------------------------------------

(quoted diff) +7 -1
-		/* PIO commands are handled by polling */
+		if (qc->tf.flags & ATA_TFLAG_POLLING)
+			queue_work(ata_wq, &ap->pio_task);
+	} else {
+		/* PIO data out protocol.
+		 * send first data block.
+		 */
+		ata_pio_sector(qc);
		ap->pio_task_state = PIO_ST;

Review

Quoted 1 line
(quoted diff) +0 -1
-		queue_work(ata_wq, &ap->pio_task);

Sorry, there is a bug here:
"ap->pio_task_state = PIO_ST;" should be before "ata_pio_sector(qc);" because ata_pio_sector() might change the state to PIO_ST_LAST.

Attached please find the revised patch for your review, thanks.

Albert

include/linux/ata.h +3 -0
@@ -181,6 +181,7 @@ enum {
 	ATA_TFLAG_ISADDR	= (1 << 1), /* enable r/w to nsect/lba regs */
 	ATA_TFLAG_DEVICE	= (1 << 2), /* enable r/w to device reg */
 	ATA_TFLAG_WRITE		= (1 << 3), /* data dir: host->dev==1 (write) */
+	ATA_TFLAG_POLLING	= (1 << 4), /* set nIEN to 1 and use polling */
 };
 
 enum ata_tf_protocols {
@@ -250,6 +251,8 @@ struct ata_taskfile {
 	  ((u64) (id)[(n) + 1] << 16) |	\
 	  ((u64) (id)[(n) + 0]) )
 
+#define ata_id_cdb_intr(id)	(((id)[0] & 0x60) == 0x20)
+
 static inline int atapi_cdb_len(u16 *dev_id)
 {
 	u16 tmp = dev_id[0] & 0x3;
include/linux/libata.h +2 -2
@@ -97,6 +97,7 @@ enum {
 	ATA_DFLAG_LBA48		= (1 << 0), /* device supports LBA48 */
 	ATA_DFLAG_PIO		= (1 << 1), /* device currently in PIO mode */
 	ATA_DFLAG_LOCK_SECTORS	= (1 << 2), /* don't adjust max_sectors */
+	ATA_DFLAG_CDB_INTR	= (1 << 3), /* device asserts INTRQ when ready for CDB */
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
@@ -115,8 +116,6 @@ enum {
 	ATA_FLAG_MMIO		= (1 << 6), /* use MMIO, not PIO */
 	ATA_FLAG_SATA_RESET	= (1 << 7), /* use COMRESET */
 	ATA_FLAG_PIO_DMA	= (1 << 8), /* PIO cmds via DMA */
-	ATA_FLAG_NOINTR		= (1 << 9), /* FIXME: Remove this once
-					     * proper HSM is in place. */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */
@@ -165,6 +164,7 @@ enum pio_task_states {
 	PIO_ST_LAST,
 	PIO_ST_LAST_POLL,
 	PIO_ST_ERR,
+	PIO_ST_FIRST,
 };
 
 /* forward declarations */
drivers/scsi/libata-core.c +274 -69

487 lines not on this page · raw message

author
#6 lore raw

PATCH 2/3 libata: rename task states/variables

In reply to Albert Lee

Patch 2/3: rename task states/variables

Changes:
s/PIO_ST_/HSM_ST_/ and s/pio_task_state/hsm_task_state/.

For your review, thanks.

Albert

include/linux/libata.h +11 -11
@@ -155,16 +155,16 @@ enum {
 	ATA_SHIFT_PIO		= 11,
 };
 
-enum pio_task_states {
-	PIO_ST_UNKNOWN,
-	PIO_ST_IDLE,
-	PIO_ST_POLL,
-	PIO_ST_TMOUT,
-	PIO_ST,
-	PIO_ST_LAST,
-	PIO_ST_LAST_POLL,
-	PIO_ST_ERR,
-	PIO_ST_FIRST,
+enum hsm_task_states {
+	HSM_ST_UNKNOWN,
+	HSM_ST_IDLE,
+	HSM_ST_POLL,
+	HSM_ST_TMOUT,
+	HSM_ST,
+	HSM_ST_LAST,
+	HSM_ST_LAST_POLL,
+	HSM_ST_ERR,
+	HSM_ST_FIRST,
 };
 
 /* forward declarations */
@@ -319,7 +319,7 @@ struct ata_port {
 	struct work_struct	packet_task;
 
 	struct work_struct	pio_task;
-	unsigned int		pio_task_state;
+	unsigned int		hsm_task_state;
 	unsigned long		pio_task_timeout;
 
 	void			*private_data;
drivers/scsi/libata-core.c +73 -73

427 lines not on this page · raw message

author
#7 lore raw

Re: [PATCH/RFC 2/3] libata: rename task states/variables

Quoted 6 lines · Albert Lee

Patch 2/3: rename task states/variables

Changes:
s/PIO_ST_/HSM_ST_/ and s/pio_task_state/hsm_task_state/.

patch 2/3 is also updated to sync with the revised patch 1/3:

"ap->pio_task_state = PIO_ST;" should be before "ata_pio_sector(qc);" because ata_pio_sector() might change the state to PIO_ST_LAST.
(http://marc.theaimsgroup.com/?l=linux-ide&m=112746756929455&w=2)

Attached please find the revised patch 2/3 for your review, thanks.

Albert

include/linux/libata.h +11 -11
@@ -155,16 +155,16 @@ enum {
 	ATA_SHIFT_PIO		= 11,
 };
 
-enum pio_task_states {
-	PIO_ST_UNKNOWN,
-	PIO_ST_IDLE,
-	PIO_ST_POLL,
-	PIO_ST_TMOUT,
-	PIO_ST,
-	PIO_ST_LAST,
-	PIO_ST_LAST_POLL,
-	PIO_ST_ERR,
-	PIO_ST_FIRST,
+enum hsm_task_states {
+	HSM_ST_UNKNOWN,
+	HSM_ST_IDLE,
+	HSM_ST_POLL,
+	HSM_ST_TMOUT,
+	HSM_ST,
+	HSM_ST_LAST,
+	HSM_ST_LAST_POLL,
+	HSM_ST_ERR,
+	HSM_ST_FIRST,
 };
 
 /* forward declarations */
@@ -319,7 +319,7 @@ struct ata_port {
 	struct work_struct	packet_task;
 
 	struct work_struct	pio_task;
-	unsigned int		pio_task_state;
+	unsigned int		hsm_task_state;
 	unsigned long		pio_task_timeout;
 
 	void			*private_data;
drivers/scsi/libata-core.c +74 -74

431 lines not on this page · raw message

author
#8 lore raw

PATCH 3/3 libata: interrupt driven pio for LLD

In reply to Albert Lee

Patch 3/3: interrupt driven pio patch for LLD

Changes:

sata_nv.c:
sata_vsc.c:
irq handler is wrapper around ata_host_intr(), only minor change.

sata_promise.c:
sata_sx4.c:
sata_mv.c:
sata_qstor.c:
Private irq handler, need modification to handle interrupt driven mode
in the future.
Use polling mode for the unsupported protocols for the time being.

For your review, thanks.

Albert

drivers/scsi/sata_promise.c +10 -5
@@ -438,11 +438,11 @@ static inline unsigned int pdc_host_intr
 		break;
 
         default:
-                ap->stats.idle_irq++;
-                break;
+		ap->stats.idle_irq++;
+		break;
         }
 
-        return handled;
+	return handled;
 }
 
 static void pdc_irq_clear(struct ata_port *ap)
@@ -493,11 +493,11 @@ static irqreturn_t pdc_interrupt (int ir
 		ap = host_set->ports[i];
 		tmp = mask & (1 << (i + 1));
 		if (tmp && ap &&
-		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
+		    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
 				handled += pdc_host_intr(ap, qc);
 		}
 	}
@@ -540,6 +540,11 @@ static int pdc_qc_issue_prot(struct ata_
 		break;
 
 	default:
+		/* FIXME: pdc_host_intr() doesn't handle
+		 * interrupt driven pio/atapi cdb intr yet.
+		 * Use polling for these protocols.
+		 */
+		qc->tf.flags |= ATA_TFLAG_POLLING;
 		break;
 	}
 
drivers/scsi/sata_sx4.c +7 -2
@@ -691,6 +691,11 @@ static int pdc20621_qc_issue_prot(struct
 		break;
 
 	default:
+		/* FIXME: pdc20621_host_intr() doesn't handle
+		 * interrupt driven pio/atapi cdb intr yet.
+		 * Use polling for these protocols.
+		 */
+		qc->tf.flags |= ATA_TFLAG_POLLING;
 		break;
 	}
 
@@ -832,11 +837,11 @@ static irqreturn_t pdc20621_interrupt (i
 		tmp = mask & (1 << i);
 		VPRINTK("seq %u, port_no %u, ap %p, tmp %x\n", i, port_no, ap, tmp);
 		if (tmp && ap &&
-		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
+		    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
 				handled += pdc20621_host_intr(ap, qc, (i > 4),
 							      mmio_base);
 		}
drivers/scsi/sata_qstor.c +12 -5
@@ -356,11 +356,19 @@ static int qs_qc_issue(struct ata_queued
 		qs_packet_start(qc);
 		return 0;
 
+	case ATA_PROT_NODATA:
+		break;
+
 	case ATA_PROT_ATAPI_DMA:
 		BUG();
 		break;
 
 	default:
+		/* FIXME: qs_intr_mmio() doesn't handle
+		 * interrupt driven pio/atapi cdb intr yet.
+		 * Use polling for these protocols.
+		 */
+		qc->tf.flags |= ATA_TFLAG_POLLING;
 		break;
 	}
 
@@ -389,14 +397,13 @@ static inline unsigned int qs_intr_pkt(s
 			DPRINTK("SFF=%08x%08x: sCHAN=%u sHST=%d sDST=%02x\n",
 					sff1, sff0, port_no, sHST, sDST);
 			handled = 1;
-			if (ap && !(ap->flags &
-				    (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
+			if (ap && !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 				struct ata_queued_cmd *qc;
 				struct qs_port_priv *pp = ap->private_data;
 				if (!pp || pp->state != qs_state_pkt)
 					continue;
 				qc = ata_qc_from_tag(ap, ap->active_tag);
-				if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
+				if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
 					switch (sHST) {
 					case 0: /* sucessful CPB */
 					case 3: /* device error */
@@ -422,13 +429,13 @@ static inline unsigned int qs_intr_mmio(
 		struct ata_port *ap;
 		ap = host_set->ports[port_no];
 		if (ap &&
-		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
+		    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 			struct ata_queued_cmd *qc;
 			struct qs_port_priv *pp = ap->private_data;
 			if (!pp || pp->state != qs_state_mmio)
 				continue;
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
 
 				/* check main status, clearing INTRQ */
 				u8 status = ata_chk_status(ap);
drivers/scsi/sata_mv.c +21 -1
@@ -189,6 +189,7 @@ static int mv_master_reset(void __iomem 
 static irqreturn_t mv_interrupt(int irq, void *dev_instance,
 				struct pt_regs *regs);
 static int mv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
+static int mv_qc_issue_prot(struct ata_queued_cmd *qc);
 
 static Scsi_Host_Template mv_sht = {
 	.module			= THIS_MODULE,
@@ -222,7 +223,7 @@ static struct ata_port_operations mv_ops
 	.phy_reset		= mv_phy_reset,
 
 	.qc_prep		= ata_qc_prep,
-	.qc_issue		= ata_qc_issue_prot,
+	.qc_issue		= mv_qc_issue_prot,
 
 	.eng_timeout		= ata_eng_timeout,
 
@@ -448,6 +449,25 @@ static int mv_master_reset(void __iomem 
 	return rc;
 }
 
+static int mv_qc_issue_prot(struct ata_queued_cmd *qc)
+{
+	switch (qc->tf.protocol) {
+	case ATA_PROT_DMA:
+	case ATA_PROT_NODATA:
+		break;
+
+	default:
+		/* FIXME: mv_host_intr() doesn't handle
+		 * interrupt driven pio/atapi cdb intr yet.
+		 * Use polling for these protocols.
+		 */
+		qc->tf.flags |= ATA_TFLAG_POLLING;
+		break;
+	}
+
+	return ata_qc_issue_prot(qc);
+}
+
 static void mv_err_intr(struct ata_port *ap)
 {
 	void __iomem *port_mmio;
drivers/scsi/sata_vsc.c +3 -3
@@ -193,12 +193,12 @@ static irqreturn_t vsc_sata_interrupt (i
 			struct ata_port *ap;
 
 			ap = host_set->ports[i];
-			if (ap && !(ap->flags &
-				    (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
+			if (ap &&
+			    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 				struct ata_queued_cmd *qc;
 
 				qc = ata_qc_from_tag(ap, ap->active_tag);
-				if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+				if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
 					handled += ata_host_intr(ap, qc);
 			}
 		}
drivers/scsi/sata_nv.c +2 -2
@@ -302,11 +302,11 @@ static irqreturn_t nv_interrupt (int irq
 
 		ap = host_set->ports[i];
 		if (ap &&
-		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
+		    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
 				handled += ata_host_intr(ap, qc);
 		}
 
#9 lore raw

Re: [PATCH/RFC 3/3] libata: interrupt driven pio for LLD

Quoted 6 lines · Albert Lee

Patch 3/3: interrupt driven pio patch for LLD

sata_qstor.c:
Private irq handler, need modification to handle interrupt driven mode
in the future.
Use polling mode for the unsupported protocols for the time being.

...
default:
+ /* FIXME: qs_intr_mmio() doesn't handle
+ * interrupt driven pio/atapi cdb intr yet.

The qs_intr_mmio() handler is set up for interrupt driven pio/atapi usage, and should already be usable for that.

How do I cause a command to use "interrupt driven" PIO mode with these patches? I could try it to make sure it still works.

Cheers

Mark

author
#10 lore raw

Re: [PATCH/RFC 3/3] libata: interrupt driven pio for LLD

Hi Mark:

Quoted 14 lines
Quoted 6 lines · Albert Lee

Patch 3/3: interrupt driven pio patch for LLD

sata_qstor.c:
Private irq handler, need modification to handle interrupt driven
mode in the future.
Use polling mode for the unsupported protocols for the time being.

...
default:

drivers/scsi/sata_qstor.c +2 -0
+        /* FIXME: qs_intr_mmio() doesn't handle
+         * interrupt driven pio/atapi cdb intr yet.
Quoted 6 lines

The qs_intr_mmio() handler is set up for interrupt driven pio/atapi usage, and should already be usable for that.

How do I cause a command to use "interrupt driven" PIO mode with these patches? I could try it to make sure it still works.

Please apply the following patch 1/3:
http://marc.theaimsgroup.com/?l=linux-ide&m=112736217011430&w=2

The ata_qc_issue_prot() is changed. So qs_intr_mmio() will be called not only to complete a command, but also for PIO data transfer, atapi cdb intr, etc.

Albert

#11 lore raw

Re: [PATCH/RFC 0/3] libata: interrupt driven pio (revised)

In reply to Albert Lee

Comments:

1) the more trivial (read: "easier" to apply) patches should come first in a patch series. In this case, that means that

* patch #2, s/pioxxx/hsmxxx/ should come first
* whitespace changes in patch #3 should come before patch #1

2) LLD should signal its interest in using the polling code paths using a new ATA_FLAG_xxx added to its host_flags list of flags. This is greatly preferred over, e.g., hand-coding this indication in pdc_qc_issue_prot() or mv_qc_issue_prot()

3) Testing of CONFIG_HIGHMEM should be eliminated. In general, #ifdefs are discouraged in Linux whereever possible. That may mean that local_irq_save() must be called unconditionally, or some other solution should be found.

author
#12 lore raw

cover libata: interrupt driven pio (revised 2)

Dear all,

The revised interrupt driven PIO patch is ready for your review.

Patch 1/4: indent and white space change
Patch 2/4: s/PIO_ST_/HSM_ST_/ and s/pio_task_state/hsm_task_state/. Patch 3/4: interrupt driven pio patch for libata-core Patch 4/4: interrupt driven pio patch for LLD

Patch for 2.6.14-rc1 (044a500e46742d39d22f1781cfb64ba93b463e39).
Tested ok on x86 with Promise PDC20275, Seagate ST380011A and LITEON 52x CD-ROM drive.
Need more testing with other host adapters and devices, especially pre-ATA4 ATAPI CD-ROM drives with CDB interrupt.

For your review and advice, thanks.

Albert

author
#13 lore raw

PATCH 1/4 libata: indent and whitespace change

Patch 1/4: indent and white space change

For your review, thanks.

Albert

drivers/scsi/libata-core.c +1 -1
@@ -3586,7 +3586,7 @@ u8 ata_bmdma_status(struct ata_port *ap)
 		void __iomem *mmio = (void __iomem *) ap->ioaddr.bmdma_addr;
 		host_stat = readb(mmio + ATA_DMA_STATUS);
 	} else
-	host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
+		host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
 	return host_stat;
 }
 
drivers/scsi/sata_promise.c +3 -3
@@ -438,11 +438,11 @@ static inline unsigned int pdc_host_intr
 		break;
 
         default:
-                ap->stats.idle_irq++;
-                break;
+		ap->stats.idle_irq++;
+		break;
         }
 
-        return handled;
+	return handled;
 }
 
 static void pdc_irq_clear(struct ata_port *ap)
author
#14 lore raw

PATCH 2/4 libata: rename host states

In reply to Albert Lee

Patch 2/4: rename host states/variables

Changes:
s/PIO_ST_/HSM_ST_/ and s/pio_task_state/hsm_task_state/.

For your review, thanks.

Albert

include/linux/libata.h +10 -10
@@ -156,15 +156,15 @@ enum {
 	ATA_SHIFT_PIO		= 11,
 };
 
-enum pio_task_states {
-	PIO_ST_UNKNOWN,
-	PIO_ST_IDLE,
-	PIO_ST_POLL,
-	PIO_ST_TMOUT,
-	PIO_ST,
-	PIO_ST_LAST,
-	PIO_ST_LAST_POLL,
-	PIO_ST_ERR,
+enum hsm_task_states {
+	HSM_ST_UNKNOWN,
+	HSM_ST_IDLE,
+	HSM_ST_POLL,
+	HSM_ST_TMOUT,
+	HSM_ST,
+	HSM_ST_LAST,
+	HSM_ST_LAST_POLL,
+	HSM_ST_ERR,
 };
 
 /* forward declarations */
@@ -319,7 +319,7 @@ struct ata_port {
 	struct work_struct	packet_task;
 
 	struct work_struct	pio_task;
-	unsigned int		pio_task_state;
+	unsigned int		hsm_task_state;
 	unsigned long		pio_task_timeout;
 
 	void			*private_data;
drivers/scsi/libata-core.c +39 -39

203 lines not on this page · raw message

#15 lore raw

Re: [PATCH/RFC 2/4] libata: rename host states

merged patches 1-2 into upstream branch of libata-dev.git

author
#16 lore raw

PATCH 3/4 libata: interrupt driven pio for libata-core

In reply to Albert Lee

Patch 3/4: interrupt driven pio for libata-core

Changes:
- add PIO_ST_FIRST for the state before sending ATAPI CDB or sending "ATA PIO data out" first data block.
- add ATA_TFLAG_POLLING and ATA_DFLAG_CDB_INTR flags
- remove the ATA_FLAG_NOINTR flag since the interrupt handler is now aware of the states
- modify ata_pio_sector() and atapi_pio_bytes() to work in the interrupt context
- modify the ata_host_intr() to handle PIO interrupts
- modify ata_qc_issue_prot() to initialize states
- atapi_packet_task() changed to handle "ATA PIO data out" first data block
- support the pre-ATA4 ATAPI device which raise interrupt when ready to receive CDB

For your review, thanks.

Albert

include/linux/ata.h +3 -0
@@ -181,6 +181,7 @@ enum {
 	ATA_TFLAG_ISADDR	= (1 << 1), /* enable r/w to nsect/lba regs */
 	ATA_TFLAG_DEVICE	= (1 << 2), /* enable r/w to device reg */
 	ATA_TFLAG_WRITE		= (1 << 3), /* data dir: host->dev==1 (write) */
+	ATA_TFLAG_POLLING	= (1 << 4), /* set nIEN to 1 and use polling */
 };
 
 enum ata_tf_protocols {
@@ -250,6 +251,8 @@ struct ata_taskfile {
 	  ((u64) (id)[(n) + 1] << 16) |	\
 	  ((u64) (id)[(n) + 0]) )
 
+#define ata_id_cdb_intr(id)	(((id)[0] & 0x60) == 0x20)
+
 static inline int atapi_cdb_len(u16 *dev_id)
 {
 	u16 tmp = dev_id[0] & 0x3;
include/linux/libata.h +2 -2
@@ -97,6 +97,7 @@ enum {
 	ATA_DFLAG_LBA48		= (1 << 0), /* device supports LBA48 */
 	ATA_DFLAG_PIO		= (1 << 1), /* device currently in PIO mode */
 	ATA_DFLAG_LOCK_SECTORS	= (1 << 2), /* don't adjust max_sectors */
+	ATA_DFLAG_CDB_INTR	= (1 << 3), /* device asserts INTRQ when ready for CDB */
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
@@ -115,8 +116,6 @@ enum {
 	ATA_FLAG_MMIO		= (1 << 6), /* use MMIO, not PIO */
 	ATA_FLAG_SATA_RESET	= (1 << 7), /* use COMRESET */
 	ATA_FLAG_PIO_DMA	= (1 << 8), /* PIO cmds via DMA */
-	ATA_FLAG_NOINTR		= (1 << 9), /* FIXME: Remove this once
-					     * proper HSM is in place. */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */
@@ -165,6 +164,7 @@ enum hsm_task_states {
 	HSM_ST_LAST,
 	HSM_ST_LAST_POLL,
 	HSM_ST_ERR,
+	HSM_ST_FIRST,
 };
 
 /* forward declarations */
drivers/scsi/libata-core.c +267 -68

483 lines not on this page · raw message

Applied
#17 lore raw

Re: [PATCH/RFC 3/4] libata: interrupt driven pio for libata-core

applied patches 3-4 to 'irq-pio' branch of libata-dev.git.

After verifying that EVERY driver continues to work, we can merge this upstream. This change will need a lot of testing.

Also, note that adding support for READ/WRITE MULTIPLE SECTORS would be nice. This will involve slight changes to the PIO code, and a SET MULTIPLE MODE command during probe time.

Thanks for working on this!

#18 lore raw

Re: [PATCH/RFC 3/4] libata: interrupt driven pio for libata-core

In reply to Albert Lee

Hi,

Quoted 4 lines · Albert Lee

Patch 3/4: interrupt driven pio for libata-core

Changes:
- add PIO_ST_FIRST for the state before sending ATAPI CDB or sending

HSM_ST_FIRST

Could you add some brief comments about HSM_* states (one line per state should be OK)?

Quoted 40 lines
drivers/scsi/libata-core.c +18 -4
@@ -3344,43 +3356,96 @@ int ata_qc_issue_prot(struct ata_queued_
 {
        struct ata_port *ap = qc->ap;
 
+       /* select the device */
        ata_dev_select(ap, qc->dev->devno, 1, 0);
 
+       /* start the command */
        switch (qc->tf.protocol) {
        case ATA_PROT_NODATA:
+               if (qc->tf.flags & ATA_TFLAG_POLLING)
+                       ata_qc_set_polling(qc);
+
                ata_tf_to_host_nolock(ap, &qc->tf);
+               ap->hsm_task_state = HSM_ST_LAST;
+
+               if (qc->tf.flags & ATA_TFLAG_POLLING)
+                       queue_work(ata_wq, &ap->pio_task);
+
                break;
 
        case ATA_PROT_DMA:
+               assert(!(qc->tf.flags & ATA_TFLAG_POLLING));
+
                ap->ops->tf_load(ap, &qc->tf);   /* load tf registers */
                ap->ops->bmdma_setup(qc);           /* set up bmdma */
                ap->ops->bmdma_start(qc);           /* initiate bmdma */
+               ap->hsm_task_state = HSM_ST_LAST;
                break;
 
-       case ATA_PROT_PIO: /* load tf registers, initiate polling pio */
-               ata_qc_set_polling(qc);
+       case ATA_PROT_PIO:
+               if (qc->tf.flags & ATA_TFLAG_POLLING)
+                       ata_qc_set_polling(qc);
+
                ata_tf_to_host_nolock(ap, &qc->tf);
-               ap->hsm_task_state = HSM_ST;
-               queue_work(ata_wq, &ap->pio_task);
+

drivers/scsi/libata-core.c int ata_qc_issue_prot(struct ata_queued_ · line 3400

Quoted 10 lines
drivers/scsi/libata-core.c +10 -0
+               if (qc->tf.flags & ATA_TFLAG_POLLING) {
+                       /* polling PIO */
+                       ap->hsm_task_state = HSM_ST;
+                       queue_work(ata_wq, &ap->pio_task);
+               } else {
+                       /* interrupt driven PIO */
+                       if (qc->tf.flags & ATA_TFLAG_WRITE) {
+                               /* PIO data out protocol */
+                               ap->hsm_task_state = HSM_ST_FIRST;
+                               queue_work(ata_wq, &ap->packet_task);

atapi_packet_task() is now abused to do ATA PIO out which is very confusing given that name of the function (and all the comments) remains unchanged. Please correct it (I think it would be good idea to separate HSM_ST_CDB from HSM_ST_FIRST).

Quoted 19 lines
drivers/scsi/libata-core.c +11 -3
        case ATA_PROT_ATAPI:
-               ata_qc_set_polling(qc);
+               if (qc->tf.flags & ATA_TFLAG_POLLING)
+                       ata_qc_set_polling(qc);
+
                ata_tf_to_host_nolock(ap, &qc->tf);
-               queue_work(ata_wq, &ap->packet_task);
+               ap->hsm_task_state = HSM_ST_FIRST;
+
+               /* 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->packet_task);
                break;
 
        case ATA_PROT_ATAPI_NODATA:
-               ap->flags |= ATA_FLAG_NOINTR;
+               if (qc->tf.flags & ATA_TFLAG_POLLING)
+                       ata_qc_set_polling(qc);

drivers/scsi/libata-core.c int ata_qc_issue_prot(struct ata_queued_ · line 3437

Quoted 10 lines
drivers/scsi/libata-core.c +7 -1
+
                ata_tf_to_host_nolock(ap, &qc->tf);
-               queue_work(ata_wq, &ap->packet_task);
+               ap->hsm_task_state = HSM_ST_FIRST;
+
+               /* 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->packet_task);
                break;

ATA_PROT_ATAPI_NODATA case is now identical to ATA_PROT_ATAPI one - no need to duplicate code.

It may be also worth to split ata_qc_issue_prot() on IRQ and polling versions because it is really hard to read now.

drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_queued_cm · line 3744

Quoted 8 lines
drivers/scsi/libata-core.c +1 -0
+/**
  *     ata_host_intr - Handle host interrupt for given (port, task)
  *     @ap: Port on which interrupt arrived (possibly...)
  *     @qc: Taskfile currently active in engine
@@ -3641,47 +3742,142 @@ void ata_bmdma_stop(struct ata_queued_cm
 inline unsigned int ata_host_intr (struct ata_port *ap,
                                   struct ata_queued_cmd *qc)
 {

We should check first if we are polling and if so we shouldn't touch hardware et all as we can interface with ata_pio_task or atapi_packet_task. Just think about HSM_ST case when hardware is ready for the next transfer and IRQ happens (from some other device).

Please note that this bug is present in original libata code.

drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_queued_cm · line 3650

Quoted 8 lines
drivers/scsi/libata-core.c +1 -6
-       u8 status, host_stat;
-
-       switch (qc->tf.protocol) {
+       u8 status, host_stat = 0;
 
-       case ATA_PROT_DMA:
-       case ATA_PROT_ATAPI_DMA:
-       case ATA_PROT_ATAPI:

Your patch changes behavior for ATA_PROT_ATAPI case - now DMA engine won't be checked. We had discussion about fixing it before but I don't remember what was the conclusion - in any case it is worth to at least mention this change in the patch description.

Quoted 81 lines
drivers/scsi/libata-core.c +52 -22
-               /* check status of DMA engine */
-               host_stat = ap->ops->bmdma_status(ap);
-               VPRINTK("ata%u: host_stat 0x%X\n", ap->id, host_stat);
+       VPRINTK("ata%u: protocol %d task_state %d\n",
+               ap->id, qc->tf.protocol, ap->hsm_task_state);
 
-               /* if it's not our irq... */
-               if (!(host_stat & ATA_DMA_INTR))
+       /* Check whether we are expecting interrupt in this state */
+       switch (ap->hsm_task_state) {
+       case HSM_ST_FIRST:
+               /* Check the ATA_DFLAG_CDB_INTR flag is enough here.
+                * The flag was turned on only for atapi devices.
+                * No need to check is_atapi_taskfile(&qc->tf) again.
+                */
+               if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
                        goto idle_irq;
+               break;
+       case HSM_ST_LAST:
+               if (qc->tf.protocol == ATA_PROT_DMA ||
+                   qc->tf.protocol == ATA_PROT_ATAPI_DMA) {
+                       /* check status of DMA engine */
+                       host_stat = ap->ops->bmdma_status(ap);
+                       VPRINTK("ata%u: host_stat 0x%X\n", ap->id, host_stat);
+
+                       /* if it's not our irq... */
+                       if (!(host_stat & ATA_DMA_INTR))
+                               goto idle_irq;
 
-               /* before we do anything else, clear DMA-Start bit */
-               ap->ops->bmdma_stop(qc);
+                       /* before we do anything else, clear DMA-Start bit */
+                       ap->ops->bmdma_stop(qc);
+               }
+               break;
+       case HSM_ST:
+               break;
+       default:
+               goto idle_irq;
+       }
 
-               /* fall through */
+       /* check altstatus */
+       status = ata_altstatus(ap);
+       if (status & ATA_BUSY)
+               goto idle_irq;
 
-       case ATA_PROT_ATAPI_NODATA:
-       case ATA_PROT_NODATA:
-               /* check altstatus */
-               status = ata_altstatus(ap);
-               if (status & ATA_BUSY)
-                       goto idle_irq;
+       /* check main status, clearing INTRQ */
+       status = ata_chk_status(ap);
+       if (unlikely(status & ATA_BUSY))
+               goto idle_irq;
 
-               /* check main status, clearing INTRQ */
-               status = ata_chk_status(ap);
-               if (unlikely(status & ATA_BUSY))
-                       goto idle_irq;
-               DPRINTK("ata%u: protocol %d (dev_stat 0x%X)\n",
-                       ap->id, qc->tf.protocol, status);
+       DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
+               ap->id, qc->tf.protocol, ap->hsm_task_state, status);
 
-               /* ack bmdma irq events */
-               ap->ops->irq_clear(ap);
+       /* ack bmdma irq events */
+       ap->ops->irq_clear(ap);
+
+       /* check error */
+       if (unlikely((status & ATA_ERR) || (host_stat & ATA_DMA_ERR)))
+               ap->hsm_task_state = HSM_ST_ERR;
+
+fsm_start:
+       switch (ap->hsm_task_state) {
+       case HSM_ST_FIRST:
+               /* Some pre-ATAPI-4 devices assert INTRQ
+                * at this state when ready to receive CDB.

drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_queued_cm · line 3815

Quoted 10 lines
drivers/scsi/libata-core.c +10 -0
+                */
+
+               /* check device status */
+               if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
+                       /* Wrong status. Let EH handle this */
+                       ap->hsm_task_state = HSM_ST_ERR;
+                       goto fsm_start;
+               }
+
+               atapi_send_cdb(ap, qc);

This case can happen also for ATA PIO out:
ata_qc_issue_prot() -> IRQ happens -> ata_host_intr().

Sending CDB unconditionally is wrong.

The rest of the patch looks fine but it is was quite difficult to review because you've combined two major logical changes into one patch:
* switching libata to use host state information
* adding support for IRQ driven PIO

Thanks,
Bartlomiej

author
#19 lore raw

Re: [PATCH/RFC 3/4] libata: interrupt driven pio for libata-core

Hi Bart:

Quoted 12 lines

Hi,

Quoted 6 lines · Albert Lee

Patch 3/4: interrupt driven pio for libata-core

Changes:
- add PIO_ST_FIRST for the state before sending ATAPI CDB or sending

Quoted 6 lines

HSM_ST_FIRST

Could you add some brief comments about HSM_* states (one line per state should be OK)?

Hmm, will add the comments in the follow up patch.

Quoted 6 lines
Quoted 6 lines
drivers/scsi/libata-core.c +0 -0
@@ -3344,43 +3356,96 @@ int ata_qc_issue_prot(struct ata_queued_
 {
       struct ata_port *ap = qc->ap;
 
Quoted 15 lines

drivers/scsi/libata-core.c int ata_qc_issue_prot(struct ata_queued_ · line 3425

Quoted 8 lines
drivers/scsi/libata-core.c +6 -0
+               } else {
+                       /* interrupt driven PIO */
+                       if (qc->tf.flags & ATA_TFLAG_WRITE) {
+                               /* PIO data out protocol */
+                               ap->hsm_task_state = HSM_ST_FIRST;
+                               queue_work(ata_wq, &ap->packet_task);
 
 

atapi_packet_task() is now abused to do ATA PIO out which is very confusing given that name of the function (and all the comments) remains unchanged. Please correct it (I think it would be good idea to separate HSM_ST_CDB from HSM_ST_FIRST).

You're right, will rename the atapi_packet_task() and revise the related variable and comments.

HSM_ST_CDB and HSM_ST_FIRST are almost the same except the content of the data being sent. Could we keep them as one state?

Quoted 37 lines
Quoted 23 lines
drivers/scsi/libata-core.c +12 -3
       case ATA_PROT_ATAPI:
-               ata_qc_set_polling(qc);
+               if (qc->tf.flags & ATA_TFLAG_POLLING)
+                       ata_qc_set_polling(qc);
+
               ata_tf_to_host_nolock(ap, &qc->tf);
-               queue_work(ata_wq, &ap->packet_task);
+               ap->hsm_task_state = HSM_ST_FIRST;
+
+               /* 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->packet_task);
               break;
 
       case ATA_PROT_ATAPI_NODATA:
-               ap->flags |= ATA_FLAG_NOINTR;
+               if (qc->tf.flags & ATA_TFLAG_POLLING)
+                       ata_qc_set_polling(qc);
+
               ata_tf_to_host_nolock(ap, &qc->tf);

drivers/scsi/libata-core.c int ata_qc_issue_prot(struct ata_queued_ · line 3450

Quoted 10 lines
drivers/scsi/libata-core.c +6 -1
-               queue_work(ata_wq, &ap->packet_task);
+               ap->hsm_task_state = HSM_ST_FIRST;
+
+               /* 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->packet_task);
               break;
 
 

ATA_PROT_ATAPI_NODATA case is now identical to ATA_PROT_ATAPI one - no need to duplicate code.

Quoted 4 lines

It may be also worth to split ata_qc_issue_prot() on IRQ and polling versions because it is really hard to read now.

Nice catch, will remove the redundant code.
will also cleanup the ata_qc_issue_prot() code to simplify the PIO data out codepath.
(Hopefully the simplified ata_qc_issue_prot() is easier to read and we could have one version for both polling and IRQ version.)

Quoted 18 lines

drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_queued_cm · line 3746

Quoted 12 lines
drivers/scsi/libata-core.c +1 -0
+/**
 *     ata_host_intr - Handle host interrupt for given (port, task)
 *     @ap: Port on which interrupt arrived (possibly...)
 *     @qc: Taskfile currently active in engine
@@ -3641,47 +3742,142 @@ void ata_bmdma_stop(struct ata_queued_cm
 inline unsigned int ata_host_intr (struct ata_port *ap,
                                  struct ata_queued_cmd *qc)
 {
 
 

We should check first if we are polling and if so we shouldn't touch hardware et all as we can interface with ata_pio_task or atapi_packet_task. Just think about HSM_ST case when hardware is ready for the next transfer and IRQ happens (from some other device).

Quoted 4 lines

Please note that this bug is present in original libata code.

The (qc->tf.flags & ATA_TFLAG_POLLING) flag is checked in ata_interrupt(). So, the race between the irq handler and ata_pio_task() is avoided.

When the state is HSM_ST_FIRST, ata_host_intr() checks the (qc->dev->flags & ATA_DFLAG_CDB_INTR) flag. Also in atapi_packet_task(), the host lock is aquired during the state transition from HSM_ST_FIRST to next state. So, the race between the irq handler and atapi_packet_task() is avoided.

Quoted 11 lines

drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_queued_cm · line 3770

Quoted 10 lines
drivers/scsi/libata-core.c +1 -6
-       u8 status, host_stat;
-
-       switch (qc->tf.protocol) {
+       u8 status, host_stat = 0;
 
-       case ATA_PROT_DMA:
-       case ATA_PROT_ATAPI_DMA:
-       case ATA_PROT_ATAPI:
 
 
Quoted 7 lines

Your patch changes behavior for ATA_PROT_ATAPI case - now DMA engine won't be checked. We had discussion about fixing it before but I don't remember what was the conclusion - in any case it is worth to at least mention this change in the patch description.

Sorry I forgot to mention it in the description of previous patch For the ATA_PROT_ATAPI case, the behavior is changed in the irq handler:
- The DMA status register is no longer checked.
- HSM state and device busy flag are used together to see whether it
is our irq or not.

This new behavior seems to be working ok on my test machine. However, not sure whether this 100% works on all hardware. Need more test and comment.

Quoted 42 lines
Quoted 28 lines
drivers/scsi/libata-core.c +11 -5
-               /* check status of DMA engine */
-               host_stat = ap->ops->bmdma_status(ap);
-               VPRINTK("ata%u: host_stat 0x%X\n", ap->id, host_stat);
+       VPRINTK("ata%u: protocol %d task_state %d\n",
+               ap->id, qc->tf.protocol, ap->hsm_task_state);
 
-               /* if it's not our irq... */
-               if (!(host_stat & ATA_DMA_INTR))
+       /* Check whether we are expecting interrupt in this state */
+       switch (ap->hsm_task_state) {
+       case HSM_ST_FIRST:
+               /* Check the ATA_DFLAG_CDB_INTR flag is enough here.
+                * The flag was turned on only for atapi devices.
+                * No need to check is_atapi_taskfile(&qc->tf) again.
+                */
+               if (!(qc->dev->flags & ATA_DFLAG_CDB_INTR))
                       goto idle_irq;
+               break;

... (snip) ...

drivers/scsi/libata-core.c +7 -0
+
+
+fsm_start:
+       switch (ap->hsm_task_state) {
+       case HSM_ST_FIRST:
+               /* Some pre-ATAPI-4 devices assert INTRQ
+                * at this state when ready to receive CDB.

drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_queued_cm · line 3815

Quoted 10 lines
drivers/scsi/libata-core.c +10 -0
+                */
+
+               /* check device status */
+               if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
+                       /* Wrong status. Let EH handle this */
+                       ap->hsm_task_state = HSM_ST_ERR;
+                       goto fsm_start;
+               }
+
+               atapi_send_cdb(ap, qc);

This case can happen also for ATA PIO out:
ata_qc_issue_prot() -> IRQ happens -> ata_host_intr().

Quoted 5 lines

Sending CDB unconditionally is wrong.

The (qc->dev->flags & ATA_DFLAG_CDB_INTR) flag is only turned on for atapi devices.
So, when we are in the HSM_ST_FIRST state, ATA devices are filtered out and won't enter the irq handler.

Thanks for the review and advice, will revise and send follow up patch later. (Will also have look at the READ/WRITE MULTIPLE mentioned by Jeff.)

Albert

#20 lore raw

Re: [PATCH/RFC 3/4] libata: interrupt driven pio for libata-core

Hi,

Quoted 16 lines · Albert Lee
Quoted 16 lines

drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_queued_cm · line 3746

Quoted 10 lines
drivers/scsi/libata-core.c +1 -0
+/**
 *     ata_host_intr - Handle host interrupt for given (port, task)
 *     @ap: Port on which interrupt arrived (possibly...)
 *     @qc: Taskfile currently active in engine
@@ -3641,47 +3742,142 @@ void ata_bmdma_stop(struct ata_queued_cm
 inline unsigned int ata_host_intr (struct ata_port *ap,
                                  struct ata_queued_cmd *qc)
 {
 
 

We should check first if we are polling and if so we shouldn't touch hardware et all as we can interface with ata_pio_task or atapi_packet_task. Just think about HSM_ST case when hardware is ready for the next transfer and IRQ happens (from some other device).

Quoted 6 lines
Quoted 4 lines

Please note that this bug is present in original libata code.

The (qc->tf.flags & ATA_TFLAG_POLLING) flag is checked in ata_interrupt(). So, the race between the irq handler and ata_pio_task() is avoided.

You are right, I completely forgot about ata_interrupt(). :-)

Quoted 5 lines

When the state is HSM_ST_FIRST, ata_host_intr() checks the (qc->dev->flags & ATA_DFLAG_CDB_INTR) flag.
Also in atapi_packet_task(), the host lock is aquired during the state transition from HSM_ST_FIRST to next state.
So, the race between the irq handler and atapi_packet_task() is avoided.

What about ATAPI device with !dev->flags & ATA_DFLAG_CDB_INTR)?

Shouldn't we do spin_lock_irqsave() in atapi_packet_task() before actually touching hardware?

Quoted 26 lines
Quoted 21 lines
Quoted 7 lines
drivers/scsi/libata-core.c +7 -0
+fsm_start:
+       switch (ap->hsm_task_state) {
+       case HSM_ST_FIRST:
+               /* Some pre-ATAPI-4 devices assert INTRQ
+                * at this state when ready to receive CDB.
+                */
+

drivers/scsi/libata-core.c void ata_bmdma_stop(struct ata_queued_cm · line 3873

Quoted 10 lines
drivers/scsi/libata-core.c +8 -0
+               /* check device status */
+               if (unlikely((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)) {
+                       /* Wrong status. Let EH handle this */
+                       ap->hsm_task_state = HSM_ST_ERR;
+                       goto fsm_start;
+               }
+
+               atapi_send_cdb(ap, qc);
 
 

This case can happen also for ATA PIO out:
ata_qc_issue_prot() -> IRQ happens -> ata_host_intr().

Quoted 5 lines

Sending CDB unconditionally is wrong.

Quoted 4 lines

The (qc->dev->flags & ATA_DFLAG_CDB_INTR) flag is only turned on for atapi devices.
So, when we are in the HSM_ST_FIRST state, ATA devices are filtered out and won't enter the irq handler.

Yep, you are right, so we can keep one state (HSM_ST_FIRST) for ATA and ATAPI.

Quoted 3 lines

Thanks for the review and advice, will revise and send follow up patch later.
(Will also have look at the READ/WRITE MULTIPLE mentioned by Jeff.)

Thanks for working on this.
Bartlomiej

author
#21 lore raw

Re: [PATCH/RFC 3/4] libata: interrupt driven pio for libata-core

Hi Bart,:

Quoted 9 lines
Quoted 8 lines

When the state is HSM_ST_FIRST, ata_host_intr() checks the (qc->dev->flags & ATA_DFLAG_CDB_INTR) flag.
Also in atapi_packet_task(), the host lock is aquired during the state transition from HSM_ST_FIRST to next state.
So, the race between the irq handler and atapi_packet_task() is avoided.

Quoted 7 lines

What about ATAPI device with !dev->flags & ATA_DFLAG_CDB_INTR)?

Shouldn't we do spin_lock_irqsave() in atapi_packet_task() before actually touching hardware?

For the ATAPI devices that don't raise CDB interrupes,
when atapi_packet_task() is entered, the state is HSM_ST_FIRST.
In this state, the irq handler will check the ATA_DFLAG_CDB_INTR flag and skip touching the hardware. atapi_packet_task() is safe to read the device status register at this state.
atapi_packet_task() does spin_lock_irqsave() when changing the states from HSM_ST_FIRST to say, HSM_ST and sending out the data.

For the ATAPI devices that raise CDB interrupes, ata_qc_issue_prot() won't queue an new atapi_packet_task().
The irq handler takes over after ata_qc_issue_prot() and can touch the hardware.

Thanks for the comments,

Albert

#22 lore raw

Re: [PATCH/RFC 3/4] libata: interrupt driven pio for libata-core

Quoted 27 lines · Albert Lee

Hi Bart,:

Quoted 9 lines
Quoted 8 lines

When the state is HSM_ST_FIRST, ata_host_intr() checks the (qc->dev->flags & ATA_DFLAG_CDB_INTR) flag.
Also in atapi_packet_task(), the host lock is aquired during the state transition from HSM_ST_FIRST to next state.
So, the race between the irq handler and atapi_packet_task() is avoided.

Quoted 7 lines

What about ATAPI device with !dev->flags & ATA_DFLAG_CDB_INTR)?

Shouldn't we do spin_lock_irqsave() in atapi_packet_task() before actually touching hardware?

For the ATAPI devices that don't raise CDB interrupes,
when atapi_packet_task() is entered, the state is HSM_ST_FIRST.
In this state, the irq handler will check the ATA_DFLAG_CDB_INTR flag
and skip touching the hardware.
atapi_packet_task() is safe to read the device status register at this state.
atapi_packet_task() does spin_lock_irqsave() when changing the states from HSM_ST_FIRST to say, HSM_ST and sending out the data.

Quoted 4 lines

For the ATAPI devices that raise CDB interrupes, ata_qc_issue_prot() won't queue an new atapi_packet_task(). The irq handler takes over after ata_qc_issue_prot() and can touch the hardware.

Yes, this is correct - thanks for detailed explanation.

Bartlomiej

author
#23 lore raw

cover libata: irq driven pio follow-up patches

In reply to Bartlomiej Zolnierkiewicz

Dear all,

The irq driven PIO follow up patches per Bart's advice.

patch 1/4:
- add comments for HSM_ST_*

ata_qc_issue_prot():
- remove the redundant code for the ATA_PROT_ATAPI case.

patch 2/4:
- rename atapi_packet_task() to ata_dataout_task() and change comments

patch 3/4:
- simplify if condition in ata_dataout_task():

     Use if (qc->tf.protocol == ATA_PROT_PIO) instead of 

is_atapi_taskfile()

patch 4/4:
ata_qc_issue_prot():
- let the PIO data out case always go through the ata_dataout_task()
codepath.

ata_dataout_task():
- handle the PIO data out + polling case.

Patch against libata-dev irq-pio tree (e50362eccd8809a224cda5f71714a088ba37b2ab).

For your review and advice, thanks.

Albert

author
#24 lore raw

PATCH 1/4 irq-pio: add comments and cleanup

Patch 1/4: add comments and remove redundant code

For your review, thanks.

Albert

include/linux/libata.h +10 -9
@@ -158,15 +158,16 @@ enum {
 };
 
 enum hsm_task_states {
-	HSM_ST_UNKNOWN,
-	HSM_ST_IDLE,
-	HSM_ST_POLL,
-	HSM_ST_TMOUT,
-	HSM_ST,
-	HSM_ST_LAST,
-	HSM_ST_LAST_POLL,
-	HSM_ST_ERR,
-	HSM_ST_FIRST,
+	HSM_ST_UNKNOWN,		/* state unknown */
+	HSM_ST_IDLE,		/* no command on going */
+	HSM_ST_POLL,		/* same as HSM_ST, waits longer */
+	HSM_ST_TMOUT,		/* timeout */
+	HSM_ST,			/* (waiting the device to) transfer data */
+	HSM_ST_LAST,		/* (waiting the device to) complete command */
+	HSM_ST_LAST_POLL,	/* same as HSM_ST_LAST, waits longer */
+	HSM_ST_ERR,		/* error */
+	HSM_ST_FIRST,		/* (waiting the device to)
+				   write CDB or first data block */
 };
 
 /* forward declarations */
drivers/scsi/libata-core.c +0 -12
@@ -3430,18 +3430,6 @@ int ata_qc_issue_prot(struct ata_queued_
 		break;
 
 	case ATA_PROT_ATAPI:
-		if (qc->tf.flags & ATA_TFLAG_POLLING)
-			ata_qc_set_polling(qc);
-
-		ata_tf_to_host_nolock(ap, &qc->tf);
-		ap->hsm_task_state = HSM_ST_FIRST;
-
-		/* 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->packet_task);
-		break;
-
 	case ATA_PROT_ATAPI_NODATA:
 		if (qc->tf.flags & ATA_TFLAG_POLLING)
 			ata_qc_set_polling(qc);
author
#25 lore raw

PATCH 2/4 irq-pio: rename atapi_packet_task() and comments

In reply to Albert Lee

Patch 2/4: rename atapi_packet_task() and change comments

For your review, thanks.

Albert

include/linux/libata.h +3 -3
@@ -129,8 +129,8 @@ enum {
 	ATA_TMOUT_PIO		= 30 * HZ,
 	ATA_TMOUT_BOOT		= 30 * HZ,	/* hueristic */
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* hueristic */
-	ATA_TMOUT_CDB		= 30 * HZ,
-	ATA_TMOUT_CDB_QUICK	= 5 * HZ,
+	ATA_TMOUT_DATAOUT	= 30 * HZ,
+	ATA_TMOUT_DATAOUT_QUICK	= 5 * HZ,
 
 	/* ATA bus states */
 	BUS_UNKNOWN		= 0,
@@ -319,7 +319,7 @@ struct ata_port {
 	struct ata_host_stats	stats;
 	struct ata_host_set	*host_set;
 
-	struct work_struct	packet_task;
+	struct work_struct	dataout_task;
 
 	struct work_struct	pio_task;
 	unsigned int		hsm_task_state;
drivers/scsi/libata-core.c +13 -12
@@ -3416,7 +3416,7 @@ 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->packet_task);
+				queue_work(ata_wq, &ap->dataout_task);
 
 				/* send first data block by polling */
 			} else {
@@ -3440,7 +3440,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->packet_task);
+			queue_work(ata_wq, &ap->dataout_task);
 		break;
 
 	case ATA_PROT_ATAPI_DMA:
@@ -3452,7 +3452,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->packet_task);
+			queue_work(ata_wq, &ap->dataout_task);
 		break;
 
 	default:
@@ -3952,20 +3952,21 @@ irqreturn_t ata_interrupt (int irq, void
 }
 
 /**
- *	atapi_packet_task - Write CDB bytes to hardware
- *	@_data: Port to which ATAPI device is attached.
+ *	ata_dataout_task - Write first data block to hardware
+ *	@_data: Port to which ATA/ATAPI device is attached.
  *
  *	When device has indicated its readiness to accept
- *	a CDB, this function is called.  Send the CDB.
- *	If DMA is to be performed, exit immediately.
- *	Otherwise, we are in polling mode, so poll
- *	status under operation succeeds or fails.
+ *	the data, this function sends out the CDB or 
+ *	the first data block by PIO.
+ *	After this, 
+ *	  - If polling, ata_pio_task() handles the rest.
+ *	  - Otherwise, interrupt handler takes over.
  *
  *	LOCKING:
  *	Kernel thread context (may sleep)
  */
 
-static void atapi_packet_task(void *_data)
+static void ata_dataout_task(void *_data)
 {
 	struct ata_port *ap = _data;
 	struct ata_queued_cmd *qc;
@@ -3978,7 +3979,7 @@ static void atapi_packet_task(void *_dat
 
 	/* sleep-wait for BSY to clear */
 	DPRINTK("busy wait\n");
-	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+	if (ata_busy_sleep(ap, ATA_TMOUT_DATAOUT_QUICK, ATA_TMOUT_DATAOUT))
 		goto err_out;
 
 	/* make sure DRQ is set */
@@ -4141,7 +4142,7 @@ static void ata_host_init(struct ata_por
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
 
-	INIT_WORK(&ap->packet_task, atapi_packet_task, ap);
+	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++)
author
#26 lore raw

PATCH 3/4 irq-pio: simplify if condition in ata_dataout_task()

In reply to Albert Lee

Patch 3/4: simplify if condition in ata_dataout_task()

Changes:
- Use if (qc->tf.protocol == ATA_PROT_PIO) instead of if(is_atapi_taskfile()) in ata_dataout_task()

For your review, thanks.

Albert

drivers/scsi/libata-core.c +7 -7
@@ -3994,13 +3994,7 @@ static void ata_dataout_task(void *_data
 	 */
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 
-	if (is_atapi_taskfile(&qc->tf)) {
-		/* send CDB */
-		atapi_send_cdb(ap, qc);
-
-		if (qc->tf.flags & ATA_TFLAG_POLLING)
-			queue_work(ata_wq, &ap->pio_task);
-	} else {
+	if (qc->tf.protocol == ATA_PROT_PIO) {
 		/* PIO data out protocol.
 		 * send first data block.
 		 */
@@ -4013,6 +4007,12 @@ static void ata_dataout_task(void *_data
 		ata_altstatus(ap); /* flush */
 
 		/* interrupt handler takes over from here */
+	} else {
+		/* send CDB */
+		atapi_send_cdb(ap, qc);
+
+		if (qc->tf.flags & ATA_TFLAG_POLLING)
+			queue_work(ata_wq, &ap->pio_task);
 	}
 
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
author
#27 lore raw

PATCH 4/4 irq-pio: cleanup ata_qc_issue_prot()

In reply to Albert Lee

Patch 4/4: cleanup ata_qc_issue_prot()

Changes:
ata_qc_issue_prot():
- cleanup and let the PIO data out case always go through the
ata_dataout_task() codepath.
(Previously for PIO data out case, 2 code pathes were used
- irq case goes through ata_data_out_task() codepath.
- polling case jumps over the HSM_ST_FIRST state and goes to
HSM_ST and ata_pio_task() directly.)

ata_dataout_task():
- rearrange the queue_work() code to handle the PIO data out +
polling case.

For your review, thanks.

Albert

drivers/scsi/libata-core.c +22 -22
@@ -3407,24 +3407,24 @@ int ata_qc_issue_prot(struct ata_queued_
 
 		ata_tf_to_host_nolock(ap, &qc->tf);
 
-		if (qc->tf.flags & ATA_TFLAG_POLLING) {
-			/* polling PIO */
-			ap->hsm_task_state = HSM_ST;
-			queue_work(ata_wq, &ap->pio_task);
+		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);
+
+			/* always send first data block using
+			 * the ata_dataout_task() codepath.
+			 */
 		} else {
-			/* interrupt driven PIO */
-			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);
-
-				/* send first data block by polling */
-			} else {
-				/* PIO data in protocol */
-				ap->hsm_task_state = HSM_ST;
+			/* PIO data in protocol */
+			ap->hsm_task_state = HSM_ST;
 
-				/* interrupt handler takes over from here */
-			}
+			if (qc->tf.flags & ATA_TFLAG_POLLING)
+				queue_work(ata_wq, &ap->pio_task);
+
+			/* if polling, ata_pio_task() handles the rest.
+			 * otherwise, interrupt handler takes over from here.
+			 */
 		}
 
 		break;
@@ -4005,15 +4005,15 @@ static void ata_dataout_task(void *_data
 		ap->hsm_task_state = HSM_ST;
 		ata_pio_sector(qc);
 		ata_altstatus(ap); /* flush */
-
-		/* interrupt handler takes over from here */
-	} else {
+	} else
 		/* send CDB */
 		atapi_send_cdb(ap, qc);
 
-		if (qc->tf.flags & ATA_TFLAG_POLLING)
-			queue_work(ata_wq, &ap->pio_task);
-	}
+	/* 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);
 
Applied
#28 lore raw

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

In reply to Albert Lee

Applied patches 1-4 to irq-pio branch.

I wonder if dataout_task and pio_task can be merged?

Jeff

author
#29 lore raw

cover libata: more irq driven pio follow-up patches

Dear all,

More irq driven PIO follow-up patches per Jeff's advice:
1. merge ata_dataout_task and ata_pio_task
2. add read/write multiple support

patch 1/4: move functions
- move atapi_send_cdb() and ata_dataout_task() to be near
ata_pio_*() functions

patch 2/4: remove ap->dataout_task
- remove ap->dataout_task
- let ata_pio_task() handle the HSM_ST_FIRST state.
- rename ata_dataout_task() to ata_pio_first_block()

patch 3/4: integrate ata_pio_first_block() with ata_pio_task()
- change the return value of ata_pio_complete()
- add return value for ata_pio_first_block()
- rename variable "qc_completed" to "has_next" in ata_pio_task()

patch 4/4: add read/write multiple support
- 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

Patch against libata-dev irq-pio branch (54f00389563c80fa1de250a21256313ba01ca07d).

For your review and advice, thanks.

Albert

author
#30 lore raw

PATCH 1/4 irq-pio: move functions

Patch 1/4:
move atapi_send_cdb() and ata_dataout_task() to be near ata_pio_*()
functions

For your review, thanks.

Albert

drivers/scsi/libata-core.c +108 -109
@@ -2696,6 +2696,114 @@ static void ata_pio_sector(struct ata_qu
 }
 
 /**
+ *	atapi_send_cdb - Write CDB bytes to hardware
+ *	@ap: Port to which ATAPI device is attached.
+ *	@qc: Taskfile currently active
+ *
+ *	When device has indicated its readiness to accept
+ *	a CDB, this function is called.  Send the CDB.
+ *
+ *	LOCKING:
+ *	caller.
+ */
+
+static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
+{
+	/* send SCSI cdb */
+	DPRINTK("send cdb\n");
+	assert(ap->cdb_len >= 12);
+
+	ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
+	ata_altstatus(ap); /* flush */
+
+	switch (qc->tf.protocol) {
+	case ATA_PROT_ATAPI:
+		ap->hsm_task_state = HSM_ST;
+		break;
+	case ATA_PROT_ATAPI_NODATA:
+		ap->hsm_task_state = HSM_ST_LAST;
+		break;
+	case ATA_PROT_ATAPI_DMA:
+		ap->hsm_task_state = HSM_ST_LAST;
+		/* initiate bmdma */
+		ap->ops->bmdma_start(qc);
+		break;
+	}
+}
+
+/**
+ *	ata_dataout_task - Write first data block to hardware
+ *	@_data: 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 
+ *	the first data block by PIO.
+ *	After this, 
+ *	  - If polling, ata_pio_task() handles the rest.
+ *	  - Otherwise, interrupt handler takes over.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ */
+
+static void ata_dataout_task(void *_data)
+{
+	struct ata_port *ap = _data;
+	struct ata_queued_cmd *qc;
+	u8 status;
+	unsigned long flags;
+
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	assert(qc != NULL);
+	assert(qc->flags & ATA_QCFLAG_ACTIVE);
+
+	/* sleep-wait for BSY to clear */
+	DPRINTK("busy wait\n");
+	if (ata_busy_sleep(ap, ATA_TMOUT_DATAOUT_QUICK, ATA_TMOUT_DATAOUT))
+		goto err_out;
+
+	/* make sure DRQ is set */
+	status = ata_chk_status(ap);
+	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)
+		goto err_out;
+
+	/* Send the CDB (atapi) or the first data block (ata pio out).
+	 * During the state transition, interrupt handler shouldn't
+	 * be invoked before the data transfer is complete and
+	 * hsm_task_state is changed. Hence, the following locking.
+	 */
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+
+	if (qc->tf.protocol == ATA_PROT_PIO) {
+		/* PIO data out protocol.
+		 * 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().
+		 */
+		ap->hsm_task_state = HSM_ST;
+		ata_pio_sector(qc);
+		ata_altstatus(ap); /* flush */
+	} else
+		/* send CDB */
+		atapi_send_cdb(ap, qc);
+
+	/* 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;
+
+err_out:
+	ata_pio_error(ap);
+}
+
+/**
  *	__atapi_pio_bytes - Transfer data from/to the ATAPI device.
  *	@qc: Command on going
  *	@bytes: number of bytes
@@ -3695,42 +3803,6 @@ void ata_bmdma_stop(struct ata_queued_cm
 }
 
 /**
- *	atapi_send_cdb - Write CDB bytes to hardware
- *	@ap: Port to which ATAPI device is attached.
- *	@qc: Taskfile currently active
- *
- *	When device has indicated its readiness to accept
- *	a CDB, this function is called.  Send the CDB.
- *
- *	LOCKING:
- *	caller.
- */
-
-static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
-{
-	/* send SCSI cdb */
-	DPRINTK("send cdb\n");
-	assert(ap->cdb_len >= 12);
-
-	ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
-	ata_altstatus(ap); /* flush */
-
-	switch (qc->tf.protocol) {
-	case ATA_PROT_ATAPI:
-		ap->hsm_task_state = HSM_ST;
-		break;
-	case ATA_PROT_ATAPI_NODATA:
-		ap->hsm_task_state = HSM_ST_LAST;
-		break;
-	case ATA_PROT_ATAPI_DMA:
-		ap->hsm_task_state = HSM_ST_LAST;
-		/* initiate bmdma */
-		ap->ops->bmdma_start(qc);
-		break;
-	}
-}
-
-/**
  *	ata_host_intr - Handle host interrupt for given (port, task)
  *	@ap: Port on which interrupt arrived (possibly...)
  *	@qc: Taskfile currently active in engine
@@ -3952,79 +4024,6 @@ irqreturn_t ata_interrupt (int irq, void
 }
 
 /**
- *	ata_dataout_task - Write first data block to hardware
- *	@_data: 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 
- *	the first data block by PIO.
- *	After this, 
- *	  - If polling, ata_pio_task() handles the rest.
- *	  - Otherwise, interrupt handler takes over.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep)
- */
-
-static void ata_dataout_task(void *_data)
-{
-	struct ata_port *ap = _data;
-	struct ata_queued_cmd *qc;
-	u8 status;
-	unsigned long flags;
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	assert(qc != NULL);
-	assert(qc->flags & ATA_QCFLAG_ACTIVE);
-
-	/* sleep-wait for BSY to clear */
-	DPRINTK("busy wait\n");
-	if (ata_busy_sleep(ap, ATA_TMOUT_DATAOUT_QUICK, ATA_TMOUT_DATAOUT))
-		goto err_out;
-
-	/* make sure DRQ is set */
-	status = ata_chk_status(ap);
-	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)
-		goto err_out;
-
-	/* Send the CDB (atapi) or the first data block (ata pio out).
-	 * During the state transition, interrupt handler shouldn't
-	 * be invoked before the data transfer is complete and
-	 * hsm_task_state is changed. Hence, the following locking.
-	 */
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-
-	if (qc->tf.protocol == ATA_PROT_PIO) {
-		/* PIO data out protocol.
-		 * 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().
-		 */
-		ap->hsm_task_state = HSM_ST;
-		ata_pio_sector(qc);
-		ata_altstatus(ap); /* flush */
-	} else
-		/* send CDB */
-		atapi_send_cdb(ap, qc);
-
-	/* 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;
-
-err_out:
-	ata_pio_error(ap);
-}
-
-
-/**
  *	ata_port_start - Set port up for dma.
  *	@ap: Port to initialize
  *
author
#31 lore raw

PATCH 2/4 irq-pio: remove ap->dataout_task

In reply to Albert Lee

Patch 2/4: remove ap->dataout_task

changes:
- remove ap->dataout_task
- let ata_pio_task() handle the HSM_ST_FIRST state.
- rename ata_dataout_task() to ata_pio_first_block()

For your review, thanks.

Albert

include/linux/libata.h +0 -2
@@ -319,8 +319,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;
drivers/scsi/libata-core.c +12 -10
@@ -2732,8 +2732,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 
@@ -2746,9 +2746,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;
@@ -3018,7 +3017,8 @@ fsm_start:
 	qc_completed = 0;
 
 	switch (ap->hsm_task_state) {
-	case HSM_ST_IDLE:
+	case HSM_ST_FIRST:
+		ata_pio_first_block(ap);
 		return;
 
 	case HSM_ST:
@@ -3038,6 +3038,9 @@ fsm_start:
 	case HSM_ST_ERR:
 		ata_pio_error(ap);
 		return;
+	default:
+		BUG();
+		return;
 	}
 
 	if (timeout)
@@ -3518,10 +3521,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 */
@@ -3548,7 +3551,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:
@@ -3560,7 +3563,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:
@@ -4141,7 +4144,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++)
#32 lore raw

Re: [PATCH/RFC 2/4] irq-pio: remove ap->dataout_task

drivers/scsi/libata-core.c fsm_start: · line 3024

Quoted 10 lines · Albert Lee
drivers/scsi/libata-core.c +2 -1
@@ -3018,7 +3017,8 @@ fsm_start:
 	qc_completed = 0;
 
 	switch (ap->hsm_task_state) {
-	case HSM_ST_IDLE:
+	case HSM_ST_FIRST:
+		ata_pio_first_block(ap);
 		return;
 
 	case HSM_ST:

Am I missing something? You're removing the handling of HSM_ST_IDLE, which looks very wrong.

AFAICS, even after your patch is applied, the completion of a qc still transitions the state to HSM_ST_IDLE. Perhaps your patch is missing some transitions -from- HSM_ST_IDLE to HSM_ST_FIRST?

Jeff

author
#33 lore raw

Re: [PATCH/RFC 2/4] irq-pio: remove ap->dataout_task

Quoted 11 lines · Jeff Garzik
Quoted 9 lines · Albert Lee
drivers/scsi/libata-core.c +2 -1
@@ -3018,7 +3017,8 @@ fsm_start:
     qc_completed = 0;
 
     switch (ap->hsm_task_state) {
-    case HSM_ST_IDLE:
+    case HSM_ST_FIRST:
+        ata_pio_first_block(ap);
         return;
 
Quoted 4 lines

drivers/scsi/libata-core.c fsm_start: · line 3024

Quoted 1 line
drivers/scsi/libata-core.c +0 -0
     case HSM_ST:

Am I missing something? You're removing the handling of HSM_ST_IDLE, which looks very wrong.

The qc_completed flag is introduced to replace the volatile HSM_ST_IDLE state.
If we access the port when ap->hsm_task_state is HSM_ST_IDLE, the command is completed, next qc might be running on the same port and changing ap->hsm_task_state. So, we cannot rely on ap->hsm_task_state when it is HSM_ST_IDLE.

Quoted 3 lines

AFAICS, even after your patch is applied, the completion of a qc still transitions the state to HSM_ST_IDLE. Perhaps your patch is missing some transitions -from- HSM_ST_IDLE to HSM_ST_FIRST?

Yes, the completion of a qc still transitions the state to HSM_ST_IDLE. But we need another way - the qc_completed flag to finish the polling loop.

Albert

author
#34 lore raw

PATCH 3/4 irq-pio: integrate ata_pio_first_block() with ata_pio_task()

In reply to Albert Lee

Patch 3/4: integrate ata_pio_first_block() with ata_pio_task()

Changes:
- change the return value of ata_pio_complete()
- add return value for ata_pio_first_block() to avoid queuing
another ata_pio_task()
- rename variable "qc_completed" to "has_next" in ata_pio_task()

For your review, thanks.

Albert

drivers/scsi/libata-core.c +30 -20
@@ -2469,7 +2469,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)
@@ -2491,14 +2492,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);
@@ -2510,7 +2511,7 @@ static int ata_pio_complete (struct ata_
 
 	/* another command may start at this point */
 
-	return 1;
+	return 0;
 }
 
 
@@ -2744,27 +2745,39 @@ 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);
 
+	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
@@ -2788,18 +2801,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;
 }
 
 /**
@@ -3010,23 +3020,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:
@@ -3045,7 +3055,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;
 }
 
#35 lore raw

Re: [PATCH/RFC 3/4] irq-pio: integrate ata_pio_first_block() with ata_pio_task()

Quoted 7 lines · Albert Lee

Patch 3/4: integrate ata_pio_first_block() with ata_pio_task()

Changes:
- change the return value of ata_pio_complete()
- add return value for ata_pio_first_block() to avoid queuing another
ata_pio_task()
- rename variable "qc_completed" to "has_next" in ata_pio_task()

seems OK.

not applied since patch #2 was not applied.

author
#36 lore raw

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

In reply to Albert Lee

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

include/linux/ata.h +12 -0
@@ -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))
include/linux/libata.h +2 -0
@@ -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 {
drivers/scsi/libata-core.c +77 -63

196 lines not on this page · raw message

#37 lore raw

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

Quoted 2 lines · Albert Lee

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

Signature · 3 lines
Mark Lord
Real-Time Remedies Inc.
[email protected]
#38 lore raw

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

Quoted 16 lines · Mark Lord
Quoted 2 lines · Albert Lee

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:

Quoted 7 lines

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...

Quoted 5 lines

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.

Quoted 4 lines

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

author
#39 lore raw

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

Jeff,

Quoted 27 lines
Quoted 16 lines · Mark Lord
Quoted 2 lines · Albert Lee

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:

Quoted 7 lines

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

Quoted 8 lines

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

#40 lore raw

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

Quoted 2 lines · Albert Lee

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

author
#41 lore raw

cover libata: CHS follow-up patches

Dear all,

CHS follow-up patches:

patch 1/4: white space, comments and debug message beautification

patch 2/4: tidy up SCSI lba and transfer length calculation

patch 3/4: add CHS support to ata_scsi_start_stop_xlat()

patch 4/4: calculate LBA28/LBA48 commands and protocol on the fly

Patch against libata-dev upstream branch (a1213499b0ef75d8c627b461047805a235c9dd00).

For your review and advice, thanks.

Albert

author
#42 lore raw

PATCH 1/4 CHS: white space beautification

Patch 1/4:
white space, comments and debug message beautification

For your review, thanks.

Albert

drivers/scsi/libata-scsi.c +17 -8
@@ -589,7 +589,8 @@ static unsigned int ata_scsi_verify_xlat
 		head  = track % dev->heads;
 		sect  = (u32)block % dev->sectors + 1;
 
-		DPRINTK("block[%u] track[%u] cyl[%u] head[%u] sect[%u] \n", (u32)block, track, cyl, head, sect);
+		DPRINTK("block %u track %u cyl %u head %u sect %u\n",
+			(u32)block, track, cyl, head, sect);
 		
 		/* Check whether the converted CHS can fit. 
 		   Cylinder: 0-65535 
@@ -665,6 +666,10 @@ static unsigned int ata_scsi_rw_xlat(str
 		block |= ((u64)scsicmd[3]);
 
 		n_block |= ((u32)scsicmd[4]);
+
+		/* for 6-byte r/w commands, transfer length 0
+		 * means 256 blocks of data, not 0 block.
+		 */
 		if (!n_block)
 			n_block = 256;
 	
@@ -692,7 +697,11 @@ static unsigned int ata_scsi_rw_xlat(str
 
 	/* Check and compose ATA command */
 	if (!n_block)
-		/* In ATA, sector count 0 means 256 or 65536 sectors, not 0 sectors. */
+		/* For 10-byte and 16-byte SCSI R/W commands, transfer
+		 * length 0 means transfer 0 block of data.
+		 * However, for ATA R/W commands, sector count 0 means
+		 * 256 or 65536 sectors, not 0 sectors as in SCSI.
+		 */
 		return 1;
 
 	if (lba) {
@@ -715,7 +724,7 @@ static unsigned int ata_scsi_rw_xlat(str
 
 			tf->device |= (block >> 24) & 0xf;
 		}
-	
+
 		qc->nsect = n_block;
 		tf->nsect = n_block & 0xff;
 
@@ -731,23 +740,23 @@ static unsigned int ata_scsi_rw_xlat(str
 		/* The request -may- be too large for CHS addressing. */
 		if ((block >> 28) || (n_block > 256))
 			return 1;
-			
+
 		/* Convert LBA to CHS */
 		track = (u32)block / dev->sectors;
 		cyl   = track / dev->heads;
 		head  = track % dev->heads;
 		sect  = (u32)block % dev->sectors + 1;
 
-		DPRINTK("block[%u] track[%u] cyl[%u] head[%u] sect[%u] \n", 
+		DPRINTK("block %u track %u cyl %u head %u sect %u\n",
 			(u32)block, track, cyl, head, sect);
-		
+
 		/* Check whether the converted CHS can fit. 
 		   Cylinder: 0-65535 
 		   Head: 0-15
 		   Sector: 1-255*/
-		if ((cyl >> 16) || (head >> 4) || (sect >> 8) || (!sect)) 
+		if ((cyl >> 16) || (head >> 4) || (sect >> 8) || (!sect))
 			return 1;
-		
+
 		qc->nsect = n_block;
 		tf->nsect = n_block & 0xff; /* Sector count 0 means 256 sectors */
 		tf->lbal = sect;
#43 lore raw

Re: [PATCH/RFC 1/4] CHS: white space beautification

applied patches 1-3.

Can you please work out an email solution that doesn't involve attaching patches? I have to manually each patch, rather than running the git-applymbox script.

author
#44 lore raw

PATCH 2/4 CHS: tidy up SCSI lba and transfer length calculation

In reply to Albert Lee

Patch 2/4: tidy up SCSI lba and transfer length calculation

changes:
- move the redundant SCSI lba and transfer length calculation code
from ata_scsi_verify_xlat() and ata_scsi_rw_xlat() to common functions.

For your review, thanks.

Albert

drivers/scsi/libata-scsi.c +115 -64
@@ -488,6 +488,99 @@ static unsigned int ata_scsi_flush_xlat(
 }
 
 /**
+ *	scsi_6_lba_len - Get LBA and transfer length
+ *	@scsicmd: SCSI command to translate
+ *
+ *	Calculate LBA and transfer length for 6-byte commands.
+ *
+ *	RETURNS:
+ *	@plba: the LBA
+ *	@plen: the transfer length
+ */
+
+static void scsi_6_lba_len(u8 *scsicmd, u64 *plba, u32 *plen)
+{
+	u64 lba = 0;
+	u32 len = 0;
+
+	VPRINTK("six-byte command\n");
+
+	lba |= ((u64)scsicmd[2]) << 8;
+	lba |= ((u64)scsicmd[3]);
+
+	len |= ((u32)scsicmd[4]);
+
+	*plba = lba;
+	*plen = len;
+}
+
+/**
+ *	scsi_10_lba_len - Get LBA and transfer length
+ *	@scsicmd: SCSI command to translate
+ *
+ *	Calculate LBA and transfer length for 10-byte commands.
+ *
+ *	RETURNS:
+ *	@plba: the LBA
+ *	@plen: the transfer length
+ */
+
+static void scsi_10_lba_len(u8 *scsicmd, u64 *plba, u32 *plen)
+{
+	u64 lba = 0;
+	u32 len = 0;
+
+	VPRINTK("ten-byte command\n");
+
+	lba |= ((u64)scsicmd[2]) << 24;
+	lba |= ((u64)scsicmd[3]) << 16;
+	lba |= ((u64)scsicmd[4]) << 8;
+	lba |= ((u64)scsicmd[5]);
+
+	len |= ((u32)scsicmd[7]) << 8;
+	len |= ((u32)scsicmd[8]);
+
+	*plba = lba;
+	*plen = len;
+}
+
+/**
+ *	scsi_16_lba_len - Get LBA and transfer length
+ *	@scsicmd: SCSI command to translate
+ *
+ *	Calculate LBA and transfer length for 16-byte commands.
+ *
+ *	RETURNS:
+ *	@plba: the LBA
+ *	@plen: the transfer length
+ */
+
+static void scsi_16_lba_len(u8 *scsicmd, u64 *plba, u32 *plen)
+{
+	u64 lba = 0;
+	u32 len = 0;
+
+	VPRINTK("sixteen-byte command\n");
+
+	lba |= ((u64)scsicmd[2]) << 56;
+	lba |= ((u64)scsicmd[3]) << 48;
+	lba |= ((u64)scsicmd[4]) << 40;
+	lba |= ((u64)scsicmd[5]) << 32;
+	lba |= ((u64)scsicmd[6]) << 24;
+	lba |= ((u64)scsicmd[7]) << 16;
+	lba |= ((u64)scsicmd[8]) << 8;
+	lba |= ((u64)scsicmd[9]);
+
+	len |= ((u32)scsicmd[10]) << 24;
+	len |= ((u32)scsicmd[11]) << 16;
+	len |= ((u32)scsicmd[12]) << 8;
+	len |= ((u32)scsicmd[13]);
+
+	*plba = lba;
+	*plen = len;
+}
+
+/**
  *	ata_scsi_verify_xlat - Translate SCSI VERIFY command into an ATA one
  *	@qc: Storage for translated ATA taskfile
  *	@scsicmd: SCSI command to translate
@@ -508,38 +601,16 @@ static unsigned int ata_scsi_verify_xlat
 	unsigned int lba   = tf->flags & ATA_TFLAG_LBA;
 	unsigned int lba48 = tf->flags & ATA_TFLAG_LBA48;
 	u64 dev_sectors = qc->dev->n_sectors;
-	u64 block = 0;
-	u32 n_block = 0;
+	u64 block;
+	u32 n_block;
 
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf->protocol = ATA_PROT_NODATA;
 
-	if (scsicmd[0] == VERIFY) {
-		block |= ((u64)scsicmd[2]) << 24;
-		block |= ((u64)scsicmd[3]) << 16;
-		block |= ((u64)scsicmd[4]) << 8;
-		block |= ((u64)scsicmd[5]);
-
-		n_block |= ((u32)scsicmd[7]) << 8;
-		n_block |= ((u32)scsicmd[8]);
-	}
-
-	else if (scsicmd[0] == VERIFY_16) {
-		block |= ((u64)scsicmd[2]) << 56;
-		block |= ((u64)scsicmd[3]) << 48;
-		block |= ((u64)scsicmd[4]) << 40;
-		block |= ((u64)scsicmd[5]) << 32;
-		block |= ((u64)scsicmd[6]) << 24;
-		block |= ((u64)scsicmd[7]) << 16;
-		block |= ((u64)scsicmd[8]) << 8;
-		block |= ((u64)scsicmd[9]);
-
-		n_block |= ((u32)scsicmd[10]) << 24;
-		n_block |= ((u32)scsicmd[11]) << 16;
-		n_block |= ((u32)scsicmd[12]) << 8;
-		n_block |= ((u32)scsicmd[13]);
-	}
-
+	if (scsicmd[0] == VERIFY)
+		scsi_10_lba_len(scsicmd, &block, &n_block);
+	else if (scsicmd[0] == VERIFY_16)
+		scsi_16_lba_len(scsicmd, &block, &n_block);
 	else
 		return 1;
 
@@ -636,8 +707,8 @@ static unsigned int ata_scsi_rw_xlat(str
 	struct ata_device *dev = qc->dev;
 	unsigned int lba   = tf->flags & ATA_TFLAG_LBA;
 	unsigned int lba48 = tf->flags & ATA_TFLAG_LBA48;
-	u64 block = 0;
-	u32 n_block = 0;
+	u64 block;
+	u32 n_block;
 
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf->protocol = qc->dev->xfer_protocol;
@@ -651,46 +722,26 @@ static unsigned int ata_scsi_rw_xlat(str
 	}
 
 	/* Calculate the SCSI LBA and transfer length. */
-	if (scsicmd[0] == READ_10 || scsicmd[0] == WRITE_10) {
-		block |= ((u64)scsicmd[2]) << 24;
-		block |= ((u64)scsicmd[3]) << 16;
-		block |= ((u64)scsicmd[4]) << 8;
-		block |= ((u64)scsicmd[5]);
-
-		n_block |= ((u32)scsicmd[7]) << 8;
-		n_block |= ((u32)scsicmd[8]);
-
-		VPRINTK("ten-byte command\n");
-	} else if (scsicmd[0] == READ_6 || scsicmd[0] == WRITE_6) {
-		block |= ((u64)scsicmd[2]) << 8;
-		block |= ((u64)scsicmd[3]);
-
-		n_block |= ((u32)scsicmd[4]);
+	switch (scsicmd[0]) {
+	case READ_10:
+	case WRITE_10:
+		scsi_10_lba_len(scsicmd, &block, &n_block);
+		break;
+	case READ_6:
+	case WRITE_6:
+		scsi_6_lba_len(scsicmd, &block, &n_block);
 
 		/* for 6-byte r/w commands, transfer length 0
 		 * means 256 blocks of data, not 0 block.
 		 */
 		if (!n_block)
 			n_block = 256;
-	
-		VPRINTK("six-byte command\n");
-	} else if (scsicmd[0] == READ_16 || scsicmd[0] == WRITE_16) {
-		block |= ((u64)scsicmd[2]) << 56;
-		block |= ((u64)scsicmd[3]) << 48;
-		block |= ((u64)scsicmd[4]) << 40;
-		block |= ((u64)scsicmd[5]) << 32;
-		block |= ((u64)scsicmd[6]) << 24;
-		block |= ((u64)scsicmd[7]) << 16;
-		block |= ((u64)scsicmd[8]) << 8;
-		block |= ((u64)scsicmd[9]);
-
-		n_block |= ((u32)scsicmd[10]) << 24;
-		n_block |= ((u32)scsicmd[11]) << 16;
-		n_block |= ((u32)scsicmd[12]) << 8;
-		n_block |= ((u32)scsicmd[13]);
-
-		VPRINTK("sixteen-byte command\n");
-	} else {
+		break;
+	case READ_16:
+	case WRITE_16:
+		scsi_16_lba_len(scsicmd, &block, &n_block);
+		break;
+	default:
 		DPRINTK("no-byte command\n");
 		return 1;
 	}
author
#45 lore raw

PATCH 3/4 CHS: add CHS support to ata_scsi_start_stop_xlat()

In reply to Albert Lee

Patch 3/4: add CHS support to ata_scsi_start_stop_xlat()

For your review, thanks.

Albert

drivers/scsi/libata-scsi.c +15 -4
@@ -435,10 +435,21 @@ static unsigned int ata_scsi_start_stop_
 		return 1;	/* power conditions not supported */
 	if (scsicmd[4] & 0x1) {
 		tf->nsect = 1;	/* 1 sector, lba=0 */
-		tf->lbah = 0x0;
-		tf->lbam = 0x0;
-		tf->lbal = 0x0;
-		tf->device |= ATA_LBA;
+
+		if (qc->dev->flags & ATA_DFLAG_LBA) {
+			qc->tf.flags |= ATA_TFLAG_LBA;
+
+			tf->lbah = 0x0;
+			tf->lbam = 0x0;
+			tf->lbal = 0x0;
+			tf->device |= ATA_LBA;
+		} else {
+			/* CHS */
+			tf->lbal = 0x1; /* sect */
+			tf->lbam = 0x0; /* cyl low */
+			tf->lbah = 0x0; /* cyl high */
+		}
+
 		tf->command = ATA_CMD_VERIFY;	/* READ VERIFY */
 	} else {
 		tf->nsect = 0;	/* time period value (0 implies now) */
author
#46 lore raw

PATCH 4/4 CHS: calculate LBA28/LBA48 commands and protocol on the fly

In reply to Albert Lee

Patch 4/4: calculate LBA28/LBA48 commands and protocol on the fly

Changes:
- calculate LBA28/LBA48 commands and protocol on the fly per Mark
and Jeff's advice
- add ATA_CMD_READ_MULTI, etc. to ata.h
- add multi_count to ata_device (just pave the road for r/w multiple)
- removed xfer_protocol, read_cmd and write_cmd from ata_device.
- merge ata_prot_to_cmd() and ata_dev_set_protocol() and move it to
libata-scsi.c

For your review, thanks.

Albert

include/linux/ata.h +14 -0
@@ -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,
@@ -287,4 +291,14 @@ static inline int ata_ok(u8 status)
 			== ATA_DRDY);
 }
 
+static inline int lba_28_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 28)) && (n_block <= 256);
+}
+
+static inline int lba_48_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 48)) && (n_block <= 65536);
+}
+
 #endif /* __LINUX_ATA_H__ */
include/linux/libata.h +2 -4
@@ -279,10 +279,8 @@ struct ata_device {
 	u8			xfer_mode;
 	unsigned int		xfer_shift;	/* ATA_SHIFT_xxx */
 
-	/* cache info about current transfer mode */
-	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 */
 
 	/* for CHS addressing */
 	u16			cylinders;	/* Number of cylinders */
drivers/scsi/libata-core.c +1 -88

113 lines not on this page · raw message

drivers/scsi/libata-scsi.c +91 -39

201 lines not on this page · raw message

#47 lore raw

Re: [PATCH/RFC 4/4] CHS: calculate LBA28/LBA48 commands and protocol on the fly

Quoted 10 lines · Albert Lee

Patch 4/4: calculate LBA28/LBA48 commands and protocol on the fly

Changes:
- calculate LBA28/LBA48 commands and protocol on the fly per Mark and
Jeff's advice
- add ATA_CMD_READ_MULTI, etc. to ata.h
- add multi_count to ata_device (just pave the road for r/w multiple)
- removed xfer_protocol, read_cmd and write_cmd from ata_device.
- merge ata_prot_to_cmd() and ata_dev_set_protocol() and move it to
libata-scsi.c

seems OK at first glance, but please move the LBA28/LBA48 optimization change into a separate patch from the rest of these cleanups.

not applied.

author
#48 lore raw

cover libata: more CHS follow-up patches

Dear all,

More CHS follow-up patches:

patch 1/4: move the initialization of ATA_TFLAG_LBA and ATA_TFLAG_LBA48 flags to the SCSI translation functions

patch 2/4: calculate read/write commands and protocols on the fly

patch 3/4: support overriding the ata_rwcmd_protocol() function

patch 4/4: optimize LBA28/LBA48 usage

Patch against libata-dev upstream branch (643736a58d2668af94aee05670c5e9ae76e7b85f).

For your review and advice, thanks.

Albert

author
#49 lore raw

PATCH 1/4 CHS: move the initialization of taskfile LBA flags

Patch 1/4:
move the initialization of taskfile LBA flags:
ATA_TFLAG_LBA and ATA_TFLAG_LBA48 flags
to the SCSI translation functions

For your review, thanks.

Albert

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

drivers/scsi/libata-core.c 2005-10-05 18:10:28.000000000 +0800 renamed from drivers/scsi/libata-core.c 2005-10-05 18:04:07.000000000 +0800 +0 -7
@@ -3241,13 +3241,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
        qc->nbytes = qc->curbytes = 0;
 
        ata_tf_init(ap, &qc->tf, dev->devno);
-
-        if (dev->flags & ATA_DFLAG_LBA) {
-            qc->tf.flags |= ATA_TFLAG_LBA;
-
-            if (dev->flags & ATA_DFLAG_LBA48)
-                qc->tf.flags |= ATA_TFLAG_LBA48;
-        }
    }
 
    return qc;
drivers/scsi/libata-scsi.c 2005-10-05 18:10:28.000000000 +0800 renamed from drivers/scsi/libata-scsi.c 2005-10-05 18:04:07.000000000 +0800 +1 -1
@@ -489,7 +489,7 @@ static unsigned int ata_scsi_flush_xlat(
    tf->flags |= ATA_TFLAG_DEVICE;
    tf->protocol = ATA_PROT_NODATA;
 
-    if ((tf->flags & ATA_TFLAG_LBA48) &&
+    if ((qc->dev->flags & ATA_DFLAG_LBA48) &&
        (ata_id_has_flush_ext(qc->dev->id)))
        tf->command = ATA_CMD_FLUSH_EXT;
    else
@@ -609,8 +609,6 @@ static unsigned int ata_scsi_verify_xlat
{
    struct ata_taskfile *tf = &qc->tf;
    struct ata_device *dev = qc->dev;
-    unsigned int lba   = tf->flags & ATA_TFLAG_LBA;
-    unsigned int lba48 = tf->flags & ATA_TFLAG_LBA48;
    u64 dev_sectors = qc->dev->n_sectors;
    u64 block;
    u32 n_block;
@@ -631,16 +629,16 @@ static unsigned int ata_scsi_verify_xlat
        return 1;
    if ((block + n_block) > dev_sectors)
        return 1;

- if (lba48) {
- if (n_block > (64 * 1024))
- return 1;
- } else {
- if (n_block > 256)
- return 1;
- }

- if (lba) {
- if (lba48) {
+ if (dev->flags & ATA_DFLAG_LBA) {
+ tf->flags |= ATA_TFLAG_LBA;
+
+ if (dev->flags & ATA_DFLAG_LBA48) {
+ if (n_block > (64 * 1024))
+ return 1;
+
+ /* use LBA48 */

+            tf->flags |= ATA_TFLAG_LBA48;
            tf->command = ATA_CMD_VERIFY_EXT;

            tf->hob_nsect = (n_block >> 8) & 0xff;
@@ -649,8 +647,11 @@ static unsigned int ata_scsi_verify_xlat
            tf->hob_lbam = (block >> 32) & 0xff;
            tf->hob_lbal = (block >> 24) & 0xff;
        } else {

- tf->command = ATA_CMD_VERIFY;
+ if (n_block > 256)
+ return 1;

+ /* use LBA28 */

+            tf->command = ATA_CMD_VERIFY;
            tf->device |= (block >> 24) & 0xf;
        }

@@ -665,6 +666,9 @@ static unsigned int ata_scsi_verify_xlat

        /* CHS */
        u32 sect, head, cyl, track;

+ if (n_block > 256)
+ return 1;
+

        /* Convert LBA to CHS */
        track = (u32)block / dev->sectors;
        cyl   = track / dev->heads;
@@ -716,8 +720,6 @@ static unsigned int ata_scsi_rw_xlat(str
{
    struct ata_taskfile *tf = &qc->tf;
    struct ata_device *dev = qc->dev;
-    unsigned int lba   = tf->flags & ATA_TFLAG_LBA;
-    unsigned int lba48 = tf->flags & ATA_TFLAG_LBA48;
    u64 block;
    u32 n_block;

@@ -766,19 +768,24 @@ static unsigned int ata_scsi_rw_xlat(str

         */
        return 1;

- if (lba) {
- if (lba48) {
+ if (dev->flags & ATA_DFLAG_LBA) {
+ tf->flags |= ATA_TFLAG_LBA;
+

+        if (dev->flags & ATA_DFLAG_LBA48) {
            /* The request -may- be too large for LBA48. */
            if ((block >> 48) || (n_block > 65536))
                return 1;

+ /* use LBA48 */
+ tf->flags |= ATA_TFLAG_LBA48;
+

            tf->hob_nsect = (n_block >> 8) & 0xff;

            tf->hob_lbah = (block >> 40) & 0xff;
            tf->hob_lbam = (block >> 32) & 0xff;
            tf->hob_lbal = (block >> 24) & 0xff;
        } else {

- /* LBA28 */
+ /* use LBA28 */

            /* The request -may- be too large for LBA28. */
            if ((block >> 28) || (n_block > 256))
author
#50 lore raw

PATCH 1/4 CHS: move the initialization of taskfile LBA flags (resend)

In reply to Albert Lee

The tab was expanded to spaces in the previous mail. :( Hope this time it is correct.

Patch 1/4:
move the initialization of taskfile LBA flags:
ATA_TFLAG_LBA and ATA_TFLAG_LBA48 flags
to the SCSI translation functions

For your review, thanks.

Albert

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

drivers/scsi/libata-core.c +0 -7
@@ -3241,13 +3241,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
 		qc->nbytes = qc->curbytes = 0;
 
 		ata_tf_init(ap, &qc->tf, dev->devno);
-
-		if (dev->flags & ATA_DFLAG_LBA) {
-			qc->tf.flags |= ATA_TFLAG_LBA;
-
-			if (dev->flags & ATA_DFLAG_LBA48)
-				qc->tf.flags |= ATA_TFLAG_LBA48;
-		}
 	}
 
 	return qc;
drivers/scsi/libata-scsi.c +25 -18

97 lines not on this page · raw message

author
#51 lore raw

PATCH 2/4 CHS: calculate read/write commands and protocol on the fly

In reply to Albert Lee

Patch 2/4: calculate read/write commands and protocols on the fly

changes:
- merge ata_prot_to_cmd() and ata_dev_set_protocol() as
ata_rwcmd_protocol()
- pave road for read/write multiple support
- remove usage of pre-cached command and protocol values and call
ata_rwcmd_protocol() instead

For your review, thanks.

Albert

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

include/linux/ata.h 2005-10-05 18:11:12.000000000 +0800 renamed from include/linux/ata.h 2005-10-05 18:04:16.000000000 +0800 +4 -0
@@ -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,
include/linux/libata.h 2005-10-05 18:11:12.000000000 +0800 renamed from include/linux/libata.h 2005-10-05 18:04:16.000000000 +0800 +2 -4
@@ -283,10 +283,8 @@ struct ata_device {
    u8            xfer_mode;
    unsigned int        xfer_shift;    /* ATA_SHIFT_xxx */
 
-    /* cache info about current transfer mode */
-    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 */
 
    /* for CHS addressing */
    u16            cylinders;    /* Number of cylinders */
drivers/scsi/libata.h 2005-10-05 18:11:12.000000000 +0800 renamed from drivers/scsi/libata.h 2005-10-05 18:04:07.000000000 +0800 +0 -0
@@ -41,6 +41,7 @@ struct ata_scsi_args {

extern int atapi_enabled;
extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,

                      struct ata_device *dev);

+extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc); extern void ata_qc_free(struct ata_queued_cmd *qc); extern int ata_qc_issue(struct ata_queued_cmd *qc); extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);

drivers/scsi/libata-core.c 2005-10-05 18:11:12.000000000 +0800 renamed from drivers/scsi/libata-core.c 2005-10-05 18:10:28.000000000 +0800 +0 -0
@@ -616,79 +616,68 @@ void ata_tf_from_fis(u8 *fis, struct ata
    tf->hob_nsect    = fis[13];

}

+static const u8 ata_rw_cmds[] = {

+    /* pio multi */
+    ATA_CMD_READ_MULTI,
+    ATA_CMD_WRITE_MULTI,
+    ATA_CMD_READ_MULTI_EXT,
+    ATA_CMD_WRITE_MULTI_EXT,
+    /* pio */
+    ATA_CMD_PIO_READ,
+    ATA_CMD_PIO_WRITE,
+    ATA_CMD_PIO_READ_EXT,
+    ATA_CMD_PIO_WRITE_EXT,
+    /* dma */
+    ATA_CMD_READ,
+    ATA_CMD_WRITE,
+    ATA_CMD_READ_EXT,
+    ATA_CMD_WRITE_EXT
+};
+
/**
- *    ata_prot_to_cmd - determine which read/write opcodes to use
- *    @protocol: ATA_PROT_xxx taskfile protocol
- *    @lba48: true is lba48 is present
+ *    ata_qc_rwcmd_protocol - set taskfile r/w commands and protocol
+ *    @qc: command to examine and configure
 *
- *    Given necessary input, determine which read/write commands
- *    to use to transfer data.
+ *    Examine the device configuration and tf->flags to calculate
+ *    the proper read/write commands and protocol to use.

*
* LOCKING:
- * None.
+ * caller.
*/
-static int ata_prot_to_cmd(int protocol, int lba48) +static void ata_qc_rwcmd_protocol(struct ata_queued_cmd *qc) {
- 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;
+ struct ata_taskfile *tf = &qc->tf;
+ struct ata_device *dev = qc->dev;

- default:
- return -1;
+ int index, lba48, write;
+
+ lba48 = (tf->flags & ATA_TFLAG_LBA48) ? 2 : 0;
+ write = (tf->flags & ATA_TFLAG_WRITE) ? 1 : 0;
+
+ if (dev->flags & ATA_DFLAG_PIO) {
+ tf->protocol = ATA_PROT_PIO;
+ index = dev->multi_count ? 0 : 4;
+ } else {
+ tf->protocol = ATA_PROT_DMA;

+        index = 8;
    }

- return rcmd | (wcmd << 8);
+ tf->command = ata_rw_cmds[index + lba48 + write];
}

/**

- *    ata_dev_set_protocol - set taskfile protocol and r/w commands
- *    @dev: device to examine and configure
+ *    ata_rwcmd_protocol - set taskfile r/w commands and protocol
+ *    @qc: command to examine and configure
 *
- *    Examine the device configuration, after we have
- *    read the identify-device page and configured the
- *    data transfer mode.  Set internal state related to
- *    the ATA taskfile protocol (pio, pio mult, dma, etc.)
- *    and calculate the proper read/write commands to use.
+ *    Examine the device configuration and tf->flags to calculate
+ *    the proper read/write commands and protocol to use.

*
* LOCKING:
* caller.
*/
-static void ata_dev_set_protocol(struct ata_device *dev) +void ata_rwcmd_protocol(struct ata_queued_cmd *qc) {

-    int pio = (dev->flags & ATA_DFLAG_PIO);
-    int lba48 = (dev->flags & ATA_DFLAG_LBA48);
-    int proto, cmd;

-
- if (pio)
- proto = dev->xfer_protocol = ATA_PROT_PIO;
- else
- proto = dev->xfer_protocol = ATA_PROT_DMA;
-
- cmd = ata_prot_to_cmd(proto, lba48);
- if (cmd < 0)
- BUG();
-

-    dev->read_cmd = cmd & 0xff;
-    dev->write_cmd = (cmd >> 8) & 0xff;
+    ata_qc_rwcmd_protocol(qc);

}

static const char * xfer_mode_str[] = {
@@ -1641,7 +1630,7 @@ static void ata_host_set_dma(struct ata_
*/
static void ata_set_mode(struct ata_port *ap)
{
- unsigned int i, xfer_shift;

+    unsigned int xfer_shift;
    u8 xfer_mode;
    int rc;

@@ -1670,11 +1659,6 @@ static void ata_set_mode(struct ata_port

    if (ap->ops->post_set_mode)
        ap->ops->post_set_mode(ap);

- for (i = 0; i < 2; i++) {
- struct ata_device *dev = &ap->device[i];
- ata_dev_set_protocol(dev);
- }
-

    return;

err_out:

drivers/scsi/libata-scsi.c 2005-10-05 18:11:12.000000000 +0800 renamed from drivers/scsi/libata-scsi.c 2005-10-05 18:10:28.000000000 +0800 +6 -7
@@ -724,15 +724,10 @@ static unsigned int ata_scsi_rw_xlat(str
    u32 n_block;
 
    tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-    tf->protocol = qc->dev->xfer_protocol;
 
-    if (scsicmd[0] == READ_10 || scsicmd[0] == READ_6 ||
-        scsicmd[0] == READ_16) {
-        tf->command = qc->dev->read_cmd;
-    } else {
-        tf->command = qc->dev->write_cmd;
+    if (scsicmd[0] == WRITE_10 || scsicmd[0] == WRITE_6 ||
+        scsicmd[0] == WRITE_16)
        tf->flags |= ATA_TFLAG_WRITE;
-    }
 
    /* Calculate the SCSI LBA and transfer length. */
    switch (scsicmd[0]) {
@@ -794,6 +789,8 @@ static unsigned int ata_scsi_rw_xlat(str
            tf->device |= (block >> 24) & 0xf;
        }
 
+        ata_rwcmd_protocol(qc);
+
        qc->nsect = n_block;
        tf->nsect = n_block & 0xff;
 
@@ -810,6 +807,8 @@ static unsigned int ata_scsi_rw_xlat(str
        if ((block >> 28) || (n_block > 256))
            return 1;
 
+        ata_rwcmd_protocol(qc);
+
        /* Convert LBA to CHS */
        track = (u32)block / dev->sectors;
        cyl   = track / dev->heads;
author
#52 lore raw

PATCH 2/4 CHS: calculate read/write commands and protocol on the fly (resend)

In reply to Albert Lee

(Resend for the tab problem)

Patch 2/4: calculate read/write commands and protocols on the fly

changes:
- merge ata_prot_to_cmd() and ata_dev_set_protocol() as
ata_rwcmd_protocol()
- pave road for read/write multiple support
- remove usage of pre-cached command and protocol values and call
ata_rwcmd_protocol() instead

For your review, thanks.

Albert

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

include/linux/ata.h +4 -0
@@ -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,
include/linux/libata.h +2 -4
@@ -283,10 +283,8 @@ struct ata_device {
 	u8			xfer_mode;
 	unsigned int		xfer_shift;	/* ATA_SHIFT_xxx */
 
-	/* cache info about current transfer mode */
-	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 */
 
 	/* for CHS addressing */
 	u16			cylinders;	/* Number of cylinders */
drivers/scsi/libata.h +1 -0
@@ -41,6 +41,7 @@ struct ata_scsi_args {
 extern int atapi_enabled;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
+extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern int ata_qc_issue(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
drivers/scsi/libata-core.c +45 -61

142 lines not on this page · raw message

drivers/scsi/libata-scsi.c +6 -7
@@ -724,15 +724,10 @@ static unsigned int ata_scsi_rw_xlat(str
 	u32 n_block;
 
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	tf->protocol = qc->dev->xfer_protocol;
 
-	if (scsicmd[0] == READ_10 || scsicmd[0] == READ_6 ||
-	    scsicmd[0] == READ_16) {
-		tf->command = qc->dev->read_cmd;
-	} else {
-		tf->command = qc->dev->write_cmd;
+	if (scsicmd[0] == WRITE_10 || scsicmd[0] == WRITE_6 ||
+	    scsicmd[0] == WRITE_16)
 		tf->flags |= ATA_TFLAG_WRITE;
-	}
 
 	/* Calculate the SCSI LBA and transfer length. */
 	switch (scsicmd[0]) {
@@ -794,6 +789,8 @@ static unsigned int ata_scsi_rw_xlat(str
 			tf->device |= (block >> 24) & 0xf;
 		}
 
+		ata_rwcmd_protocol(qc);
+
 		qc->nsect = n_block;
 		tf->nsect = n_block & 0xff;
 
@@ -810,6 +807,8 @@ static unsigned int ata_scsi_rw_xlat(str
 		if ((block >> 28) || (n_block > 256))
 			return 1;
 
+		ata_rwcmd_protocol(qc);
+
 		/* Convert LBA to CHS */
 		track = (u32)block / dev->sectors;
 		cyl   = track / dev->heads;
author
#53 lore raw

PATCH 3/4 CHS: support overriding the ata_rwcmd_protocol() function

In reply to Albert Lee

Patch 3/4:
support overriding the ata_rwcmd_protocol() function for some VIA
adapters per Alan's advice.

For your review, thanks.

Albert

=======

include/linux/libata.h 2005-10-05 18:11:51.000000000 +0800 renamed from include/linux/libata.h 2005-10-05 18:11:12.000000000 +0800 +2 -0
@@ -353,6 +353,8 @@ struct ata_port_operations {
    void (*phy_reset) (struct ata_port *ap);
    void (*post_set_mode) (struct ata_port *ap);
 
+    /* workaround host adapter limitations */
+    void (*rwcmd_protocol) (struct ata_queued_cmd *qc);
    int (*check_atapi_dma) (struct ata_queued_cmd *qc);
 
    void (*bmdma_setup) (struct ata_queued_cmd *qc);
drivers/scsi/libata-core.c 2005-10-05 18:11:51.000000000 +0800 renamed from drivers/scsi/libata-core.c 2005-10-05 18:11:12.000000000 +0800 +0 -0
@@ -677,7 +677,14 @@ static void ata_qc_rwcmd_protocol(struct
 */

void ata_rwcmd_protocol(struct ata_queued_cmd *qc) {
- ata_qc_rwcmd_protocol(qc);
+ /* give LLD a chance to override

+     * ex. for some VIA adapters, LLD have to use
+     *     PIO commands if LBA48 + DMA is seen.
+     */

+ if (qc->ap->ops->rwcmd_protocol)
+ qc->ap->ops->rwcmd_protocol(qc);
+ else
+ ata_qc_rwcmd_protocol(qc);
}

static const char * xfer_mode_str[] = {

author
#54 lore raw

PATCH 3/4 CHS: support overriding the ata_rwcmd_protocol() function (resend)

In reply to Albert Lee

(Resend for the tab problem)
Patch 3/4:
support overriding the ata_rwcmd_protocol() function for some VIA
adapters per Alan's advice.

For your review, thanks.

Albert

=======

include/linux/libata.h +2 -0
@@ -353,6 +353,8 @@ struct ata_port_operations {
 	void (*phy_reset) (struct ata_port *ap);
 	void (*post_set_mode) (struct ata_port *ap);
 
+	/* workaround host adapter limitations */
+	void (*rwcmd_protocol) (struct ata_queued_cmd *qc);
 	int (*check_atapi_dma) (struct ata_queued_cmd *qc);
 
 	void (*bmdma_setup) (struct ata_queued_cmd *qc);
drivers/scsi/libata-core.c +8 -1
@@ -677,7 +677,14 @@ static void ata_qc_rwcmd_protocol(struct
  */
 void ata_rwcmd_protocol(struct ata_queued_cmd *qc)
 {
-	ata_qc_rwcmd_protocol(qc);
+	/* give LLD a chance to override
+	 * ex. for some VIA adapters, LLD have to use
+	 *     PIO commands if LBA48 + DMA is seen.
+	 */
+	if (qc->ap->ops->rwcmd_protocol)
+		qc->ap->ops->rwcmd_protocol(qc);
+	else
+		ata_qc_rwcmd_protocol(qc);
 }
 
 static const char * xfer_mode_str[] = {
author
#55 lore raw

PATCH 4/4 CHS: optimize LBA28/LBA48 usage

In reply to Albert Lee

Patch 4/4: optimize LBA28/LBA48 per Mark's advice.

Changes:
- add lba_28_ok() and lba_48_ok() to ata.h
- use lba_28_ok() for CHS range check
- LBA28/LBA48 optimization

For your review, thanks.

Albert

=====

include/linux/ata.h 2005-10-05 18:12:09.000000000 +0800 renamed from include/linux/ata.h 2005-10-05 18:11:12.000000000 +0800 +0 -0
@@ -291,4 +291,14 @@ static inline int ata_ok(u8 status)
            == ATA_DRDY);

}

+static inline int lba_28_ok(u64 block, u32 n_block) +{
+ return (block < ((u64)1 << 28)) && (n_block <= 256);
+}
+
+static inline int lba_48_ok(u64 block, u32 n_block) +{
+ return (block < ((u64)1 << 48)) && (n_block <= 65536);
+}
+
#endif /* __LINUX_ATA_H__ */

drivers/scsi/libata-scsi.c 2005-10-05 18:12:09.000000000 +0800 renamed from drivers/scsi/libata-scsi.c 2005-10-05 18:11:12.000000000 +0800 +19 -24

79 lines not on this page · raw message

author
#56 lore raw

PATCH 4/4 CHS: optimize LBA28/LBA48 usage (resend)

In reply to Albert Lee

(Resend for the tab problem)

Patch 4/4: optimize LBA28/LBA48 per Mark's advice.

Changes:
- add lba_28_ok() and lba_48_ok() to ata.h
- use lba_28_ok() for CHS range check
- LBA28/LBA48 optimization

For your review, thanks.

Albert

=====

include/linux/ata.h +10 -0
@@ -291,4 +291,14 @@ static inline int ata_ok(u8 status)
 			== ATA_DRDY);
 }
 
+static inline int lba_28_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 28)) && (n_block <= 256);
+}
+
+static inline int lba_48_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 48)) && (n_block <= 65536);
+}
+
 #endif /* __LINUX_ATA_H__ */
drivers/scsi/libata-scsi.c +19 -24

79 lines not on this page · raw message

#57 lore raw

Re: [PATCH/RFC 0/4] libata: more CHS follow-up patches

In reply to Albert Lee
Quoted 7 lines · Albert Lee

Dear all,

More CHS follow-up patches:

patch 1/4: move the initialization of ATA_TFLAG_LBA and ATA_TFLAG_LBA48 flags to the SCSI translation functions

patch 2/4: calculate read/write commands and protocols on the fly

These two patches are OK, but failed to apply, probably due to mailer issues.

Quoted 5 lines

[jgarzik@pretzel libata-dev]$ patch -sp1 < /g/tmp/mbox 1 out of 1 hunk FAILED -- saving rejects to file drivers/scsi/libata-core.c.rej 7 out of 7 hunks FAILED -- saving rejects to file drivers/scsi/libata-scsi.c.rej

patch 3/4: support overriding the ata_rwcmd_protocol() function

Your above patches make it easy enough to add this patch at any time. I would rather wait until Alan is actually using the hook in his code.

Quoted 1 line

patch 4/4: optimize LBA28/LBA48 usage

seems OK, but I would like this to see a lot of testing before getting merged. Once they are mailer-unmunged, I will apply patches #1 and #2 to 'upstream' branch. Then I will clone a branch from upstream called optimize-lba48, and apply patch #4.

I'm worried, for example, that some hardware may not have been tested much in the field with rapid switching between LBA28 and LBA48.

Jeff

author
#58 lore raw

cover libata: CHS follow-up patches (resend #2)

Dear all,

CHS follow-up patches - resend #2 due to whitespace problem in the previous patches.

patch 1/3: move the initialization of ATA_TFLAG_LBA and ATA_TFLAG_LBA48 flags to the SCSI translation functions

patch 2/3: calculate read/write commands and protocols on the fly

patch 3/3: LBA28/LBA48 optimization

Patch against libata-dev upstream branch
(643736a58d2668af94aee05670c5e9ae76e7b85f).

Thanks.

Albert

author
#59 lore raw

PATCH 1/3 CHS: move the initialization of taskfile LBA flags

Patch 1/3:

move the initialization of taskfile LBA flags
"ATA_TFLAG_LBA" and "ATA_TFLAG_LBA48 flags"
to the SCSI translation functions

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

drivers/scsi/libata-core.c +0 -7
@@ -3241,13 +3241,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
 		qc->nbytes = qc->curbytes = 0;
 
 		ata_tf_init(ap, &qc->tf, dev->devno);
-
-		if (dev->flags & ATA_DFLAG_LBA) {
-			qc->tf.flags |= ATA_TFLAG_LBA;
-
-			if (dev->flags & ATA_DFLAG_LBA48)
-				qc->tf.flags |= ATA_TFLAG_LBA48;
-		}
 	}
 
 	return qc;
drivers/scsi/libata-scsi.c +25 -18

97 lines not on this page · raw message

author
#60 lore raw

PATCH 2/3 CHS: calculate read/write commands and protocol on the fly

In reply to Albert Lee

(Resend #2)

Patch 2/3: calculate read/write commands and protocols on the fly

changes:
- merge ata_prot_to_cmd() and ata_dev_set_protocol() as ata_rwcmd_protocol()
- pave road for read/write multiple support
- remove usage of pre-cached command and protocol values and call ata_rwcmd_protocol() instead

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

include/linux/ata.h +4 -0
@@ -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,
include/linux/libata.h +2 -4
@@ -283,10 +283,8 @@ struct ata_device {
 	u8			xfer_mode;
 	unsigned int		xfer_shift;	/* ATA_SHIFT_xxx */
 
-	/* cache info about current transfer mode */
-	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 */
 
 	/* for CHS addressing */
 	u16			cylinders;	/* Number of cylinders */
drivers/scsi/libata.h +1 -0
@@ -41,6 +41,7 @@ struct ata_scsi_args {
 extern int atapi_enabled;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
+extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern int ata_qc_issue(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
drivers/scsi/libata-core.c +38 -69

135 lines not on this page · raw message

drivers/scsi/libata-scsi.c +6 -7
@@ -724,15 +724,10 @@ static unsigned int ata_scsi_rw_xlat(str
 	u32 n_block;
 
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	tf->protocol = qc->dev->xfer_protocol;
 
-	if (scsicmd[0] == READ_10 || scsicmd[0] == READ_6 ||
-	    scsicmd[0] == READ_16) {
-		tf->command = qc->dev->read_cmd;
-	} else {
-		tf->command = qc->dev->write_cmd;
+	if (scsicmd[0] == WRITE_10 || scsicmd[0] == WRITE_6 ||
+	    scsicmd[0] == WRITE_16)
 		tf->flags |= ATA_TFLAG_WRITE;
-	}
 
 	/* Calculate the SCSI LBA and transfer length. */
 	switch (scsicmd[0]) {
@@ -794,6 +789,8 @@ static unsigned int ata_scsi_rw_xlat(str
 			tf->device |= (block >> 24) & 0xf;
 		}
 
+		ata_rwcmd_protocol(qc);
+
 		qc->nsect = n_block;
 		tf->nsect = n_block & 0xff;
 
@@ -810,6 +807,8 @@ static unsigned int ata_scsi_rw_xlat(str
 		if ((block >> 28) || (n_block > 256))
 			return 1;
 
+		ata_rwcmd_protocol(qc);
+
 		/* Convert LBA to CHS */
 		track = (u32)block / dev->sectors;
 		cyl   = track / dev->heads;
author
#61 lore raw

PATCH 3/3 CHS: LBA28/LBA48 optimization

In reply to Albert Lee

(resend #2)

Patch 3/3: LBA28/LBA48 optimization per Mark's advice.

Changes:
- add lba_28_ok() and lba_48_ok() to ata.h
- use lba_28_ok() for CHS range check
- LBA28/LBA48 optimization

=====

include/linux/ata.h +10 -0
@@ -291,4 +291,14 @@ static inline int ata_ok(u8 status)
 			== ATA_DRDY);
 }
 
+static inline int lba_28_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 28)) && (n_block <= 256);
+}
+
+static inline int lba_48_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 48)) && (n_block <= 65536);
+}
+
 #endif /* __LINUX_ATA_H__ */
drivers/scsi/libata-scsi.c +19 -24

79 lines not on this page · raw message

author
#62 lore raw

cover libata: CHS follow-up patches (resend #4)

In reply to Albert Lee

(Resend #4, mailnews.wraplength set to 0 to keep Thunderbird from trimming trailing whitespaces.
Sorry for all the noises.)

CHS follow-up patches.

patch 1/3: move the initialization of ATA_TFLAG_LBA and ATA_TFLAG_LBA48 flags to the SCSI translation functions

patch 2/3: calculate read/write commands and protocols on the fly

patch 3/3: LBA28/LBA48 optimization

Patch against libata-dev upstream branch
(643736a58d2668af94aee05670c5e9ae76e7b85f).

Thanks.

Albert

author
#63 lore raw

PATCH 1/3 CHS: move the initialization of taskfile LBA flags (resend #4)

(resend #4)

Patch 1/3:

move the initialization of taskfile LBA flags
"ATA_TFLAG_LBA" and "ATA_TFLAG_LBA48 flags"
to the SCSI translation functions

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

drivers/scsi/libata-core.c +0 -7
@@ -3241,13 +3241,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
 		qc->nbytes = qc->curbytes = 0;
 
 		ata_tf_init(ap, &qc->tf, dev->devno);
-
-		if (dev->flags & ATA_DFLAG_LBA) {
-			qc->tf.flags |= ATA_TFLAG_LBA;
-
-			if (dev->flags & ATA_DFLAG_LBA48)
-				qc->tf.flags |= ATA_TFLAG_LBA48;
-		}
 	}
 
 	return qc;
drivers/scsi/libata-scsi.c +25 -18

97 lines not on this page · raw message

author
#64 lore raw

PATCH 2/3 CHS: calculate read/write commands and protocol on the fly (resend #4)

In reply to Albert Lee

(Resend #4)

Patch 2/3: calculate read/write commands and protocols on the fly

changes:
- merge ata_prot_to_cmd() and ata_dev_set_protocol() as ata_rwcmd_protocol()
- pave road for read/write multiple support
- remove usage of pre-cached command and protocol values and call ata_rwcmd_protocol() instead

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

include/linux/ata.h +4 -0
@@ -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,
include/linux/libata.h +2 -4
@@ -283,10 +283,8 @@ struct ata_device {
 	u8			xfer_mode;
 	unsigned int		xfer_shift;	/* ATA_SHIFT_xxx */
 
-	/* cache info about current transfer mode */
-	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 */
 
 	/* for CHS addressing */
 	u16			cylinders;	/* Number of cylinders */
drivers/scsi/libata.h +1 -0
@@ -41,6 +41,7 @@ struct ata_scsi_args {
 extern int atapi_enabled;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
+extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern int ata_qc_issue(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
drivers/scsi/libata-core.c +38 -69

135 lines not on this page · raw message

drivers/scsi/libata-scsi.c +6 -7
@@ -724,15 +724,10 @@ static unsigned int ata_scsi_rw_xlat(str
 	u32 n_block;
 
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	tf->protocol = qc->dev->xfer_protocol;
 
-	if (scsicmd[0] == READ_10 || scsicmd[0] == READ_6 ||
-	    scsicmd[0] == READ_16) {
-		tf->command = qc->dev->read_cmd;
-	} else {
-		tf->command = qc->dev->write_cmd;
+	if (scsicmd[0] == WRITE_10 || scsicmd[0] == WRITE_6 ||
+	    scsicmd[0] == WRITE_16)
 		tf->flags |= ATA_TFLAG_WRITE;
-	}
 
 	/* Calculate the SCSI LBA and transfer length. */
 	switch (scsicmd[0]) {
@@ -794,6 +789,8 @@ static unsigned int ata_scsi_rw_xlat(str
 			tf->device |= (block >> 24) & 0xf;
 		}
 
+		ata_rwcmd_protocol(qc);
+
 		qc->nsect = n_block;
 		tf->nsect = n_block & 0xff;
 
@@ -810,6 +807,8 @@ static unsigned int ata_scsi_rw_xlat(str
 		if ((block >> 28) || (n_block > 256))
 			return 1;
 
+		ata_rwcmd_protocol(qc);
+
 		/* Convert LBA to CHS */
 		track = (u32)block / dev->sectors;
 		cyl   = track / dev->heads;
author
#65 lore raw

PATCH 3/3 CHS: LBA28/LBA48 optimization (resend #4)

In reply to Albert Lee

(resend #4)

Patch 3/3: LBA28/LBA48 optimization per Mark's advice.

Changes:
- add lba_28_ok() and lba_48_ok() to ata.h
- use lba_28_ok() for CHS range check
- LBA28/LBA48 optimization

=====

include/linux/ata.h +10 -0
@@ -291,4 +291,14 @@ static inline int ata_ok(u8 status)
 			== ATA_DRDY);
 }
 
+static inline int lba_28_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 28)) && (n_block <= 256);
+}
+
+static inline int lba_48_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 48)) && (n_block <= 65536);
+}
+
 #endif /* __LINUX_ATA_H__ */
drivers/scsi/libata-scsi.c +19 -24

79 lines not on this page · raw message

#66 lore raw

Re: [PATCH 0/3] libata: CHS follow-up patches (resend #4)

In reply to Albert Lee
Quoted 4 lines · Albert Lee

(Resend #4, mailnews.wraplength set to 0 to keep Thunderbird from trimming trailing whitespaces.
Sorry for all the noises.)

Quoted 8 lines

CHS follow-up patches.

patch 1/3: move the initialization of ATA_TFLAG_LBA and ATA_TFLAG_LBA48 flags to the SCSI translation functions

patch 2/3: calculate read/write commands and protocols on the fly

patch 3/3: LBA28/LBA48 optimization

You've been very patient in resending, thanks :)

I wanted to go ahead and merge Doug Gilbert's error handling updates, which in turn caused some conflicts with your patches. Would you please rediff and resend the same three patches, against the latest 'upstream' branch?

Jeff

author
#67 lore raw

cover libata: CHS follow-up patches (resend #5)

Jeff,

CHS follow-up patches. (Keep sync with the scsi error handling patch. Also added patch 3/4.)

patch 1/4: move the initialization of ATA_TFLAG_LBA and ATA_TFLAG_LBA48 flags to the SCSI translation functions

patch 2/4: calculate read/write commands and protocols on the fly

patch 3/4: reread device identify info after initializing device CHS settings

patch 4/4: LBA28/LBA48 optimization

Patch against libata-dev upstream branch
(edea3ab58f8edd5f72d31f891ab4f34382e97e3a).

For your review, thanks.

Albert

author
#68 lore raw

PATCH 1/4 CHS: move the initialization of taskfile LBA flags (resend #5)

Patch 1/4:

move the initialization of taskfile LBA flags
"ATA_TFLAG_LBA" and "ATA_TFLAG_LBA48 flags"
to the SCSI translation functions

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

drivers/scsi/libata-core.c +0 -7
@@ -3195,13 +3195,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
 		qc->nbytes = qc->curbytes = 0;
 
 		ata_tf_init(ap, &qc->tf, dev->devno);
-
-		if (dev->flags & ATA_DFLAG_LBA) {
-			qc->tf.flags |= ATA_TFLAG_LBA;
-
-			if (dev->flags & ATA_DFLAG_LBA48)
-				qc->tf.flags |= ATA_TFLAG_LBA48;
-		}
 	}
 
 	return qc;
drivers/scsi/libata-scsi.c +25 -17

95 lines not on this page · raw message

author
#69 lore raw

PATCH 2/4 CHS: calculate read/write commands and protocol on the fly (resend #5)

In reply to Albert Lee

Patch 2/4: calculate read/write commands and protocols on the fly

changes:
- merge ata_prot_to_cmd() and ata_dev_set_protocol() as ata_rwcmd_protocol()
- pave road for read/write multiple support
- remove usage of pre-cached command and protocol values and call ata_rwcmd_protocol() instead

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

include/linux/ata.h +4 -0
@@ -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,
include/linux/libata.h +2 -4
@@ -283,10 +283,8 @@ struct ata_device {
 	u8			xfer_mode;
 	unsigned int		xfer_shift;	/* ATA_SHIFT_xxx */
 
-	/* cache info about current transfer mode */
-	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 */
 
 	/* for CHS addressing */
 	u16			cylinders;	/* Number of cylinders */
drivers/scsi/libata.h +1 -0
@@ -42,6 +42,7 @@ extern int atapi_enabled;
 extern int ata_qc_complete_noop(struct ata_queued_cmd *qc, u8 drv_stat);
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
+extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern int ata_qc_issue(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
drivers/scsi/libata-core.c +38 -69

135 lines not on this page · raw message

drivers/scsi/libata-scsi.c +6 -7
@@ -742,15 +742,10 @@ static unsigned int ata_scsi_rw_xlat(str
 	u32 n_block;
 
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	tf->protocol = qc->dev->xfer_protocol;
 
-	if (scsicmd[0] == READ_10 || scsicmd[0] == READ_6 ||
-	    scsicmd[0] == READ_16) {
-		tf->command = qc->dev->read_cmd;
-	} else {
-		tf->command = qc->dev->write_cmd;
+	if (scsicmd[0] == WRITE_10 || scsicmd[0] == WRITE_6 ||
+	    scsicmd[0] == WRITE_16)
 		tf->flags |= ATA_TFLAG_WRITE;
-	}
 
 	/* Calculate the SCSI LBA and transfer length. */
 	switch (scsicmd[0]) {
@@ -812,6 +807,8 @@ static unsigned int ata_scsi_rw_xlat(str
 			tf->device |= (block >> 24) & 0xf;
 		}
 
+		ata_rwcmd_protocol(qc);
+
 		qc->nsect = n_block;
 		tf->nsect = n_block & 0xff;
 
@@ -828,6 +825,8 @@ static unsigned int ata_scsi_rw_xlat(str
 		if ((block >> 28) || (n_block > 256))
 			goto out_of_range;
 
+		ata_rwcmd_protocol(qc);
+
 		/* Convert LBA to CHS */
 		track = (u32)block / dev->sectors;
 		cyl   = track / dev->heads;
author
#70 lore raw

PATCH 4/4 CHS: LBA28/LBA48 optimization (resend #5)

In reply to Albert Lee

Patch 4/4: LBA28/LBA48 optimization per Mark's advice.

Changes:
- add lba_28_ok() and lba_48_ok() to ata.h
- use lba_28_ok() for CHS range check
- LBA28/LBA48 optimization

=====

include/linux/ata.h +10 -0
@@ -291,4 +291,14 @@ static inline int ata_ok(u8 status)
 			== ATA_DRDY);
 }
 
+static inline int lba_28_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 28)) && (n_block <= 256);
+}
+
+static inline int lba_48_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 48)) && (n_block <= 65536);
+}
+
 #endif /* __LINUX_ATA_H__ */
drivers/scsi/libata-scsi.c +21 -27

84 lines not on this page · raw message

#71 lore raw

Re: [PATCH 4/4] CHS: LBA28/LBA48 optimization (resend #5)

Quoted 1 line · Albert Lee

Patch 4/4: LBA28/LBA48 optimization per Mark's advice.

I must confess I'm so far behind on following these patches that I don't know the answer to this, so..

Does this patch compare the *ending* block number when checking for 28-bit LBA, or the *starting* block number?

It's the *ending* block number that should be used, because some drives don't deal well with boundary crossings.

Cheers

#72 lore raw

Re: [PATCH 4/4] CHS: LBA28/LBA48 optimization (resend #5)

In reply to Albert Lee

Ah, here it is:

include/linux/ata.h static inline int ata_ok(u8 status) · line 298

Quoted 5 lines
include/linux/ata.h +5 -0
+static inline int lba_28_ok(u64 block, u32 n_block)
+{
+	return (block < ((u64)1 << 28)) && (n_block <= 256);
+}
+

That will need to change, so something like this:

return ((block + n_block - 1) < ((u64)1 << 28)) && (n_block <= 256);

author
#73 lore raw

Re: [PATCH 4/4] CHS: LBA28/LBA48 optimization (resend #5)

Quoted 11 lines · Mark Lord

Ah, here it is:

include/linux/ata.h static inline int ata_ok(u8 status) · line 298

Quoted 5 lines
include/linux/ata.h +5 -0
+static inline int lba_28_ok(u64 block, u32 n_block)
+{
+    return (block < ((u64)1 << 28)) && (n_block <= 256);
+}
+

That will need to change, so something like this:

Quoted 2 lines

return ((block + n_block - 1) < ((u64)1 << 28)) && (n_block <= 256); -

Thanks for the advice. I didn't think of it.
Will revise and resend the patch.

Albert

#74 lore raw

Re: [PATCH/RFC 4/4] CHS: calculate LBA28/LBA48 commands and protocol on the fly

In reply to Albert Lee
Quoted 5 lines
drivers/scsi/libata-scsi.c +4 -0
+	if (dev->flags & ATA_DFLAG_LBA) {
+		qc->tf.flags |= ATA_TFLAG_LBA;
+
+		if (lba_28_ok(block, n_block)) {

drivers/scsi/libata-scsi.c static unsigned int ata_scsi_verify_xlat · line 646

Quoted 10 lines
drivers/scsi/libata-scsi.c +10 -0
+			/* use LBA28 */
+			tf->command = ATA_CMD_VERIFY;
+
+			tf->device |= (block >> 24) & 0xf;
+		} else if (lba_48_ok(block, n_block)) {
+			if (!(dev->flags & ATA_DFLAG_LBA48))
+				return 1;
+
+			/* use LBA48 */
+			qc->tf.flags |= ATA_TFLAG_LBA48;

Can you make this decision a function because older ALi controllers at least will need to replace it (they do LBA48 PIO, LBA28 DMA)

#75 lore raw

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

In reply to Albert Lee
Quoted 117 lines · Albert Lee

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

------------------------------------------------------------------------

drivers/scsi/libata-core.c +29 -51
@@ -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;

drivers/scsi/libata-core.c static void ata_dev_set_protocol(struct · line 673

Quoted 10 lines
drivers/scsi/libata-core.c +4 -2
+		}
+	}
 
-	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 :)

Quoted 9 lines
drivers/scsi/libata-core.c +1 -3
@@ -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)) {

drivers/scsi/libata-core.c static int ata_pio_complete (struct ata_ · line 2481

Quoted 10 lines
drivers/scsi/libata-core.c +4 -2
+	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.

author
#76 lore raw

PATCH 4/4 libata: interrupt driven pio for LLD

In reply to Albert Lee

Patch 4/4: interrupt driven pio patch for LLD

Changes:

libata.h:
libata-core:
Add ATA_FLAG_PIO_POLLING flag for LLDs that expect interrupt for
command completion only.

sata_nv.c:
sata_vsc.c:
irq handler is wrapper around ata_host_intr(), can handle PIO interrupts.

sata_promise.c:
sata_sx4.c:
sata_qstor.c:
sata_mv.c:
Private irq handler.
Polling mode ATA_FLAG_PIO_POLLING used for compatibility.

For your review, thanks.

Albert

include/linux/libata.h +2 -0
@@ -116,6 +116,8 @@ enum {
 	ATA_FLAG_MMIO		= (1 << 6), /* use MMIO, not PIO */
 	ATA_FLAG_SATA_RESET	= (1 << 7), /* use COMRESET */
 	ATA_FLAG_PIO_DMA	= (1 << 8), /* PIO cmds via DMA */
+	ATA_FLAG_PIO_POLLING	= (1 << 9), /* use polling PIO if LLD
+					     * doesn't handle PIO interrupts */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */
drivers/scsi/libata-core.c +19 -0
@@ -3356,6 +3356,25 @@ int ata_qc_issue_prot(struct ata_queued_
 {
 	struct ata_port *ap = qc->ap;
 
+	/* Use polling pio if the LLD doesn't handle
+	 * interrupt driven pio and atapi CDB interrupt.
+	 */
+	if (ap->flags & ATA_FLAG_PIO_POLLING) {
+		switch (qc->tf.protocol) {
+		case ATA_PROT_PIO:
+		case ATA_PROT_ATAPI:
+		case ATA_PROT_ATAPI_NODATA:
+			qc->tf.flags |= ATA_TFLAG_POLLING;
+			break;
+		case ATA_PROT_ATAPI_DMA:
+			if (qc->dev->flags & ATA_DFLAG_CDB_INTR)
+				BUG();
+			break;
+		default:
+			break;
+		}
+	}
+
 	/* select the device */
 	ata_dev_select(ap, qc->dev->devno, 1, 0);
 
drivers/scsi/sata_nv.c +2 -2
@@ -302,11 +302,11 @@ static irqreturn_t nv_interrupt (int irq
 
 		ap = host_set->ports[i];
 		if (ap &&
-		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
+		    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
 				handled += ata_host_intr(ap, qc);
 		}
 
drivers/scsi/sata_vsc.c +3 -3
@@ -193,12 +193,12 @@ static irqreturn_t vsc_sata_interrupt (i
 			struct ata_port *ap;
 
 			ap = host_set->ports[i];
-			if (ap && !(ap->flags &
-				    (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
+			if (ap &&
+			    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 				struct ata_queued_cmd *qc;
 
 				qc = ata_qc_from_tag(ap, ap->active_tag);
-				if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+				if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
 					handled += ata_host_intr(ap, qc);
 			}
 		}
drivers/scsi/sata_promise.c +8 -5
@@ -162,7 +162,8 @@ static struct ata_port_info pdc_port_inf
 	{
 		.sht		= &pdc_ata_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
+				  ATA_FLAG_PIO_POLLING,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
@@ -173,7 +174,8 @@ static struct ata_port_info pdc_port_inf
 	{
 		.sht		= &pdc_ata_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
+				  ATA_FLAG_PIO_POLLING,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
@@ -184,7 +186,8 @@ static struct ata_port_info pdc_port_inf
 	{
 		.sht		= &pdc_ata_sht,
 		.host_flags	= ATA_FLAG_NO_LEGACY | ATA_FLAG_SRST |
-				  ATA_FLAG_MMIO | ATA_FLAG_SLAVE_POSS,
+				  ATA_FLAG_MMIO | ATA_FLAG_SLAVE_POSS |
+				  ATA_FLAG_PIO_POLLING,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
@@ -493,11 +496,11 @@ static irqreturn_t pdc_interrupt (int ir
 		ap = host_set->ports[i];
 		tmp = mask & (1 << (i + 1));
 		if (tmp && ap &&
-		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
+		    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
 				handled += pdc_host_intr(ap, qc);
 		}
 	}
drivers/scsi/sata_sx4.c +4 -3
@@ -219,7 +219,8 @@ static struct ata_port_info pdc_port_inf
 	{
 		.sht		= &pdc_sata_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
+				  ATA_FLAG_PIO_POLLING,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
@@ -832,11 +833,11 @@ static irqreturn_t pdc20621_interrupt (i
 		tmp = mask & (1 << i);
 		VPRINTK("seq %u, port_no %u, ap %p, tmp %x\n", i, port_no, ap, tmp);
 		if (tmp && ap &&
-		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
+		    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.ctl & ATA_NIEN)))
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
 				handled += pdc20621_host_intr(ap, qc, (i > 4),
 							      mmio_base);
 		}
drivers/scsi/sata_qstor.c +5 -6
@@ -175,7 +175,7 @@ static struct ata_port_info qs_port_info
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				  ATA_FLAG_SATA_RESET |
 				  //FIXME ATA_FLAG_SRST |
-				  ATA_FLAG_MMIO,
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_POLLING,
 		.pio_mask	= 0x10, /* pio4 */
 		.udma_mask	= 0x7f, /* udma0-6 */
 		.port_ops	= &qs_ata_ops,
@@ -389,14 +389,13 @@ static inline unsigned int qs_intr_pkt(s
 			DPRINTK("SFF=%08x%08x: sCHAN=%u sHST=%d sDST=%02x\n",
 					sff1, sff0, port_no, sHST, sDST);
 			handled = 1;
-			if (ap && !(ap->flags &
-				    (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
+			if (ap && !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 				struct ata_queued_cmd *qc;
 				struct qs_port_priv *pp = ap->private_data;
 				if (!pp || pp->state != qs_state_pkt)
 					continue;
 				qc = ata_qc_from_tag(ap, ap->active_tag);
-				if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
+				if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
 					switch (sHST) {
 					case 0: /* sucessful CPB */
 					case 3: /* device error */
@@ -422,13 +421,13 @@ static inline unsigned int qs_intr_mmio(
 		struct ata_port *ap;
 		ap = host_set->ports[port_no];
 		if (ap &&
-		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
+		    !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 			struct ata_queued_cmd *qc;
 			struct qs_port_priv *pp = ap->private_data;
 			if (!pp || pp->state != qs_state_mmio)
 				continue;
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.ctl & ATA_NIEN))) {
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
 
 				/* check main status, clearing INTRQ */
 				u8 status = ata_chk_status(ap);
drivers/scsi/sata_mv.c +5 -2
@@ -241,7 +241,8 @@ static struct ata_port_info mv_port_info
 	{  /* chip_504x */
 		.sht		= &mv_sht,
 		.host_flags	= (ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				   ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO),
+				   ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
+				   ATA_FLAG_PIO_POLLING),
 		.pio_mask	= 0x1f,	/* pio4-0 */
 		.udma_mask	= 0,	/* 0x7f (udma6-0 disabled for now) */
 		.port_ops	= &mv_ops,
@@ -250,7 +251,7 @@ static struct ata_port_info mv_port_info
 		.sht		= &mv_sht,
 		.host_flags	= (ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				   ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO | 
-				   MV_FLAG_DUAL_HC),
+				   ATA_FLAG_PIO_POLLING | MV_FLAG_DUAL_HC),
 		.pio_mask	= 0x1f,	/* pio4-0 */
 		.udma_mask	= 0,	/* 0x7f (udma6-0 disabled for now) */
 		.port_ops	= &mv_ops,
@@ -259,6 +260,7 @@ static struct ata_port_info mv_port_info
 		.sht		= &mv_sht,
 		.host_flags	= (ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				   ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO | 
+				   ATA_FLAG_PIO_POLLING |
 				   MV_FLAG_IRQ_COALESCE | MV_FLAG_BDMA),
 		.pio_mask	= 0x1f,	/* pio4-0 */
 		.udma_mask	= 0,	/* 0x7f (udma6-0 disabled for now) */
@@ -268,6 +270,7 @@ static struct ata_port_info mv_port_info
 		.sht		= &mv_sht,
 		.host_flags	= (ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				   ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
+				   ATA_FLAG_PIO_POLLING |
 				   MV_FLAG_IRQ_COALESCE | MV_FLAG_DUAL_HC |
 				   MV_FLAG_BDMA),
 		.pio_mask	= 0x1f,	/* pio4-0 */