* [PATCH] libata ATAPI transfer size cleanups
@ 2007-11-01 9:07 Jeff Garzik
2007-11-01 9:43 ` Alan Cox
2007-11-01 19:51 ` Torsten Kaiser
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Garzik @ 2007-11-01 9:07 UTC (permalink / raw)
To: linux-ide; +Cc: LKML
This is purely for comment and testing, not for merging (yet?).
A common recipe in several vendor drivers (either GPL'd, or I have NDA'd
access to use them as a documentation-like reference) for ATAPI was
slightly different from ours. This recipe can be found in
atapi_tf_xfer_size(), and it's slightly different from Alan's.
This patch also adds some byte count checks, consolidated
ata_pio_use_silly() use, and adds a dmadir-related FIXME.
The 'xfer-size' branch of
master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/libata-dev.git xfer-size
to receive the following updates:
drivers/ata/libata-core.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
drivers/ata/libata-eh.c | 11 +--------
drivers/ata/libata-scsi.c | 48 +++++++++-------------------------------
drivers/ata/libata.h | 7 ++++++
4 files changed, 72 insertions(+), 47 deletions(-)
Jeff Garzik (1):
[libata] Standardize ATAPI byte count [limit] handling
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 63035d7..5c616a8 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5258,6 +5258,55 @@ next_sg:
goto next_sg;
}
+void atapi_tf_xfer_size(struct ata_taskfile *tf,
+ unsigned int total_xfer_len,
+ bool dma)
+{
+ if (dma) {
+ tf->protocol = ATA_PROT_ATAPI_DMA;
+ tf->feature |= ATAPI_PKT_DMA;
+ tf->lbam = 0;
+ tf->lbah = 0;
+ } else {
+ tf->protocol = ATA_PROT_ATAPI;
+ tf->feature &= ~ATAPI_PKT_DMA;
+ if (total_xfer_len <= 0xffff) {
+ tf->lbam = total_xfer_len;
+ tf->lbah = total_xfer_len >> 8;
+ } else {
+ tf->lbam = 0xff;
+ tf->lbah = 0xff;
+ }
+ }
+}
+
+static bool atapi_valid_bcount(const struct ata_taskfile *tf,
+ const struct ata_taskfile *result_tf)
+{
+ unsigned int ibyte, obyte, lo, hi;
+
+ lo = tf->lbam;
+ hi = tf->lbam;
+ ibyte = (hi << 8) | lo;
+
+ lo = result_tf->lbam;
+ hi = result_tf->lbam;
+ obyte = (hi << 8) | lo;
+
+ if (unlikely(obyte > ibyte))
+ return false;
+ if (unlikely(obyte == 0))
+ return false;
+ if (unlikely(obyte > 0xfffe))
+ return false;
+
+ /* another test, omitted due to lack of data point:
+ * obyte should be even, if it is not the last DRQ block
+ */
+
+ return true;
+}
+
/**
* atapi_pio_bytes - Transfer data from/to the ATAPI device.
* @qc: Command on going
@@ -5282,6 +5331,10 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
* So, the correctness of qc->result_tf is not affected.
*/
ap->ops->tf_read(ap, &qc->result_tf);
+
+ if (!atapi_valid_bcount(&qc->tf, &qc->result_tf))
+ goto err_out;
+
ireason = qc->result_tf.nsect;
bc_lo = qc->result_tf.lbam;
bc_hi = qc->result_tf.lbah;
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 8d64f8f..505b2e9 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1352,16 +1352,7 @@ 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 = ATA_PROT_ATAPI_DMA;
- tf.feature |= ATAPI_PKT_DMA;
- } else {
- tf.protocol = ATA_PROT_ATAPI;
- tf.lbam = (8 * 1024) & 0xff;
- tf.lbah = (8 * 1024) >> 8;
- }
+ atapi_tf_xfer_size(&tf, SCSI_SENSE_BUFFERSIZE, ata_pio_use_silly(ap));
return ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,
sense_buf, SCSI_SENSE_BUFFERSIZE, 0);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index fc89590..bec28c2 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2313,12 +2313,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;
@@ -2346,15 +2340,9 @@ 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;
+ atapi_tf_xfer_size(&qc->tf, SCSI_SENSE_BUFFERSIZE,
+ ata_pio_use_silly(ap));
- if (ata_pio_use_silly(ap)) {
- qc->tf.protocol = ATA_PROT_ATAPI_DMA;
- qc->tf.feature |= ATAPI_PKT_DMA;
- } else {
- qc->tf.protocol = ATA_PROT_ATAPI;
- qc->tf.lbam = SCSI_SENSE_BUFFERSIZE;
- qc->tf.lbah = 0;
- }
qc->nbytes = SCSI_SENSE_BUFFERSIZE;
qc->complete_fn = atapi_sense_complete;
@@ -2462,7 +2450,6 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
struct ata_device *dev = qc->dev;
int using_pio = (dev->flags & ATA_DFLAG_PIO);
int nodata = (scmd->sc_data_direction == DMA_NONE);
- unsigned int nbytes;
memset(qc->cdb, 0, dev->cdb_len);
memcpy(qc->cdb, scmd->cmnd, scmd->cmd_len);
@@ -2482,31 +2469,18 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
if (!using_pio && ata_check_atapi_dma(qc))
using_pio = 1;
- /* Some controller variants snoop this value for Packet transfers
- to do state machine and FIFO management. Thus we want to set it
- properly, and for DMA where it is effectively meaningless */
- nbytes = min(qc->nbytes, (unsigned int)63 * 1024);
-
- qc->tf.lbam = (nbytes & 0xFF);
- qc->tf.lbah = (nbytes >> 8);
-
- if (using_pio || nodata) {
- /* no data, or PIO data xfer */
- if (nodata)
- qc->tf.protocol = ATA_PROT_ATAPI_NODATA;
- else
- qc->tf.protocol = ATA_PROT_ATAPI;
- } else {
- /* DMA data xfer */
- qc->tf.protocol = ATA_PROT_ATAPI_DMA;
- qc->tf.feature |= ATAPI_PKT_DMA;
+ if (nodata)
+ qc->tf.protocol = ATA_PROT_ATAPI_NODATA;
+ else
+ atapi_tf_xfer_size(&qc->tf, qc->nbytes, !using_pio);
- if (atapi_dmadir && (scmd->sc_data_direction != DMA_TO_DEVICE))
- /* some SATA bridges need us to indicate data xfer direction */
- qc->tf.feature |= ATAPI_DMADIR;
+ /* FIXME: also check dmadir capability bit added in ATA-7 */
+ if (atapi_dmadir && (qc->tf.protocol == ATA_PROT_ATAPI_DMA) &&
+ (scmd->sc_data_direction != DMA_TO_DEVICE)) {
+ /* some SATA bridges need us to indicate data xfer direction */
+ qc->tf.feature |= ATAPI_DMADIR;
}
-
/* FIXME: We need to translate 0x05 READ_BLOCK_LIMITS to a MODE_SENSE
as ATAPI tape drives don't get this right otherwise */
return 0;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 0e6cf3a..f62029a 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -103,6 +103,8 @@ extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
extern struct ata_port *ata_port_alloc(struct ata_host *host);
extern void ata_dev_enable_pm(struct ata_device *dev, enum link_pm policy);
extern void ata_lpm_schedule(struct ata_port *ap, enum link_pm);
+extern void atapi_tf_xfer_size(struct ata_taskfile *tf,
+ unsigned int total_xfer_len, bool dma);
/* libata-acpi.c */
#ifdef CONFIG_ATA_ACPI
@@ -188,5 +190,10 @@ extern void ata_eh_finish(struct ata_port *ap);
/* libata-sff.c */
extern u8 ata_irq_on(struct ata_port *ap);
+/* 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);
+}
#endif /* __LIBATA_H__ */
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] libata ATAPI transfer size cleanups
2007-11-01 9:07 [PATCH] libata ATAPI transfer size cleanups Jeff Garzik
@ 2007-11-01 9:43 ` Alan Cox
2007-11-01 9:54 ` Jeff Garzik
2007-11-01 19:51 ` Torsten Kaiser
1 sibling, 1 reply; 6+ messages in thread
From: Alan Cox @ 2007-11-01 9:43 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, LKML
On Thu, 1 Nov 2007 05:07:33 -0400
Jeff Garzik <jeff@garzik.org> wrote:
>
> This is purely for comment and testing, not for merging (yet?).
>
> A common recipe in several vendor drivers (either GPL'd, or I have NDA'd
> access to use them as a documentation-like reference) for ATAPI was
> slightly different from ours. This recipe can be found in
> atapi_tf_xfer_size(), and it's slightly different from Alan's.
Looks mostly good. Might cause breakage on one or two controllers by
setting lbam/lbah to 0 for DMA but we can test that and find out.
Note however - the scheme in use now has been tested for over ten years
in drivers/ide. The scheme below has not...
Having an ata_tf_xfer_size method is definitely a good idea whatever
methods we eventually decide on.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libata ATAPI transfer size cleanups
2007-11-01 9:43 ` Alan Cox
@ 2007-11-01 9:54 ` Jeff Garzik
2007-11-01 10:59 ` Alan Cox
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Garzik @ 2007-11-01 9:54 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-ide, LKML
Alan Cox wrote:
> On Thu, 1 Nov 2007 05:07:33 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
>
>> This is purely for comment and testing, not for merging (yet?).
>>
>> A common recipe in several vendor drivers (either GPL'd, or I have NDA'd
>> access to use them as a documentation-like reference) for ATAPI was
>> slightly different from ours. This recipe can be found in
>> atapi_tf_xfer_size(), and it's slightly different from Alan's.
>
> Looks mostly good. Might cause breakage on one or two controllers by
> setting lbam/lbah to 0 for DMA but we can test that and find out.
DMA always zeroed lbam/lbah thanks to memset(), my patch merely made it
explicit.
> Note however - the scheme in use now has been tested for over ten years
> in drivers/ide. The scheme below has not...
See my other email just posted, differences and errata abound :)
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libata ATAPI transfer size cleanups
2007-11-01 9:54 ` Jeff Garzik
@ 2007-11-01 10:59 ` Alan Cox
0 siblings, 0 replies; 6+ messages in thread
From: Alan Cox @ 2007-11-01 10:59 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, LKML
On Thu, 01 Nov 2007 05:54:06 -0400
Jeff Garzik <jeff@garzik.org> wrote:
> Alan Cox wrote:
> > On Thu, 1 Nov 2007 05:07:33 -0400
> > Jeff Garzik <jeff@garzik.org> wrote:
> >
> >> This is purely for comment and testing, not for merging (yet?).
> >>
> >> A common recipe in several vendor drivers (either GPL'd, or I have NDA'd
> >> access to use them as a documentation-like reference) for ATAPI was
> >> slightly different from ours. This recipe can be found in
> >> atapi_tf_xfer_size(), and it's slightly different from Alan's.
> >
> > Looks mostly good. Might cause breakage on one or two controllers by
> > setting lbam/lbah to 0 for DMA but we can test that and find out.
>
> DMA always zeroed lbam/lbah thanks to memset(), my patch merely made it
> explicit.
That would be a bug if so - need to try fixing that and see if it sorts
out the remaining recalcitrant ALi chips.
Alan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libata ATAPI transfer size cleanups
2007-11-01 9:07 [PATCH] libata ATAPI transfer size cleanups Jeff Garzik
2007-11-01 9:43 ` Alan Cox
@ 2007-11-01 19:51 ` Torsten Kaiser
2007-11-02 5:16 ` Jeff Garzik
1 sibling, 1 reply; 6+ messages in thread
From: Torsten Kaiser @ 2007-11-01 19:51 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-ide, LKML
On 11/1/07, Jeff Garzik <jeff@garzik.org> wrote:
> + lo = tf->lbam;
> + hi = tf->lbam;
> + ibyte = (hi << 8) | lo;
> +
> + lo = result_tf->lbam;
> + hi = result_tf->lbam;
That doesn't look right.
I suspect this was intended:
lo = tf->lbam;
hi = tf->lbah;
Torsten
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libata ATAPI transfer size cleanups
2007-11-01 19:51 ` Torsten Kaiser
@ 2007-11-02 5:16 ` Jeff Garzik
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Garzik @ 2007-11-02 5:16 UTC (permalink / raw)
To: Torsten Kaiser; +Cc: linux-ide, LKML
Torsten Kaiser wrote:
> On 11/1/07, Jeff Garzik <jeff@garzik.org> wrote:
>> + lo = tf->lbam;
>> + hi = tf->lbam;
>> + ibyte = (hi << 8) | lo;
>> +
>> + lo = result_tf->lbam;
>> + hi = result_tf->lbam;
>
> That doesn't look right.
> I suspect this was intended:
>
> lo = tf->lbam;
> hi = tf->lbah;
Agreed, will correct.
Thanks,
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-11-02 5:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-11-01 9:07 [PATCH] libata ATAPI transfer size cleanups Jeff Garzik
2007-11-01 9:43 ` Alan Cox
2007-11-01 9:54 ` Jeff Garzik
2007-11-01 10:59 ` Alan Cox
2007-11-01 19:51 ` Torsten Kaiser
2007-11-02 5:16 ` Jeff Garzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).