[PATCH] libata sg chaining support fix

[PATCH] libata sg chaining support fix

[FUJITA Tomonori]

This fixes 'trailing data' bug in the sglist libata patch in Jens'
block git tree.

Aug 18 16:03:03 tulip kernel: ata1.00: 36 bytes trailing data
Aug 18 16:03:03 tulip kernel: scsi scan: INQUIRY result too short (5), using 36


With the patch, sglist could be sent to -mm (I guess that Andrew hit
this bug). But I'm still not sure about the sglist libata patch
(especially about the padding path). It would be better to drop the
sglist libata patch with my use_sg_chaining option patch and send
sglist to -mm, I think.

http://marc.info/?l=linux-scsi&m=118728307617958&w=2

I guess that it would be better to leave sglist libata conversion in
the maintainers' hands.

---
>From 86d7a796417bde84fc3cbe6ac6ebaaa524266092 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Sat, 18 Aug 2007 18:27:36 +0900
Subject: [PATCH] libata sg chaining support fix

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/ata/libata-core.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f37eecc..52f75c3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4648,16 +4648,18 @@ static void __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);
 	struct ata_port *ap = qc->ap;
 	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;
 
 next_sg:
-	if (unlikely(qc->cursg == sg_last(qc->__sg, qc->n_elem))) {
+	if (unlikely(no_more_sg)) {
 		/*
 		 * The end of qc->sg is reached and the device expects
 		 * more data to transfer. In order not to overrun qc->sg
@@ -4719,6 +4721,9 @@ next_sg:
 	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;
 	}
-- 
1.5.2.4

Re: [PATCH] libata sg chaining support fix

[Jens Axboe]

On Sat, Aug 18 2007, FUJITA Tomonori wrote:
> This fixes 'trailing data' bug in the sglist libata patch in Jens'
> block git tree.
> 
> Aug 18 16:03:03 tulip kernel: ata1.00: 36 bytes trailing data
> Aug 18 16:03:03 tulip kernel: scsi scan: INQUIRY result too short (5), using 36
> 
> 
> With the patch, sglist could be sent to -mm (I guess that Andrew hit
> this bug). But I'm still not sure about the sglist libata patch
> (especially about the padding path). It would be better to drop the
> sglist libata patch with my use_sg_chaining option patch and send
> sglist to -mm, I think.
> 
> http://marc.info/?l=linux-scsi&m=118728307617958&w=2
> 
> I guess that it would be better to leave sglist libata conversion in
> the maintainers' hands.
> 
> ---
> From 86d7a796417bde84fc3cbe6ac6ebaaa524266092 Mon Sep 17 00:00:00 2001
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Date: Sat, 18 Aug 2007 18:27:36 +0900
> Subject: [PATCH] libata sg chaining support fix
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/ata/libata-core.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index f37eecc..52f75c3 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4648,16 +4648,18 @@ static void __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);
>  	struct ata_port *ap = qc->ap;
>  	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;
>  
>  next_sg:
> -	if (unlikely(qc->cursg == sg_last(qc->__sg, qc->n_elem))) {
> +	if (unlikely(no_more_sg)) {
>  		/*
>  		 * The end of qc->sg is reached and the device expects
>  		 * more data to transfer. In order not to overrun qc->sg
> @@ -4719,6 +4721,9 @@ next_sg:
>  	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;
>  	}

Tomo, this looks promising! Andrew, I have updated #for-akpm with the
sglist bits so it's enabled for the next -mm in the hope that this fixes
the issue.

-- 
Jens Axboe