[PATCH] ps3rom: fix wrong resid calculation bug

[PATCH] ps3rom: fix wrong resid calculation bug

[FUJITA Tomonori]

sg driver rounds up the length in struct scatterlist to be a multiple
of 512 in some conditions. So LLDs can't use the data length in a sg
list to calculate residual. Instead, the length in struct scsi_cmnd
should be used.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/ps3rom.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/ps3rom.c b/drivers/scsi/ps3rom.c
index 0cd614a..6944bda 100644
--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -124,7 +124,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf)
 		}
 		req_len += sgpnt->length;
 	}
-	scsi_set_resid(cmd, req_len - act_len);
+	scsi_set_resid(cmd, scsi_bufflen(cmd) - act_len);
 	return 0;
 }
 
-- 
1.5.3.7

Re: [PATCH] ps3rom: fix wrong resid calculation bug

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1422 bytes --]

On Sun, 24 Feb 2008, FUJITA Tomonori wrote:
> sg driver rounds up the length in struct scatterlist to be a multiple
> of 512 in some conditions. So LLDs can't use the data length in a sg
> list to calculate residual. Instead, the length in struct scsi_cmnd
> should be used.

Thanks!

> --- a/drivers/scsi/ps3rom.c
> +++ b/drivers/scsi/ps3rom.c
> @@ -124,7 +124,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf)
>  		}
>  		req_len += sgpnt->length;
>  	}
> -	scsi_set_resid(cmd, req_len - act_len);
> +	scsi_set_resid(cmd, scsi_bufflen(cmd) - act_len);
>  	return 0;
>  }

My 2 comments:
  - The variable buflen already contains scsi_bufflen(cmd),
  - We no longer need to calculate the data length of the whole scatterlist.

I'll follow up to this email with a replacement patch and an addendum patch.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

[PATCH 1/2] ps3rom: fix wrong resid calculation bug

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1465 bytes --]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

sg driver rounds up the length in struct scatterlist to be a multiple
of 512 in some conditions. So LLDs can't use the data length in a sg
list to calculate residual. Instead, the length in struct scsi_cmnd
should be used.

[Geert: the variable buflen already contains scsi_bufflen(cmd)]

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/ps3rom.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -124,7 +124,7 @@ static int fill_from_dev_buffer(struct s
 		}
 		req_len += sgpnt->length;
 	}
-	scsi_set_resid(cmd, req_len - act_len);
+	scsi_set_resid(cmd, buflen - act_len);
 	return 0;
 }
 

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

[PATCH 2/2] ps3rom: Simplify fill_from_dev_buffer()

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2408 bytes --]

Subject: [PATCH] ps3rom: Simplify fill_from_dev_buffer()

As we no longer need to calculate the data length of the whole scatterlist,
we can abort the loop earlier and coalesce req_len and act_len into one
variable, making fill_from_dev_buffer() more similar to fetch_to_dev_buffer().

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/scsi/ps3rom.c |   30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -95,7 +95,7 @@ static int ps3rom_slave_configure(struct
  */
 static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf)
 {
-	int k, req_len, act_len, len, active;
+	int k, req_len, len, fin;
 	void *kaddr;
 	struct scatterlist *sgpnt;
 	unsigned int buflen;
@@ -107,24 +107,22 @@ static int fill_from_dev_buffer(struct s
 	if (!scsi_sglist(cmd))
 		return -1;
 
-	active = 1;
-	req_len = act_len = 0;
+	req_len = fin = 0;
 	scsi_for_each_sg(cmd, sgpnt, scsi_sg_count(cmd), k) {
-		if (active) {
-			kaddr = kmap_atomic(sg_page(sgpnt), KM_IRQ0);
-			len = sgpnt->length;
-			if ((req_len + len) > buflen) {
-				active = 0;
-				len = buflen - req_len;
-			}
-			memcpy(kaddr + sgpnt->offset, buf + req_len, len);
-			flush_kernel_dcache_page(sg_page(sgpnt));
-			kunmap_atomic(kaddr, KM_IRQ0);
-			act_len += len;
+		kaddr = kmap_atomic(sg_page(sgpnt), KM_IRQ0);
+		len = sgpnt->length;
+		if ((req_len + len) > buflen) {
+			len = buflen - req_len;
+			fin = 1;
 		}
-		req_len += sgpnt->length;
+		memcpy(kaddr + sgpnt->offset, buf + req_len, len);
+		flush_kernel_dcache_page(sg_page(sgpnt));
+		kunmap_atomic(kaddr, KM_IRQ0);
+		req_len += len;
+		if (fin)
+			break;
 	}
-	scsi_set_resid(cmd, buflen - act_len);
+	scsi_set_resid(cmd, buflen - req_len);
 	return 0;
 }
 
With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [PATCH 2/2] ps3rom: Simplify fill_from_dev_buffer()

[FUJITA Tomonori]

On Mon, 25 Feb 2008 14:24:31 +0100 (CET)
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> Subject: [PATCH] ps3rom: Simplify fill_from_dev_buffer()
> 
> As we no longer need to calculate the data length of the whole scatterlist,
> we can abort the loop earlier and coalesce req_len and act_len into one
> variable, making fill_from_dev_buffer() more similar to fetch_to_dev_buffer().

I'll add new APIs to copy data between a sg list and a buffer soon
after cleaning up (and fixing) some drivers on this area. I plan to
remove fill_from_dev_buffer and fetch_to_dev_buffer in ps3rom. As you
know, they are same functions that scsi_debug uses. There are other
drivers that need similar functions. I expect my ps3rom patch to be
applied to the scsi-fixes tree so I didn't change much as you did in
this patch.

Re: [PATCH 2/2] ps3rom: Simplify fill_from_dev_buffer()

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1499 bytes --]

On Tue, 26 Feb 2008, FUJITA Tomonori wrote:
> On Mon, 25 Feb 2008 14:24:31 +0100 (CET)
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > Subject: [PATCH] ps3rom: Simplify fill_from_dev_buffer()
> > 
> > As we no longer need to calculate the data length of the whole scatterlist,
> > we can abort the loop earlier and coalesce req_len and act_len into one
> > variable, making fill_from_dev_buffer() more similar to fetch_to_dev_buffer().
> 
> I'll add new APIs to copy data between a sg list and a buffer soon
> after cleaning up (and fixing) some drivers on this area. I plan to
> remove fill_from_dev_buffer and fetch_to_dev_buffer in ps3rom. As you
> know, they are same functions that scsi_debug uses. There are other
> drivers that need similar functions. I expect my ps3rom patch to be
> applied to the scsi-fixes tree so I didn't change much as you did in
> this patch.

IC, thanks!

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619