* [PATCHSET] libata: improve ATAPI data transfer handling, take #2
@ 2007-11-29 14:33 Tejun Heo
2007-11-29 14:33 ` [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size Tejun Heo
` (13 more replies)
0 siblings, 14 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe
Hello,
This is the second take of improve-ATAPI-data-transfer patchset.
Changes from the last take [L] are.
* 0005-libata-make-data_xfer-return-the-number-of-consum.patch added
which converts ->data_xfer to return the number of consumed bytes.
* 0006-libata-improve-ATAPI-draining.patch is fixed and updated.
Drain limit check is fixed. Now both partial and whole chunk
draining uses the correct byte count. LLDs doing 32bit IO can now
drain properly instead of draining twice as much. Also, draining is
done in large chunks (PAGE_SIZE at maximum).
* ata_drain_page allocation is moved to 0006 from
0012-libata-implement-ATAPI-drain-buffer.patch.
This patchset is against
#upstream (51a7ee37eaa85b8c35fe6090618e34ed4ce1d316)
+ improve-speed-down patchset, take #3 [1]
+ improve-timing-cod-and-fix-pata_amd, take #2 [2]
This patchset is also available as the following git tree.
master.kernel.org:/pub/scm/linux/kernel/git/tj/libata-dev.git improve-ATAPI-data-transfer
Thanks.
--
tejun
[L] http://thread.gmane.org/gmane.linux.ide/25437
[1] http://thread.gmane.org/gmane.linux.ide/25406
[2] http://thread.gmane.org/gmane.linux.ide/25416
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 15:51 ` Alan Cox
2007-12-04 19:27 ` Jeff Garzik
2007-11-29 14:33 ` [PATCH 02/14] cdrom: add more GPCMD_* constants Tejun Heo
` (12 subsequent siblings)
13 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
While updating lbam/h for ATAPI commands, atapi_eh_request_sense() was
left out. Update it.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/ata/libata-eh.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 77083b5..2e3d3a2 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1302,8 +1302,8 @@ static unsigned int atapi_eh_request_sense(struct ata_queued_cmd *qc)
tf.feature |= ATAPI_PKT_DMA;
} else {
tf.protocol = ATA_PROT_ATAPI;
- tf.lbam = (8 * 1024) & 0xff;
- tf.lbah = (8 * 1024) >> 8;
+ tf.lbam = SCSI_SENSE_BUFFERSIZE;
+ tf.lbah = 0;
}
return ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 02/14] cdrom: add more GPCMD_* constants
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
2007-11-29 14:33 ` [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 03/14] libata: rename ATA_PROT_ATAPI_* to ATAPI_PROT_* Tejun Heo
` (11 subsequent siblings)
13 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
Add GPCMD_* constants for READ_BUFFER, WRITE_12 and WRITE_BUFFER for
completeness. These will be used libata.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
include/linux/cdrom.h | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index c6d3e22..fcdc11b 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -451,6 +451,7 @@ struct cdrom_generic_command
#define GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL 0x1e
#define GPCMD_READ_10 0x28
#define GPCMD_READ_12 0xa8
+#define GPCMD_READ_BUFFER 0x3c
#define GPCMD_READ_BUFFER_CAPACITY 0x5c
#define GPCMD_READ_CDVD_CAPACITY 0x25
#define GPCMD_READ_CD 0xbe
@@ -480,7 +481,9 @@ struct cdrom_generic_command
#define GPCMD_TEST_UNIT_READY 0x00
#define GPCMD_VERIFY_10 0x2f
#define GPCMD_WRITE_10 0x2a
+#define GPCMD_WRITE_12 0xaa
#define GPCMD_WRITE_AND_VERIFY_10 0x2e
+#define GPCMD_WRITE_BUFFER 0x3b
/* This is listed as optional in ATAPI 2.6, but is (curiously)
* missing from Mt. Fuji, Table 57. It _is_ mentioned in Mt. Fuji
* Table 377 as an MMC command for SCSi devices though... Most ATAPI
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 03/14] libata: rename ATA_PROT_ATAPI_* to ATAPI_PROT_*
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
2007-11-29 14:33 ` [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size Tejun Heo
2007-11-29 14:33 ` [PATCH 02/14] cdrom: add more GPCMD_* constants Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 15:52 ` Alan Cox
2007-11-29 14:33 ` [PATCH 04/14] libata: add ATAPI_* cmd types and implement atapi_cmd_type() Tejun Heo
` (10 subsequent siblings)
13 siblings, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
ATA_PROT_ATAPI_* are ugly and naming schemes between ATA_PROT_* and
ATA_PROT_ATAPI_* are inconsistent causing confusion. Rename them to
ATAPI_PROT_* and make them consistent with ATA counterpart.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/ata/libata-core.c | 24 ++++++++++++------------
drivers/ata/libata-eh.c | 4 ++--
drivers/ata/libata-scsi.c | 10 +++++-----
drivers/ata/libata-sff.c | 2 +-
drivers/ata/pata_pdc202xx_old.c | 5 ++---
drivers/ata/pdc_adma.c | 2 +-
drivers/ata/sata_inic162x.c | 2 +-
drivers/ata/sata_promise.c | 26 ++++++++++++--------------
drivers/ata/sata_qstor.c | 2 +-
drivers/ata/sata_sx4.c | 2 +-
drivers/scsi/ipr.c | 6 +++---
drivers/scsi/libsas/sas_ata.c | 2 +-
include/linux/ata.h | 12 ++++++------
13 files changed, 48 insertions(+), 51 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e778dee..bc53492 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5133,13 +5133,13 @@ static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
ata_altstatus(ap); /* flush */
switch (qc->tf.protocol) {
- case ATA_PROT_ATAPI:
+ case ATAPI_PROT_PIO:
ap->hsm_task_state = HSM_ST;
break;
- case ATA_PROT_ATAPI_NODATA:
+ case ATAPI_PROT_NODATA:
ap->hsm_task_state = HSM_ST_LAST;
break;
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
ap->hsm_task_state = HSM_ST_LAST;
/* initiate bmdma */
ap->ops->bmdma_start(qc);
@@ -5482,7 +5482,7 @@ fsm_start:
case HSM_ST:
/* complete command or read/write the data register */
- if (qc->tf.protocol == ATA_PROT_ATAPI) {
+ if (qc->tf.protocol == ATAPI_PROT_PIO) {
/* ATAPI PIO protocol */
if ((status & ATA_DRQ) == 0) {
/* No more data to transfer or device error.
@@ -6037,11 +6037,11 @@ unsigned int ata_qc_issue_prot(struct ata_queued_cmd *qc)
switch (qc->tf.protocol) {
case ATA_PROT_PIO:
case ATA_PROT_NODATA:
- case ATA_PROT_ATAPI:
- case ATA_PROT_ATAPI_NODATA:
+ case ATAPI_PROT_PIO:
+ case ATAPI_PROT_NODATA:
qc->tf.flags |= ATA_TFLAG_POLLING;
break;
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
if (qc->dev->flags & ATA_DFLAG_CDB_INTR)
/* see ata_dma_blacklisted() */
BUG();
@@ -6105,8 +6105,8 @@ unsigned int ata_qc_issue_prot(struct ata_queued_cmd *qc)
break;
- case ATA_PROT_ATAPI:
- case ATA_PROT_ATAPI_NODATA:
+ case ATAPI_PROT_PIO:
+ case ATAPI_PROT_NODATA:
if (qc->tf.flags & ATA_TFLAG_POLLING)
ata_qc_set_polling(qc);
@@ -6120,7 +6120,7 @@ unsigned int ata_qc_issue_prot(struct ata_queued_cmd *qc)
ata_port_queue_task(ap, ata_pio_task, qc, 0);
break;
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
WARN_ON(qc->tf.flags & ATA_TFLAG_POLLING);
ap->ops->tf_load(ap, &qc->tf); /* load tf registers */
@@ -6181,7 +6181,7 @@ inline unsigned int ata_host_intr(struct ata_port *ap,
break;
case HSM_ST_LAST:
if (qc->tf.protocol == ATA_PROT_DMA ||
- qc->tf.protocol == ATA_PROT_ATAPI_DMA) {
+ qc->tf.protocol == ATAPI_PROT_DMA) {
/* check status of DMA engine */
host_stat = ap->ops->bmdma_status(ap);
VPRINTK("ata%u: host_stat 0x%X\n",
@@ -6223,7 +6223,7 @@ inline unsigned int ata_host_intr(struct ata_port *ap,
ata_hsm_move(ap, qc, status, 0);
if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
- qc->tf.protocol == ATA_PROT_ATAPI_DMA))
+ qc->tf.protocol == ATAPI_PROT_DMA))
ata_ehi_push_desc(ehi, "BMDMA stat 0x%x", host_stat);
return 1; /* irq handled */
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 2e3d3a2..baa7f4f 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1298,10 +1298,10 @@ static unsigned int atapi_eh_request_sense(struct ata_queued_cmd *qc)
/* is it pointless to prefer PIO for "safety reasons"? */
if (ap->flags & ATA_FLAG_PIO_DMA) {
- tf.protocol = ATA_PROT_ATAPI_DMA;
+ tf.protocol = ATAPI_PROT_DMA;
tf.feature |= ATAPI_PKT_DMA;
} else {
- tf.protocol = ATA_PROT_ATAPI;
+ tf.protocol = ATAPI_PROT_PIO;
tf.lbam = SCSI_SENSE_BUFFERSIZE;
tf.lbah = 0;
}
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a1ec970..b02e1ac 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2351,10 +2351,10 @@ static void atapi_request_sense(struct ata_queued_cmd *qc)
qc->tf.command = ATA_CMD_PACKET;
if (ata_pio_use_silly(ap)) {
- qc->tf.protocol = ATA_PROT_ATAPI_DMA;
+ qc->tf.protocol = ATAPI_PROT_DMA;
qc->tf.feature |= ATAPI_PKT_DMA;
} else {
- qc->tf.protocol = ATA_PROT_ATAPI;
+ qc->tf.protocol = ATAPI_PROT_PIO;
qc->tf.lbam = SCSI_SENSE_BUFFERSIZE;
qc->tf.lbah = 0;
}
@@ -2525,12 +2525,12 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
if (using_pio || nodata) {
/* no data, or PIO data xfer */
if (nodata)
- qc->tf.protocol = ATA_PROT_ATAPI_NODATA;
+ qc->tf.protocol = ATAPI_PROT_NODATA;
else
- qc->tf.protocol = ATA_PROT_ATAPI;
+ qc->tf.protocol = ATAPI_PROT_PIO;
} else {
/* DMA data xfer */
- qc->tf.protocol = ATA_PROT_ATAPI_DMA;
+ qc->tf.protocol = ATAPI_PROT_DMA;
qc->tf.feature |= ATAPI_PKT_DMA;
if (atapi_dmadir && (scmd->sc_data_direction != DMA_TO_DEVICE))
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index fb69476..3b62479 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -417,7 +417,7 @@ void ata_bmdma_drive_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
ap->hsm_task_state = HSM_ST_IDLE;
if (qc && (qc->tf.protocol == ATA_PROT_DMA ||
- qc->tf.protocol == ATA_PROT_ATAPI_DMA)) {
+ qc->tf.protocol == ATAPI_PROT_DMA)) {
u8 host_stat;
host_stat = ap->ops->bmdma_status(ap);
diff --git a/drivers/ata/pata_pdc202xx_old.c b/drivers/ata/pata_pdc202xx_old.c
index bc7c2d5..407dbcf 100644
--- a/drivers/ata/pata_pdc202xx_old.c
+++ b/drivers/ata/pata_pdc202xx_old.c
@@ -168,8 +168,7 @@ static void pdc2026x_bmdma_start(struct ata_queued_cmd *qc)
pdc202xx_set_dmamode(ap, qc->dev);
/* Cases the state machine will not complete correctly without help */
- if ((tf->flags & ATA_TFLAG_LBA48) || tf->protocol == ATA_PROT_ATAPI_DMA)
- {
+ if ((tf->flags & ATA_TFLAG_LBA48) || tf->protocol == ATAPI_PROT_DMA) {
len = qc->nbytes / 2;
if (tf->flags & ATA_TFLAG_WRITE)
@@ -208,7 +207,7 @@ static void pdc2026x_bmdma_stop(struct ata_queued_cmd *qc)
void __iomem *atapi_reg = master + 0x20 + (4 * ap->port_no);
/* Cases the state machine will not complete correctly */
- if (tf->protocol == ATA_PROT_ATAPI_DMA || ( tf->flags & ATA_TFLAG_LBA48)) {
+ if (tf->protocol == ATAPI_PROT_DMA || (tf->flags & ATA_TFLAG_LBA48)) {
iowrite32(0, atapi_reg);
iowrite8(ioread8(clock) & ~sel66, clock);
}
diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
index bd4c2a3..459cb7b 100644
--- a/drivers/ata/pdc_adma.c
+++ b/drivers/ata/pdc_adma.c
@@ -455,7 +455,7 @@ static unsigned int adma_qc_issue(struct ata_queued_cmd *qc)
adma_packet_start(qc);
return 0;
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
BUG();
break;
diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
index 323c087..96e614a 100644
--- a/drivers/ata/sata_inic162x.c
+++ b/drivers/ata/sata_inic162x.c
@@ -585,7 +585,7 @@ static struct ata_port_operations inic_port_ops = {
};
static struct ata_port_info inic_port_info = {
- /* For some reason, ATA_PROT_ATAPI is broken on this
+ /* For some reason, ATAPI_PROT_PIO is broken on this
* controller, and no, PIO_POLLING does't fix it. It somehow
* manages to report the wrong ireason and ignoring ireason
* results in machine lock up. Tell libata to always prefer
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index 7914def..25b34d6 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -456,13 +456,13 @@ static void pdc_atapi_pkt(struct ata_queued_cmd *qc)
* and seq id (byte 2)
*/
switch (qc->tf.protocol) {
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
if (!(qc->tf.flags & ATA_TFLAG_WRITE))
buf32[0] = cpu_to_le32(PDC_PKT_READ);
else
buf32[0] = 0;
break;
- case ATA_PROT_ATAPI_NODATA:
+ case ATAPI_PROT_NODATA:
buf32[0] = cpu_to_le32(PDC_PKT_NODATA);
break;
default:
@@ -491,7 +491,7 @@ static void pdc_atapi_pkt(struct ata_queued_cmd *qc)
buf[19] = 0x00;
/* set feature and byte counter registers */
- if (qc->tf.protocol != ATA_PROT_ATAPI_DMA) {
+ if (qc->tf.protocol != ATAPI_PROT_DMA) {
feature = PDC_FEATURE_ATAPI_PIO;
/* set byte counter register to real transfer byte count */
nbytes = qc->nbytes;
@@ -627,14 +627,14 @@ static void pdc_qc_prep(struct ata_queued_cmd *qc)
pdc_pkt_footer(&qc->tf, pp->pkt, i);
break;
- case ATA_PROT_ATAPI:
+ case ATAPI_PROT_PIO:
pdc_fill_sg(qc);
break;
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
pdc_fill_sg(qc);
/*FALLTHROUGH*/
- case ATA_PROT_ATAPI_NODATA:
+ case ATAPI_PROT_NODATA:
pdc_atapi_pkt(qc);
break;
@@ -754,8 +754,8 @@ static inline unsigned int pdc_host_intr(struct ata_port *ap,
switch (qc->tf.protocol) {
case ATA_PROT_DMA:
case ATA_PROT_NODATA:
- case ATA_PROT_ATAPI_DMA:
- case ATA_PROT_ATAPI_NODATA:
+ case ATAPI_PROT_DMA:
+ case ATAPI_PROT_NODATA:
qc->err_mask |= ac_err_mask(ata_wait_idle(ap));
ata_qc_complete(qc);
handled = 1;
@@ -900,7 +900,7 @@ static inline void pdc_packet_start(struct ata_queued_cmd *qc)
static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
{
switch (qc->tf.protocol) {
- case ATA_PROT_ATAPI_NODATA:
+ case ATAPI_PROT_NODATA:
if (qc->dev->flags & ATA_DFLAG_CDB_INTR)
break;
/*FALLTHROUGH*/
@@ -908,7 +908,7 @@ static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
if (qc->tf.flags & ATA_TFLAG_POLLING)
break;
/*FALLTHROUGH*/
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
case ATA_PROT_DMA:
pdc_packet_start(qc);
return 0;
@@ -922,16 +922,14 @@ static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf)
{
- WARN_ON(tf->protocol == ATA_PROT_DMA ||
- tf->protocol == ATA_PROT_ATAPI_DMA);
+ WARN_ON(tf->protocol == ATA_PROT_DMA || tf->protocol == ATAPI_PROT_DMA);
ata_tf_load(ap, tf);
}
static void pdc_exec_command_mmio(struct ata_port *ap,
const struct ata_taskfile *tf)
{
- WARN_ON(tf->protocol == ATA_PROT_DMA ||
- tf->protocol == ATA_PROT_ATAPI_DMA);
+ WARN_ON(tf->protocol == ATA_PROT_DMA || tf->protocol == ATAPI_PROT_DMA);
ata_exec_command(ap, tf);
}
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index 2f1de6e..2875549 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -376,7 +376,7 @@ static unsigned int qs_qc_issue(struct ata_queued_cmd *qc)
qs_packet_start(qc);
return 0;
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
BUG();
break;
diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index 4d85718..3de0c27 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -700,7 +700,7 @@ static unsigned int pdc20621_qc_issue_prot(struct ata_queued_cmd *qc)
pdc20621_packet_start(qc);
return 0;
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
BUG();
break;
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 0841df0..3e78bc2 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -5222,12 +5222,12 @@ static unsigned int ipr_qc_issue(struct ata_queued_cmd *qc)
regs->flags |= IPR_ATA_FLAG_XFER_TYPE_DMA;
break;
- case ATA_PROT_ATAPI:
- case ATA_PROT_ATAPI_NODATA:
+ case ATAPI_PROT_PIO:
+ case ATAPI_PROT_NODATA:
regs->flags |= IPR_ATA_FLAG_PACKET_CMD;
break;
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
regs->flags |= IPR_ATA_FLAG_PACKET_CMD;
regs->flags |= IPR_ATA_FLAG_XFER_TYPE_DMA;
break;
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 831294d..f78d060 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -200,7 +200,7 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
case ATA_PROT_NCQ:
task->ata_task.use_ncq = 1;
/* fall through */
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
case ATA_PROT_DMA:
task->ata_task.dma_xfer = 1;
break;
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 3992557..0c52476 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -326,9 +326,9 @@ enum ata_tf_protocols {
ATA_PROT_PIO, /* PIO data xfer */
ATA_PROT_DMA, /* DMA */
ATA_PROT_NCQ, /* NCQ */
- ATA_PROT_ATAPI, /* packet command, PIO data xfer*/
- ATA_PROT_ATAPI_NODATA, /* packet command, no data */
- ATA_PROT_ATAPI_DMA, /* packet command with special DMA sauce */
+ ATAPI_PROT_NODATA, /* packet command, no data */
+ ATAPI_PROT_PIO, /* packet command, PIO data xfer*/
+ ATAPI_PROT_DMA, /* packet command with special DMA sauce */
};
enum ata_ioctls {
@@ -380,11 +380,11 @@ static inline unsigned int ata_prot_flags(u8 prot)
return ATA_PROT_FLAG_DMA;
case ATA_PROT_NCQ:
return ATA_PROT_FLAG_DMA | ATA_PROT_FLAG_NCQ;
- case ATA_PROT_ATAPI_NODATA:
+ case ATAPI_PROT_NODATA:
return ATA_PROT_FLAG_ATAPI;
- case ATA_PROT_ATAPI:
+ case ATAPI_PROT_PIO:
return ATA_PROT_FLAG_ATAPI | ATA_PROT_FLAG_PIO;
- case ATA_PROT_ATAPI_DMA:
+ case ATAPI_PROT_DMA:
return ATA_PROT_FLAG_ATAPI | ATA_PROT_FLAG_DMA;
}
return 0;
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 04/14] libata: add ATAPI_* cmd types and implement atapi_cmd_type()
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (2 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 03/14] libata: rename ATA_PROT_ATAPI_* to ATAPI_PROT_* Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes Tejun Heo
` (9 subsequent siblings)
13 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
Add ATAPI command types - ATAPI_READ, WRITE, RW_BUF, READ_CD and MISC,
and implement atapi_cmd_type() which takes SCSI opcode and returns to
which class the opcode belongs. This will be used later to improve
ATAPI handling.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
include/linux/libata.h | 28 ++++++++++++++++++++++++++++
1 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 62d09c8..72df408 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -35,6 +35,7 @@
#include <linux/workqueue.h>
#include <scsi/scsi_host.h>
#include <linux/acpi.h>
+#include <linux/cdrom.h>
/*
* Define if arch has non-standard setup. This is a _PCI_ standard
@@ -345,6 +346,12 @@ enum {
ATA_DMA_MASK_ATA = (1 << 0), /* DMA on ATA Disk */
ATA_DMA_MASK_ATAPI = (1 << 1), /* DMA on ATAPI */
ATA_DMA_MASK_CFA = (1 << 2), /* DMA on CF Card */
+
+ /* ATAPI command types */
+ ATAPI_READ = 0, /* READs */
+ ATAPI_WRITE = 1, /* WRITEs */
+ ATAPI_READ_CD = 2, /* READ CD [MSF] */
+ ATAPI_MISC = 3, /* the rest */
};
enum ata_xfer_mask {
@@ -1407,6 +1414,27 @@ static inline int ata_try_flush_cache(const struct ata_device *dev)
ata_id_has_flush_ext(dev->id);
}
+static inline int atapi_cmd_type(u8 opcode)
+{
+ switch (opcode) {
+ case GPCMD_READ_10:
+ case GPCMD_READ_12:
+ return ATAPI_READ;
+
+ case GPCMD_WRITE_10:
+ case GPCMD_WRITE_12:
+ case GPCMD_WRITE_AND_VERIFY_10:
+ return ATAPI_WRITE;
+
+ case GPCMD_READ_CD:
+ case GPCMD_READ_CD_MSF:
+ return ATAPI_READ_CD;
+
+ default:
+ return ATAPI_MISC;
+ }
+}
+
static inline unsigned int ac_err_mask(u8 status)
{
if (status & (ATA_BUSY | ATA_DRQ))
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (3 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 04/14] libata: add ATAPI_* cmd types and implement atapi_cmd_type() Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 15:55 ` Alan Cox
2007-12-04 19:30 ` Jeff Garzik
2007-11-29 14:33 ` [PATCH 06/14] libata: improve ATAPI draining Tejun Heo
` (8 subsequent siblings)
13 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
Depending on how many bytes are transferred as a unit, PIO data
tranasfer may consume more bytes than requested. Knowing how much
data is consumed is necessary to determine how much is left for
draining. This patch update ->data_xfer such that it returns the
number of consumed bytes.
While at it, it also makes the following changes.
* s/adev/dev/
* s/buflen/len/
* use READ/WRITE constants for rw indication
* misc clean ups
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/ata/libata-core.c | 56 ++++++++++++++++++++++++++---------------
drivers/ata/pata_bf54x.c | 34 +++++++++++++------------
drivers/ata/pata_ixp4xx_cf.c | 32 ++++++++++++-----------
drivers/ata/pata_legacy.c | 38 +++++++++++++++-------------
drivers/ata/pata_qdi.c | 32 +++++++++++++----------
drivers/ata/pata_scc.c | 38 +++++++++++++++-------------
drivers/ata/pata_winbond.c | 32 +++++++++++++----------
include/linux/libata.h | 11 ++++---
8 files changed, 152 insertions(+), 121 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index bc53492..10f3b42 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4966,48 +4966,55 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
/**
* ata_data_xfer - Transfer data by PIO
- * @adev: device to target
+ * @dev: device to target
* @buf: data buffer
- * @buflen: buffer length
+ * @len: buffer length
* @write_data: read/write
*
* Transfer data from/to the device data register by PIO.
*
* LOCKING:
* Inherited from caller.
+ *
+ * RETURNS:
+ * Bytes consumed.
*/
-void ata_data_xfer(struct ata_device *adev, unsigned char *buf,
- unsigned int buflen, int write_data)
+unsigned int ata_data_xfer(struct ata_device *dev, unsigned char *buf,
+ unsigned int len, int rw)
{
- struct ata_port *ap = adev->link->ap;
- unsigned int words = buflen >> 1;
+ struct ata_port *ap = dev->link->ap;
+ void __iomem *data_addr = ap->ioaddr.data_addr;
+ unsigned int words = len >> 1;
/* Transfer multiple of 2 bytes */
- if (write_data)
- iowrite16_rep(ap->ioaddr.data_addr, buf, words);
+ if (rw == READ)
+ ioread16_rep(data_addr, buf, words);
else
- ioread16_rep(ap->ioaddr.data_addr, buf, words);
+ iowrite16_rep(data_addr, buf, words);
/* Transfer trailing 1 byte, if any. */
- if (unlikely(buflen & 0x01)) {
+ if (unlikely(len & 0x01)) {
u16 align_buf[1] = { 0 };
- unsigned char *trailing_buf = buf + buflen - 1;
+ unsigned char *trailing_buf = buf + len - 1;
- if (write_data) {
- memcpy(align_buf, trailing_buf, 1);
- iowrite16(le16_to_cpu(align_buf[0]), ap->ioaddr.data_addr);
- } else {
- align_buf[0] = cpu_to_le16(ioread16(ap->ioaddr.data_addr));
+ if (rw == READ) {
+ align_buf[0] = cpu_to_le16(ioread16(data_addr));
memcpy(trailing_buf, align_buf, 1);
+ } else {
+ memcpy(align_buf, trailing_buf, 1);
+ iowrite16(le16_to_cpu(align_buf[0]), data_addr);
}
+ words++;
}
+
+ return words << 1;
}
/**
* ata_data_xfer_noirq - Transfer data by PIO
- * @adev: device to target
+ * @dev: device to target
* @buf: data buffer
- * @buflen: buffer length
+ * @len: buffer length
* @write_data: read/write
*
* Transfer data from/to the device data register by PIO. Do the
@@ -5015,14 +5022,21 @@ void ata_data_xfer(struct ata_device *adev, unsigned char *buf,
*
* LOCKING:
* Inherited from caller.
+ *
+ * RETURNS:
+ * Bytes consumed.
*/
-void ata_data_xfer_noirq(struct ata_device *adev, unsigned char *buf,
- unsigned int buflen, int write_data)
+unsigned int ata_data_xfer_noirq(struct ata_device *dev, unsigned char *buf,
+ unsigned int len, int rw)
{
unsigned long flags;
+ unsigned int consumed;
+
local_irq_save(flags);
- ata_data_xfer(adev, buf, buflen, write_data);
+ consumed = ata_data_xfer(dev, buf, len, rw);
local_irq_restore(flags);
+
+ return consumed;
}
diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
index 81db405..53ae7d3 100644
--- a/drivers/ata/pata_bf54x.c
+++ b/drivers/ata/pata_bf54x.c
@@ -1161,40 +1161,42 @@ static unsigned char bfin_bmdma_status(struct ata_port *ap)
* bfin_data_xfer - Transfer data by PIO
* @adev: device for this I/O
* @buf: data buffer
- * @buflen: buffer length
+ * @len: buffer length
* @write_data: read/write
*
* Note: Original code is ata_data_xfer().
*/
-static void bfin_data_xfer(struct ata_device *adev, unsigned char *buf,
- unsigned int buflen, int write_data)
+static unsigned int bfin_data_xfer(struct ata_device *dev, unsigned char *buf,
+ unsigned int len, int rw)
{
- struct ata_port *ap = adev->link->ap;
- unsigned int words = buflen >> 1;
- unsigned short *buf16 = (u16 *) buf;
+ struct ata_port *ap = dev->link->ap;
void __iomem *base = (void __iomem *)ap->ioaddr.ctl_addr;
+ unsigned int words = len >> 1;
+ unsigned short *buf16 = (u16 *)buf;
/* Transfer multiple of 2 bytes */
- if (write_data) {
- write_atapi_data(base, words, buf16);
- } else {
+ if (rw == READ)
read_atapi_data(base, words, buf16);
- }
+ else
+ write_atapi_data(base, words, buf16);
/* Transfer trailing 1 byte, if any. */
- if (unlikely(buflen & 0x01)) {
+ if (unlikely(len & 0x01)) {
unsigned short align_buf[1] = { 0 };
- unsigned char *trailing_buf = buf + buflen - 1;
+ unsigned char *trailing_buf = buf + len - 1;
- if (write_data) {
- memcpy(align_buf, trailing_buf, 1);
- write_atapi_data(base, 1, align_buf);
- } else {
+ if (rw == READ) {
read_atapi_data(base, 1, align_buf);
memcpy(trailing_buf, align_buf, 1);
+ } else {
+ memcpy(align_buf, trailing_buf, 1);
+ write_atapi_data(base, 1, align_buf);
}
+ words++;
}
+
+ return words << 1;
}
/**
diff --git a/drivers/ata/pata_ixp4xx_cf.c b/drivers/ata/pata_ixp4xx_cf.c
index fcd532a..b8a6ce3 100644
--- a/drivers/ata/pata_ixp4xx_cf.c
+++ b/drivers/ata/pata_ixp4xx_cf.c
@@ -42,13 +42,13 @@ static int ixp4xx_set_mode(struct ata_link *link, struct ata_device **error)
return 0;
}
-static void ixp4xx_mmio_data_xfer(struct ata_device *adev, unsigned char *buf,
- unsigned int buflen, int write_data)
+static unsigned int ixp4xx_mmio_data_xfer(struct ata_device *dev,
+ unsigned char *buf, unsigned int len, int rw)
{
unsigned int i;
- unsigned int words = buflen >> 1;
+ unsigned int words = len >> 1;
u16 *buf16 = (u16 *) buf;
- struct ata_port *ap = adev->link->ap;
+ struct ata_port *ap = dev->link->ap;
void __iomem *mmio = ap->ioaddr.data_addr;
struct ixp4xx_pata_data *data = ap->host->dev->platform_data;
@@ -59,30 +59,32 @@ static void ixp4xx_mmio_data_xfer(struct ata_device *adev, unsigned char *buf,
udelay(100);
/* Transfer multiple of 2 bytes */
- if (write_data) {
- for (i = 0; i < words; i++)
- writew(buf16[i], mmio);
- } else {
+ if (rw == READ)
for (i = 0; i < words; i++)
buf16[i] = readw(mmio);
- }
+ else
+ for (i = 0; i < words; i++)
+ writew(buf16[i], mmio);
/* Transfer trailing 1 byte, if any. */
- if (unlikely(buflen & 0x01)) {
+ if (unlikely(len & 0x01)) {
u16 align_buf[1] = { 0 };
- unsigned char *trailing_buf = buf + buflen - 1;
+ unsigned char *trailing_buf = buf + len - 1;
- if (write_data) {
- memcpy(align_buf, trailing_buf, 1);
- writew(align_buf[0], mmio);
- } else {
+ if (rw == READ) {
align_buf[0] = readw(mmio);
memcpy(trailing_buf, align_buf, 1);
+ } else {
+ memcpy(align_buf, trailing_buf, 1);
+ writew(align_buf[0], mmio);
}
+ words++;
}
udelay(100);
*data->cs0_cfg |= 0x01;
+
+ return words << 1;
}
static struct scsi_host_template ixp4xx_sht = {
diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
index 7bed8d8..f04e980 100644
--- a/drivers/ata/pata_legacy.c
+++ b/drivers/ata/pata_legacy.c
@@ -249,13 +249,14 @@ static void pdc20230_set_piomode(struct ata_port *ap, struct ata_device *adev)
}
-static void pdc_data_xfer_vlb(struct ata_device *adev, unsigned char *buf, unsigned int buflen, int write_data)
+static unsigned int pdc_data_xfer_vlb(struct ata_device *dev,
+ unsigned char *buf, unsigned int len, int rw)
{
- struct ata_port *ap = adev->link->ap;
- int slop = buflen & 3;
- unsigned long flags;
-
if (ata_id_has_dword_io(adev->id)) {
+ struct ata_port *ap = dev->link->ap;
+ int slop = len & 3;
+ unsigned long flags;
+
local_irq_save(flags);
/* Perform the 32bit I/O synchronization sequence */
@@ -264,28 +265,29 @@ static void pdc_data_xfer_vlb(struct ata_device *adev, unsigned char *buf, unsig
ioread8(ap->ioaddr.nsect_addr);
/* Now the data */
-
- if (write_data)
- iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
+ if (rw == READ)
+ ioread32_rep(ap->ioaddr.data_addr, buf, len >> 2);
else
- ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
+ iowrite32_rep(ap->ioaddr.data_addr, buf, len >> 2);
if (unlikely(slop)) {
u32 pad;
- if (write_data) {
- memcpy(&pad, buf + buflen - slop, slop);
- pad = le32_to_cpu(pad);
- iowrite32(pad, ap->ioaddr.data_addr);
- } else {
+ if (rw == READ) {
pad = ioread32(ap->ioaddr.data_addr);
pad = cpu_to_le16(pad);
- memcpy(buf + buflen - slop, &pad, slop);
+ memcpy(buf + len - slop, &pad, slop);
+ } else {
+ memcpy(&pad, buf + len - slop, slop);
+ pad = le32_to_cpu(pad);
+ iowrite32(pad, ap->ioaddr.data_addr);
}
+ len += 4 - slop;
}
local_irq_restore(flags);
- }
- else
- ata_data_xfer_noirq(adev, buf, buflen, write_data);
+ } else
+ len = ata_data_xfer_noirq(dev, buf, len, rw);
+
+ return len;
}
static struct ata_port_operations pdc20230_port_ops = {
diff --git a/drivers/ata/pata_qdi.c b/drivers/ata/pata_qdi.c
index 7d4c696..9e1802e 100644
--- a/drivers/ata/pata_qdi.c
+++ b/drivers/ata/pata_qdi.c
@@ -124,31 +124,35 @@ static unsigned int qdi_qc_issue_prot(struct ata_queued_cmd *qc)
return ata_qc_issue_prot(qc);
}
-static void qdi_data_xfer(struct ata_device *adev, unsigned char *buf, unsigned int buflen, int write_data)
+static unsigned int qdi_data_xfer(struct ata_device *dev, unsigned char *buf,
+ unsigned int len, int rw)
{
- struct ata_port *ap = adev->link->ap;
- int slop = buflen & 3;
+ if (ata_id_has_dword_io(dev->id)) {
+ struct ata_port *ap = dev->link->ap;
+ int slop = len & 3;
- if (ata_id_has_dword_io(adev->id)) {
- if (write_data)
- iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
+ if (rw == READ)
+ ioread32_rep(ap->ioaddr.data_addr, buf, len >> 2);
else
- ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
+ iowrite32_rep(ap->ioaddr.data_addr, buf, len >> 2);
if (unlikely(slop)) {
u32 pad;
- if (write_data) {
- memcpy(&pad, buf + buflen - slop, slop);
- pad = le32_to_cpu(pad);
- iowrite32(pad, ap->ioaddr.data_addr);
- } else {
+ if (rw == READ) {
pad = ioread32(ap->ioaddr.data_addr);
pad = cpu_to_le32(pad);
- memcpy(buf + buflen - slop, &pad, slop);
+ memcpy(buf + len - slop, &pad, slop);
+ } else {
+ memcpy(&pad, buf + len - slop, slop);
+ pad = le32_to_cpu(pad);
+ iowrite32(pad, ap->ioaddr.data_addr);
}
+ len += 4 - slop;
}
} else
- ata_data_xfer(adev, buf, buflen, write_data);
+ len = ata_data_xfer(dev, buf, len, rw);
+
+ return len;
}
static struct scsi_host_template qdi_sht = {
diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
index ea2ef9f..755e2b9 100644
--- a/drivers/ata/pata_scc.c
+++ b/drivers/ata/pata_scc.c
@@ -768,45 +768,47 @@ static u8 scc_bmdma_status (struct ata_port *ap)
/**
* scc_data_xfer - Transfer data by PIO
- * @adev: device for this I/O
+ * @dev: device for this I/O
* @buf: data buffer
- * @buflen: buffer length
- * @write_data: read/write
+ * @len: buffer length
+ * @rw: read/write
*
* Note: Original code is ata_data_xfer().
*/
-static void scc_data_xfer (struct ata_device *adev, unsigned char *buf,
- unsigned int buflen, int write_data)
+static unsigned int scc_data_xfer (struct ata_device *dev, unsigned char *buf,
+ unsigned int len, int rw)
{
- struct ata_port *ap = adev->link->ap;
- unsigned int words = buflen >> 1;
+ struct ata_port *ap = dev->link->ap;
+ unsigned int words = len >> 1;
unsigned int i;
u16 *buf16 = (u16 *) buf;
void __iomem *mmio = ap->ioaddr.data_addr;
/* Transfer multiple of 2 bytes */
- if (write_data) {
- for (i = 0; i < words; i++)
- out_be32(mmio, cpu_to_le16(buf16[i]));
- } else {
+ if (rw == READ)
for (i = 0; i < words; i++)
buf16[i] = le16_to_cpu(in_be32(mmio));
- }
+ else
+ for (i = 0; i < words; i++)
+ out_be32(mmio, cpu_to_le16(buf16[i]));
/* Transfer trailing 1 byte, if any. */
- if (unlikely(buflen & 0x01)) {
+ if (unlikely(len & 0x01)) {
u16 align_buf[1] = { 0 };
- unsigned char *trailing_buf = buf + buflen - 1;
+ unsigned char *trailing_buf = buf + len - 1;
- if (write_data) {
- memcpy(align_buf, trailing_buf, 1);
- out_be32(mmio, cpu_to_le16(align_buf[0]));
- } else {
+ if (rw == READ) {
align_buf[0] = le16_to_cpu(in_be32(mmio));
memcpy(trailing_buf, align_buf, 1);
+ } else {
+ memcpy(align_buf, trailing_buf, 1);
+ out_be32(mmio, cpu_to_le16(align_buf[0]));
}
+ words++;
}
+
+ return words << 1;
}
/**
diff --git a/drivers/ata/pata_winbond.c b/drivers/ata/pata_winbond.c
index 311cdb3..9ded084 100644
--- a/drivers/ata/pata_winbond.c
+++ b/drivers/ata/pata_winbond.c
@@ -92,31 +92,35 @@ static void winbond_set_piomode(struct ata_port *ap, struct ata_device *adev)
}
-static void winbond_data_xfer(struct ata_device *adev, unsigned char *buf, unsigned int buflen, int write_data)
+static void winbond_data_xfer(struct ata_device *dev, unsigned char *buf,
+ unsigned int len, int rw)
{
- struct ata_port *ap = adev->link->ap;
- int slop = buflen & 3;
+ struct ata_port *ap = dev->link->ap;
+ int slop = len & 3;
- if (ata_id_has_dword_io(adev->id)) {
- if (write_data)
- iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
+ if (ata_id_has_dword_io(dev->id)) {
+ if (rw == READ)
+ ioread32_rep(ap->ioaddr.data_addr, buf, len >> 2);
else
- ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
+ iowrite32_rep(ap->ioaddr.data_addr, buf, len >> 2);
if (unlikely(slop)) {
u32 pad;
- if (write_data) {
- memcpy(&pad, buf + buflen - slop, slop);
- pad = le32_to_cpu(pad);
- iowrite32(pad, ap->ioaddr.data_addr);
- } else {
+ if (rw == read) {
pad = ioread32(ap->ioaddr.data_addr);
pad = cpu_to_le16(pad);
- memcpy(buf + buflen - slop, &pad, slop);
+ memcpy(buf + len - slop, &pad, slop);
+ } else {
+ memcpy(&pad, buf + len - slop, slop);
+ pad = le32_to_cpu(pad);
+ iowrite32(pad, ap->ioaddr.data_addr);
}
+ len += 4 - slop;
}
} else
- ata_data_xfer(adev, buf, buflen, write_data);
+ len = ata_data_xfer(dev, buf, len, rw);
+
+ return len;
}
static struct scsi_host_template winbond_sht = {
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 72df408..4b39650 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -700,7 +700,8 @@ struct ata_port_operations {
void (*bmdma_setup) (struct ata_queued_cmd *qc);
void (*bmdma_start) (struct ata_queued_cmd *qc);
- void (*data_xfer) (struct ata_device *, unsigned char *, unsigned int, int);
+ unsigned int (*data_xfer) (struct ata_device *dev, unsigned char *buf,
+ unsigned int len, int rw);
int (*qc_defer) (struct ata_queued_cmd *qc);
void (*qc_prep) (struct ata_queued_cmd *qc);
@@ -880,10 +881,10 @@ extern void ata_exec_command(struct ata_port *ap, const struct ata_taskfile *tf)
extern int ata_port_start(struct ata_port *ap);
extern int ata_sff_port_start(struct ata_port *ap);
extern irqreturn_t ata_interrupt(int irq, void *dev_instance);
-extern void ata_data_xfer(struct ata_device *adev, unsigned char *buf,
- unsigned int buflen, int write_data);
-extern void ata_data_xfer_noirq(struct ata_device *adev, unsigned char *buf,
- unsigned int buflen, int write_data);
+extern unsigned int ata_data_xfer(struct ata_device *dev,
+ unsigned char *buf, unsigned int len, int rw);
+extern unsigned int ata_data_xfer_noirq(struct ata_device *dev,
+ unsigned char *buf, unsigned int len, int rw);
extern int ata_std_qc_defer(struct ata_queued_cmd *qc);
extern void ata_dumb_qc_prep(struct ata_queued_cmd *qc);
extern void ata_qc_prep(struct ata_queued_cmd *qc);
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 06/14] libata: improve ATAPI draining
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (4 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 15:59 ` Alan Cox
2007-12-04 10:06 ` Albert Lee
2007-11-29 14:33 ` [PATCH 07/14] libata: make atapi_request_sense() use sg Tejun Heo
` (7 subsequent siblings)
13 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo, Albert Lee
For misc ATAPI commands which transfer variable length data to the
host, overflow can occur due to application or hardware bug. Such
overflows can be ignored safely as long as overflow data is properly
drained. libata HSM implementation has this implemented in
__atapi_pio_bytes() but it isn't enough. Improve drain logic such
that...
* Multiple PIO data phases are allowed. Not allowing this used to be
okay when transfer chunk size was set to 8k unconditionally but with
transfer hcunk size set to allocation size, treating extra PIO data
phases as HSM violations cause a lot of trouble.
* Limit the amount of draining to ATAPI_MAX_DRAIN (16k currently).
* Don't whine if overflow is allowed and safe. When unexpected
overflow occurs, trigger HSM violation and report the problem using
ehi error description.
* Properly calculate the number of bytes to be drained considering
actual number of consumed bytes for partial draining.
* Add and use ata_drain_page for draining. This change fixes the
problem where LLDs which do 32bit IOs consumes 4 bytes on each 2
byte draining resulting in draining twice more data than requested.
This patch fixes ATAPI regressions introduced by setting transfer
chunk size to allocation size.
Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Albert Lee <albertcc@tw.ibm.com>
---
drivers/ata/libata-core.c | 128 ++++++++++++++++++++++++++++++---------------
include/linux/libata.h | 2 +
2 files changed, 88 insertions(+), 42 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 10f3b42..76b8dc9 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -83,8 +83,8 @@ static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
unsigned int ata_print_id = 1;
static struct workqueue_struct *ata_wq;
-
struct workqueue_struct *ata_aux_wq;
+static void *ata_drain_page;
int atapi_enabled = 1;
module_param(atapi_enabled, int, 0444);
@@ -4671,6 +4671,28 @@ int ata_check_atapi_dma(struct ata_queued_cmd *qc)
}
/**
+ * atapi_qc_may_overflow - Check whether data transfer may overflow
+ * @qc: ATA command in question
+ *
+ * ATAPI commands which transfer variable length data to host
+ * might overflow due to application error or hardare bug. This
+ * function checks whether overflow should be drained and ignored
+ * for @qc.
+ *
+ * LOCKING:
+ * None.
+ *
+ * RETURNS:
+ * 1 if @qc may overflow; otherwise, 0.
+ */
+static int atapi_qc_may_overflow(struct ata_queued_cmd *qc)
+{
+ return ata_is_atapi(qc->tf.protocol) && ata_is_data(qc->tf.protocol) &&
+ atapi_cmd_type(qc->cdb[0]) == ATAPI_MISC &&
+ !(qc->tf.flags & ATA_TFLAG_WRITE);
+}
+
+/**
* ata_std_qc_defer - Check whether a qc needs to be deferred
* @qc: ATA command in question
*
@@ -5172,23 +5194,23 @@ static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
* Inherited from caller.
*
*/
-
-static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
+static int __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
{
- int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
- struct scatterlist *sg = qc->__sg;
- struct scatterlist *lsg = sg_last(qc->__sg, qc->n_elem);
+ int rw = (qc->tf.flags & ATA_TFLAG_WRITE) ? WRITE : READ;
struct ata_port *ap = qc->ap;
+ struct ata_device *dev = qc->dev;
+ struct ata_eh_info *ehi = &dev->link->eh_info;
+ struct scatterlist *sg;
struct page *page;
unsigned char *buf;
- unsigned int offset, count;
- int no_more_sg = 0;
-
- if (qc->curbytes + bytes >= qc->nbytes)
- ap->hsm_task_state = HSM_ST_LAST;
+ unsigned int offset, count, consumed;
next_sg:
- if (unlikely(no_more_sg)) {
+ if (!bytes)
+ return 0;
+
+ sg = qc->cursg;
+ if (unlikely(!sg)) {
/*
* The end of qc->sg is reached and the device expects
* more data to transfer. In order not to overrun qc->sg
@@ -5196,23 +5218,32 @@ next_sg:
* - for read case, discard trailing data from the device
* - for write case, padding zero data to the device
*/
- u16 pad_buf[1] = { 0 };
- unsigned int words = bytes >> 1;
- unsigned int i;
+ if (qc->curbytes + bytes > qc->nbytes + ATAPI_MAX_DRAIN) {
+ ata_ehi_push_desc(ehi, "too much trailing data "
+ "buf=%u cur=%u bytes=%u",
+ qc->nbytes, qc->curbytes, bytes);
+ return -1;
+ }
- if (words) /* warning if bytes > 1 */
- ata_dev_printk(qc->dev, KERN_WARNING,
- "%u bytes trailing data\n", bytes);
+ /* allow overflow only for misc ATAPI commands */
+ if (!atapi_qc_may_overflow(qc)) {
+ ata_ehi_push_desc(ehi, "unexpected trailing data "
+ "%u bytes", bytes);
+ return -1;
+ }
- for (i = 0; i < words; i++)
- ap->ops->data_xfer(qc->dev, (unsigned char *)pad_buf, 2, do_write);
+ consumed = 0;
+ while (consumed < bytes) {
+ count = min_t(unsigned int,
+ bytes - consumed, PAGE_SIZE);
+ consumed += ap->ops->data_xfer(dev, ata_drain_page,
+ count, rw);
+ }
- ap->hsm_task_state = HSM_ST_LAST;
- return;
+ qc->curbytes += bytes;
+ return 0;
}
- sg = qc->cursg;
-
page = sg_page(sg);
offset = sg->offset + qc->cursg_ofs;
@@ -5236,29 +5267,28 @@ next_sg:
buf = kmap_atomic(page, KM_IRQ0);
/* do the actual data transfer */
- ap->ops->data_xfer(qc->dev, buf + offset, count, do_write);
+ consumed = ap->ops->data_xfer(dev, buf + offset, count, rw);
kunmap_atomic(buf, KM_IRQ0);
local_irq_restore(flags);
} else {
buf = page_address(page);
- ap->ops->data_xfer(qc->dev, buf + offset, count, do_write);
+ consumed = ap->ops->data_xfer(dev, buf + offset, count, rw);
}
- bytes -= count;
+ bytes -= min(bytes, consumed);
qc->curbytes += count;
qc->cursg_ofs += count;
if (qc->cursg_ofs == sg->length) {
- if (qc->cursg == lsg)
- no_more_sg = 1;
-
qc->cursg = sg_next(qc->cursg);
qc->cursg_ofs = 0;
}
- if (bytes)
- goto next_sg;
+ /* consumed can be larger than count only for the last transfer */
+ WARN_ON(qc->cursg && count != consumed);
+
+ goto next_sg;
}
/**
@@ -5275,6 +5305,7 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct ata_device *dev = qc->dev;
+ struct ata_eh_info *ehi = &dev->link->eh_info;
unsigned int ireason, bc_lo, bc_hi, bytes;
int i_write, do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? 1 : 0;
@@ -5292,22 +5323,24 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
/* shall be cleared to zero, indicating xfer of data */
if (ireason & (1 << 0))
- goto err_out;
+ goto atapi_check;
/* make sure transfer direction matches expected */
i_write = ((ireason & (1 << 1)) == 0) ? 1 : 0;
if (do_write != i_write)
- goto err_out;
+ goto atapi_check;
VPRINTK("ata%u: xfering %d bytes\n", ap->print_id, bytes);
- __atapi_pio_bytes(qc, bytes);
+ if (__atapi_pio_bytes(qc, bytes))
+ goto err_out;
ata_altstatus(ap); /* flush */
return;
-err_out:
- ata_dev_printk(dev, KERN_INFO, "ATAPI check failed\n");
+ atapi_check:
+ ata_ehi_push_desc(ehi, "ATAPI check failed ireason=0x%x", ireason);
+ err_out:
qc->err_mask |= AC_ERR_HSM;
ap->hsm_task_state = HSM_ST_ERR;
}
@@ -7429,24 +7462,35 @@ int ata_pci_device_resume(struct pci_dev *pdev)
static int __init ata_init(void)
{
ata_probe_timeout *= HZ;
+
+ ata_drain_page = (void *)__get_free_page(GFP_DMA32);
+ if (!ata_drain_page)
+ goto err_drain_page;
+
ata_wq = create_workqueue("ata");
if (!ata_wq)
- return -ENOMEM;
+ goto err_ata_wq;
ata_aux_wq = create_singlethread_workqueue("ata_aux");
- if (!ata_aux_wq) {
- destroy_workqueue(ata_wq);
- return -ENOMEM;
- }
+ if (!ata_aux_wq)
+ goto err_ata_aux_wq;
printk(KERN_DEBUG "libata version " DRV_VERSION " loaded.\n");
return 0;
+
+ err_ata_aux_wq:
+ destroy_workqueue(ata_wq);
+ err_ata_wq:
+ free_page((unsigned long)ata_drain_page);
+ err_drain_page:
+ return -ENOMEM;
}
static void __exit ata_exit(void)
{
destroy_workqueue(ata_wq);
destroy_workqueue(ata_aux_wq);
+ free_page((unsigned long)ata_drain_page);
}
subsys_initcall(ata_init);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4b39650..a2070cf 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -120,6 +120,8 @@ enum {
ATA_DEF_BUSY_WAIT = 10000,
ATA_SHORT_PAUSE = (HZ >> 6) + 1,
+ ATAPI_MAX_DRAIN = 16 << 10,
+
ATA_SHT_EMULATED = 1,
ATA_SHT_CMD_PER_LUN = 1,
ATA_SHT_THIS_ID = -1,
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 07/14] libata: make atapi_request_sense() use sg
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (5 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 06/14] libata: improve ATAPI draining Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 08/14] libata: kill non-sg DMA interface Tejun Heo
` (6 subsequent siblings)
13 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo, Rusty Russel
atapi_request_sense() is now the only left user of ata_sg_init_one().
Convert it to use sg interface.
Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Rusty Russel <rusty@rustcorp.com.au>
---
drivers/ata/libata-scsi.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b02e1ac..6251caa 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2340,7 +2340,9 @@ static void atapi_request_sense(struct ata_queued_cmd *qc)
ata_qc_reinit(qc);
- ata_sg_init_one(qc, cmd->sense_buffer, sizeof(cmd->sense_buffer));
+ /* setup sg table and init transfer direction */
+ sg_init_one(&qc->sgent, cmd->sense_buffer, sizeof(cmd->sense_buffer));
+ ata_sg_init(qc, &qc->sgent, 1);
qc->dma_dir = DMA_FROM_DEVICE;
memset(&qc->cdb, 0, qc->dev->cdb_len);
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 08/14] libata: kill non-sg DMA interface
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (6 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 07/14] libata: make atapi_request_sense() use sg Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 16:00 ` Alan Cox
2007-12-04 19:31 ` Jeff Garzik
2007-11-29 14:33 ` [PATCH 09/14] libata: change ATA_QCFLAG_DMAMAP semantics Tejun Heo
` (5 subsequent siblings)
13 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo, Rusty Russel
With atapi_request_sense() converted to use sg, there's no user of
non-sg interface. Kill non-sg interface.
* ATA_QCFLAG_SINGLE and ATA_QCFLAG_SG are removed. ATA_QCFLAG_DMAMAP
is used instead. (this way no LLD change is necessary)
* qc->buf_virt is removed.
* ata_sg_init_one() and ata_sg_setup_one() are removed.
Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Rusty Russel <rusty@rustcorp.com.au>
---
drivers/ata/libata-core.c | 148 +++++----------------------------------------
include/linux/libata.h | 7 +--
2 files changed, 16 insertions(+), 139 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 76b8dc9..c492746 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4487,9 +4487,6 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
WARN_ON(!(qc->flags & ATA_QCFLAG_DMAMAP));
WARN_ON(sg == NULL);
- if (qc->flags & ATA_QCFLAG_SINGLE)
- WARN_ON(qc->n_elem > 1);
-
VPRINTK("unmapping %u sg elements\n", qc->n_elem);
/* if we padded the buffer out to 32-bit bound, and data
@@ -4499,27 +4496,15 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
if (qc->pad_len && !(qc->tf.flags & ATA_TFLAG_WRITE))
pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
- if (qc->flags & ATA_QCFLAG_SG) {
- if (qc->n_elem)
- dma_unmap_sg(ap->dev, sg, qc->n_elem, dir);
- /* restore last sg */
- sg_last(sg, qc->orig_n_elem)->length += qc->pad_len;
- if (pad_buf) {
- struct scatterlist *psg = &qc->pad_sgent;
- void *addr = kmap_atomic(sg_page(psg), KM_IRQ0);
- memcpy(addr + psg->offset, pad_buf, qc->pad_len);
- kunmap_atomic(addr, KM_IRQ0);
- }
- } else {
- if (qc->n_elem)
- dma_unmap_single(ap->dev,
- sg_dma_address(&sg[0]), sg_dma_len(&sg[0]),
- dir);
- /* restore sg */
- sg->length += qc->pad_len;
- if (pad_buf)
- memcpy(qc->buf_virt + sg->length - qc->pad_len,
- pad_buf, qc->pad_len);
+ if (qc->n_elem)
+ dma_unmap_sg(ap->dev, sg, qc->n_elem, dir);
+ /* restore last sg */
+ sg_last(sg, qc->orig_n_elem)->length += qc->pad_len;
+ if (pad_buf) {
+ struct scatterlist *psg = &qc->pad_sgent;
+ void *addr = kmap_atomic(sg_page(psg), KM_IRQ0);
+ memcpy(addr + psg->offset, pad_buf, qc->pad_len);
+ kunmap_atomic(addr, KM_IRQ0);
}
qc->flags &= ~ATA_QCFLAG_DMAMAP;
@@ -4759,33 +4744,6 @@ void ata_dumb_qc_prep(struct ata_queued_cmd *qc)
void ata_noop_qc_prep(struct ata_queued_cmd *qc) { }
/**
- * ata_sg_init_one - Associate command with memory buffer
- * @qc: Command to be associated
- * @buf: Memory buffer
- * @buflen: Length of memory buffer, in bytes.
- *
- * Initialize the data-related elements of queued_cmd @qc
- * to point to a single memory buffer, @buf of byte length @buflen.
- *
- * LOCKING:
- * spin_lock_irqsave(host lock)
- */
-
-void ata_sg_init_one(struct ata_queued_cmd *qc, void *buf, unsigned int buflen)
-{
- qc->flags |= ATA_QCFLAG_SINGLE;
-
- qc->__sg = &qc->sgent;
- qc->n_elem = 1;
- qc->orig_n_elem = 1;
- qc->buf_virt = buf;
- qc->nbytes = buflen;
- qc->cursg = qc->__sg;
-
- sg_init_one(&qc->sgent, buf, buflen);
-}
-
-/**
* ata_sg_init - Associate command with scatter-gather table.
* @qc: Command to be associated
* @sg: Scatter-gather table.
@@ -4802,7 +4760,7 @@ void ata_sg_init_one(struct ata_queued_cmd *qc, void *buf, unsigned int buflen)
void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
unsigned int n_elem)
{
- qc->flags |= ATA_QCFLAG_SG;
+ qc->flags |= ATA_QCFLAG_DMAMAP;
qc->__sg = sg;
qc->n_elem = n_elem;
qc->orig_n_elem = n_elem;
@@ -4810,75 +4768,6 @@ void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
}
/**
- * ata_sg_setup_one - DMA-map the memory buffer associated with a command.
- * @qc: Command with memory buffer to be mapped.
- *
- * DMA-map the memory buffer associated with queued_cmd @qc.
- *
- * LOCKING:
- * spin_lock_irqsave(host lock)
- *
- * RETURNS:
- * Zero on success, negative on error.
- */
-
-static int ata_sg_setup_one(struct ata_queued_cmd *qc)
-{
- struct ata_port *ap = qc->ap;
- int dir = qc->dma_dir;
- struct scatterlist *sg = qc->__sg;
- dma_addr_t dma_address;
- int trim_sg = 0;
-
- /* we must lengthen transfers to end on a 32-bit boundary */
- qc->pad_len = sg->length & 3;
- if (qc->pad_len) {
- void *pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
- struct scatterlist *psg = &qc->pad_sgent;
-
- WARN_ON(qc->dev->class != ATA_DEV_ATAPI);
-
- memset(pad_buf, 0, ATA_DMA_PAD_SZ);
-
- if (qc->tf.flags & ATA_TFLAG_WRITE)
- memcpy(pad_buf, qc->buf_virt + sg->length - qc->pad_len,
- qc->pad_len);
-
- sg_dma_address(psg) = ap->pad_dma + (qc->tag * ATA_DMA_PAD_SZ);
- sg_dma_len(psg) = ATA_DMA_PAD_SZ;
- /* trim sg */
- sg->length -= qc->pad_len;
- if (sg->length == 0)
- trim_sg = 1;
-
- DPRINTK("padding done, sg->length=%u pad_len=%u\n",
- sg->length, qc->pad_len);
- }
-
- if (trim_sg) {
- qc->n_elem--;
- goto skip_map;
- }
-
- dma_address = dma_map_single(ap->dev, qc->buf_virt,
- sg->length, dir);
- if (dma_mapping_error(dma_address)) {
- /* restore sg */
- sg->length += qc->pad_len;
- return -1;
- }
-
- sg_dma_address(sg) = dma_address;
- sg_dma_len(sg) = sg->length;
-
-skip_map:
- DPRINTK("mapped buffer of %d bytes for %s\n", sg_dma_len(sg),
- qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
-
- return 0;
-}
-
-/**
* ata_sg_setup - DMA-map the scatter-gather table associated with a command.
* @qc: Command with scatter-gather table to be mapped.
*
@@ -4900,7 +4789,7 @@ static int ata_sg_setup(struct ata_queued_cmd *qc)
int n_elem, pre_n_elem, dir, trim_sg = 0;
VPRINTK("ENTER, ata%u\n", ap->print_id);
- WARN_ON(!(qc->flags & ATA_QCFLAG_SG));
+ WARN_ON(!(qc->flags & ATA_QCFLAG_DMAMAP));
/* we must lengthen transfers to end on a 32-bit boundary */
qc->pad_len = lsg->length & 3;
@@ -6022,16 +5911,10 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
if (ata_is_dma(prot) || (ata_is_pio(prot) &&
(ap->flags & ATA_FLAG_PIO_DMA))) {
- if (qc->flags & ATA_QCFLAG_SG) {
- if (ata_sg_setup(qc))
- goto sg_err;
- } else if (qc->flags & ATA_QCFLAG_SINGLE) {
- if (ata_sg_setup_one(qc))
- goto sg_err;
- }
- } else {
- qc->flags &= ~ATA_QCFLAG_DMAMAP;
- }
+ if (ata_sg_setup(qc))
+ goto sg_err;
+ } else
+ qc->flags &= ATA_QCFLAG_DMAMAP;
/* if device is sleeping, schedule softreset and abort the link */
if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
@@ -7620,7 +7503,6 @@ EXPORT_SYMBOL_GPL(ata_host_register);
EXPORT_SYMBOL_GPL(ata_host_activate);
EXPORT_SYMBOL_GPL(ata_host_detach);
EXPORT_SYMBOL_GPL(ata_sg_init);
-EXPORT_SYMBOL_GPL(ata_sg_init_one);
EXPORT_SYMBOL_GPL(ata_hsm_move);
EXPORT_SYMBOL_GPL(ata_qc_complete);
EXPORT_SYMBOL_GPL(ata_qc_complete_multiple);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index a2070cf..6869900 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -220,9 +220,7 @@ enum {
/* struct ata_queued_cmd flags */
ATA_QCFLAG_ACTIVE = (1 << 0), /* cmd not yet ack'd to scsi lyer */
- ATA_QCFLAG_SG = (1 << 1), /* have s/g table? */
- ATA_QCFLAG_SINGLE = (1 << 2), /* no s/g, just a single buffer */
- ATA_QCFLAG_DMAMAP = ATA_QCFLAG_SG | ATA_QCFLAG_SINGLE,
+ ATA_QCFLAG_DMAMAP = (1 << 1), /* SG table is DMA mapped */
ATA_QCFLAG_IO = (1 << 3), /* standard IO command */
ATA_QCFLAG_RESULT_TF = (1 << 4), /* result TF requested */
ATA_QCFLAG_CLEAR_EXCL = (1 << 5), /* clear excl_link on completion */
@@ -476,7 +474,6 @@ struct ata_queued_cmd {
struct scatterlist sgent;
struct scatterlist pad_sgent;
- void *buf_virt;
/* DO NOT iterate over __sg manually, use ata_for_each_sg() */
struct scatterlist *__sg;
@@ -892,8 +889,6 @@ extern void ata_dumb_qc_prep(struct ata_queued_cmd *qc);
extern void ata_qc_prep(struct ata_queued_cmd *qc);
extern void ata_noop_qc_prep(struct ata_queued_cmd *qc);
extern unsigned int ata_qc_issue_prot(struct ata_queued_cmd *qc);
-extern void ata_sg_init_one(struct ata_queued_cmd *qc, void *buf,
- unsigned int buflen);
extern void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
unsigned int n_elem);
extern unsigned int ata_dev_classify(const struct ata_taskfile *tf);
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 09/14] libata: change ATA_QCFLAG_DMAMAP semantics
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (7 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 08/14] libata: kill non-sg DMA interface Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 10/14] libata: convert to chained sg Tejun Heo
` (4 subsequent siblings)
13 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
ATA_QCFLAG_DMAMAP was a bit peculiar in that it got set during qc
initialization and cleared if DMA mapping wasn't necessary. Make it
more straight forward by making the following changes.
* Don't set it during initialization. Set it after DMA is actually
mapped.
* Add BUG_ON() to guarantee that there is data to transfer if DMAMAP
is set. This always holds for the current code. The BUG_ON() is
for docummentation and sanity check.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/ata/libata-core.c | 15 +++++++--------
1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c492746..1bc4edb 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4484,7 +4484,6 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
int dir = qc->dma_dir;
void *pad_buf = NULL;
- WARN_ON(!(qc->flags & ATA_QCFLAG_DMAMAP));
WARN_ON(sg == NULL);
VPRINTK("unmapping %u sg elements\n", qc->n_elem);
@@ -4756,11 +4755,9 @@ void ata_noop_qc_prep(struct ata_queued_cmd *qc) { }
* LOCKING:
* spin_lock_irqsave(host lock)
*/
-
void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
unsigned int n_elem)
{
- qc->flags |= ATA_QCFLAG_DMAMAP;
qc->__sg = sg;
qc->n_elem = n_elem;
qc->orig_n_elem = n_elem;
@@ -4789,7 +4786,6 @@ static int ata_sg_setup(struct ata_queued_cmd *qc)
int n_elem, pre_n_elem, dir, trim_sg = 0;
VPRINTK("ENTER, ata%u\n", ap->print_id);
- WARN_ON(!(qc->flags & ATA_QCFLAG_DMAMAP));
/* we must lengthen transfers to end on a 32-bit boundary */
qc->pad_len = lsg->length & 3;
@@ -4849,6 +4845,7 @@ static int ata_sg_setup(struct ata_queued_cmd *qc)
skip_map:
qc->n_elem = n_elem;
+ qc->flags |= ATA_QCFLAG_DMAMAP;
return 0;
}
@@ -5909,12 +5906,15 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
qc->flags |= ATA_QCFLAG_ACTIVE;
ap->qc_active |= 1 << qc->tag;
+ /* We guarantee to LLDs that they will have at least one
+ * non-zero sg if the command is a data command.
+ */
+ BUG_ON(ata_is_data(prot) && (!qc->__sg || !qc->n_elem || !qc->nbytes));
+
if (ata_is_dma(prot) || (ata_is_pio(prot) &&
- (ap->flags & ATA_FLAG_PIO_DMA))) {
+ (ap->flags & ATA_FLAG_PIO_DMA)))
if (ata_sg_setup(qc))
goto sg_err;
- } else
- qc->flags &= ATA_QCFLAG_DMAMAP;
/* if device is sleeping, schedule softreset and abort the link */
if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
@@ -5932,7 +5932,6 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
return;
sg_err:
- qc->flags &= ~ATA_QCFLAG_DMAMAP;
qc->err_mask |= AC_ERR_SYSTEM;
err:
ata_qc_complete(qc);
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 10/14] libata: convert to chained sg
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (8 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 09/14] libata: change ATA_QCFLAG_DMAMAP semantics Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 11/14] libata: add qc->dma_nbytes Tejun Heo
` (3 subsequent siblings)
13 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl; +Cc: Tejun Heo, Jens Axboe
libata used private sg iterator to handle padding sg. Now that sg can
be chained, padding can be handled using standard sg ops. Convert to
chained sg.
* s/qc->__sg/qc->sg/
* s/qc->pad_sgent/qc->extra_sg[]/. Because chaining consumes one sg
entry. There need to be two extra sg entries. The renaming is also
for future addition of other extra sg entries.
* Padding setup is moved into ata_sg_setup_extra() which is organized
in a way that future addition of other extra sg entries is easy.
* qc->orig_n_elem is unused and removed.
* qc->n_elem now contains the number of sg entries that LLDs should
map. qc->mapped_n_elem is added to carry the original number of
mapped sgs for unmapping.
* The last sg of the original sg list is used to chain to extra sg
list. The original last sg is pointed to by qc->last_sg and the
content is stored in qc->saved_last_sg. It's restored during
ata_sg_clean().
* All sg walking code has been updated. Unnecessary assertions and
checks for conditions the core layer already guarantees are removed.
Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
drivers/ata/ahci.c | 18 ++---
drivers/ata/libata-core.c | 201 ++++++++++++++++++++++++-----------------
drivers/ata/libata-scsi.c | 2 +-
drivers/ata/pata_bf54x.c | 13 ++-
drivers/ata/pata_icside.c | 3 +-
drivers/ata/pdc_adma.c | 3 +-
drivers/ata/sata_fsl.c | 4 +-
drivers/ata/sata_mv.c | 3 +-
drivers/ata/sata_nv.c | 25 ++----
drivers/ata/sata_promise.c | 40 ++++-----
drivers/ata/sata_qstor.c | 13 +--
drivers/ata/sata_sil24.c | 6 +-
drivers/ata/sata_sx4.c | 4 +-
drivers/scsi/ipr.c | 3 +-
drivers/scsi/libsas/sas_ata.c | 10 +--
include/linux/libata.h | 42 ++-------
16 files changed, 193 insertions(+), 197 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index a8fc9c0..6fb67ba 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1477,28 +1477,24 @@ static void ahci_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
static unsigned int ahci_fill_sg(struct ata_queued_cmd *qc, void *cmd_tbl)
{
struct scatterlist *sg;
- struct ahci_sg *ahci_sg;
- unsigned int n_sg = 0;
+ struct ahci_sg *ahci_sg = cmd_tbl + AHCI_CMD_TBL_HDR_SZ;
+ unsigned int si;
VPRINTK("ENTER\n");
/*
* Next, the S/G list.
*/
- ahci_sg = cmd_tbl + AHCI_CMD_TBL_HDR_SZ;
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
dma_addr_t addr = sg_dma_address(sg);
u32 sg_len = sg_dma_len(sg);
- ahci_sg->addr = cpu_to_le32(addr & 0xffffffff);
- ahci_sg->addr_hi = cpu_to_le32((addr >> 16) >> 16);
- ahci_sg->flags_size = cpu_to_le32(sg_len - 1);
-
- ahci_sg++;
- n_sg++;
+ ahci_sg[si].addr = cpu_to_le32(addr & 0xffffffff);
+ ahci_sg[si].addr_hi = cpu_to_le32((addr >> 16) >> 16);
+ ahci_sg[si].flags_size = cpu_to_le32(sg_len - 1);
}
- return n_sg;
+ return si;
}
static void ahci_qc_prep(struct ata_queued_cmd *qc)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1bc4edb..652c5c5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4480,13 +4480,13 @@ static unsigned int ata_dev_init_params(struct ata_device *dev,
void ata_sg_clean(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
- struct scatterlist *sg = qc->__sg;
+ struct scatterlist *sg = qc->sg;
int dir = qc->dma_dir;
void *pad_buf = NULL;
WARN_ON(sg == NULL);
- VPRINTK("unmapping %u sg elements\n", qc->n_elem);
+ VPRINTK("unmapping %u sg elements\n", qc->mapped_n_elem);
/* if we padded the buffer out to 32-bit bound, and data
* xfer direction is from-device, we must copy from the
@@ -4495,19 +4495,20 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
if (qc->pad_len && !(qc->tf.flags & ATA_TFLAG_WRITE))
pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
- if (qc->n_elem)
- dma_unmap_sg(ap->dev, sg, qc->n_elem, dir);
+ if (qc->mapped_n_elem)
+ dma_unmap_sg(ap->dev, sg, qc->mapped_n_elem, dir);
/* restore last sg */
- sg_last(sg, qc->orig_n_elem)->length += qc->pad_len;
+ if (qc->last_sg)
+ *qc->last_sg = qc->saved_last_sg;
if (pad_buf) {
- struct scatterlist *psg = &qc->pad_sgent;
+ struct scatterlist *psg = &qc->extra_sg[1];
void *addr = kmap_atomic(sg_page(psg), KM_IRQ0);
memcpy(addr + psg->offset, pad_buf, qc->pad_len);
kunmap_atomic(addr, KM_IRQ0);
}
qc->flags &= ~ATA_QCFLAG_DMAMAP;
- qc->__sg = NULL;
+ qc->sg = NULL;
}
/**
@@ -4525,13 +4526,10 @@ static void ata_fill_sg(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
- unsigned int idx;
+ unsigned int si, pi;
- WARN_ON(qc->__sg == NULL);
- WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
-
- idx = 0;
- ata_for_each_sg(sg, qc) {
+ pi = 0;
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
u32 addr, offset;
u32 sg_len, len;
@@ -4548,18 +4546,17 @@ static void ata_fill_sg(struct ata_queued_cmd *qc)
if ((offset + sg_len) > 0x10000)
len = 0x10000 - offset;
- ap->prd[idx].addr = cpu_to_le32(addr);
- ap->prd[idx].flags_len = cpu_to_le32(len & 0xffff);
- VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", idx, addr, len);
+ ap->prd[pi].addr = cpu_to_le32(addr);
+ ap->prd[pi].flags_len = cpu_to_le32(len & 0xffff);
+ VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", pi, addr, len);
- idx++;
+ pi++;
sg_len -= len;
addr += len;
}
}
- if (idx)
- ap->prd[idx - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
+ ap->prd[pi - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
}
/**
@@ -4579,13 +4576,10 @@ static void ata_fill_sg_dumb(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
- unsigned int idx;
-
- WARN_ON(qc->__sg == NULL);
- WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
+ unsigned int si, pi;
- idx = 0;
- ata_for_each_sg(sg, qc) {
+ pi = 0;
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
u32 addr, offset;
u32 sg_len, len, blen;
@@ -4603,25 +4597,24 @@ static void ata_fill_sg_dumb(struct ata_queued_cmd *qc)
len = 0x10000 - offset;
blen = len & 0xffff;
- ap->prd[idx].addr = cpu_to_le32(addr);
+ ap->prd[pi].addr = cpu_to_le32(addr);
if (blen == 0) {
/* Some PATA chipsets like the CS5530 can't
cope with 0x0000 meaning 64K as the spec says */
- ap->prd[idx].flags_len = cpu_to_le32(0x8000);
+ ap->prd[pi].flags_len = cpu_to_le32(0x8000);
blen = 0x8000;
- ap->prd[++idx].addr = cpu_to_le32(addr + 0x8000);
+ ap->prd[++pi].addr = cpu_to_le32(addr + 0x8000);
}
- ap->prd[idx].flags_len = cpu_to_le32(blen);
- VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", idx, addr, len);
+ ap->prd[pi].flags_len = cpu_to_le32(blen);
+ VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", pi, addr, len);
- idx++;
+ pi++;
sg_len -= len;
addr += len;
}
}
- if (idx)
- ap->prd[idx - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
+ ap->prd[pi - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
}
/**
@@ -4758,54 +4751,48 @@ void ata_noop_qc_prep(struct ata_queued_cmd *qc) { }
void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
unsigned int n_elem)
{
- qc->__sg = sg;
+ qc->sg = sg;
qc->n_elem = n_elem;
- qc->orig_n_elem = n_elem;
- qc->cursg = qc->__sg;
+ qc->cursg = qc->sg;
}
-/**
- * ata_sg_setup - DMA-map the scatter-gather table associated with a command.
- * @qc: Command with scatter-gather table to be mapped.
- *
- * DMA-map the scatter-gather table associated with queued_cmd @qc.
- *
- * LOCKING:
- * spin_lock_irqsave(host lock)
- *
- * RETURNS:
- * Zero on success, negative on error.
- *
- */
-
-static int ata_sg_setup(struct ata_queued_cmd *qc)
+static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
+ unsigned int *n_elem_extra)
{
struct ata_port *ap = qc->ap;
- struct scatterlist *sg = qc->__sg;
- struct scatterlist *lsg = sg_last(qc->__sg, qc->n_elem);
- int n_elem, pre_n_elem, dir, trim_sg = 0;
+ unsigned int n_elem = qc->n_elem;
+ struct scatterlist *lsg, *copy_lsg = NULL, *tsg = NULL, *esg = NULL;
- VPRINTK("ENTER, ata%u\n", ap->print_id);
+ *n_elem_extra = 0;
+
+ /* needs padding? */
+ qc->pad_len = qc->nbytes & 3;
+
+ if (likely(!qc->pad_len))
+ return n_elem;
+
+ /* locate last sg and save it */
+ lsg = sg_last(qc->sg, n_elem);
+ qc->last_sg = lsg;
+ qc->saved_last_sg = *lsg;
+
+ sg_init_table(qc->extra_sg, ARRAY_SIZE(qc->extra_sg));
- /* we must lengthen transfers to end on a 32-bit boundary */
- qc->pad_len = lsg->length & 3;
if (qc->pad_len) {
+ struct scatterlist *psg = &qc->extra_sg[1];
void *pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
- struct scatterlist *psg = &qc->pad_sgent;
unsigned int offset;
WARN_ON(qc->dev->class != ATA_DEV_ATAPI);
memset(pad_buf, 0, ATA_DMA_PAD_SZ);
- /*
- * psg->page/offset are used to copy to-be-written
+ /* psg->page/offset are used to copy to-be-written
* data in this function or read data in ata_sg_clean.
*/
offset = lsg->offset + lsg->length - qc->pad_len;
- sg_init_table(psg, 1);
sg_set_page(psg, nth_page(sg_page(lsg), offset >> PAGE_SHIFT),
- qc->pad_len, offset_in_page(offset));
+ qc->pad_len, offset_in_page(offset));
if (qc->tf.flags & ATA_TFLAG_WRITE) {
void *addr = kmap_atomic(sg_page(psg), KM_IRQ0);
@@ -4815,36 +4802,84 @@ static int ata_sg_setup(struct ata_queued_cmd *qc)
sg_dma_address(psg) = ap->pad_dma + (qc->tag * ATA_DMA_PAD_SZ);
sg_dma_len(psg) = ATA_DMA_PAD_SZ;
- /* trim last sg */
+
+ /* Trim the last sg entry and chain the original and
+ * padding sg lists.
+ *
+ * Because chaining consumes one sg entry, one extra
+ * sg entry is allocated and the last sg entry is
+ * copied to it if the length isn't zero after padded
+ * amount is removed.
+ *
+ * If the last sg entry is completely replaced by
+ * padding sg entry, the first sg entry is skipped
+ * while chaining.
+ */
lsg->length -= qc->pad_len;
- if (lsg->length == 0)
- trim_sg = 1;
+ if (lsg->length) {
+ copy_lsg = &qc->extra_sg[0];
+ tsg = &qc->extra_sg[0];
+ } else {
+ n_elem--;
+ tsg = &qc->extra_sg[1];
+ }
+
+ esg = &qc->extra_sg[1];
- DPRINTK("padding done, sg[%d].length=%u pad_len=%u\n",
- qc->n_elem - 1, lsg->length, qc->pad_len);
+ (*n_elem_extra)++;
}
- pre_n_elem = qc->n_elem;
- if (trim_sg && pre_n_elem)
- pre_n_elem--;
+ if (copy_lsg)
+ sg_set_page(copy_lsg, sg_page(lsg), lsg->length, lsg->offset);
- if (!pre_n_elem) {
- n_elem = 0;
- goto skip_map;
+ sg_chain(lsg, 1, tsg);
+ sg_mark_end(esg);
+
+ /* sglist can't start with chaining sg entry, fast forward */
+ if (qc->sg == lsg) {
+ qc->sg = tsg;
+ qc->cursg = tsg;
}
- dir = qc->dma_dir;
- n_elem = dma_map_sg(ap->dev, sg, pre_n_elem, dir);
- if (n_elem < 1) {
- /* restore last sg */
- lsg->length += qc->pad_len;
- return -1;
+ return n_elem;
+}
+
+/**
+ * ata_sg_setup - DMA-map the scatter-gather table associated with a command.
+ * @qc: Command with scatter-gather table to be mapped.
+ *
+ * DMA-map the scatter-gather table associated with queued_cmd @qc.
+ *
+ * LOCKING:
+ * spin_lock_irqsave(host lock)
+ *
+ * RETURNS:
+ * Zero on success, negative on error.
+ *
+ */
+static int ata_sg_setup(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ unsigned int n_elem, n_elem_extra;
+
+ VPRINTK("ENTER, ata%u\n", ap->print_id);
+
+ n_elem = ata_sg_setup_extra(qc, &n_elem_extra);
+
+ if (n_elem) {
+ n_elem = dma_map_sg(ap->dev, qc->sg, n_elem, qc->dma_dir);
+ if (n_elem < 1) {
+ /* restore last sg */
+ if (qc->last_sg)
+ *qc->last_sg = qc->saved_last_sg;
+ return -1;
+ }
+ DPRINTK("%d sg elements mapped\n", n_elem);
}
- DPRINTK("%d sg elements mapped\n", n_elem);
+ qc->n_elem = qc->mapped_n_elem = n_elem;
+ qc->n_elem += n_elem_extra;
-skip_map:
- qc->n_elem = n_elem;
qc->flags |= ATA_QCFLAG_DMAMAP;
return 0;
@@ -5909,7 +5944,7 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
/* We guarantee to LLDs that they will have at least one
* non-zero sg if the command is a data command.
*/
- BUG_ON(ata_is_data(prot) && (!qc->__sg || !qc->n_elem || !qc->nbytes));
+ BUG_ON(ata_is_data(prot) && (!qc->sg || !qc->n_elem || !qc->nbytes));
if (ata_is_dma(prot) || (ata_is_pio(prot) &&
(ap->flags & ATA_FLAG_PIO_DMA)))
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 6251caa..f523e66 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -517,7 +517,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
qc->scsicmd = cmd;
qc->scsidone = done;
- qc->__sg = scsi_sglist(cmd);
+ qc->sg = scsi_sglist(cmd);
qc->n_elem = scsi_sg_count(cmd);
} else {
cmd->result = (DID_OK << 16) | (QUEUE_FULL << 1);
diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
index 53ae7d3..d00d219 100644
--- a/drivers/ata/pata_bf54x.c
+++ b/drivers/ata/pata_bf54x.c
@@ -832,6 +832,7 @@ static void bfin_bmdma_setup(struct ata_queued_cmd *qc)
{
unsigned short config = WDSIZE_16;
struct scatterlist *sg;
+ unsigned int si;
pr_debug("in atapi dma setup\n");
/* Program the ATA_CTRL register with dir */
@@ -839,7 +840,7 @@ static void bfin_bmdma_setup(struct ata_queued_cmd *qc)
/* fill the ATAPI DMA controller */
set_dma_config(CH_ATAPI_TX, config);
set_dma_x_modify(CH_ATAPI_TX, 2);
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
set_dma_start_addr(CH_ATAPI_TX, sg_dma_address(sg));
set_dma_x_count(CH_ATAPI_TX, sg_dma_len(sg) >> 1);
}
@@ -848,7 +849,7 @@ static void bfin_bmdma_setup(struct ata_queued_cmd *qc)
/* fill the ATAPI DMA controller */
set_dma_config(CH_ATAPI_RX, config);
set_dma_x_modify(CH_ATAPI_RX, 2);
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
set_dma_start_addr(CH_ATAPI_RX, sg_dma_address(sg));
set_dma_x_count(CH_ATAPI_RX, sg_dma_len(sg) >> 1);
}
@@ -867,6 +868,7 @@ static void bfin_bmdma_start(struct ata_queued_cmd *qc)
struct ata_port *ap = qc->ap;
void __iomem *base = (void __iomem *)ap->ioaddr.ctl_addr;
struct scatterlist *sg;
+ unsigned int si;
pr_debug("in atapi dma start\n");
if (!(ap->udma_mask || ap->mwdma_mask))
@@ -881,7 +883,7 @@ static void bfin_bmdma_start(struct ata_queued_cmd *qc)
* data cache is enabled. Otherwise, this loop
* is an empty loop and optimized out.
*/
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
flush_dcache_range(sg_dma_address(sg),
sg_dma_address(sg) + sg_dma_len(sg));
}
@@ -910,7 +912,7 @@ static void bfin_bmdma_start(struct ata_queued_cmd *qc)
ATAPI_SET_CONTROL(base, ATAPI_GET_CONTROL(base) | TFRCNT_RST);
/* Set transfer length to buffer len */
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
ATAPI_SET_XFER_LEN(base, (sg_dma_len(sg) >> 1));
}
@@ -932,6 +934,7 @@ static void bfin_bmdma_stop(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
+ unsigned int si;
pr_debug("in atapi dma stop\n");
if (!(ap->udma_mask || ap->mwdma_mask))
@@ -950,7 +953,7 @@ static void bfin_bmdma_stop(struct ata_queued_cmd *qc)
* data cache is enabled. Otherwise, this loop
* is an empty loop and optimized out.
*/
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
invalidate_dcache_range(
sg_dma_address(sg),
sg_dma_address(sg)
diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
index 842fe08..5b8586d 100644
--- a/drivers/ata/pata_icside.c
+++ b/drivers/ata/pata_icside.c
@@ -224,6 +224,7 @@ static void pata_icside_bmdma_setup(struct ata_queued_cmd *qc)
struct pata_icside_state *state = ap->host->private_data;
struct scatterlist *sg, *rsg = state->sg;
unsigned int write = qc->tf.flags & ATA_TFLAG_WRITE;
+ unsigned int si;
/*
* We are simplex; BUG if we try to fiddle with DMA
@@ -234,7 +235,7 @@ static void pata_icside_bmdma_setup(struct ata_queued_cmd *qc)
/*
* Copy ATAs scattered sg list into a contiguous array of sg
*/
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
memcpy(rsg, sg, sizeof(*sg));
rsg++;
}
diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
index 459cb7b..8e1b7e9 100644
--- a/drivers/ata/pdc_adma.c
+++ b/drivers/ata/pdc_adma.c
@@ -321,8 +321,9 @@ static int adma_fill_sg(struct ata_queued_cmd *qc)
u8 *buf = pp->pkt, *last_buf = NULL;
int i = (2 + buf[3]) * 8;
u8 pFLAGS = pORD | ((qc->tf.flags & ATA_TFLAG_WRITE) ? pDIRO : 0);
+ unsigned int si;
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
u32 addr;
u32 len;
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index a3c33f1..d041709 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -323,6 +323,7 @@ static unsigned int sata_fsl_fill_sg(struct ata_queued_cmd *qc, void *cmd_desc,
struct scatterlist *sg;
unsigned int num_prde = 0;
u32 ttl_dwords = 0;
+ unsigned int si;
/*
* NOTE : direct & indirect prdt's are contigiously allocated
@@ -333,13 +334,14 @@ static unsigned int sata_fsl_fill_sg(struct ata_queued_cmd *qc, void *cmd_desc,
struct prde *prd_ptr_to_indirect_ext = NULL;
unsigned indirect_ext_segment_sz = 0;
dma_addr_t indirect_ext_segment_paddr;
+ unsigned int si;
VPRINTK("SATA FSL : cd = 0x%x, prd = 0x%x\n", cmd_desc, prd);
indirect_ext_segment_paddr = cmd_desc_paddr +
SATA_FSL_CMD_DESC_OFFSET_TO_PRDT + SATA_FSL_MAX_PRD_DIRECT * 16;
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
dma_addr_t sg_addr = sg_dma_address(sg);
u32 sg_len = sg_dma_len(sg);
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index a43f64d..503d3d4 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -1127,9 +1127,10 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
struct mv_port_priv *pp = qc->ap->private_data;
struct scatterlist *sg;
struct mv_sg *mv_sg, *last_sg = NULL;
+ unsigned int si;
mv_sg = pp->sg_tbl;
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
dma_addr_t addr = sg_dma_address(sg);
u32 sg_len = sg_dma_len(sg);
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 44f9e5d..b7b5945 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -1334,21 +1334,18 @@ static void nv_adma_fill_aprd(struct ata_queued_cmd *qc,
static void nv_adma_fill_sg(struct ata_queued_cmd *qc, struct nv_adma_cpb *cpb)
{
struct nv_adma_port_priv *pp = qc->ap->private_data;
- unsigned int idx;
struct nv_adma_prd *aprd;
struct scatterlist *sg;
+ unsigned int si;
VPRINTK("ENTER\n");
- idx = 0;
-
- ata_for_each_sg(sg, qc) {
- aprd = (idx < 5) ? &cpb->aprd[idx] :
- &pp->aprd[NV_ADMA_SGTBL_LEN * qc->tag + (idx-5)];
- nv_adma_fill_aprd(qc, sg, idx, aprd);
- idx++;
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
+ aprd = (si < 5) ? &cpb->aprd[si] :
+ &pp->aprd[NV_ADMA_SGTBL_LEN * qc->tag + (si-5)];
+ nv_adma_fill_aprd(qc, sg, si, aprd);
}
- if (idx > 5)
+ if (si > 5)
cpb->next_aprd = cpu_to_le64(((u64)(pp->aprd_dma + NV_ADMA_SGTBL_SZ * qc->tag)));
else
cpb->next_aprd = cpu_to_le64(0);
@@ -1981,17 +1978,14 @@ static void nv_swncq_fill_sg(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
- unsigned int idx;
struct nv_swncq_port_priv *pp = ap->private_data;
struct ata_prd *prd;
-
- WARN_ON(qc->__sg == NULL);
- WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
+ unsigned int si, idx;
prd = pp->prd + ATA_MAX_PRD * qc->tag;
idx = 0;
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
u32 addr, offset;
u32 sg_len, len;
@@ -2013,8 +2007,7 @@ static void nv_swncq_fill_sg(struct ata_queued_cmd *qc)
}
}
- if (idx)
- prd[idx - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
+ prd[idx - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
}
static unsigned int nv_swncq_issue_atacmd(struct ata_port *ap,
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index 25b34d6..2ab20a2 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -541,17 +541,15 @@ static void pdc_fill_sg(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
- unsigned int idx;
const u32 SG_COUNT_ASIC_BUG = 41*4;
+ unsigned int si, idx;
+ u32 len;
if (!(qc->flags & ATA_QCFLAG_DMAMAP))
return;
- WARN_ON(qc->__sg == NULL);
- WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
-
idx = 0;
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
u32 addr, offset;
u32 sg_len, len;
@@ -578,29 +576,27 @@ static void pdc_fill_sg(struct ata_queued_cmd *qc)
}
}
- if (idx) {
- u32 len = le32_to_cpu(ap->prd[idx - 1].flags_len);
+ len = le32_to_cpu(ap->prd[idx - 1].flags_len);
- if (len > SG_COUNT_ASIC_BUG) {
- u32 addr;
+ if (len > SG_COUNT_ASIC_BUG) {
+ u32 addr;
- VPRINTK("Splitting last PRD.\n");
+ VPRINTK("Splitting last PRD.\n");
- addr = le32_to_cpu(ap->prd[idx - 1].addr);
- ap->prd[idx - 1].flags_len = cpu_to_le32(len - SG_COUNT_ASIC_BUG);
- VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", idx - 1, addr, SG_COUNT_ASIC_BUG);
+ addr = le32_to_cpu(ap->prd[idx - 1].addr);
+ ap->prd[idx - 1].flags_len = cpu_to_le32(len - SG_COUNT_ASIC_BUG);
+ VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", idx - 1, addr, SG_COUNT_ASIC_BUG);
- addr = addr + len - SG_COUNT_ASIC_BUG;
- len = SG_COUNT_ASIC_BUG;
- ap->prd[idx].addr = cpu_to_le32(addr);
- ap->prd[idx].flags_len = cpu_to_le32(len);
- VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", idx, addr, len);
+ addr = addr + len - SG_COUNT_ASIC_BUG;
+ len = SG_COUNT_ASIC_BUG;
+ ap->prd[idx].addr = cpu_to_le32(addr);
+ ap->prd[idx].flags_len = cpu_to_le32(len);
+ VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", idx, addr, len);
- idx++;
- }
-
- ap->prd[idx - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
+ idx++;
}
+
+ ap->prd[idx - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
}
static void pdc_qc_prep(struct ata_queued_cmd *qc)
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index 2875549..c55ab77 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -287,14 +287,10 @@ static unsigned int qs_fill_sg(struct ata_queued_cmd *qc)
struct scatterlist *sg;
struct ata_port *ap = qc->ap;
struct qs_port_priv *pp = ap->private_data;
- unsigned int nelem;
u8 *prd = pp->pkt + QS_CPB_BYTES;
+ unsigned int si;
- WARN_ON(qc->__sg == NULL);
- WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
-
- nelem = 0;
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
u64 addr;
u32 len;
@@ -306,12 +302,11 @@ static unsigned int qs_fill_sg(struct ata_queued_cmd *qc)
*(__le32 *)prd = cpu_to_le32(len);
prd += sizeof(u64);
- VPRINTK("PRD[%u] = (0x%llX, 0x%X)\n", nelem,
+ VPRINTK("PRD[%u] = (0x%llX, 0x%X)\n", si,
(unsigned long long)addr, len);
- nelem++;
}
- return nelem;
+ return si;
}
static void qs_qc_prep(struct ata_queued_cmd *qc)
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 4f25209..b3c4f44 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -813,8 +813,9 @@ static inline void sil24_fill_sg(struct ata_queued_cmd *qc,
{
struct scatterlist *sg;
struct sil24_sge *last_sge = NULL;
+ unsigned int si;
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
sge->addr = cpu_to_le64(sg_dma_address(sg));
sge->cnt = cpu_to_le32(sg_dma_len(sg));
sge->flags = 0;
@@ -823,8 +824,7 @@ static inline void sil24_fill_sg(struct ata_queued_cmd *qc,
sge++;
}
- if (likely(last_sge))
- last_sge->flags = cpu_to_le32(SGE_TRM);
+ last_sge->flags = cpu_to_le32(SGE_TRM);
}
static int sil24_qc_defer(struct ata_queued_cmd *qc)
diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index 3de0c27..211ba8d 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -473,7 +473,7 @@ static void pdc20621_dma_prep(struct ata_queued_cmd *qc)
void __iomem *mmio = ap->host->iomap[PDC_MMIO_BAR];
void __iomem *dimm_mmio = ap->host->iomap[PDC_DIMM_BAR];
unsigned int portno = ap->port_no;
- unsigned int i, idx, total_len = 0, sgt_len;
+ unsigned int i, si, idx, total_len = 0, sgt_len;
u32 *buf = (u32 *) &pp->dimm_buf[PDC_DIMM_HEADER_SZ];
WARN_ON(!(qc->flags & ATA_QCFLAG_DMAMAP));
@@ -487,7 +487,7 @@ static void pdc20621_dma_prep(struct ata_queued_cmd *qc)
* Build S/G table
*/
idx = 0;
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
buf[idx++] = cpu_to_le32(sg_dma_address(sg));
buf[idx++] = cpu_to_le32(sg_dma_len(sg));
total_len += sg_dma_len(sg);
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 3e78bc2..aa0df0a 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -5142,6 +5142,7 @@ static void ipr_build_ata_ioadl(struct ipr_cmnd *ipr_cmd,
struct ipr_ioadl_desc *last_ioadl = NULL;
int len = qc->nbytes + qc->pad_len;
struct scatterlist *sg;
+ unsigned int si;
if (len == 0)
return;
@@ -5159,7 +5160,7 @@ static void ipr_build_ata_ioadl(struct ipr_cmnd *ipr_cmd,
cpu_to_be32(sizeof(struct ipr_ioadl_desc) * ipr_cmd->dma_use_sg);
}
- ata_for_each_sg(sg, qc) {
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
ioadl->flags_and_data_len = cpu_to_be32(ioadl_flags | sg_dma_len(sg));
ioadl->address = cpu_to_be32(sg_dma_address(sg));
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index f78d060..827cfb1 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -158,8 +158,8 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
struct Scsi_Host *host = sas_ha->core.shost;
struct sas_internal *i = to_sas_internal(host->transportt);
struct scatterlist *sg;
- unsigned int num = 0;
unsigned int xfer = 0;
+ unsigned int si;
task = sas_alloc_task(GFP_ATOMIC);
if (!task)
@@ -181,17 +181,15 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
task->total_xfer_len = qc->nbytes + qc->pad_len;
task->num_scatter = qc->pad_len ? qc->n_elem + 1 : qc->n_elem;
} else {
- ata_for_each_sg(sg, qc) {
- num++;
+ for_each_sg(qc->sg, sg, qc->n_elem, si)
xfer += sg->length;
- }
task->total_xfer_len = xfer;
- task->num_scatter = num;
+ task->num_scatter = si;
}
task->data_dir = qc->dma_dir;
- task->scatter = qc->__sg;
+ task->scatter = qc->sg;
task->ata_task.retry_count = 1;
task->task_state_flags = SAS_TASK_STATE_PENDING;
qc->lldd_task = task;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 6869900..7bd1cc2 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -459,7 +459,7 @@ struct ata_queued_cmd {
unsigned int tag;
unsigned int n_elem;
unsigned int n_iter;
- unsigned int orig_n_elem;
+ unsigned int mapped_n_elem;
int dma_dir;
@@ -472,11 +472,12 @@ struct ata_queued_cmd {
struct scatterlist *cursg;
unsigned int cursg_ofs;
+ struct scatterlist *last_sg;
+ struct scatterlist saved_last_sg;
struct scatterlist sgent;
- struct scatterlist pad_sgent;
+ struct scatterlist extra_sg[2];
- /* DO NOT iterate over __sg manually, use ata_for_each_sg() */
- struct scatterlist *__sg;
+ struct scatterlist *sg;
unsigned int err_mask;
struct ata_taskfile result_tf;
@@ -1124,35 +1125,6 @@ extern void ata_port_pbar_desc(struct ata_port *ap, int bar, ssize_t offset,
const char *name);
#endif
-/*
- * qc helpers
- */
-static inline struct scatterlist *
-ata_qc_first_sg(struct ata_queued_cmd *qc)
-{
- qc->n_iter = 0;
- if (qc->n_elem)
- return qc->__sg;
- if (qc->pad_len)
- return &qc->pad_sgent;
- return NULL;
-}
-
-static inline struct scatterlist *
-ata_qc_next_sg(struct scatterlist *sg, struct ata_queued_cmd *qc)
-{
- if (sg == &qc->pad_sgent)
- return NULL;
- if (++qc->n_iter < qc->n_elem)
- return sg_next(sg);
- if (qc->pad_len)
- return &qc->pad_sgent;
- return NULL;
-}
-
-#define ata_for_each_sg(sg, qc) \
- for (sg = ata_qc_first_sg(qc); sg; sg = ata_qc_next_sg(sg, qc))
-
static inline unsigned int ata_tag_valid(unsigned int tag)
{
return (tag < ATA_MAX_QUEUE) ? 1 : 0;
@@ -1387,15 +1359,17 @@ static inline void ata_tf_init(struct ata_device *dev, struct ata_taskfile *tf)
static inline void ata_qc_reinit(struct ata_queued_cmd *qc)
{
qc->dma_dir = DMA_NONE;
- qc->__sg = NULL;
+ qc->sg = NULL;
qc->flags = 0;
qc->cursg = NULL;
qc->cursg_ofs = 0;
qc->nbytes = qc->curbytes = 0;
qc->n_elem = 0;
+ qc->mapped_n_elem = 0;
qc->n_iter = 0;
qc->err_mask = 0;
qc->pad_len = 0;
+ qc->last_sg = NULL;
qc->sect_size = ATA_SECT_SIZE;
ata_tf_init(qc->dev, &qc->tf);
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 11/14] libata: add qc->dma_nbytes
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (9 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 10/14] libata: convert to chained sg Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 16:02 ` Alan Cox
2007-12-04 19:33 ` Jeff Garzik
2007-11-29 14:33 ` [PATCH 12/14] libata: implement ATAPI drain buffer Tejun Heo
` (2 subsequent siblings)
13 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
qc->nbytes doesn't include extra buffers setup by libata core layer
and my be odd. This patch adds qc->dma_nbytes which includes any
extra buffers setup by libata core layer and is guaranteed to be
aligned on 4 byte boundary.
This value is to be used to program the host controller. As this
represents the actual length of buffer available to the controller and
the controller must be able to deal with short transfers for ATAPI
commands which can transfer variable length, this shouldn't break any
controllers while making problems like rounding-down and controllers
choking up on odd transfer bytes much less likely.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/ata/libata-core.c | 11 +++++++----
drivers/ata/pata_pdc202xx_old.c | 2 +-
drivers/ata/sata_inic162x.c | 2 +-
drivers/ata/sata_qstor.c | 2 +-
include/linux/libata.h | 3 ++-
5 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 652c5c5..62a8479 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4757,13 +4757,15 @@ void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
}
static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
- unsigned int *n_elem_extra)
+ unsigned int *n_elem_extra,
+ unsigned int *nbytes_extra)
{
struct ata_port *ap = qc->ap;
unsigned int n_elem = qc->n_elem;
struct scatterlist *lsg, *copy_lsg = NULL, *tsg = NULL, *esg = NULL;
*n_elem_extra = 0;
+ *nbytes_extra = 0;
/* needs padding? */
qc->pad_len = qc->nbytes & 3;
@@ -4827,6 +4829,7 @@ static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
esg = &qc->extra_sg[1];
(*n_elem_extra)++;
+ (*nbytes_extra) += 4 - qc->pad_len;
}
if (copy_lsg)
@@ -4860,11 +4863,11 @@ static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
static int ata_sg_setup(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
- unsigned int n_elem, n_elem_extra;
+ unsigned int n_elem, n_elem_extra, nbytes_extra;
VPRINTK("ENTER, ata%u\n", ap->print_id);
- n_elem = ata_sg_setup_extra(qc, &n_elem_extra);
+ n_elem = ata_sg_setup_extra(qc, &n_elem_extra, &nbytes_extra);
if (n_elem) {
n_elem = dma_map_sg(ap->dev, qc->sg, n_elem, qc->dma_dir);
@@ -4879,7 +4882,7 @@ static int ata_sg_setup(struct ata_queued_cmd *qc)
qc->n_elem = qc->mapped_n_elem = n_elem;
qc->n_elem += n_elem_extra;
-
+ qc->dma_nbytes = qc->nbytes + nbytes_extra;
qc->flags |= ATA_QCFLAG_DMAMAP;
return 0;
diff --git a/drivers/ata/pata_pdc202xx_old.c b/drivers/ata/pata_pdc202xx_old.c
index 407dbcf..f7790fe 100644
--- a/drivers/ata/pata_pdc202xx_old.c
+++ b/drivers/ata/pata_pdc202xx_old.c
@@ -169,7 +169,7 @@ static void pdc2026x_bmdma_start(struct ata_queued_cmd *qc)
/* Cases the state machine will not complete correctly without help */
if ((tf->flags & ATA_TFLAG_LBA48) || tf->protocol == ATAPI_PROT_DMA) {
- len = qc->nbytes / 2;
+ len = qc->dma_nbytes / 2;
if (tf->flags & ATA_TFLAG_WRITE)
len |= 0x06000000;
diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
index 96e614a..5d9f9ad 100644
--- a/drivers/ata/sata_inic162x.c
+++ b/drivers/ata/sata_inic162x.c
@@ -238,7 +238,7 @@ static void inic_bmdma_setup(struct ata_queued_cmd *qc)
wmb();
/* load transfer length */
- writel(qc->nbytes, port_base + PORT_PRD_XFERLEN);
+ writel(qc->dma_nbytes, port_base + PORT_PRD_XFERLEN);
/* turn on DMA and specify data direction */
pp->cached_prdctl = pp->dfl_prdctl | PRD_CTL_DMAEN;
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index c55ab77..b5c0031 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -335,7 +335,7 @@ static void qs_qc_prep(struct ata_queued_cmd *qc)
/* host control block (HCB) */
buf[ 0] = QS_HCB_HDR;
buf[ 1] = hflags;
- *(__le32 *)(&buf[ 4]) = cpu_to_le32(qc->nbytes);
+ *(__le32 *)(&buf[ 4]) = cpu_to_le32(qc->dma_nbytes);
*(__le32 *)(&buf[ 8]) = cpu_to_le32(nelem);
addr = ((u64)pp->pkt_dma) + QS_CPB_BYTES;
*(__le64 *)(&buf[16]) = cpu_to_le64(addr);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7bd1cc2..a20a8a8 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -467,6 +467,7 @@ struct ata_queued_cmd {
unsigned int sect_size;
unsigned int nbytes;
+ unsigned int dma_nbytes;
unsigned int curbytes;
struct scatterlist *cursg;
@@ -1363,7 +1364,7 @@ static inline void ata_qc_reinit(struct ata_queued_cmd *qc)
qc->flags = 0;
qc->cursg = NULL;
qc->cursg_ofs = 0;
- qc->nbytes = qc->curbytes = 0;
+ qc->nbytes = qc->curbytes = qc->dma_nbytes = 0;
qc->n_elem = 0;
qc->mapped_n_elem = 0;
qc->n_iter = 0;
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 12/14] libata: implement ATAPI drain buffer
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (10 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 11/14] libata: add qc->dma_nbytes Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 13/14] libata: implement ATAPI per-command-type DMA horkages Tejun Heo
2007-11-29 14:33 ` [PATCH 14/14] libata: use PIO for misc ATAPI commands Tejun Heo
13 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
Misc ATAPI commands may try to transfer more bytes than requested.
For PIO which is performed by libata HSM, this is worked around by
draining extra bytes from __atapi_pio_bytes().
This patch implements drain buffer to perform draining for DMA and
PIO-over-DMA cases. One page is allocated w/ GFP_DMA32 during libata
core layer initialization. On host registration, this drain page is
DMA mapped and ATAPI_MAX_DRAIN_PAGES sg entries are reserved.
ata_sg_setup_extra() uses these extra sg entries to map the drain page
ATAPI_MAX_DRAIN_PAGES times, extending sg list by ATAPI_MAX_DRAIN
bytes. This allows both DMA and PIO-over-DMA misc ATAPI commands to
overflow by ATAPI_MAX_DRAIN bytes just like PIO commands.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/ata/libata-core.c | 116 ++++++++++++++++++++++++++++++++++++++++-----
drivers/ata/libata-scsi.c | 14 ++++--
include/linux/libata.h | 4 +-
3 files changed, 116 insertions(+), 18 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 62a8479..4da4f26 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4756,6 +4756,60 @@ void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
qc->cursg = qc->sg;
}
+/**
+ * ata_sg_setup_extra - Setup extra sg entries
+ * @qc: Command to setup extra sg entries for
+ * @n_elem_extra: Out parameter for the number of extra sg entries
+ * @nbytes_extra: Out parameter for the number of extra bytes
+ *
+ * Extra sg entries are used to deal with ATAPI peculiarities.
+ * First, the content to be transferred can be of any size but
+ * transfer length should be aligned to 4 bytes, so if data size
+ * isn't aligned, it needs to be padded.
+ *
+ * Second, for commands whose repsonse can be variable, due to
+ * userland bugs (more likely) and hardware bugs, devices can try
+ * to transfer more bytes than requested. This can be worked
+ * around by appending drain buffers at the end.
+ *
+ * This function sets up both padding and draining sg entries.
+ * For this purpose, each qc has 2 + ATAPI_MAX_DRAIN_PAGES extra
+ * sg entries. Each extra sg has assigned function.
+ *
+ * e[0] | e[1] | e[2] | ... | e[2 + ATAPI_MAX_DRAIN_PAGES - 1]
+ * ----------------------------------------------------------------------
+ * link | padding | draining ...
+ * or link
+ *
+ * After sg setup is complete, sg list looks like the following.
+ *
+ * 1. Padding necessary, padding doesn't replace the last sg
+ *
+ * o[0][1][2]...[last] e[0][1]([2]... if draining is necessary)
+ * | ^
+ * \------/
+ * e[0] carries the original content of o[last].
+ *
+ * 2. Padding necessary, padding replaces the last sg
+ *
+ * o[0][1][2]...[last] e[0][1]([2]... if draining is necessary)
+ * | ^
+ * \---------/
+ * e[1] completely includes what o[last] used to point to.
+ *
+ * 3. Only draining is necessary.
+ *
+ * [0][1][2]...[last] e[0][1][2]...
+ * | ^
+ * \---------/
+ * e[1] carries the original conetent of o[last].
+ *
+ * LOCKING:
+ * spin_lock_irqsave(host lock)
+ *
+ * RETURNS:
+ * Adjusted n_elem which should be mapped.
+ */
static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
unsigned int *n_elem_extra,
unsigned int *nbytes_extra)
@@ -4763,6 +4817,7 @@ static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
struct ata_port *ap = qc->ap;
unsigned int n_elem = qc->n_elem;
struct scatterlist *lsg, *copy_lsg = NULL, *tsg = NULL, *esg = NULL;
+ int drain;
*n_elem_extra = 0;
*nbytes_extra = 0;
@@ -4770,7 +4825,10 @@ static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
/* needs padding? */
qc->pad_len = qc->nbytes & 3;
- if (likely(!qc->pad_len))
+ /* needs drain? */
+ drain = atapi_qc_may_overflow(qc);
+
+ if (likely(!qc->pad_len && !drain))
return n_elem;
/* locate last sg and save it */
@@ -4832,6 +4890,29 @@ static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
(*nbytes_extra) += 4 - qc->pad_len;
}
+ if (drain) {
+ struct scatterlist *dsg = qc->extra_sg + 2;
+ int i;
+
+ for (i = 0; i < ATAPI_MAX_DRAIN_PAGES; i++) {
+ sg_set_page(dsg, virt_to_page(ata_drain_page),
+ PAGE_SIZE, 0);
+ sg_dma_address(dsg) = ap->host->drain_page_dma;
+ sg_dma_len(dsg) = PAGE_SIZE;
+ dsg++;
+ }
+
+ if (!tsg) {
+ copy_lsg = &qc->extra_sg[1];
+ tsg = &qc->extra_sg[1];
+ }
+
+ esg = dsg - 1;
+
+ (*n_elem_extra) += ATAPI_MAX_DRAIN_PAGES;
+ (*nbytes_extra) += ATAPI_MAX_DRAIN_PAGES * PAGE_SIZE;
+ }
+
if (copy_lsg)
sg_set_page(copy_lsg, sg_page(lsg), lsg->length, lsg->offset);
@@ -6883,6 +6964,9 @@ static void ata_host_stop(struct device *gendev, void *res)
ap->ops->port_stop(ap);
}
+ dma_unmap_single(host->dev, host->drain_page_dma, PAGE_SIZE,
+ DMA_FROM_DEVICE);
+
if (host->ops->host_stop)
host->ops->host_stop(host);
}
@@ -6905,8 +6989,8 @@ static void ata_host_stop(struct device *gendev, void *res)
*/
int ata_host_start(struct ata_host *host)
{
- int have_stop = 0;
void *start_dr = NULL;
+ dma_addr_t dma;
int i, rc;
if (host->flags & ATA_HOST_STARTED)
@@ -6917,20 +7001,23 @@ int ata_host_start(struct ata_host *host)
if (!host->ops && !ata_port_is_dummy(ap))
host->ops = ap->ops;
-
- if (ap->ops->port_stop)
- have_stop = 1;
}
- if (host->ops->host_stop)
- have_stop = 1;
+ start_dr = devres_alloc(ata_host_stop, 0, GFP_KERNEL);
+ if (!start_dr)
+ return -ENOMEM;
- if (have_stop) {
- start_dr = devres_alloc(ata_host_stop, 0, GFP_KERNEL);
- if (!start_dr)
- return -ENOMEM;
+ /* map drain page */
+ dma = dma_map_single(host->dev, ata_drain_page, PAGE_SIZE,
+ DMA_FROM_DEVICE);
+ if (dma_mapping_error(dma)) {
+ dev_printk(KERN_ERR, host->dev, "failed to map drain page\n");
+ rc = -ENOMEM;
+ goto err_map;
}
+ host->drain_page_dma = dma;
+ /* start ports */
for (i = 0; i < host->n_ports; i++) {
struct ata_port *ap = host->ports[i];
@@ -6939,7 +7026,7 @@ int ata_host_start(struct ata_host *host)
if (rc) {
ata_port_printk(ap, KERN_ERR, "failed to "
"start port (errno=%d)\n", rc);
- goto err_out;
+ goto err_start;
}
}
@@ -6951,13 +7038,16 @@ int ata_host_start(struct ata_host *host)
host->flags |= ATA_HOST_STARTED;
return 0;
- err_out:
+ err_start:
while (--i >= 0) {
struct ata_port *ap = host->ports[i];
if (ap->ops->port_stop)
ap->ops->port_stop(ap);
}
+ err_map:
+ dma_unmap_single(host->dev, host->drain_page_dma, PAGE_SIZE,
+ DMA_FROM_DEVICE);
devres_free(start_dr);
return rc;
}
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index f523e66..f07704c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -832,13 +832,19 @@ static void ata_scsi_dev_config(struct scsi_device *sdev,
/* configure max sectors */
blk_queue_max_sectors(sdev->request_queue, dev->max_sectors);
- /* SATA DMA transfers must be multiples of 4 byte, so
- * we need to pad ATAPI transfers using an extra sg.
- * Decrement max hw segments accordingly.
+ /* SATA DMA transfers must be multiples of 4 byte, so we need
+ * to pad ATAPI transfers using an extra sg. Also, ATAPI
+ * commands with variable length reponse needs draining of
+ * extra data. Decrement max hw segments accordingly.
*/
if (dev->class == ATA_DEV_ATAPI) {
struct request_queue *q = sdev->request_queue;
- blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
+ unsigned short hw_segments = q->max_hw_segments;
+
+ BUG_ON(hw_segments <= 1 + ATAPI_MAX_DRAIN_PAGES);
+ hw_segments -= 1 + ATAPI_MAX_DRAIN_PAGES;
+
+ blk_queue_max_hw_segments(q, hw_segments);
}
if (dev->flags & ATA_DFLAG_AN)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index a20a8a8..449307d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -121,6 +121,7 @@ enum {
ATA_SHORT_PAUSE = (HZ >> 6) + 1,
ATAPI_MAX_DRAIN = 16 << 10,
+ ATAPI_MAX_DRAIN_PAGES = ATAPI_MAX_DRAIN >> PAGE_SHIFT,
ATA_SHT_EMULATED = 1,
ATA_SHT_CMD_PER_LUN = 1,
@@ -438,6 +439,7 @@ struct ata_host {
void *private_data;
const struct ata_port_operations *ops;
unsigned long flags;
+ dma_addr_t drain_page_dma;
#ifdef CONFIG_ATA_ACPI
acpi_handle acpi_handle;
#endif
@@ -476,7 +478,7 @@ struct ata_queued_cmd {
struct scatterlist *last_sg;
struct scatterlist saved_last_sg;
struct scatterlist sgent;
- struct scatterlist extra_sg[2];
+ struct scatterlist extra_sg[2 + ATAPI_MAX_DRAIN_PAGES];
struct scatterlist *sg;
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 13/14] libata: implement ATAPI per-command-type DMA horkages
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (11 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 12/14] libata: implement ATAPI drain buffer Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 16:04 ` Alan Cox
2007-11-29 14:33 ` [PATCH 14/14] libata: use PIO for misc ATAPI commands Tejun Heo
13 siblings, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
ATAPI DMA is filled with mines. Sector aligned READ transfers usually
work but many other commands transfer non-sector aligned or variable
number of bytes, and there are devices and controllers which have
problems dealing with such non-aligned DMA transactions.
This patch implement ATAPI per-command-type DMA horkages and EH logic
to set those quickly. This way, failures localized to certain command
type don't affect other operations (most importantly READs) and
working configuration is found quickly such that the device can be
used.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/ata/libata-core.c | 21 +++++++++++++++++++++
drivers/ata/libata-eh.c | 35 +++++++++++++++++++++++++++++++++++
include/linux/libata.h | 5 +++++
3 files changed, 61 insertions(+), 0 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4da4f26..81feaa9 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4634,6 +4634,27 @@ static void ata_fill_sg_dumb(struct ata_queued_cmd *qc)
int ata_check_atapi_dma(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
+ unsigned int horkage = qc->dev->horkage;
+
+ switch (atapi_cmd_type(qc->cdb[0])) {
+ case ATAPI_READ:
+ break;
+
+ case ATAPI_WRITE:
+ if (horkage & ATAPI_DMA_HORKAGE_WRITE)
+ return 1;
+ break;
+
+ case ATAPI_READ_CD:
+ if (horkage & ATAPI_DMA_HORKAGE_READ_CD)
+ return 1;
+ break;
+
+ case ATAPI_MISC:
+ if (horkage & ATAPI_DMA_HORKAGE_MISC)
+ return 1;
+ break;
+ }
/* Don't allow DMA if it isn't multiple of 16 bytes. Quite a
* few ATAPI devices choke on such DMA requests.
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index baa7f4f..3b78fb9 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1489,6 +1489,38 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc,
return action;
}
+static void atapi_eh_dma_horkages(struct ata_queued_cmd *qc)
+{
+ struct ata_device *dev = qc->dev;
+ const char *type;
+
+ if (!ata_is_atapi(qc->tf.protocol) || !ata_is_dma(qc->tf.protocol) ||
+ !(qc->err_mask & (AC_ERR_HSM | AC_ERR_TIMEOUT)))
+ return;
+
+ switch (atapi_cmd_type(qc->cdb[0])) {
+ case ATAPI_WRITE:
+ type = "WRITE";
+ dev->horkage |= ATAPI_DMA_HORKAGE_WRITE;
+ break;
+ case ATAPI_READ_CD:
+ type = "READ CD [MSF]";
+ dev->horkage |= ATAPI_DMA_HORKAGE_READ_CD;
+ break;
+ case ATAPI_MISC:
+ type = "MISC";
+ dev->horkage |= ATAPI_DMA_HORKAGE_MISC;
+ break;
+ default:
+ return;
+ }
+
+ ata_dev_printk(dev, KERN_WARNING, "ATAPI DMA for %s disabled (0x%x). \n"
+ " If this continues to happen, please report to\n"
+ " linux-ide@vger.kernel.org\n",
+ type, dev->horkage);
+}
+
static int ata_eh_categorize_error(unsigned int eflags, unsigned int err_mask,
int *xfer_ok)
{
@@ -1809,6 +1841,9 @@ static void ata_eh_link_autopsy(struct ata_link *link)
all_err_mask |= qc->err_mask;
if (qc->flags & ATA_QCFLAG_IO)
eflags |= ATA_EFLAG_IS_IO;
+
+ /* handle ATAPI DMA horkages */
+ atapi_eh_dma_horkages(qc);
}
/* enforce default EH actions */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 449307d..5ad3230 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -342,6 +342,11 @@ enum {
ATA_HORKAGE_IVB = (1 << 8), /* cbl det validity bit bugs */
ATA_HORKAGE_STUCK_ERR = (1 << 9), /* stuck ERR on next PACKET */
+ ATAPI_DMA_HORKAGE_WRITE = (1 << 28), /* PIO for WRITEs */
+ ATAPI_DMA_HORKAGE_READ_CD = (1 << 29), /* PIO for READ_CDs */
+ ATAPI_DMA_HORKAGE_READ_DVD_STR = (1 << 30), /* PIO for READ DVD STR */
+ ATAPI_DMA_HORKAGE_MISC = (1 << 31), /* PIO for MISC commands */
+
/* DMA mask for user DMA control: User visible values; DO NOT
renumber */
ATA_DMA_MASK_ATA = (1 << 0), /* DMA on ATA Disk */
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
` (12 preceding siblings ...)
2007-11-29 14:33 ` [PATCH 13/14] libata: implement ATAPI per-command-type DMA horkages Tejun Heo
@ 2007-11-29 14:33 ` Tejun Heo
2007-11-29 16:05 ` Alan Cox
2007-12-04 19:34 ` Jeff Garzik
13 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 14:33 UTC (permalink / raw)
To: jeff, linux-ide, alan, liml, albertl, jens.axboe; +Cc: Tejun Heo
ATAPI devices come with plethora of bugs and many ATA controllers have
trouble dealing with odd byte DMA transfers. The problem is currently
somewhat masked by not allowing DMA transfers if the transfer size
isn't aligned to 16 bytes plus partial masking in problematic LLDs.
This masking is taken verbatim from IDE and is far from perfection.
There's no fundamental explanation why this should be sufficient and
there are ATAPI devices which don't work with the IDE driver.
In addition, this mixture of PIO and DMA commands reduces test
coverage which is already insufficient considering the crazy number of
ATAPI devices out in the wild.
Also, these misc ATAPI commands usually transfer small amount of data
and are used infrequently. There isn't much to be gained from using
DMA. Combined with the fact that another OS which is probably the
only one that most ATAPI device vendors test against uses PIO for
these commands, it's much wiser to use PIO for these commands and
concentrate debugging efforts on getting PIO right for misc commands
and DMA for bulk transfer commands.
Private command type / transfer length filtering in sata_promise,
pata_it821x and pata_pdc2027x are removed as core layer filtering is
more conservative.
Signed-off-by: Tejun Heo <htejun@gmail.com>
---
drivers/ata/libata-core.c | 10 +--------
drivers/ata/libata-eh.c | 17 ++-------------
drivers/ata/libata-scsi.c | 17 ++-------------
drivers/ata/pata_it821x.c | 4 ---
drivers/ata/pata_pdc2027x.c | 44 -------------------------------------------
drivers/ata/sata_promise.c | 31 ++++++++---------------------
include/linux/libata.h | 1 -
7 files changed, 16 insertions(+), 108 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 81feaa9..1f73cf7 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4651,16 +4651,8 @@ int ata_check_atapi_dma(struct ata_queued_cmd *qc)
break;
case ATAPI_MISC:
- if (horkage & ATAPI_DMA_HORKAGE_MISC)
- return 1;
- break;
- }
-
- /* Don't allow DMA if it isn't multiple of 16 bytes. Quite a
- * few ATAPI devices choke on such DMA requests.
- */
- if (unlikely(qc->nbytes & 15))
return 1;
+ }
if (ap->ops->check_atapi_dma)
return ap->ops->check_atapi_dma(qc);
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 3b78fb9..656b06e 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1271,7 +1271,6 @@ static unsigned int atapi_eh_request_sense(struct ata_queued_cmd *qc)
{
struct ata_device *dev = qc->dev;
unsigned char *sense_buf = qc->scsicmd->sense_buffer;
- struct ata_port *ap = dev->link->ap;
struct ata_taskfile tf;
u8 cdb[ATAPI_CDB_LEN];
@@ -1296,15 +1295,9 @@ static unsigned int atapi_eh_request_sense(struct ata_queued_cmd *qc)
tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
tf.command = ATA_CMD_PACKET;
- /* is it pointless to prefer PIO for "safety reasons"? */
- if (ap->flags & ATA_FLAG_PIO_DMA) {
- tf.protocol = ATAPI_PROT_DMA;
- tf.feature |= ATAPI_PKT_DMA;
- } else {
- tf.protocol = ATAPI_PROT_PIO;
- tf.lbam = SCSI_SENSE_BUFFERSIZE;
- tf.lbah = 0;
- }
+ tf.protocol = ATAPI_PROT_PIO;
+ tf.lbam = SCSI_SENSE_BUFFERSIZE;
+ tf.lbah = 0;
return ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,
sense_buf, SCSI_SENSE_BUFFERSIZE, 0);
@@ -1507,10 +1500,6 @@ static void atapi_eh_dma_horkages(struct ata_queued_cmd *qc)
type = "READ CD [MSF]";
dev->horkage |= ATAPI_DMA_HORKAGE_READ_CD;
break;
- case ATAPI_MISC:
- type = "MISC";
- dev->horkage |= ATAPI_DMA_HORKAGE_MISC;
- break;
default:
return;
}
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index f07704c..6a580ef 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2322,12 +2322,6 @@ static void atapi_sense_complete(struct ata_queued_cmd *qc)
ata_qc_free(qc);
}
-/* is it pointless to prefer PIO for "safety reasons"? */
-static inline int ata_pio_use_silly(struct ata_port *ap)
-{
- return (ap->flags & ATA_FLAG_PIO_DMA);
-}
-
static void atapi_request_sense(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
@@ -2357,15 +2351,10 @@ static void atapi_request_sense(struct ata_queued_cmd *qc)
qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
qc->tf.command = ATA_CMD_PACKET;
+ qc->tf.protocol = ATAPI_PROT_PIO;
+ qc->tf.lbam = SCSI_SENSE_BUFFERSIZE;
+ qc->tf.lbah = 0;
- if (ata_pio_use_silly(ap)) {
- qc->tf.protocol = ATAPI_PROT_DMA;
- qc->tf.feature |= ATAPI_PKT_DMA;
- } else {
- qc->tf.protocol = ATAPI_PROT_PIO;
- qc->tf.lbam = SCSI_SENSE_BUFFERSIZE;
- qc->tf.lbah = 0;
- }
qc->nbytes = SCSI_SENSE_BUFFERSIZE;
qc->complete_fn = atapi_sense_complete;
diff --git a/drivers/ata/pata_it821x.c b/drivers/ata/pata_it821x.c
index ca9aae0..b38d0a9 100644
--- a/drivers/ata/pata_it821x.c
+++ b/drivers/ata/pata_it821x.c
@@ -532,10 +532,6 @@ static int it821x_check_atapi_dma(struct ata_queued_cmd *qc)
struct ata_port *ap = qc->ap;
struct it821x_dev *itdev = ap->private_data;
- /* Only use dma for transfers to/from the media. */
- if (qc->nbytes < 2048)
- return -EOPNOTSUPP;
-
/* No ATAPI DMA in smart mode */
if (itdev->smart)
return -EOPNOTSUPP;
diff --git a/drivers/ata/pata_pdc2027x.c b/drivers/ata/pata_pdc2027x.c
index 2622577..c73c6e2 100644
--- a/drivers/ata/pata_pdc2027x.c
+++ b/drivers/ata/pata_pdc2027x.c
@@ -66,7 +66,6 @@ static int pdc2027x_init_one(struct pci_dev *pdev, const struct pci_device_id *e
static void pdc2027x_error_handler(struct ata_port *ap);
static void pdc2027x_set_piomode(struct ata_port *ap, struct ata_device *adev);
static void pdc2027x_set_dmamode(struct ata_port *ap, struct ata_device *adev);
-static int pdc2027x_check_atapi_dma(struct ata_queued_cmd *qc);
static unsigned long pdc2027x_mode_filter(struct ata_device *adev, unsigned long mask);
static int pdc2027x_cable_detect(struct ata_port *ap);
static int pdc2027x_set_mode(struct ata_link *link, struct ata_device **r_failed);
@@ -155,7 +154,6 @@ static struct ata_port_operations pdc2027x_pata100_ops = {
.exec_command = ata_exec_command,
.dev_select = ata_std_dev_select,
- .check_atapi_dma = pdc2027x_check_atapi_dma,
.bmdma_setup = ata_bmdma_setup,
.bmdma_start = ata_bmdma_start,
.bmdma_stop = ata_bmdma_stop,
@@ -188,7 +186,6 @@ static struct ata_port_operations pdc2027x_pata133_ops = {
.exec_command = ata_exec_command,
.dev_select = ata_std_dev_select,
- .check_atapi_dma = pdc2027x_check_atapi_dma,
.bmdma_setup = ata_bmdma_setup,
.bmdma_start = ata_bmdma_start,
.bmdma_stop = ata_bmdma_stop,
@@ -506,47 +503,6 @@ static int pdc2027x_set_mode(struct ata_link *link, struct ata_device **r_failed
}
/**
- * pdc2027x_check_atapi_dma - Check whether ATAPI DMA can be supported for this command
- * @qc: Metadata associated with taskfile to check
- *
- * LOCKING:
- * None (inherited from caller).
- *
- * RETURNS: 0 when ATAPI DMA can be used
- * 1 otherwise
- */
-static int pdc2027x_check_atapi_dma(struct ata_queued_cmd *qc)
-{
- struct scsi_cmnd *cmd = qc->scsicmd;
- u8 *scsicmd = cmd->cmnd;
- int rc = 1; /* atapi dma off by default */
-
- /*
- * This workaround is from Promise's GPL driver.
- * If ATAPI DMA is used for commands not in the
- * following white list, say MODE_SENSE and REQUEST_SENSE,
- * pdc2027x might hit the irq lost problem.
- */
- switch (scsicmd[0]) {
- case READ_10:
- case WRITE_10:
- case READ_12:
- case WRITE_12:
- case READ_6:
- case WRITE_6:
- case 0xad: /* READ_DVD_STRUCTURE */
- case 0xbe: /* READ_CD */
- /* ATAPI DMA is ok */
- rc = 0;
- break;
- default:
- ;
- }
-
- return rc;
-}
-
-/**
* pdc_read_counter - Read the ctr counter
* @host: target ATA host
*/
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index 2ab20a2..2633efb 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -931,32 +931,19 @@ static void pdc_exec_command_mmio(struct ata_port *ap,
static int pdc_check_atapi_dma(struct ata_queued_cmd *qc)
{
- u8 *scsicmd = qc->scsicmd->cmnd;
- int pio = 1; /* atapi dma off by default */
-
- /* Whitelist commands that may use DMA. */
- switch (scsicmd[0]) {
- case WRITE_12:
- case WRITE_10:
- case WRITE_6:
- case READ_12:
- case READ_10:
- case READ_6:
- case 0xad: /* READ_DVD_STRUCTURE */
- case 0xbe: /* READ_CD */
- pio = 0;
- }
+ const u8 *cdb = qc->cdb;
+
/* -45150 (FFFF4FA2) to -1 (FFFFFFFF) shall use PIO mode */
- if (scsicmd[0] == WRITE_10) {
+ if (cdb[0] == WRITE_10) {
unsigned int lba =
- (scsicmd[2] << 24) |
- (scsicmd[3] << 16) |
- (scsicmd[4] << 8) |
- scsicmd[5];
+ (cdb[2] << 24) |
+ (cdb[3] << 16) |
+ (cdb[4] << 8) |
+ cdb[5];
if (lba >= 0xFFFF4FA2)
- pio = 1;
+ return 1;
}
- return pio;
+ return 0;
}
static int pdc_old_sata_check_atapi_dma(struct ata_queued_cmd *qc)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5ad3230..9fcdeda 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -345,7 +345,6 @@ enum {
ATAPI_DMA_HORKAGE_WRITE = (1 << 28), /* PIO for WRITEs */
ATAPI_DMA_HORKAGE_READ_CD = (1 << 29), /* PIO for READ_CDs */
ATAPI_DMA_HORKAGE_READ_DVD_STR = (1 << 30), /* PIO for READ DVD STR */
- ATAPI_DMA_HORKAGE_MISC = (1 << 31), /* PIO for MISC commands */
/* DMA mask for user DMA control: User visible values; DO NOT
renumber */
--
1.5.2.4
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size
2007-11-29 14:33 ` [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size Tejun Heo
@ 2007-11-29 15:51 ` Alan Cox
2007-12-04 19:27 ` Jeff Garzik
1 sibling, 0 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-29 15:51 UTC (permalink / raw)
Cc: jeff, linux-ide, liml, albertl, jens.axboe, Tejun Heo
On Thu, 29 Nov 2007 23:33:24 +0900
Tejun Heo <htejun@gmail.com> wrote:
> While updating lbam/h for ATAPI commands, atapi_eh_request_sense() was
> left out. Update it.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
Acked-by: Alan Cox <alan@redhat.com>
> ---
> drivers/ata/libata-eh.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 77083b5..2e3d3a2 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1302,8 +1302,8 @@ static unsigned int atapi_eh_request_sense(struct ata_queued_cmd *qc)
> tf.feature |= ATAPI_PKT_DMA;
> } else {
> tf.protocol = ATA_PROT_ATAPI;
> - tf.lbam = (8 * 1024) & 0xff;
> - tf.lbah = (8 * 1024) >> 8;
> + tf.lbam = SCSI_SENSE_BUFFERSIZE;
> + tf.lbah = 0;
> }
>
> return ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,
--
--
"Alan, I'm getting a bit worried about you."
-- Linus Torvalds
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 03/14] libata: rename ATA_PROT_ATAPI_* to ATAPI_PROT_*
2007-11-29 14:33 ` [PATCH 03/14] libata: rename ATA_PROT_ATAPI_* to ATAPI_PROT_* Tejun Heo
@ 2007-11-29 15:52 ` Alan Cox
0 siblings, 0 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-29 15:52 UTC (permalink / raw)
Cc: jeff, linux-ide, liml, albertl, jens.axboe, Tejun Heo
On Thu, 29 Nov 2007 23:33:26 +0900
Tejun Heo <htejun@gmail.com> wrote:
> ATA_PROT_ATAPI_* are ugly and naming schemes between ATA_PROT_* and
> ATA_PROT_ATAPI_* are inconsistent causing confusion. Rename them to
> ATAPI_PROT_* and make them consistent with ATA counterpart.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
Acked-by: Alan Cox <alan@redhat.com>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 14:33 ` [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes Tejun Heo
@ 2007-11-29 15:55 ` Alan Cox
2007-11-29 16:06 ` Tejun Heo
2007-12-04 19:30 ` Jeff Garzik
1 sibling, 1 reply; 72+ messages in thread
From: Alan Cox @ 2007-11-29 15:55 UTC (permalink / raw)
Cc: jeff, linux-ide, liml, albertl, jens.axboe, Tejun Heo
On Thu, 29 Nov 2007 23:33:28 +0900
Tejun Heo <htejun@gmail.com> wrote:
> Depending on how many bytes are transferred as a unit, PIO data
> tranasfer may consume more bytes than requested. Knowing how much
> data is consumed is necessary to determine how much is left for
> draining. This patch update ->data_xfer such that it returns the
> number of consumed bytes.
Why do we care ?
If the drive and controller disagree or the drive has gone off its
trolley all we actually care about is when the thing stops saying there
is data. In addition all our methods transfer the entire block they are
asked to each time.
Alan
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/14] libata: improve ATAPI draining
2007-11-29 14:33 ` [PATCH 06/14] libata: improve ATAPI draining Tejun Heo
@ 2007-11-29 15:59 ` Alan Cox
2007-11-29 16:09 ` Tejun Heo
2007-11-29 16:42 ` Tejun Heo
2007-12-04 10:06 ` Albert Lee
1 sibling, 2 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-29 15:59 UTC (permalink / raw)
Cc: jeff, linux-ide, liml, albertl, jens.axboe, Tejun Heo, Albert Lee
> * Limit the amount of draining to ATAPI_MAX_DRAIN (16k currently).
Why 16 not 64K ?
Please add a comment to note that the drivers assume
- drain is only ever done on a stuck PIO xfer
- drain occurs before any controller error/reset handling is done
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 08/14] libata: kill non-sg DMA interface
2007-11-29 14:33 ` [PATCH 08/14] libata: kill non-sg DMA interface Tejun Heo
@ 2007-11-29 16:00 ` Alan Cox
2007-12-04 19:31 ` Jeff Garzik
1 sibling, 0 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-29 16:00 UTC (permalink / raw)
Cc: jeff, linux-ide, liml, albertl, jens.axboe, Tejun Heo, Rusty Russel
On Thu, 29 Nov 2007 23:33:31 +0900
Tejun Heo <htejun@gmail.com> wrote:
> With atapi_request_sense() converted to use sg, there's no user of
> non-sg interface. Kill non-sg interface.
>
> * ATA_QCFLAG_SINGLE and ATA_QCFLAG_SG are removed. ATA_QCFLAG_DMAMAP
> is used instead. (this way no LLD change is necessary)
>
> * qc->buf_virt is removed.
>
> * ata_sg_init_one() and ata_sg_setup_one() are removed.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Rusty Russel <rusty@rustcorp.com.au>
Acked-by: Alan Cox <alan@redhat.com>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 11/14] libata: add qc->dma_nbytes
2007-11-29 14:33 ` [PATCH 11/14] libata: add qc->dma_nbytes Tejun Heo
@ 2007-11-29 16:02 ` Alan Cox
2007-12-04 19:33 ` Jeff Garzik
1 sibling, 0 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-29 16:02 UTC (permalink / raw)
Cc: jeff, linux-ide, liml, albertl, jens.axboe, Tejun Heo
On Thu, 29 Nov 2007 23:33:34 +0900
Tejun Heo <htejun@gmail.com> wrote:
> qc->nbytes doesn't include extra buffers setup by libata core layer
> and my be odd. This patch adds qc->dma_nbytes which includes any
> extra buffers setup by libata core layer and is guaranteed to be
> aligned on 4 byte boundary.
Acked-by: Alan Cox <alan@redhat.com>
May be worth eventually making the alignment a variable in the host
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 13/14] libata: implement ATAPI per-command-type DMA horkages
2007-11-29 14:33 ` [PATCH 13/14] libata: implement ATAPI per-command-type DMA horkages Tejun Heo
@ 2007-11-29 16:04 ` Alan Cox
2007-11-29 16:10 ` Tejun Heo
0 siblings, 1 reply; 72+ messages in thread
From: Alan Cox @ 2007-11-29 16:04 UTC (permalink / raw)
Cc: jeff, linux-ide, liml, albertl, jens.axboe, Tejun Heo
On Thu, 29 Nov 2007 23:33:36 +0900
Tejun Heo <htejun@gmail.com> wrote:
> ATAPI DMA is filled with mines. Sector aligned READ transfers usually
> work but many other commands transfer non-sector aligned or variable
> number of bytes, and there are devices and controllers which have
> problems dealing with such non-aligned DMA transactions.
>
> This patch implement ATAPI per-command-type DMA horkages and EH logic
> to set those quickly. This way, failures localized to certain command
> type don't affect other operations (most importantly READs) and
> working configuration is found quickly such that the device can be
> used.
This I think makes sense. We want to fail rapidly when we discover DMA
problems with ATAPI and non core commands. OTOH we want to be pretty
resistant to randomly dropping into PIO on stuff we know works reliably.
> + ATAPI_DMA_HORKAGE_WRITE = (1 << 28), /* PIO for WRITEs */
> + ATAPI_DMA_HORKAGE_READ_CD = (1 << 29), /* PIO for READ_CDs */
> + ATAPI_DMA_HORKAGE_READ_DVD_STR = (1 << 30), /* PIO for READ DVD STR */
> + ATAPI_DMA_HORKAGE_MISC = (1 << 31), /* PIO for MISC commands */
> +
> /* DMA mask for user DMA control: User visible values; DO NOT
> renumber */
> ATA_DMA_MASK_ATA = (1 << 0), /* DMA on ATA Disk */
What are the locking rules for ->horkage at this point ?
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 14:33 ` [PATCH 14/14] libata: use PIO for misc ATAPI commands Tejun Heo
@ 2007-11-29 16:05 ` Alan Cox
2007-11-29 16:14 ` Tejun Heo
2007-12-04 19:34 ` Jeff Garzik
1 sibling, 1 reply; 72+ messages in thread
From: Alan Cox @ 2007-11-29 16:05 UTC (permalink / raw)
Cc: jeff, linux-ide, liml, albertl, jens.axboe, Tejun Heo
> Private command type / transfer length filtering in sata_promise,
> pata_it821x and pata_pdc2027x are removed as core layer filtering is
> more conservative.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
NAK - as before
This is the wrong way to do stuff. Stop penalizing the 99.99% of users
with perfectly sane hardware. In addition I note that the whitelist for
drives would rapidly be thousands of entries long and unmanagable.
Blacklist the problem drives - there aren't that many as far as I can see
from Fedora. Also don't confuse this with the pata_ali ATAPI problems
which are most of the ones I do find.
Alan
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 15:55 ` Alan Cox
@ 2007-11-29 16:06 ` Tejun Heo
2007-11-29 17:42 ` Alan Cox
0 siblings, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 16:06 UTC (permalink / raw)
To: Alan Cox; +Cc: jeff, linux-ide, liml, albertl, jens.axboe
Alan Cox wrote:
> On Thu, 29 Nov 2007 23:33:28 +0900
> Tejun Heo <htejun@gmail.com> wrote:
>
>> Depending on how many bytes are transferred as a unit, PIO data
>> tranasfer may consume more bytes than requested. Knowing how much
>> data is consumed is necessary to determine how much is left for
>> draining. This patch update ->data_xfer such that it returns the
>> number of consumed bytes.
>
> Why do we care ?
>
> If the drive and controller disagree or the drive has gone off its
> trolley all we actually care about is when the thing stops saying there
> is data. In addition all our methods transfer the entire block they are
> asked to each time.
It's for draining. Let's say drive says it wanna transfer 18 bytes but
the buffer is only 13 bytes long. If the transfer method consumes 2
bytes per read, it would consume 14 bytes. If the transfer method
consumes 4 bytes per read, it would consume 16 bytes. If we drain too
much, we risk hanging the machine.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/14] libata: improve ATAPI draining
2007-11-29 15:59 ` Alan Cox
@ 2007-11-29 16:09 ` Tejun Heo
2007-11-29 16:42 ` Tejun Heo
1 sibling, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 16:09 UTC (permalink / raw)
To: Alan Cox; +Cc: jeff, linux-ide, liml, albertl, jens.axboe, Albert Lee
Alan Cox wrote:
>> * Limit the amount of draining to ATAPI_MAX_DRAIN (16k currently).
>
> Why 16 not 64K ?
>
> Please add a comment to note that the drivers assume
> - drain is only ever done on a stuck PIO xfer
No. ATAPI draining is to accommodate buggy software and hardware and
it's for all misc ATAPI commands whether PIO or DMA.
> - drain occurs before any controller error/reset handling is done
This is part of normal command processing, not really related to EH.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 13/14] libata: implement ATAPI per-command-type DMA horkages
2007-11-29 16:04 ` Alan Cox
@ 2007-11-29 16:10 ` Tejun Heo
0 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 16:10 UTC (permalink / raw)
To: Alan Cox; +Cc: jeff, linux-ide, liml, albertl, jens.axboe
Alan Cox wrote:
> On Thu, 29 Nov 2007 23:33:36 +0900
> Tejun Heo <htejun@gmail.com> wrote:
>
>> ATAPI DMA is filled with mines. Sector aligned READ transfers usually
>> work but many other commands transfer non-sector aligned or variable
>> number of bytes, and there are devices and controllers which have
>> problems dealing with such non-aligned DMA transactions.
>>
>> This patch implement ATAPI per-command-type DMA horkages and EH logic
>> to set those quickly. This way, failures localized to certain command
>> type don't affect other operations (most importantly READs) and
>> working configuration is found quickly such that the device can be
>> used.
>
> This I think makes sense. We want to fail rapidly when we discover DMA
> problems with ATAPI and non core commands. OTOH we want to be pretty
> resistant to randomly dropping into PIO on stuff we know works reliably.
>
>> + ATAPI_DMA_HORKAGE_WRITE = (1 << 28), /* PIO for WRITEs */
>> + ATAPI_DMA_HORKAGE_READ_CD = (1 << 29), /* PIO for READ_CDs */
>> + ATAPI_DMA_HORKAGE_READ_DVD_STR = (1 << 30), /* PIO for READ DVD STR */
>> + ATAPI_DMA_HORKAGE_MISC = (1 << 31), /* PIO for MISC commands */
>> +
>> /* DMA mask for user DMA control: User visible values; DO NOT
>> renumber */
>> ATA_DMA_MASK_ATA = (1 << 0), /* DMA on ATA Disk */
>
> What are the locking rules for ->horkage at this point ?
It's owned by EH. Only EH sets it.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 16:05 ` Alan Cox
@ 2007-11-29 16:14 ` Tejun Heo
2007-11-29 16:16 ` Tejun Heo
2007-11-29 17:38 ` Alan Cox
0 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 16:14 UTC (permalink / raw)
To: Alan Cox; +Cc: jeff, linux-ide, liml, albertl, jens.axboe
Alan Cox wrote:
>> Private command type / transfer length filtering in sata_promise,
>> pata_it821x and pata_pdc2027x are removed as core layer filtering is
>> more conservative.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>
> NAK - as before
>
> This is the wrong way to do stuff. Stop penalizing the 99.99% of users
> with perfectly sane hardware. In addition I note that the whitelist for
> drives would rapidly be thousands of entries long and unmanagable.
Well, I guess we'll have to agree to disagree here. I just don't think
that penalty is worth considering and definitely don't think whitelist
is necessary at all.
> Blacklist the problem drives - there aren't that many as far as I can see
> from Fedora. Also don't confuse this with the pata_ali ATAPI problems
> which are most of the ones I do find.
If we're gonna take that road, let's lift 16 byte alignment check during
-rc cycles. It's something which masks the problem half way. It only
hinders the debugging process.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 16:14 ` Tejun Heo
@ 2007-11-29 16:16 ` Tejun Heo
2007-11-29 18:55 ` Mark Lord
2007-11-29 17:38 ` Alan Cox
1 sibling, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 16:16 UTC (permalink / raw)
To: Alan Cox; +Cc: jeff, linux-ide, liml, albertl, jens.axboe
Tejun Heo wrote:
> Alan Cox wrote:
>>> Private command type / transfer length filtering in sata_promise,
>>> pata_it821x and pata_pdc2027x are removed as core layer filtering is
>>> more conservative.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> NAK - as before
>>
>> This is the wrong way to do stuff. Stop penalizing the 99.99% of users
>> with perfectly sane hardware. In addition I note that the whitelist for
>> drives would rapidly be thousands of entries long and unmanagable.
>
> Well, I guess we'll have to agree to disagree here. I just don't think
> that penalty is worth considering and definitely don't think whitelist
> is necessary at all.
Also, what do other people think about the subject? IIRC, Jeff agrees
with Alan, right? Albert, Mark, what do you guys think?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/14] libata: improve ATAPI draining
2007-11-29 15:59 ` Alan Cox
2007-11-29 16:09 ` Tejun Heo
@ 2007-11-29 16:42 ` Tejun Heo
2007-11-29 17:40 ` Alan Cox
1 sibling, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 16:42 UTC (permalink / raw)
To: Alan Cox; +Cc: jeff, linux-ide, liml, albertl, jens.axboe, Albert Lee
Alan Cox wrote:
>> * Limit the amount of draining to ATAPI_MAX_DRAIN (16k currently).
>
> Why 16 not 64K ?
Oops, forgot to answer here. That would result in 16 sg entries for
draining which felt a bit too much. As draining is for misc commands, I
thought 16k should be enough.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 16:14 ` Tejun Heo
2007-11-29 16:16 ` Tejun Heo
@ 2007-11-29 17:38 ` Alan Cox
1 sibling, 0 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-29 17:38 UTC (permalink / raw)
To: Tejun Heo; +Cc: jeff, linux-ide, liml, albertl, jens.axboe
> If we're gonna take that road, let's lift 16 byte alignment check during
> -rc cycles. It's something which masks the problem half way. It only
> hinders the debugging process.
That would be a good project for the -mm tree. Perhaps lift it to 2, and
also see what happens if the DMA padding is set to 2 or to 16 - as for
some cases this could be a controller limit.
Alan
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/14] libata: improve ATAPI draining
2007-11-29 16:42 ` Tejun Heo
@ 2007-11-29 17:40 ` Alan Cox
2007-11-29 18:11 ` Tejun Heo
0 siblings, 1 reply; 72+ messages in thread
From: Alan Cox @ 2007-11-29 17:40 UTC (permalink / raw)
To: Tejun Heo; +Cc: jeff, linux-ide, liml, albertl, jens.axboe, Albert Lee
On Fri, 30 Nov 2007 01:42:46 +0900
Tejun Heo <htejun@gmail.com> wrote:
> Alan Cox wrote:
> >> * Limit the amount of draining to ATAPI_MAX_DRAIN (16k currently).
> >
> > Why 16 not 64K ?
>
> Oops, forgot to answer here. That would result in 16 sg entries for
> draining which felt a bit too much.
16 sg entries but only one page of memory so its not a bit bump ?
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 16:06 ` Tejun Heo
@ 2007-11-29 17:42 ` Alan Cox
2007-11-29 18:00 ` Tejun Heo
0 siblings, 1 reply; 72+ messages in thread
From: Alan Cox @ 2007-11-29 17:42 UTC (permalink / raw)
To: Tejun Heo; +Cc: jeff, linux-ide, liml, albertl, jens.axboe
> It's for draining. Let's say drive says it wanna transfer 18 bytes but
> the buffer is only 13 bytes long. If the transfer method consumes 2
> bytes per read, it would consume 14 bytes. If the transfer method
> consumes 4 bytes per read, it would consume 16 bytes. If we drain too
> much, we risk hanging the machine.
Do we have any actual cases where trying to drain beyond the controller
granuality is a problem ?
Might be cleaner to sort this (and the DMA assumption of 2 byte
alignment, and possibly 16 byte for some devices/controllers) with
dev->pio_io_size;
dev->dma_io_size;
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 17:42 ` Alan Cox
@ 2007-11-29 18:00 ` Tejun Heo
2007-11-29 18:24 ` Alan Cox
0 siblings, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 18:00 UTC (permalink / raw)
To: Alan Cox; +Cc: jeff, linux-ide, liml, albertl, jens.axboe
Alan Cox wrote:
>> It's for draining. Let's say drive says it wanna transfer 18 bytes but
>> the buffer is only 13 bytes long. If the transfer method consumes 2
>> bytes per read, it would consume 14 bytes. If the transfer method
>> consumes 4 bytes per read, it would consume 16 bytes. If we drain too
>> much, we risk hanging the machine.
>
> Do we have any actual cases where trying to drain beyond the controller
> granuality is a problem ?
>
> Might be cleaner to sort this (and the DMA assumption of 2 byte
> alignment, and possibly 16 byte for some devices/controllers) with
>
> dev->pio_io_size;
> dev->dma_io_size;
>
That could be cleaner although slightly less flexible. For example, a
driver may do 32bit IOs normally but choose to do smaller size IO at the
end for whatever reason.
DMA alignment is host restriction so I think it belongs to ata_host if
we ever need it. Do you know of any controller which require such
thing? No need to add complexity when it's not necessary.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/14] libata: improve ATAPI draining
2007-11-29 17:40 ` Alan Cox
@ 2007-11-29 18:11 ` Tejun Heo
2007-11-29 18:19 ` Alan Cox
0 siblings, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 18:11 UTC (permalink / raw)
To: Alan Cox; +Cc: jeff, linux-ide, liml, albertl, jens.axboe, Albert Lee
Alan Cox wrote:
> On Fri, 30 Nov 2007 01:42:46 +0900
> Tejun Heo <htejun@gmail.com> wrote:
>
>> Alan Cox wrote:
>>>> * Limit the amount of draining to ATAPI_MAX_DRAIN (16k currently).
>>> Why 16 not 64K ?
>> Oops, forgot to answer here. That would result in 16 sg entries for
>> draining which felt a bit too much.
>
> 16 sg entries but only one page of memory so its not a bit bump ?
The only added cpu cycle overhead is to setup sg entries and controller
DMA table accordingly which should be pretty small. I was more worried
about reserving that many sg entries as it can affect bulk data
transfers. I thought about appending draining sg's on left-over SGs.
ie. drain only min(nr_left_sgs, ATA_MAX_DRAIN_PAGES) pages but it ends
up dereferencing deep upto request_queue and things get a bit complex
because the way libata currently associates ata_device's with scsi_device's.
So, I thought 16k was a good trade off. Not as comfortable as 64k tho,
I agree. Would it be worth to add more complexity for this?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/14] libata: improve ATAPI draining
2007-11-29 18:11 ` Tejun Heo
@ 2007-11-29 18:19 ` Alan Cox
0 siblings, 0 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-29 18:19 UTC (permalink / raw)
To: Tejun Heo; +Cc: jeff, linux-ide, liml, albertl, jens.axboe, Albert Lee
> So, I thought 16k was a good trade off. Not as comfortable as 64k tho,
> I agree. Would it be worth to add more complexity for this?
Fair point .. I don't know. Perhaps we should wait and see.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 18:00 ` Tejun Heo
@ 2007-11-29 18:24 ` Alan Cox
2007-11-29 18:57 ` Mark Lord
2007-11-29 19:36 ` Jeff Garzik
0 siblings, 2 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-29 18:24 UTC (permalink / raw)
To: Tejun Heo; +Cc: jeff, linux-ide, liml, albertl, jens.axboe
> DMA alignment is host restriction so I think it belongs to ata_host if
> we ever need it. Do you know of any controller which require such
> thing? No need to add complexity when it's not necessary.
If we ever get the blasted inic162x working then that appears to have
some alignment limits. At least the docs say the DMA buffers must be quad
word aligned and sized (although it doesn't describe what occurs if the
total length of xfer disagrees with the buffers)
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 16:16 ` Tejun Heo
@ 2007-11-29 18:55 ` Mark Lord
2007-11-29 22:52 ` Tejun Heo
0 siblings, 1 reply; 72+ messages in thread
From: Mark Lord @ 2007-11-29 18:55 UTC (permalink / raw)
To: Tejun Heo; +Cc: Alan Cox, jeff, linux-ide, albertl, jens.axboe
Tejun Heo wrote:
> Tejun Heo wrote:
>> Alan Cox wrote:
>>>> Private command type / transfer length filtering in sata_promise,
>>>> pata_it821x and pata_pdc2027x are removed as core layer filtering is
>>>> more conservative.
>>>>
>>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>> NAK - as before
>>>
>>> This is the wrong way to do stuff. Stop penalizing the 99.99% of users
>>> with perfectly sane hardware. In addition I note that the whitelist for
>>> drives would rapidly be thousands of entries long and unmanagable.
>> Well, I guess we'll have to agree to disagree here. I just don't think
>> that penalty is worth considering and definitely don't think whitelist
>> is necessary at all.
>
> Also, what do other people think about the subject? IIRC, Jeff agrees
> with Alan, right? Albert, Mark, what do you guys think?
...
I'm split on this one. For fast systems (typical notebook/desktop) it's
almost a non-issue either way.
But a lot of media boxes will be using this code, and they tend to have
very low-power, slow-clockrate CPUs in the 200-800Mhz range, and so the
real-time hit there (from PIO) will have a much more significant impact.
Using DMA as much as possible on those slower platforms is definitely
a plus towards avoiding non-jerky, skipped frames, or start-stoppy DVD recording.
Now the most intensive commands are still DMA under the proposed scheme,
so it's not those that one would be concerned with. But dropping to PIO
even for a few uncommon commands will still peg a real-time hit or two
on a slower media processor.
So.. ????
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 18:24 ` Alan Cox
@ 2007-11-29 18:57 ` Mark Lord
2007-11-29 19:37 ` Jeff Garzik
2007-11-29 19:36 ` Jeff Garzik
1 sibling, 1 reply; 72+ messages in thread
From: Mark Lord @ 2007-11-29 18:57 UTC (permalink / raw)
To: Alan Cox; +Cc: Tejun Heo, jeff, linux-ide, albertl, jens.axboe
Alan Cox wrote:
>> DMA alignment is host restriction so I think it belongs to ata_host if
>> we ever need it. Do you know of any controller which require such
>> thing? No need to add complexity when it's not necessary.
>
> If we ever get the blasted inic162x working then that appears to have
> some alignment limits. At least the docs say the DMA buffers must be quad
> word aligned and sized (although it doesn't describe what occurs if the
> total length of xfer disagrees with the buffers)
...
If it's an ADMA device, then they may support bits in the CPB
to direct what should happen for various overrun/underrun conditions.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 18:24 ` Alan Cox
2007-11-29 18:57 ` Mark Lord
@ 2007-11-29 19:36 ` Jeff Garzik
2007-11-29 21:22 ` Alan Cox
1 sibling, 1 reply; 72+ messages in thread
From: Jeff Garzik @ 2007-11-29 19:36 UTC (permalink / raw)
To: Alan Cox; +Cc: Tejun Heo, linux-ide, liml, albertl, jens.axboe
Alan Cox wrote:
>> DMA alignment is host restriction so I think it belongs to ata_host if
>> we ever need it. Do you know of any controller which require such
>> thing? No need to add complexity when it's not necessary.
>
> If we ever get the blasted inic162x working then that appears to have
> some alignment limits. At least the docs say the DMA buffers must be quad
> word aligned and sized (although it doesn't describe what occurs if the
> total length of xfer disagrees with the buffers)
Most DMA has alignment limits, for the front not the back of the buffer.
Jeff
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 18:57 ` Mark Lord
@ 2007-11-29 19:37 ` Jeff Garzik
0 siblings, 0 replies; 72+ messages in thread
From: Jeff Garzik @ 2007-11-29 19:37 UTC (permalink / raw)
To: Mark Lord; +Cc: Alan Cox, Tejun Heo, linux-ide, albertl, jens.axboe
Mark Lord wrote:
> Alan Cox wrote:
>>> DMA alignment is host restriction so I think it belongs to ata_host if
>>> we ever need it. Do you know of any controller which require such
>>> thing? No need to add complexity when it's not necessary.
>>
>> If we ever get the blasted inic162x working then that appears to have
>> some alignment limits. At least the docs say the DMA buffers must be quad
>> word aligned and sized (although it doesn't describe what occurs if the
>> total length of xfer disagrees with the buffers)
> ...
>
> If it's an ADMA device, then they may support bits in the CPB
> to direct what should happen for various overrun/underrun conditions.
Yeah most devices that are not strictly SFF (i.e. have a real register
space) are like that.
Jeff
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 19:36 ` Jeff Garzik
@ 2007-11-29 21:22 ` Alan Cox
0 siblings, 0 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-29 21:22 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Tejun Heo, linux-ide, liml, albertl, jens.axboe
On Thu, 29 Nov 2007 14:36:30 -0500
Jeff Garzik <jeff@garzik.org> wrote:
> Alan Cox wrote:
> >> DMA alignment is host restriction so I think it belongs to ata_host if
> >> we ever need it. Do you know of any controller which require such
> >> thing? No need to add complexity when it's not necessary.
> >
> > If we ever get the blasted inic162x working then that appears to have
> > some alignment limits. At least the docs say the DMA buffers must be quad
> > word aligned and sized (although it doesn't describe what occurs if the
> > total length of xfer disagrees with the buffers)
>
> Most DMA has alignment limits, for the front not the back of the buffer.
Note I said "and sized" - its got restrictions for both buffer ends in
effect
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 18:55 ` Mark Lord
@ 2007-11-29 22:52 ` Tejun Heo
2007-11-30 1:03 ` Mark Lord
2007-11-30 13:09 ` Alan Cox
0 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-29 22:52 UTC (permalink / raw)
To: Mark Lord; +Cc: Alan Cox, jeff, linux-ide, albertl, jens.axboe
Mark Lord wrote:
> I'm split on this one. For fast systems (typical notebook/desktop) it's
> almost a non-issue either way.
>
> But a lot of media boxes will be using this code, and they tend to have
> very low-power, slow-clockrate CPUs in the 200-800Mhz range, and so the
> real-time hit there (from PIO) will have a much more significant impact.
>
> Using DMA as much as possible on those slower platforms is definitely
> a plus towards avoiding non-jerky, skipped frames, or start-stoppy DVD
> recording.
>
> Now the most intensive commands are still DMA under the proposed scheme,
> so it's not those that one would be concerned with. But dropping to PIO
> even for a few uncommon commands will still peg a real-time hit or two
> on a slower media processor.
>
> So.. ????
One thing I don't understand about this argument is that PIO cycle time
is not determined by CPU power. It's bound by PCI and ATA bus speed.
If you put a faster CPU on the job, it just ends up wasting the same
amount of time burning up more CPU cycles or am I misjudging power of
those embedded CPUs?
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 22:52 ` Tejun Heo
@ 2007-11-30 1:03 ` Mark Lord
2007-11-30 1:34 ` Tejun Heo
2007-11-30 13:09 ` Alan Cox
1 sibling, 1 reply; 72+ messages in thread
From: Mark Lord @ 2007-11-30 1:03 UTC (permalink / raw)
To: Tejun Heo; +Cc: Alan Cox, jeff, linux-ide, albertl, jens.axboe
Tejun Heo wrote:
> Mark Lord wrote:
>> I'm split on this one. For fast systems (typical notebook/desktop) it's
>> almost a non-issue either way.
>>
>> But a lot of media boxes will be using this code, and they tend to have
>> very low-power, slow-clockrate CPUs in the 200-800Mhz range, and so the
>> real-time hit there (from PIO) will have a much more significant impact.
>>
>> Using DMA as much as possible on those slower platforms is definitely
>> a plus towards avoiding non-jerky, skipped frames, or start-stoppy DVD
>> recording.
>>
>> Now the most intensive commands are still DMA under the proposed scheme,
>> so it's not those that one would be concerned with. But dropping to PIO
>> even for a few uncommon commands will still peg a real-time hit or two
>> on a slower media processor.
>>
>> So.. ????
>
> One thing I don't understand about this argument is that PIO cycle time
> is not determined by CPU power. It's bound by PCI and ATA bus speed.
> If you put a faster CPU on the job, it just ends up wasting the same
> amount of time burning up more CPU cycles or am I misjudging power of
> those embedded CPUs?
...
Yes, it wastes the same amount of real (clock on the wall) time.
But since it is typically a much slower system to begin with,
it has little in the way of "excess" computing capability,
so those few hundred microseconds of PIO time mean a lot
more to it than they would to a GHz+ processor.
In other words, such devices are usually close to their real-time
limits to begin with, so a 200-300usec real-time delay is difficult
to "make up" afterwards.
For a 1.8GHz Pentium-M, no problem --> it can churn away about
2-3 thousand instructions per microsecond afterwards.
But an 800MHz geode would require 2-8X that amount of real time
to execute the same number of "catch up" instructions.
Which means latencies for other things will be that much longer.
Or at least that's how I see it.
* * *
If I were doing this, I'd probably just do PIO for all non bulk xfer
opcodes, same as you, for ease of maintainership.
But I'd also think really hard about some way to allow full DMA
for chipsets/drives that can be trusted. Not a whitelist (like Alan,
I don't believe in those), but something..
Cheers
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-30 1:03 ` Mark Lord
@ 2007-11-30 1:34 ` Tejun Heo
2007-11-30 4:19 ` Mark Lord
2007-11-30 13:40 ` Alan Cox
0 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-30 1:34 UTC (permalink / raw)
To: Mark Lord; +Cc: Alan Cox, jeff, linux-ide, albertl, jens.axboe
Hello,
Mark Lord wrote:
>> One thing I don't understand about this argument is that PIO cycle time
>> is not determined by CPU power. It's bound by PCI and ATA bus speed.
>> If you put a faster CPU on the job, it just ends up wasting the same
>> amount of time burning up more CPU cycles or am I misjudging power of
>> those embedded CPUs?
> ...
>
> Yes, it wastes the same amount of real (clock on the wall) time.
>
> But since it is typically a much slower system to begin with,
> it has little in the way of "excess" computing capability,
> so those few hundred microseconds of PIO time mean a lot
> more to it than they would to a GHz+ processor.
>
> In other words, such devices are usually close to their real-time
> limits to begin with, so a 200-300usec real-time delay is difficult
> to "make up" afterwards.
>
> For a 1.8GHz Pentium-M, no problem --> it can churn away about
> 2-3 thousand instructions per microsecond afterwards.
>
> But an 800MHz geode would require 2-8X that amount of real time
> to execute the same number of "catch up" instructions.
Okay, that makes more sense. The only thing which happens regularly is
polling for media change which involves reading maybe a few tens of
bytes. The overhead of doing those using PIO wouldn't be too much more
than the cost of writing CDBs out. It would help more if we can cut
down the number of commands used for testing media changed event (on my
to do list).
During probing, it doesn't really matter. The only other case where
many of these misc ATAPI commands can occur is when new media is
inserted and starting up writing. And yeah I can see it might hurt if
the processor is already fully loaded.
> If I were doing this, I'd probably just do PIO for all non bulk xfer
> opcodes, same as you, for ease of maintainership.
>
> But I'd also think really hard about some way to allow full DMA
> for chipsets/drives that can be trusted. Not a whitelist (like Alan,
> I don't believe in those), but something..
Does module parameter / sysfs node sound good enough to you?
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-30 1:34 ` Tejun Heo
@ 2007-11-30 4:19 ` Mark Lord
2007-11-30 13:40 ` Alan Cox
1 sibling, 0 replies; 72+ messages in thread
From: Mark Lord @ 2007-11-30 4:19 UTC (permalink / raw)
To: Tejun Heo; +Cc: Alan Cox, jeff, linux-ide, albertl, jens.axboe
Tejun Heo wrote:
> Mark Lord wrote:
>
>> If I were doing this, I'd probably just do PIO for all non bulk xfer
>> opcodes, same as you, for ease of maintainership.
>>
>> But I'd also think really hard about some way to allow full DMA
>> for chipsets/drives that can be trusted. Not a whitelist (like Alan,
>> I don't believe in those), but something..
>
> Does module parameter / sysfs node sound good enough to you?
..
Oooohh.. I *like* the idea of a sysfs attr or two (per drive) for this..
:)
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 22:52 ` Tejun Heo
2007-11-30 1:03 ` Mark Lord
@ 2007-11-30 13:09 ` Alan Cox
1 sibling, 0 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-30 13:09 UTC (permalink / raw)
To: Tejun Heo; +Cc: Mark Lord, jeff, linux-ide, albertl, jens.axboe
> One thing I don't understand about this argument is that PIO cycle time
> is not determined by CPU power. It's bound by PCI and ATA bus speed.
> If you put a faster CPU on the job, it just ends up wasting the same
> amount of time burning up more CPU cycles or am I misjudging power of
> those embedded CPUs?
The faster your CPU the more processor cycles you have between ATA stalls
to do the work. You also probably have hypedthreading which will help a
bit as you'll stall half the CPU not all of it some of the time at least.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-30 1:34 ` Tejun Heo
2007-11-30 4:19 ` Mark Lord
@ 2007-11-30 13:40 ` Alan Cox
2007-11-30 13:59 ` Tejun Heo
1 sibling, 1 reply; 72+ messages in thread
From: Alan Cox @ 2007-11-30 13:40 UTC (permalink / raw)
To: Tejun Heo; +Cc: Mark Lord, jeff, linux-ide, albertl, jens.axboe
> polling for media change which involves reading maybe a few tens of
> bytes. The overhead of doing those using PIO wouldn't be too much more
> than the cost of writing CDBs out. It would help more if we can cut
> down the number of commands used for testing media changed event (on my
> to do list).
Actually the overhead is foul on some laptops as the command byte write
wakes up the controller, which then whirrs away for a bit recovering its
status before deciding to give us an answer.
> Does module parameter / sysfs node sound good enough to you?
You mean like the existing "dma" one we already have which lets you turn
on/off atapi DMA and could be extended with another bit so we went from
1 - disk
2 - cd
4 - cf
to
1 - disk
2 - cd (basic commands)
4 - cf
8 - cd (all)
(The idea being that its ordered so troubleshooting almost always comes
down to 1,3 - or 1,3,7 with this). So we have 99.9% of this
As to the sysfs node for per device setup that makes sense as does fixing
the support to allow users to force a mode of their choice, and putting
in the geometry fields.
Alan
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-30 13:40 ` Alan Cox
@ 2007-11-30 13:59 ` Tejun Heo
2007-11-30 14:12 ` Jeff Garzik
2007-11-30 15:36 ` Alan Cox
0 siblings, 2 replies; 72+ messages in thread
From: Tejun Heo @ 2007-11-30 13:59 UTC (permalink / raw)
To: Alan Cox; +Cc: Mark Lord, jeff, linux-ide, albertl, jens.axboe
Alan Cox wrote:
>> polling for media change which involves reading maybe a few tens of
>> bytes. The overhead of doing those using PIO wouldn't be too much more
>> than the cost of writing CDBs out. It would help more if we can cut
>> down the number of commands used for testing media changed event (on my
>> to do list).
>
> Actually the overhead is foul on some laptops as the command byte write
> wakes up the controller, which then whirrs away for a bit recovering its
> status before deciding to give us an answer.
Yeah, but that happens whether DMA is used or not. Probably what's
needed is disabling media presence polling according to power profile.
>> Does module parameter / sysfs node sound good enough to you?
>
> You mean like the existing "dma" one we already have which lets you turn
> on/off atapi DMA and could be extended with another bit so we went from
>
> 1 - disk
> 2 - cd
> 4 - cf
>
> to
>
> 1 - disk
> 2 - cd (basic commands)
> 4 - cf
> 8 - cd (all)
>
>
> (The idea being that its ordered so troubleshooting almost always comes
> down to 1,3 - or 1,3,7 with this). So we have 99.9% of this
Basically, yes but I think it would be nice to be able to change the
value on the fly.
> As to the sysfs node for per device setup that makes sense as does fixing
> the support to allow users to force a mode of their choice, and putting
> in the geometry fields.
Yeap.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-30 13:59 ` Tejun Heo
@ 2007-11-30 14:12 ` Jeff Garzik
2007-11-30 15:36 ` Alan Cox
1 sibling, 0 replies; 72+ messages in thread
From: Jeff Garzik @ 2007-11-30 14:12 UTC (permalink / raw)
To: Tejun Heo; +Cc: Alan Cox, Mark Lord, linux-ide, albertl, jens.axboe
Tejun Heo wrote:
> Alan Cox wrote:
>>> polling for media change which involves reading maybe a few tens of
>>> bytes. The overhead of doing those using PIO wouldn't be too much more
>>> than the cost of writing CDBs out. It would help more if we can cut
>>> down the number of commands used for testing media changed event (on my
>>> to do list).
>> Actually the overhead is foul on some laptops as the command byte write
>> wakes up the controller, which then whirrs away for a bit recovering its
>> status before deciding to give us an answer.
>
> Yeah, but that happens whether DMA is used or not. Probably what's
> needed is disabling media presence polling according to power profile.
>
>>> Does module parameter / sysfs node sound good enough to you?
>> You mean like the existing "dma" one we already have which lets you turn
>> on/off atapi DMA and could be extended with another bit so we went from
>>
>> 1 - disk
>> 2 - cd
>> 4 - cf
>>
>> to
>>
>> 1 - disk
>> 2 - cd (basic commands)
>> 4 - cf
>> 8 - cd (all)
>>
>>
>> (The idea being that its ordered so troubleshooting almost always comes
>> down to 1,3 - or 1,3,7 with this). So we have 99.9% of this
>
> Basically, yes but I think it would be nice to be able to change the
> value on the fly.
It would also be nice to change it per-device rather than globally...
Jeff
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-30 13:59 ` Tejun Heo
2007-11-30 14:12 ` Jeff Garzik
@ 2007-11-30 15:36 ` Alan Cox
1 sibling, 0 replies; 72+ messages in thread
From: Alan Cox @ 2007-11-30 15:36 UTC (permalink / raw)
To: Tejun Heo; +Cc: Mark Lord, jeff, linux-ide, albertl, jens.axboe
> Basically, yes but I think it would be nice to be able to change the
> value on the fly.
If you export it then that will "just work". If you want to be able to
set it per device we just need to inherit it at creation time for the
device.
>
> > As to the sysfs node for per device setup that makes sense as does fixing
> > the support to allow users to force a mode of their choice, and putting
> > in the geometry fields.
>
> Yeap.
We need the sysfs for geometry for ATA-1 and pre ATA devices as well
which means we want a way to write to it when we find a device of unknown
geometry we can't start.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/14] libata: improve ATAPI draining
2007-11-29 14:33 ` [PATCH 06/14] libata: improve ATAPI draining Tejun Heo
2007-11-29 15:59 ` Alan Cox
@ 2007-12-04 10:06 ` Albert Lee
2007-12-05 1:00 ` Tejun Heo
1 sibling, 1 reply; 72+ messages in thread
From: Albert Lee @ 2007-12-04 10:06 UTC (permalink / raw)
To: Tejun Heo; +Cc: jeff, linux-ide, alan, liml, albertl, jens.axboe
Tejun Heo wrote:
> For misc ATAPI commands which transfer variable length data to the
> host, overflow can occur due to application or hardware bug. Such
> overflows can be ignored safely as long as overflow data is properly
> drained. libata HSM implementation has this implemented in
> __atapi_pio_bytes() but it isn't enough. Improve drain logic such
> that...
>
> * Multiple PIO data phases are allowed. Not allowing this used to be
> okay when transfer chunk size was set to 8k unconditionally but with
> transfer hcunk size set to allocation size, treating extra PIO data
> phases as HSM violations cause a lot of trouble.
>
> * Limit the amount of draining to ATAPI_MAX_DRAIN (16k currently).
>
> * Don't whine if overflow is allowed and safe. When unexpected
> overflow occurs, trigger HSM violation and report the problem using
> ehi error description.
>
> * Properly calculate the number of bytes to be drained considering
> actual number of consumed bytes for partial draining.
>
> * Add and use ata_drain_page for draining. This change fixes the
> problem where LLDs which do 32bit IOs consumes 4 bytes on each 2
> byte draining resulting in draining twice more data than requested.
>
> This patch fixes ATAPI regressions introduced by setting transfer
> chunk size to allocation size.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Albert Lee <albertcc@tw.ibm.com>
Sorry for the late reply. The new patch looks good.
However, I am a little worried about the following code segment in
__atapi_pio_bytes():
next_sg:
if (!bytes)
return 0;
Maybe we should add an additional check to atapi_pio_bytes() such
that "DRQ asserted with bytes = 0" is considered as AC_ERR_HSM?
(patch attached below.) Otherwise the device might keep interruping us
with DRQ asserted + zero byte count, and we are in infinite loop...
--
albert
diff -Nrup 00_libata-dev/drivers/ata/libata-core.c 07_more_check/drivers/ata/libata-core.c
--- 00_libata-dev/drivers/ata/libata-core.c 2007-11-14 10:08:36.000000000 +0800
+++ 07_more_check/drivers/ata/libata-core.c 2007-12-04 18:03:48.000000000 +0800
@@ -5341,6 +5341,9 @@ static void atapi_pio_bytes(struct ata_q
if (do_write != i_write)
goto err_out;
+ if (!bytes)
+ goto err_out;
+
VPRINTK("ata%u: xfering %d bytes\n", ap->print_id, bytes);
__atapi_pio_bytes(qc, bytes);
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size
2007-11-29 14:33 ` [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size Tejun Heo
2007-11-29 15:51 ` Alan Cox
@ 2007-12-04 19:27 ` Jeff Garzik
2007-12-05 0:47 ` Tejun Heo
1 sibling, 1 reply; 72+ messages in thread
From: Jeff Garzik @ 2007-12-04 19:27 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, alan, liml, albertl, jens.axboe
Tejun Heo wrote:
> While updating lbam/h for ATAPI commands, atapi_eh_request_sense() was
> left out. Update it.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> drivers/ata/libata-eh.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 77083b5..2e3d3a2 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1302,8 +1302,8 @@ static unsigned int atapi_eh_request_sense(struct ata_queued_cmd *qc)
> tf.feature |= ATAPI_PKT_DMA;
> } else {
> tf.protocol = ATA_PROT_ATAPI;
> - tf.lbam = (8 * 1024) & 0xff;
> - tf.lbah = (8 * 1024) >> 8;
> + tf.lbam = SCSI_SENSE_BUFFERSIZE;
> + tf.lbah = 0;
seems like #upstream-fixes material?
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-11-29 14:33 ` [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes Tejun Heo
2007-11-29 15:55 ` Alan Cox
@ 2007-12-04 19:30 ` Jeff Garzik
2007-12-05 0:49 ` Tejun Heo
1 sibling, 1 reply; 72+ messages in thread
From: Jeff Garzik @ 2007-12-04 19:30 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, alan, liml, albertl, jens.axboe
Tejun Heo wrote:
> Depending on how many bytes are transferred as a unit, PIO data
> tranasfer may consume more bytes than requested. Knowing how much
> data is consumed is necessary to determine how much is left for
> draining. This patch update ->data_xfer such that it returns the
> number of consumed bytes.
>
> While at it, it also makes the following changes.
>
> * s/adev/dev/
> * s/buflen/len/
> * use READ/WRITE constants for rw indication
> * misc clean ups
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> drivers/ata/libata-core.c | 56 ++++++++++++++++++++++++++---------------
> drivers/ata/pata_bf54x.c | 34 +++++++++++++------------
> drivers/ata/pata_ixp4xx_cf.c | 32 ++++++++++++-----------
> drivers/ata/pata_legacy.c | 38 +++++++++++++++-------------
> drivers/ata/pata_qdi.c | 32 +++++++++++++----------
> drivers/ata/pata_scc.c | 38 +++++++++++++++-------------
> drivers/ata/pata_winbond.c | 32 +++++++++++++----------
> include/linux/libata.h | 11 ++++---
> 8 files changed, 152 insertions(+), 121 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index bc53492..10f3b42 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4966,48 +4966,55 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
>
> /**
> * ata_data_xfer - Transfer data by PIO
> - * @adev: device to target
> + * @dev: device to target
> * @buf: data buffer
> - * @buflen: buffer length
> + * @len: buffer length
> * @write_data: read/write
> *
> * Transfer data from/to the device data register by PIO.
> *
> * LOCKING:
> * Inherited from caller.
> + *
> + * RETURNS:
> + * Bytes consumed.
> */
> -void ata_data_xfer(struct ata_device *adev, unsigned char *buf,
> - unsigned int buflen, int write_data)
> +unsigned int ata_data_xfer(struct ata_device *dev, unsigned char *buf,
> + unsigned int len, int rw)
> {
> - struct ata_port *ap = adev->link->ap;
> - unsigned int words = buflen >> 1;
> + struct ata_port *ap = dev->link->ap;
> + void __iomem *data_addr = ap->ioaddr.data_addr;
> + unsigned int words = len >> 1;
>
> /* Transfer multiple of 2 bytes */
> - if (write_data)
> - iowrite16_rep(ap->ioaddr.data_addr, buf, words);
> + if (rw == READ)
> + ioread16_rep(data_addr, buf, words);
> else
> - ioread16_rep(ap->ioaddr.data_addr, buf, words);
> + iowrite16_rep(data_addr, buf, words);
>
> /* Transfer trailing 1 byte, if any. */
> - if (unlikely(buflen & 0x01)) {
> + if (unlikely(len & 0x01)) {
> u16 align_buf[1] = { 0 };
> - unsigned char *trailing_buf = buf + buflen - 1;
> + unsigned char *trailing_buf = buf + len - 1;
>
> - if (write_data) {
> - memcpy(align_buf, trailing_buf, 1);
> - iowrite16(le16_to_cpu(align_buf[0]), ap->ioaddr.data_addr);
> - } else {
> - align_buf[0] = cpu_to_le16(ioread16(ap->ioaddr.data_addr));
> + if (rw == READ) {
> + align_buf[0] = cpu_to_le16(ioread16(data_addr));
> memcpy(trailing_buf, align_buf, 1);
> + } else {
> + memcpy(align_buf, trailing_buf, 1);
> + iowrite16(le16_to_cpu(align_buf[0]), data_addr);
> }
> + words++;
> }
> +
> + return words << 1;
> }
>
> /**
> * ata_data_xfer_noirq - Transfer data by PIO
> - * @adev: device to target
> + * @dev: device to target
> * @buf: data buffer
> - * @buflen: buffer length
> + * @len: buffer length
> * @write_data: read/write
> *
> * Transfer data from/to the device data register by PIO. Do the
> @@ -5015,14 +5022,21 @@ void ata_data_xfer(struct ata_device *adev, unsigned char *buf,
> *
> * LOCKING:
> * Inherited from caller.
> + *
> + * RETURNS:
> + * Bytes consumed.
> */
> -void ata_data_xfer_noirq(struct ata_device *adev, unsigned char *buf,
> - unsigned int buflen, int write_data)
> +unsigned int ata_data_xfer_noirq(struct ata_device *dev, unsigned char *buf,
> + unsigned int len, int rw)
> {
> unsigned long flags;
> + unsigned int consumed;
> +
> local_irq_save(flags);
> - ata_data_xfer(adev, buf, buflen, write_data);
> + consumed = ata_data_xfer(dev, buf, len, rw);
> local_irq_restore(flags);
> +
> + return consumed;
> }
>
>
> diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
> index 81db405..53ae7d3 100644
> --- a/drivers/ata/pata_bf54x.c
> +++ b/drivers/ata/pata_bf54x.c
> @@ -1161,40 +1161,42 @@ static unsigned char bfin_bmdma_status(struct ata_port *ap)
> * bfin_data_xfer - Transfer data by PIO
> * @adev: device for this I/O
> * @buf: data buffer
> - * @buflen: buffer length
> + * @len: buffer length
> * @write_data: read/write
> *
> * Note: Original code is ata_data_xfer().
> */
>
> -static void bfin_data_xfer(struct ata_device *adev, unsigned char *buf,
> - unsigned int buflen, int write_data)
> +static unsigned int bfin_data_xfer(struct ata_device *dev, unsigned char *buf,
> + unsigned int len, int rw)
> {
> - struct ata_port *ap = adev->link->ap;
> - unsigned int words = buflen >> 1;
> - unsigned short *buf16 = (u16 *) buf;
> + struct ata_port *ap = dev->link->ap;
> void __iomem *base = (void __iomem *)ap->ioaddr.ctl_addr;
> + unsigned int words = len >> 1;
> + unsigned short *buf16 = (u16 *)buf;
>
> /* Transfer multiple of 2 bytes */
> - if (write_data) {
> - write_atapi_data(base, words, buf16);
> - } else {
> + if (rw == READ)
> read_atapi_data(base, words, buf16);
> - }
> + else
> + write_atapi_data(base, words, buf16);
>
> /* Transfer trailing 1 byte, if any. */
> - if (unlikely(buflen & 0x01)) {
> + if (unlikely(len & 0x01)) {
> unsigned short align_buf[1] = { 0 };
> - unsigned char *trailing_buf = buf + buflen - 1;
> + unsigned char *trailing_buf = buf + len - 1;
>
> - if (write_data) {
> - memcpy(align_buf, trailing_buf, 1);
> - write_atapi_data(base, 1, align_buf);
> - } else {
> + if (rw == READ) {
> read_atapi_data(base, 1, align_buf);
> memcpy(trailing_buf, align_buf, 1);
> + } else {
> + memcpy(align_buf, trailing_buf, 1);
> + write_atapi_data(base, 1, align_buf);
> }
> + words++;
> }
> +
> + return words << 1;
> }
>
> /**
> diff --git a/drivers/ata/pata_ixp4xx_cf.c b/drivers/ata/pata_ixp4xx_cf.c
> index fcd532a..b8a6ce3 100644
> --- a/drivers/ata/pata_ixp4xx_cf.c
> +++ b/drivers/ata/pata_ixp4xx_cf.c
> @@ -42,13 +42,13 @@ static int ixp4xx_set_mode(struct ata_link *link, struct ata_device **error)
> return 0;
> }
>
> -static void ixp4xx_mmio_data_xfer(struct ata_device *adev, unsigned char *buf,
> - unsigned int buflen, int write_data)
> +static unsigned int ixp4xx_mmio_data_xfer(struct ata_device *dev,
> + unsigned char *buf, unsigned int len, int rw)
> {
> unsigned int i;
> - unsigned int words = buflen >> 1;
> + unsigned int words = len >> 1;
> u16 *buf16 = (u16 *) buf;
> - struct ata_port *ap = adev->link->ap;
> + struct ata_port *ap = dev->link->ap;
> void __iomem *mmio = ap->ioaddr.data_addr;
> struct ixp4xx_pata_data *data = ap->host->dev->platform_data;
>
> @@ -59,30 +59,32 @@ static void ixp4xx_mmio_data_xfer(struct ata_device *adev, unsigned char *buf,
> udelay(100);
>
> /* Transfer multiple of 2 bytes */
> - if (write_data) {
> - for (i = 0; i < words; i++)
> - writew(buf16[i], mmio);
> - } else {
> + if (rw == READ)
> for (i = 0; i < words; i++)
> buf16[i] = readw(mmio);
> - }
> + else
> + for (i = 0; i < words; i++)
> + writew(buf16[i], mmio);
>
> /* Transfer trailing 1 byte, if any. */
> - if (unlikely(buflen & 0x01)) {
> + if (unlikely(len & 0x01)) {
> u16 align_buf[1] = { 0 };
> - unsigned char *trailing_buf = buf + buflen - 1;
> + unsigned char *trailing_buf = buf + len - 1;
>
> - if (write_data) {
> - memcpy(align_buf, trailing_buf, 1);
> - writew(align_buf[0], mmio);
> - } else {
> + if (rw == READ) {
> align_buf[0] = readw(mmio);
> memcpy(trailing_buf, align_buf, 1);
> + } else {
> + memcpy(align_buf, trailing_buf, 1);
> + writew(align_buf[0], mmio);
> }
> + words++;
> }
>
> udelay(100);
> *data->cs0_cfg |= 0x01;
> +
> + return words << 1;
> }
>
> static struct scsi_host_template ixp4xx_sht = {
> diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
> index 7bed8d8..f04e980 100644
> --- a/drivers/ata/pata_legacy.c
> +++ b/drivers/ata/pata_legacy.c
> @@ -249,13 +249,14 @@ static void pdc20230_set_piomode(struct ata_port *ap, struct ata_device *adev)
>
> }
>
> -static void pdc_data_xfer_vlb(struct ata_device *adev, unsigned char *buf, unsigned int buflen, int write_data)
> +static unsigned int pdc_data_xfer_vlb(struct ata_device *dev,
> + unsigned char *buf, unsigned int len, int rw)
> {
> - struct ata_port *ap = adev->link->ap;
> - int slop = buflen & 3;
> - unsigned long flags;
> -
> if (ata_id_has_dword_io(adev->id)) {
> + struct ata_port *ap = dev->link->ap;
> + int slop = len & 3;
> + unsigned long flags;
> +
> local_irq_save(flags);
>
> /* Perform the 32bit I/O synchronization sequence */
> @@ -264,28 +265,29 @@ static void pdc_data_xfer_vlb(struct ata_device *adev, unsigned char *buf, unsig
> ioread8(ap->ioaddr.nsect_addr);
>
> /* Now the data */
> -
> - if (write_data)
> - iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
> + if (rw == READ)
> + ioread32_rep(ap->ioaddr.data_addr, buf, len >> 2);
> else
> - ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
> + iowrite32_rep(ap->ioaddr.data_addr, buf, len >> 2);
>
> if (unlikely(slop)) {
> u32 pad;
> - if (write_data) {
> - memcpy(&pad, buf + buflen - slop, slop);
> - pad = le32_to_cpu(pad);
> - iowrite32(pad, ap->ioaddr.data_addr);
> - } else {
> + if (rw == READ) {
> pad = ioread32(ap->ioaddr.data_addr);
> pad = cpu_to_le16(pad);
> - memcpy(buf + buflen - slop, &pad, slop);
> + memcpy(buf + len - slop, &pad, slop);
> + } else {
> + memcpy(&pad, buf + len - slop, slop);
> + pad = le32_to_cpu(pad);
> + iowrite32(pad, ap->ioaddr.data_addr);
> }
> + len += 4 - slop;
> }
> local_irq_restore(flags);
> - }
> - else
> - ata_data_xfer_noirq(adev, buf, buflen, write_data);
> + } else
> + len = ata_data_xfer_noirq(dev, buf, len, rw);
> +
> + return len;
> }
>
> static struct ata_port_operations pdc20230_port_ops = {
> diff --git a/drivers/ata/pata_qdi.c b/drivers/ata/pata_qdi.c
> index 7d4c696..9e1802e 100644
> --- a/drivers/ata/pata_qdi.c
> +++ b/drivers/ata/pata_qdi.c
> @@ -124,31 +124,35 @@ static unsigned int qdi_qc_issue_prot(struct ata_queued_cmd *qc)
> return ata_qc_issue_prot(qc);
> }
>
> -static void qdi_data_xfer(struct ata_device *adev, unsigned char *buf, unsigned int buflen, int write_data)
> +static unsigned int qdi_data_xfer(struct ata_device *dev, unsigned char *buf,
> + unsigned int len, int rw)
> {
> - struct ata_port *ap = adev->link->ap;
> - int slop = buflen & 3;
> + if (ata_id_has_dword_io(dev->id)) {
> + struct ata_port *ap = dev->link->ap;
> + int slop = len & 3;
>
> - if (ata_id_has_dword_io(adev->id)) {
> - if (write_data)
> - iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
> + if (rw == READ)
> + ioread32_rep(ap->ioaddr.data_addr, buf, len >> 2);
> else
> - ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
> + iowrite32_rep(ap->ioaddr.data_addr, buf, len >> 2);
>
> if (unlikely(slop)) {
> u32 pad;
> - if (write_data) {
> - memcpy(&pad, buf + buflen - slop, slop);
> - pad = le32_to_cpu(pad);
> - iowrite32(pad, ap->ioaddr.data_addr);
> - } else {
> + if (rw == READ) {
> pad = ioread32(ap->ioaddr.data_addr);
> pad = cpu_to_le32(pad);
> - memcpy(buf + buflen - slop, &pad, slop);
> + memcpy(buf + len - slop, &pad, slop);
> + } else {
> + memcpy(&pad, buf + len - slop, slop);
> + pad = le32_to_cpu(pad);
> + iowrite32(pad, ap->ioaddr.data_addr);
> }
> + len += 4 - slop;
> }
> } else
> - ata_data_xfer(adev, buf, buflen, write_data);
> + len = ata_data_xfer(dev, buf, len, rw);
> +
> + return len;
> }
>
> static struct scsi_host_template qdi_sht = {
> diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
> index ea2ef9f..755e2b9 100644
> --- a/drivers/ata/pata_scc.c
> +++ b/drivers/ata/pata_scc.c
> @@ -768,45 +768,47 @@ static u8 scc_bmdma_status (struct ata_port *ap)
>
> /**
> * scc_data_xfer - Transfer data by PIO
> - * @adev: device for this I/O
> + * @dev: device for this I/O
> * @buf: data buffer
> - * @buflen: buffer length
> - * @write_data: read/write
> + * @len: buffer length
> + * @rw: read/write
> *
> * Note: Original code is ata_data_xfer().
> */
>
> -static void scc_data_xfer (struct ata_device *adev, unsigned char *buf,
> - unsigned int buflen, int write_data)
> +static unsigned int scc_data_xfer (struct ata_device *dev, unsigned char *buf,
> + unsigned int len, int rw)
> {
> - struct ata_port *ap = adev->link->ap;
> - unsigned int words = buflen >> 1;
> + struct ata_port *ap = dev->link->ap;
> + unsigned int words = len >> 1;
> unsigned int i;
> u16 *buf16 = (u16 *) buf;
> void __iomem *mmio = ap->ioaddr.data_addr;
>
> /* Transfer multiple of 2 bytes */
> - if (write_data) {
> - for (i = 0; i < words; i++)
> - out_be32(mmio, cpu_to_le16(buf16[i]));
> - } else {
> + if (rw == READ)
> for (i = 0; i < words; i++)
> buf16[i] = le16_to_cpu(in_be32(mmio));
> - }
> + else
> + for (i = 0; i < words; i++)
> + out_be32(mmio, cpu_to_le16(buf16[i]));
>
> /* Transfer trailing 1 byte, if any. */
> - if (unlikely(buflen & 0x01)) {
> + if (unlikely(len & 0x01)) {
> u16 align_buf[1] = { 0 };
> - unsigned char *trailing_buf = buf + buflen - 1;
> + unsigned char *trailing_buf = buf + len - 1;
>
> - if (write_data) {
> - memcpy(align_buf, trailing_buf, 1);
> - out_be32(mmio, cpu_to_le16(align_buf[0]));
> - } else {
> + if (rw == READ) {
> align_buf[0] = le16_to_cpu(in_be32(mmio));
> memcpy(trailing_buf, align_buf, 1);
> + } else {
> + memcpy(align_buf, trailing_buf, 1);
> + out_be32(mmio, cpu_to_le16(align_buf[0]));
> }
> + words++;
> }
> +
> + return words << 1;
> }
>
> /**
> diff --git a/drivers/ata/pata_winbond.c b/drivers/ata/pata_winbond.c
> index 311cdb3..9ded084 100644
> --- a/drivers/ata/pata_winbond.c
> +++ b/drivers/ata/pata_winbond.c
> @@ -92,31 +92,35 @@ static void winbond_set_piomode(struct ata_port *ap, struct ata_device *adev)
> }
>
>
> -static void winbond_data_xfer(struct ata_device *adev, unsigned char *buf, unsigned int buflen, int write_data)
> +static void winbond_data_xfer(struct ata_device *dev, unsigned char *buf,
> + unsigned int len, int rw)
> {
> - struct ata_port *ap = adev->link->ap;
> - int slop = buflen & 3;
> + struct ata_port *ap = dev->link->ap;
> + int slop = len & 3;
>
> - if (ata_id_has_dword_io(adev->id)) {
> - if (write_data)
> - iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
> + if (ata_id_has_dword_io(dev->id)) {
> + if (rw == READ)
> + ioread32_rep(ap->ioaddr.data_addr, buf, len >> 2);
> else
> - ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
> + iowrite32_rep(ap->ioaddr.data_addr, buf, len >> 2);
>
> if (unlikely(slop)) {
> u32 pad;
> - if (write_data) {
> - memcpy(&pad, buf + buflen - slop, slop);
> - pad = le32_to_cpu(pad);
> - iowrite32(pad, ap->ioaddr.data_addr);
> - } else {
> + if (rw == read) {
> pad = ioread32(ap->ioaddr.data_addr);
> pad = cpu_to_le16(pad);
> - memcpy(buf + buflen - slop, &pad, slop);
> + memcpy(buf + len - slop, &pad, slop);
> + } else {
> + memcpy(&pad, buf + len - slop, slop);
> + pad = le32_to_cpu(pad);
> + iowrite32(pad, ap->ioaddr.data_addr);
> }
> + len += 4 - slop;
> }
> } else
> - ata_data_xfer(adev, buf, buflen, write_data);
> + len = ata_data_xfer(dev, buf, len, rw);
> +
> + return len;
IMO s/buflen/len/ causes needless patch noise, and makes this harder review
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 08/14] libata: kill non-sg DMA interface
2007-11-29 14:33 ` [PATCH 08/14] libata: kill non-sg DMA interface Tejun Heo
2007-11-29 16:00 ` Alan Cox
@ 2007-12-04 19:31 ` Jeff Garzik
2007-12-05 1:02 ` Tejun Heo
1 sibling, 1 reply; 72+ messages in thread
From: Jeff Garzik @ 2007-12-04 19:31 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, alan, liml, albertl, jens.axboe, Rusty Russel
Tejun Heo wrote:
> With atapi_request_sense() converted to use sg, there's no user of
> non-sg interface. Kill non-sg interface.
>
> * ATA_QCFLAG_SINGLE and ATA_QCFLAG_SG are removed. ATA_QCFLAG_DMAMAP
> is used instead. (this way no LLD change is necessary)
>
> * qc->buf_virt is removed.
>
> * ata_sg_init_one() and ata_sg_setup_one() are removed.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Rusty Russel <rusty@rustcorp.com.au>
I would move the kill-non-sg-path changes to the front of the patchset
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 11/14] libata: add qc->dma_nbytes
2007-11-29 14:33 ` [PATCH 11/14] libata: add qc->dma_nbytes Tejun Heo
2007-11-29 16:02 ` Alan Cox
@ 2007-12-04 19:33 ` Jeff Garzik
2007-12-05 1:02 ` Tejun Heo
1 sibling, 1 reply; 72+ messages in thread
From: Jeff Garzik @ 2007-12-04 19:33 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, alan, liml, albertl, jens.axboe
Tejun Heo wrote:
> qc->nbytes doesn't include extra buffers setup by libata core layer
> and my be odd. This patch adds qc->dma_nbytes which includes any
> extra buffers setup by libata core layer and is guaranteed to be
> aligned on 4 byte boundary.
>
> This value is to be used to program the host controller. As this
> represents the actual length of buffer available to the controller and
> the controller must be able to deal with short transfers for ATAPI
> commands which can transfer variable length, this shouldn't break any
> controllers while making problems like rounding-down and controllers
> choking up on odd transfer bytes much less likely.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> drivers/ata/libata-core.c | 11 +++++++----
> drivers/ata/pata_pdc202xx_old.c | 2 +-
> drivers/ata/sata_inic162x.c | 2 +-
> drivers/ata/sata_qstor.c | 2 +-
> include/linux/libata.h | 3 ++-
> 5 files changed, 12 insertions(+), 8 deletions(-)
I would suggest two values:
qc->nbytes -> value to program host controllers with
qc->raw_nbytes -> the precise value, without any padding etc.
IMO this makes it more likely that people will use the "right" value
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-11-29 14:33 ` [PATCH 14/14] libata: use PIO for misc ATAPI commands Tejun Heo
2007-11-29 16:05 ` Alan Cox
@ 2007-12-04 19:34 ` Jeff Garzik
2007-12-05 1:14 ` Tejun Heo
1 sibling, 1 reply; 72+ messages in thread
From: Jeff Garzik @ 2007-12-04 19:34 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, alan, liml, albertl, jens.axboe
Tejun Heo wrote:
> ATAPI devices come with plethora of bugs and many ATA controllers have
> trouble dealing with odd byte DMA transfers. The problem is currently
> somewhat masked by not allowing DMA transfers if the transfer size
> isn't aligned to 16 bytes plus partial masking in problematic LLDs.
>
> This masking is taken verbatim from IDE and is far from perfection.
> There's no fundamental explanation why this should be sufficient and
> there are ATAPI devices which don't work with the IDE driver.
>
> In addition, this mixture of PIO and DMA commands reduces test
> coverage which is already insufficient considering the crazy number of
> ATAPI devices out in the wild.
>
> Also, these misc ATAPI commands usually transfer small amount of data
> and are used infrequently. There isn't much to be gained from using
> DMA. Combined with the fact that another OS which is probably the
> only one that most ATAPI device vendors test against uses PIO for
> these commands, it's much wiser to use PIO for these commands and
> concentrate debugging efforts on getting PIO right for misc commands
> and DMA for bulk transfer commands.
>
> Private command type / transfer length filtering in sata_promise,
> pata_it821x and pata_pdc2027x are removed as core layer filtering is
> more conservative.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
The other patches were OK, but I'm still not happy about "punishing the
good guys"
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size
2007-12-04 19:27 ` Jeff Garzik
@ 2007-12-05 0:47 ` Tejun Heo
0 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-12-05 0:47 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, alan, liml, albertl, jens.axboe
Jeff Garzik wrote:
> Tejun Heo wrote:
>> While updating lbam/h for ATAPI commands, atapi_eh_request_sense() was
>> left out. Update it.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>> drivers/ata/libata-eh.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index 77083b5..2e3d3a2 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -1302,8 +1302,8 @@ static unsigned int
>> atapi_eh_request_sense(struct ata_queued_cmd *qc)
>> tf.feature |= ATAPI_PKT_DMA;
>> } else {
>> tf.protocol = ATA_PROT_ATAPI;
>> - tf.lbam = (8 * 1024) & 0xff;
>> - tf.lbah = (8 * 1024) >> 8;
>> + tf.lbam = SCSI_SENSE_BUFFERSIZE;
>> + tf.lbah = 0;
>
> seems like #upstream-fixes material?
I'm not too sure yet whether we'll need to revert ATAPI transfer chunk
size change for #upstream-fixes or not. It fixes some cases while
breaking others. Dunno which side is larger but we definitely don't
wanna regress in a released kernel. I thought adding full chunk
draining would fix regressions but apparently not. Fortunately, with
full ATAPI data transfer improvement patch applied, it works. I'll
investigate more.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-12-04 19:30 ` Jeff Garzik
@ 2007-12-05 0:49 ` Tejun Heo
2007-12-05 1:05 ` Jeff Garzik
0 siblings, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-12-05 0:49 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, alan, liml, albertl, jens.axboe
Jeff Garzik wrote:
> IMO s/buflen/len/ causes needless patch noise, and makes this harder review
Separating out s/buflen/len/ to a separate patch seemed silly yet I
wanted to renamed it. :-P
If you want to kill it, I'll kill the renaming. If you want it in a
separate patch, I'll separate it out.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 06/14] libata: improve ATAPI draining
2007-12-04 10:06 ` Albert Lee
@ 2007-12-05 1:00 ` Tejun Heo
0 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-12-05 1:00 UTC (permalink / raw)
To: albertl; +Cc: jeff, linux-ide, alan, liml, jens.axboe
Albert Lee wrote:
> Maybe we should add an additional check to atapi_pio_bytes() such
> that "DRQ asserted with bytes = 0" is considered as AC_ERR_HSM?
> (patch attached below.) Otherwise the device might keep interruping us
> with DRQ asserted + zero byte count, and we are in infinite loop...
Yeah, the check will probably look best with ireason check. I'll add it.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 08/14] libata: kill non-sg DMA interface
2007-12-04 19:31 ` Jeff Garzik
@ 2007-12-05 1:02 ` Tejun Heo
0 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-12-05 1:02 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, alan, liml, albertl, jens.axboe, Rusty Russel
Jeff Garzik wrote:
> Tejun Heo wrote:
>> With atapi_request_sense() converted to use sg, there's no user of
>> non-sg interface. Kill non-sg interface.
>>
>> * ATA_QCFLAG_SINGLE and ATA_QCFLAG_SG are removed. ATA_QCFLAG_DMAMAP
>> is used instead. (this way no LLD change is necessary)
>>
>> * qc->buf_virt is removed.
>>
>> * ata_sg_init_one() and ata_sg_setup_one() are removed.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> Cc: Rusty Russel <rusty@rustcorp.com.au>
>
> I would move the kill-non-sg-path changes to the front of the patchset
I was trying to put SG DMA stuff together but well I'll move this one
forward.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 11/14] libata: add qc->dma_nbytes
2007-12-04 19:33 ` Jeff Garzik
@ 2007-12-05 1:02 ` Tejun Heo
0 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-12-05 1:02 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, alan, liml, albertl, jens.axboe
Jeff Garzik wrote:
> Tejun Heo wrote:
>> qc->nbytes doesn't include extra buffers setup by libata core layer
>> and my be odd. This patch adds qc->dma_nbytes which includes any
>> extra buffers setup by libata core layer and is guaranteed to be
>> aligned on 4 byte boundary.
>>
>> This value is to be used to program the host controller. As this
>> represents the actual length of buffer available to the controller and
>> the controller must be able to deal with short transfers for ATAPI
>> commands which can transfer variable length, this shouldn't break any
>> controllers while making problems like rounding-down and controllers
>> choking up on odd transfer bytes much less likely.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>> drivers/ata/libata-core.c | 11 +++++++----
>> drivers/ata/pata_pdc202xx_old.c | 2 +-
>> drivers/ata/sata_inic162x.c | 2 +-
>> drivers/ata/sata_qstor.c | 2 +-
>> include/linux/libata.h | 3 ++-
>> 5 files changed, 12 insertions(+), 8 deletions(-)
>
> I would suggest two values:
>
> qc->nbytes -> value to program host controllers with
> qc->raw_nbytes -> the precise value, without any padding etc.
>
> IMO this makes it more likely that people will use the "right" value
Yeap, agreed. Will change.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes
2007-12-05 0:49 ` Tejun Heo
@ 2007-12-05 1:05 ` Jeff Garzik
0 siblings, 0 replies; 72+ messages in thread
From: Jeff Garzik @ 2007-12-05 1:05 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, alan, liml, albertl, jens.axboe
Tejun Heo wrote:
> Jeff Garzik wrote:
>> IMO s/buflen/len/ causes needless patch noise, and makes this harder review
>
> Separating out s/buflen/len/ to a separate patch seemed silly yet I
> wanted to renamed it. :-P
>
> If you want to kill it, I'll kill the renaming. If you want it in a
> separate patch, I'll separate it out.
IMO just kill the renaming.
But if you really really really want to rename, put it in a separate
patch. :)
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-04 19:34 ` Jeff Garzik
@ 2007-12-05 1:14 ` Tejun Heo
2007-12-05 12:47 ` Alan Cox
0 siblings, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-12-05 1:14 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, alan, liml, albertl, jens.axboe
Jeff Garzik wrote:
> The other patches were OK, but I'm still not happy about "punishing the
> good guys"
Yeah, I know. I guess you know my but-what's-the-actual-punishment
argument too by now.
This is about time we make decision on this. If we're gonna go
DMA-for-everything way, we should lift 16 byte default masking for -mm
and -rc's as that only makes debugging difficult and try to blacklist
offending devices and controllers individually.
My primary concern is about not achieving enough test coverage and
isolated cases where it's difficult to tell whether it's the device or
the controller. e.g. Loading recent distro on an ancient and/or strange
machine, scratching head while cursing and giving up. Unfortunately,
I've seen similar things happening often enough (and I did so myself
more times than I'm proud of). Only very small fraction of problems
actually reaches developers.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-05 1:14 ` Tejun Heo
@ 2007-12-05 12:47 ` Alan Cox
2007-12-05 13:03 ` Tejun Heo
0 siblings, 1 reply; 72+ messages in thread
From: Alan Cox @ 2007-12-05 12:47 UTC (permalink / raw)
To: Tejun Heo; +Cc: Jeff Garzik, linux-ide, liml, albertl, jens.axboe
> This is about time we make decision on this. If we're gonna go
I thought everyone but you had made a decision ;)
> DMA-for-everything way, we should lift 16 byte default masking for -mm
> and -rc's as that only makes debugging difficult and try to blacklist
> offending devices and controllers individually.
Would be a useful experiment for -mm not really I think for -rc.
> My primary concern is about not achieving enough test coverage and
> isolated cases where it's difficult to tell whether it's the device or
> the controller. e.g. Loading recent distro on an ancient and/or strange
> machine, scratching head while cursing and giving up.
Watching the Fedora bugzilla I don't see things that match up to your
descriptions and fixes. Indeed I've now got verification that the weird
ALi ATAPI problem a few people have isn't even fixed by your extreme
changes.
Alan
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-05 12:47 ` Alan Cox
@ 2007-12-05 13:03 ` Tejun Heo
2007-12-05 14:01 ` Alan Cox
0 siblings, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-12-05 13:03 UTC (permalink / raw)
To: Alan Cox
Cc: Jeff Garzik, linux-ide, liml, albertl, jens.axboe, Mikael Pettersson
Alan Cox wrote:
>> This is about time we make decision on this. If we're gonna go
>
> I thought everyone but you had made a decision ;)
Hey, at least Mark was loosely on my side! 8-P
>> DMA-for-everything way, we should lift 16 byte default masking for -mm
>> and -rc's as that only makes debugging difficult and try to blacklist
>> offending devices and controllers individually.
>
> Would be a useful experiment for -mm not really I think for -rc.
Well, if we change that in #upstream now, -mm will receive it and after
2.6.24 is released, it will end up in -rc1. We can change it back if
the damage is too grave.
>> My primary concern is about not achieving enough test coverage and
>> isolated cases where it's difficult to tell whether it's the device or
>> the controller. e.g. Loading recent distro on an ancient and/or strange
>> machine, scratching head while cursing and giving up.
>
> Watching the Fedora bugzilla I don't see things that match up to your
> descriptions and fixes. Indeed I've now got verification that the weird
> ALi ATAPI problem a few people have isn't even fixed by your extreme
> changes.
Yeah, ALi seems to be genuine driver problem. I don't think using PIO
for misc ATAPI commands is extreme considering Windows is doing it.
Another thing is check_atapi_dma in sata_promise. It mentions losing
interrupt which is exactly what happens if the drive tries to transfer
more data but DMA buffer is short on several controllers. I wonder
whether this is unnecessary with DMA draining added.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-05 13:03 ` Tejun Heo
@ 2007-12-05 14:01 ` Alan Cox
2007-12-05 14:22 ` Tejun Heo
0 siblings, 1 reply; 72+ messages in thread
From: Alan Cox @ 2007-12-05 14:01 UTC (permalink / raw)
To: Tejun Heo
Cc: Jeff Garzik, linux-ide, liml, albertl, jens.axboe, Mikael Pettersson
> Well, if we change that in #upstream now, -mm will receive it and after
> 2.6.24 is released, it will end up in -rc1. We can change it back if
> the damage is too grave.
Getting it in -rc is going to mess up other testing. It's perfect for -mm
testing not -rc testing.
> Yeah, ALi seems to be genuine driver problem. I don't think using PIO
> for misc ATAPI commands is extreme considering Windows is doing it.
Windows does a lot of things that are bad and ugly workarounds for messes
in the past. We have the ability to do a lot better than that.
> Another thing is check_atapi_dma in sata_promise. It mentions losing
> interrupt which is exactly what happens if the drive tries to transfer
> more data but DMA buffer is short on several controllers. I wonder
> whether this is unnecessary with DMA draining added.
Is it like the inic where you must do transfers in specific chunk sizes ?
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-05 14:01 ` Alan Cox
@ 2007-12-05 14:22 ` Tejun Heo
2007-12-05 14:46 ` Alan Cox
0 siblings, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-12-05 14:22 UTC (permalink / raw)
To: Alan Cox
Cc: Jeff Garzik, linux-ide, liml, albertl, jens.axboe, Mikael Pettersson
Alan Cox wrote:
>> Well, if we change that in #upstream now, -mm will receive it and after
>> 2.6.24 is released, it will end up in -rc1. We can change it back if
>> the damage is too grave.
>
> Getting it in -rc is going to mess up other testing. It's perfect for -mm
> testing not -rc testing.
It eventually has to end up in -rc. If not for 2.6.25-rc1 is too early,
we can put it in #testing and put it into #upstream later.
>> Yeah, ALi seems to be genuine driver problem. I don't think using PIO
>> for misc ATAPI commands is extreme considering Windows is doing it.
>
> Windows does a lot of things that are bad and ugly workarounds for messes
> in the past. We have the ability to do a lot better than that.
I agree. It's double edged sword tho. We're not gonna get much test
coverage over those messes of the past and as I said, those are what I'm
primarily worried about. Command type dependent quick fallback might
help but ancient controllers are more likely to bring the whole machine
down when a DMA transaction goes south.
>> Another thing is check_atapi_dma in sata_promise. It mentions losing
>> interrupt which is exactly what happens if the drive tries to transfer
>> more data but DMA buffer is short on several controllers. I wonder
>> whether this is unnecessary with DMA draining added.
>
> Is it like the inic where you must do transfers in specific chunk sizes ?
I don't think so. It does READ_CD and READ_DVD_STRUCTURE via DMA. The
first one isn't 2k aligned the second is of variable size. I think the
driver was just working around buggy userland which issues commands like
mode sense with longer allocation size than actual buffer size.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-05 14:22 ` Tejun Heo
@ 2007-12-05 14:46 ` Alan Cox
2007-12-05 15:13 ` Tejun Heo
2007-12-06 11:03 ` Petr Vandrovec
0 siblings, 2 replies; 72+ messages in thread
From: Alan Cox @ 2007-12-05 14:46 UTC (permalink / raw)
To: Tejun Heo
Cc: Jeff Garzik, linux-ide, liml, albertl, jens.axboe, Mikael Pettersson
> It eventually has to end up in -rc. If not for 2.6.25-rc1 is too early,
> we can put it in #testing and put it into #upstream later.
Nobody cares about libata git trees. If you want some initial test
coverage put it in -mm.
> primarily worried about. Command type dependent quick fallback might
> help but ancient controllers are more likely to bring the whole machine
> down when a DMA transaction goes south.
Quite the reverse in my experience - the dumber the controller the more
likely that ATAPI DMA and LBA48 and other stuff just works anyway.
Alan
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-05 14:46 ` Alan Cox
@ 2007-12-05 15:13 ` Tejun Heo
2007-12-05 18:52 ` Jeff Garzik
2007-12-06 11:03 ` Petr Vandrovec
1 sibling, 1 reply; 72+ messages in thread
From: Tejun Heo @ 2007-12-05 15:13 UTC (permalink / raw)
To: Alan Cox
Cc: Jeff Garzik, linux-ide, liml, albertl, jens.axboe, Mikael Pettersson
Alan Cox wrote:
>> It eventually has to end up in -rc. If not for 2.6.25-rc1 is too early,
>> we can put it in #testing and put it into #upstream later.
>
> Nobody cares about libata git trees. If you want some initial test
> coverage put it in -mm.
#for-testing ends up in #ALL which Andrew always includes in -mm.
#upstream also ends up in #ALL but is different in that it will get into
mainline when the next -rc1 opens. Jeff, right?
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-05 15:13 ` Tejun Heo
@ 2007-12-05 18:52 ` Jeff Garzik
0 siblings, 0 replies; 72+ messages in thread
From: Jeff Garzik @ 2007-12-05 18:52 UTC (permalink / raw)
To: Tejun Heo
Cc: Alan Cox, linux-ide, liml, albertl, jens.axboe, Mikael Pettersson
On Thu, Dec 06, 2007 at 12:13:18AM +0900, Tejun Heo wrote:
> Alan Cox wrote:
> >> It eventually has to end up in -rc. If not for 2.6.25-rc1 is too early,
> >> we can put it in #testing and put it into #upstream later.
> >
> > Nobody cares about libata git trees. If you want some initial test
> > coverage put it in -mm.
99% incorrect.
> #for-testing ends up in #ALL which Andrew always includes in -mm.
> #upstream also ends up in #ALL but is different in that it will get into
> mainline when the next -rc1 opens. Jeff, right?
100% correct.
Jeff
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-05 14:46 ` Alan Cox
2007-12-05 15:13 ` Tejun Heo
@ 2007-12-06 11:03 ` Petr Vandrovec
2007-12-06 11:10 ` Tejun Heo
1 sibling, 1 reply; 72+ messages in thread
From: Petr Vandrovec @ 2007-12-06 11:03 UTC (permalink / raw)
To: Tejun Heo
Cc: Alan Cox, Jeff Garzik, linux-ide, liml, albertl, jens.axboe,
Mikael Pettersson
Alan Cox wrote:
>> It eventually has to end up in -rc. If not for 2.6.25-rc1 is too early,
>> we can put it in #testing and put it into #upstream later.
>
> Nobody cares about libata git trees. If you want some initial test
> coverage put it in -mm.
>
>> primarily worried about. Command type dependent quick fallback might
>> help but ancient controllers are more likely to bring the whole machine
>> down when a DMA transaction goes south.
>
> Quite the reverse in my experience - the dumber the controller the more
> likely that ATAPI DMA and LBA48 and other stuff just works anyway.
Yes. FYI, if you'll start sending ATAPI commands with DATA_OUT phase
using PIO from VM under VMware, it will politely ask you to reconfigure
OS in the virtual machine to use DMA, and most probably it won't work
until you really do so... Windows are sending all these commands using
DMA, and I believe they do same for majority of DATA_IN commands as well
(and Windows also set byte count to correct PIO-like value even for DMA
commands)
Given that very few customers reported this problem in past 8 years, I
would guess that your attempt to use PIO only will actually exercise
more untested code in the firmware than DMA code paths.
Petr
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH 14/14] libata: use PIO for misc ATAPI commands
2007-12-06 11:03 ` Petr Vandrovec
@ 2007-12-06 11:10 ` Tejun Heo
0 siblings, 0 replies; 72+ messages in thread
From: Tejun Heo @ 2007-12-06 11:10 UTC (permalink / raw)
To: Petr Vandrovec
Cc: Alan Cox, Jeff Garzik, linux-ide, liml, albertl, jens.axboe,
Mikael Pettersson
Petr Vandrovec wrote:
> Alan Cox wrote:
>>> It eventually has to end up in -rc. If not for 2.6.25-rc1 is too early,
>>> we can put it in #testing and put it into #upstream later.
>>
>> Nobody cares about libata git trees. If you want some initial test
>> coverage put it in -mm.
>>
>>> primarily worried about. Command type dependent quick fallback might
>>> help but ancient controllers are more likely to bring the whole machine
>>> down when a DMA transaction goes south.
>>
>> Quite the reverse in my experience - the dumber the controller the more
>> likely that ATAPI DMA and LBA48 and other stuff just works anyway.
>
> Yes. FYI, if you'll start sending ATAPI commands with DATA_OUT phase
> using PIO from VM under VMware, it will politely ask you to reconfigure
> OS in the virtual machine to use DMA, and most probably it won't work
> until you really do so... Windows are sending all these commands using
> DMA, and I believe they do same for majority of DATA_IN commands as well
> (and Windows also set byte count to correct PIO-like value even for DMA
> commands)
There are few DATA OUT commands and most of them are sector (2k)
aligned. We use DMA for them. The same goes for sector aligned DATA IN
commands and READ CD but what you say is inconsistent with what I've
seen from SATA bus trace. Windows was using PIO for all misc DATA IN
commands - such as REQUEST SENSE, GET CONFIGURATION, MODE SENSE, etc...
And, libata will set byte count to PIO-like value for DMA from 2.6.25 if
not from 24.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 72+ messages in thread
end of thread, other threads:[~2007-12-06 11:27 UTC | newest]
Thread overview: 72+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-11-29 14:33 [PATCHSET] libata: improve ATAPI data transfer handling, take #2 Tejun Heo
2007-11-29 14:33 ` [PATCH 01/14] libata: update atapi_eh_request_sense() such that lbam/lbah contains buffer size Tejun Heo
2007-11-29 15:51 ` Alan Cox
2007-12-04 19:27 ` Jeff Garzik
2007-12-05 0:47 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 02/14] cdrom: add more GPCMD_* constants Tejun Heo
2007-11-29 14:33 ` [PATCH 03/14] libata: rename ATA_PROT_ATAPI_* to ATAPI_PROT_* Tejun Heo
2007-11-29 15:52 ` Alan Cox
2007-11-29 14:33 ` [PATCH 04/14] libata: add ATAPI_* cmd types and implement atapi_cmd_type() Tejun Heo
2007-11-29 14:33 ` [PATCH 05/14] libata: make ->data_xfer return the number of consumed bytes Tejun Heo
2007-11-29 15:55 ` Alan Cox
2007-11-29 16:06 ` Tejun Heo
2007-11-29 17:42 ` Alan Cox
2007-11-29 18:00 ` Tejun Heo
2007-11-29 18:24 ` Alan Cox
2007-11-29 18:57 ` Mark Lord
2007-11-29 19:37 ` Jeff Garzik
2007-11-29 19:36 ` Jeff Garzik
2007-11-29 21:22 ` Alan Cox
2007-12-04 19:30 ` Jeff Garzik
2007-12-05 0:49 ` Tejun Heo
2007-12-05 1:05 ` Jeff Garzik
2007-11-29 14:33 ` [PATCH 06/14] libata: improve ATAPI draining Tejun Heo
2007-11-29 15:59 ` Alan Cox
2007-11-29 16:09 ` Tejun Heo
2007-11-29 16:42 ` Tejun Heo
2007-11-29 17:40 ` Alan Cox
2007-11-29 18:11 ` Tejun Heo
2007-11-29 18:19 ` Alan Cox
2007-12-04 10:06 ` Albert Lee
2007-12-05 1:00 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 07/14] libata: make atapi_request_sense() use sg Tejun Heo
2007-11-29 14:33 ` [PATCH 08/14] libata: kill non-sg DMA interface Tejun Heo
2007-11-29 16:00 ` Alan Cox
2007-12-04 19:31 ` Jeff Garzik
2007-12-05 1:02 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 09/14] libata: change ATA_QCFLAG_DMAMAP semantics Tejun Heo
2007-11-29 14:33 ` [PATCH 10/14] libata: convert to chained sg Tejun Heo
2007-11-29 14:33 ` [PATCH 11/14] libata: add qc->dma_nbytes Tejun Heo
2007-11-29 16:02 ` Alan Cox
2007-12-04 19:33 ` Jeff Garzik
2007-12-05 1:02 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 12/14] libata: implement ATAPI drain buffer Tejun Heo
2007-11-29 14:33 ` [PATCH 13/14] libata: implement ATAPI per-command-type DMA horkages Tejun Heo
2007-11-29 16:04 ` Alan Cox
2007-11-29 16:10 ` Tejun Heo
2007-11-29 14:33 ` [PATCH 14/14] libata: use PIO for misc ATAPI commands Tejun Heo
2007-11-29 16:05 ` Alan Cox
2007-11-29 16:14 ` Tejun Heo
2007-11-29 16:16 ` Tejun Heo
2007-11-29 18:55 ` Mark Lord
2007-11-29 22:52 ` Tejun Heo
2007-11-30 1:03 ` Mark Lord
2007-11-30 1:34 ` Tejun Heo
2007-11-30 4:19 ` Mark Lord
2007-11-30 13:40 ` Alan Cox
2007-11-30 13:59 ` Tejun Heo
2007-11-30 14:12 ` Jeff Garzik
2007-11-30 15:36 ` Alan Cox
2007-11-30 13:09 ` Alan Cox
2007-11-29 17:38 ` Alan Cox
2007-12-04 19:34 ` Jeff Garzik
2007-12-05 1:14 ` Tejun Heo
2007-12-05 12:47 ` Alan Cox
2007-12-05 13:03 ` Tejun Heo
2007-12-05 14:01 ` Alan Cox
2007-12-05 14:22 ` Tejun Heo
2007-12-05 14:46 ` Alan Cox
2007-12-05 15:13 ` Tejun Heo
2007-12-05 18:52 ` Jeff Garzik
2007-12-06 11:03 ` Petr Vandrovec
2007-12-06 11:10 ` Tejun Heo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.