Subject: [I Think] sddone() Doesn't Count Partial xfers Correctly
To: None <netbsd-bugs@netbsd.org>
From: Chris Jepeway <jepeway@blasted-heath.com>
List: netbsd-bugs
Date: 03/28/2002 14:20:37
scsipi_complete() calls sddone() through the (*psw_done)() completion
hook in a scsipi_xfer struct.  sddone() uses xs->bp to, among other
things, count up the # of bytes xferred to/from the disk by checking
the b_resid field of the buffer, as in

    disk_unbusy(&sd->sc_dk, xs->bp->b_bcount - xs->bp->b_resid);

However, scspi_complete() calls through (*psw_done)() b/4 setting
xs->bp->b_resid to anything.  I'm betting, then, that sddone() will
mis-count partial xfers.

This patch illustrates/fixes the problem.  It's from a local repository
forked from NetBSD-current at revision 1.61 of scsipi_base.c, so
ignore its versions and line numbers:

Index: scsipi_base.c
===================================================================
RCS file: /cvsroot/src/sys/dev/scsipi/scsipi_base.c,v
retrieving revision 1.3
retrieving revision 1.4
diff -u -r1.3 -r1.4
--- scsipi_base.c	2002/02/27 23:22:54	1.3
+++ scsipi_base.c	2002/03/07 17:37:59	1.4
@@ -1532,9 +1532,10 @@
 	if (xs->error != XS_NOERROR)
 		scsipi_periph_thaw(periph, 1);
 
-
-	if (periph->periph_switch->psw_done)
-		periph->periph_switch->psw_done(xs);
+	/*
+	 * Set buffer fields in case the periph
+	 * switch done func uses them
+	 */
 	if ((bp = xs->bp) != NULL) {
 		if (error) {
 			bp->b_error = error;
@@ -1543,9 +1544,13 @@
 		} else {
 			bp->b_error = 0;
 			bp->b_resid = xs->resid;
-																		}
-		biodone(bp);
+		}
 	}
+
+	if (periph->periph_switch->psw_done)
+		periph->periph_switch->psw_done(xs);
+	if (bp)
+		biodone(bp);
 
 	if (xs->xs_control & XS_CTL_ASYNC)
 		scsipi_put_xs(xs);

Is a better fix to make sddone() use the values in the xs proper,
instead of using those in xs->bp?  Such an approach was suggested
back in late 96 in kern/3031, but it's clear it wasn't taken.

But, nevermind the fix, should I file a PR on this?

Chris <jepeway@blasted-heath.com>.