[PATCH #upstream-fixes] libata: use atapi_cmd_type() to determine cmd type instead of transfer size

[PATCH #upstream-fixes] libata: use atapi_cmd_type() to determine cmd type instead of transfer size

[Tejun Heo]

pata_ali and pata_it821x were using qc->nbytes to determine whether a
command is data transfer type or not.  As now qc->nbytes can be
extended by padding and draining buffers, this tests are not useful
anymore.  Use atapi_cmd_type() instead.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/ata/pata_ali.c    |    2 +-
 drivers/ata/pata_it821x.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
index 7e68edf..8786455 100644
--- a/drivers/ata/pata_ali.c
+++ b/drivers/ata/pata_ali.c
@@ -295,7 +295,7 @@ static void ali_lock_sectors(struct ata_device *adev)
 static int ali_check_atapi_dma(struct ata_queued_cmd *qc)
 {
 	/* If its not a media command, its not worth it */
-	if (qc->nbytes < 2048)
+	if (atapi_cmd_type(qc->cdb[0]) == ATAPI_MISC)
 		return -EOPNOTSUPP;
 	return 0;
 }
diff --git a/drivers/ata/pata_it821x.c b/drivers/ata/pata_it821x.c
index 109ddd4..f751749 100644
--- a/drivers/ata/pata_it821x.c
+++ b/drivers/ata/pata_it821x.c
@@ -564,7 +564,7 @@ static int it821x_check_atapi_dma(struct ata_queued_cmd *qc)
 	struct it821x_dev *itdev = ap->private_data;
 
 	/* Only use dma for transfers to/from the media. */
-	if (qc->nbytes < 2048)
+	if (atapi_cmd_type(qc->cdb[0]) == ATAPI_MISC)
 		return -EOPNOTSUPP;
 
 	/* No ATAPI DMA in smart mode */

Re: [PATCH #upstream-fixes] libata: use atapi_cmd_type() to determine cmd type instead of transfer size

[Rafael J. Wysocki]

On Tuesday, 11 of March 2008, Tejun Heo wrote:
> pata_ali and pata_it821x were using qc->nbytes to determine whether a
> command is data transfer type or not.  As now qc->nbytes can be
> extended by padding and draining buffers, this tests are not useful
> anymore.  Use atapi_cmd_type() instead.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>

Tested-by: Rafael J. Wysocki <rjw@sisk.pl>

> ---
>  drivers/ata/pata_ali.c    |    2 +-
>  drivers/ata/pata_it821x.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
> index 7e68edf..8786455 100644
> --- a/drivers/ata/pata_ali.c
> +++ b/drivers/ata/pata_ali.c
> @@ -295,7 +295,7 @@ static void ali_lock_sectors(struct ata_device *adev)
>  static int ali_check_atapi_dma(struct ata_queued_cmd *qc)
>  {
>  	/* If its not a media command, its not worth it */
> -	if (qc->nbytes < 2048)
> +	if (atapi_cmd_type(qc->cdb[0]) == ATAPI_MISC)
>  		return -EOPNOTSUPP;
>  	return 0;
>  }
> diff --git a/drivers/ata/pata_it821x.c b/drivers/ata/pata_it821x.c
> index 109ddd4..f751749 100644
> --- a/drivers/ata/pata_it821x.c
> +++ b/drivers/ata/pata_it821x.c
> @@ -564,7 +564,7 @@ static int it821x_check_atapi_dma(struct ata_queued_cmd *qc)
>  	struct it821x_dev *itdev = ap->private_data;
>  
>  	/* Only use dma for transfers to/from the media. */
> -	if (qc->nbytes < 2048)
> +	if (atapi_cmd_type(qc->cdb[0]) == ATAPI_MISC)
>  		return -EOPNOTSUPP;
>  
>  	/* No ATAPI DMA in smart mode */
> 
> 

Re: [PATCH #upstream-fixes] libata: use atapi_cmd_type() to determine cmd type instead of transfer size

[Alan Cox]

On Tue, 11 Mar 2008 11:35:00 +0900
Tejun Heo <htejun@gmail.com> wrote:

> pata_ali and pata_it821x were using qc->nbytes to determine whether a
> command is data transfer type or not.  As now qc->nbytes can be
> extended by padding and draining buffers, this tests are not useful
> anymore.  Use atapi_cmd_type() instead.

NAK

The it821x case really seems to be about bytes transferred not command
type. ALi one looks fine

Re: [PATCH #upstream-fixes] libata: use atapi_cmd_type() to determine cmd type instead of transfer size

[Jeff Garzik]

Tejun Heo wrote:
> pata_ali and pata_it821x were using qc->nbytes to determine whether a
> command is data transfer type or not.  As now qc->nbytes can be
> extended by padding and draining buffers, this tests are not useful
> anymore.  Use atapi_cmd_type() instead.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/ata/pata_ali.c    |    2 +-
>  drivers/ata/pata_it821x.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
> index 7e68edf..8786455 100644
> --- a/drivers/ata/pata_ali.c
> +++ b/drivers/ata/pata_ali.c
> @@ -295,7 +295,7 @@ static void ali_lock_sectors(struct ata_device *adev)
>  static int ali_check_atapi_dma(struct ata_queued_cmd *qc)
>  {
>  	/* If its not a media command, its not worth it */
> -	if (qc->nbytes < 2048)
> +	if (atapi_cmd_type(qc->cdb[0]) == ATAPI_MISC)
>  		return -EOPNOTSUPP;
>  	return 0;
>  }

applied the pata_ali portion of this patch

[PATCH #upstream-fixes 1/2] libata: implement ata_qc_raw_nbytes()

[Tejun Heo]

Implement ata_qc_raw_nbytes() which determines the raw user-requested
size of a PC command.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-scsi.c |   14 +++++++++++---
 include/linux/libata.h    |    8 +++++++-
 2 files changed, 18 insertions(+), 4 deletions(-)

Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -527,6 +527,14 @@ static struct ata_queued_cmd *ata_scsi_q
 	return qc;
 }
 
+static void ata_qc_set_pc_nbytes(struct ata_queued_cmd *qc)
+{
+	struct scsi_cmnd *scmd = qc->scsicmd;
+
+	qc->extrabytes = scmd->request->extra_len;
+	qc->nbytes = scsi_bufflen(scmd) + qc->extrabytes;
+}
+
 /**
  *	ata_dump_status - user friendly display of error info
  *	@id: id of the port in question
@@ -2539,7 +2547,7 @@ static unsigned int atapi_xlat(struct at
 	}
 
 	qc->tf.command = ATA_CMD_PACKET;
-	qc->nbytes = scsi_bufflen(scmd) + scmd->request->extra_len;
+	ata_qc_set_pc_nbytes(qc);
 
 	/* check whether ATAPI DMA is safe */
 	if (!using_pio && ata_check_atapi_dma(qc))
@@ -2550,7 +2558,7 @@ static unsigned int atapi_xlat(struct at
 	 * want to set it properly, and for DMA where it is
 	 * effectively meaningless.
 	 */
-	nbytes = min(scmd->request->data_len, (unsigned int)63 * 1024);
+	nbytes = min(ata_qc_raw_nbytes(qc), (unsigned int)63 * 1024);
 
 	/* Most ATAPI devices which honor transfer chunk size don't
 	 * behave according to the spec when odd chunk size which
@@ -2876,7 +2884,7 @@ static unsigned int ata_scsi_pass_thru(s
 	 * TODO: find out if we need to do more here to
 	 *       cover scatter/gather case.
 	 */
-	qc->nbytes = scsi_bufflen(scmd) + scmd->request->extra_len;
+	ata_qc_set_pc_nbytes(qc);
 
 	/* request result TF and be quiet about device error */
 	qc->flags |= ATA_QCFLAG_RESULT_TF | ATA_QCFLAG_QUIET;
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -463,6 +463,7 @@ struct ata_queued_cmd {
 	unsigned int		sect_size;
 
 	unsigned int		nbytes;
+	unsigned int		extrabytes;
 	unsigned int		curbytes;
 
 	struct scatterlist	*cursg;
@@ -1336,6 +1337,11 @@ static inline struct ata_queued_cmd *ata
 	return NULL;
 }
 
+static inline unsigned int ata_qc_raw_nbytes(struct ata_queued_cmd *qc)
+{
+	return qc->nbytes - min(qc->extrabytes, qc->nbytes);
+}
+
 static inline void ata_tf_init(struct ata_device *dev, struct ata_taskfile *tf)
 {
 	memset(tf, 0, sizeof(*tf));
@@ -1354,7 +1360,7 @@ static inline void ata_qc_reinit(struct 
 	qc->flags = 0;
 	qc->cursg = NULL;
 	qc->cursg_ofs = 0;
-	qc->nbytes = qc->curbytes = 0;
+	qc->nbytes = qc->extrabytes = qc->curbytes = 0;
 	qc->n_elem = 0;
 	qc->err_mask = 0;
 	qc->sect_size = ATA_SECT_SIZE;

[PATCH #upstream-fixes 2/2] pata_it821x: use raw nbytes in check_atapi_dma

[Tejun Heo]

pata_it821x needs to look at raw request size in check_atapi_dma().

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
This should restore the original behavior.  However, there's a catch.
Alan, you said DMA behavior on pata_it821x genuinely depends on
transfer size, right?  Then, it can be one of the following two.

1. It depends on the size of the buffer handed to the controller, in
   which case qc->nbytes is the right value to look at.  For pata_ali,
   this definitely wasn't the case as small DMA transfers failed even
   though large aligned buffer was supplied.

2. It depends on the size of the data the drive transfers in response
   of the command.  In this case, checking raw or any size on host
   size is useless as the drive may transfer any number of bytes.  In
   this case, we should use command type to determine whether DMA can
   be used or not as we can guarantee certain transfer sizes for
   certain commands.  pata_ali fell into this category.

All in all, I think discerning #1 from #2 is hair-splitting for not
enough benefit (heh.. yeah, I'm going there again) and applying #1 for
controllers which fall in #2 is dangerous while the other way is fine.
I think it's better to just implement atapi_check_dma_rw_only() and
use it for these controllers.

Anyways, this patch restores the old behavior and should be fine for
2.6.25.

Thanks.

 drivers/ata/pata_it821x.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: work/drivers/ata/pata_it821x.c
===================================================================
--- work.orig/drivers/ata/pata_it821x.c
+++ work/drivers/ata/pata_it821x.c
@@ -564,7 +564,7 @@ static int it821x_check_atapi_dma(struct
 	struct it821x_dev *itdev = ap->private_data;
 
 	/* Only use dma for transfers to/from the media. */
-	if (qc->nbytes < 2048)
+	if (ata_qc_raw_nbytes(qc) < 2048)
 		return -EOPNOTSUPP;
 
 	/* No ATAPI DMA in smart mode */

Re: [PATCH #upstream-fixes 1/2] libata: implement ata_qc_raw_nbytes()

[Jeff Garzik]

Tejun Heo wrote:
> Implement ata_qc_raw_nbytes() which determines the raw user-requested
> size of a PC command.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-scsi.c |   14 +++++++++++---
>  include/linux/libata.h    |    8 +++++++-
>  2 files changed, 18 insertions(+), 4 deletions(-)

applied 1-2