bouyer@antioche.eu.org writes:
> On Sun, Nov 19, 2006 at 10:05:34PM +0900, ITOH Yasufumi wrote:
> > Hello,
> > 
> > Tracking down kern/26537, I noticed our cd(4) and sd(4)
> > drivers issue SCSI Mode Sense command on page 0.
> > 
> > The SCSI specification says the page 0 is ``vendor-specific'' and
> > even the parameter format may be non-standard.
> > 
> > For what reasons our drivers use such a vendor-specific behavior?
> 
> I suspect it was the way to retrieve the block size on older scsi
> devices. Maybe we should try to use READ_FORMAT_CAPACITIES before going
> to Mode Sense page 0.

I noticed the page contents of Mode Sense is not used, but
the block descriptor (sent prior to the page contents) is used,
so it is not that vendor-specific (but this device don't like it).
On the contrary the Read Format Capacities command seems vendor-specific.

Hmm, then, I'd like to change current method of detecting block size

	if Read Capacity failed
		use Read Format Capacities
		if invalid use 512
	else
		use Mode Sense page 0
		if failed, use value of Read Capacity or 512 if invalid

to a new method:

	if Read Capacity failed
		use Read Format Capacities
	else if value of Read Capacity is invalid
		use Mode Sense 0
	if above block size is invalid
		use 512

We don't support multiple block sizes per LUN, and if Read Capacity
reports valid block size, why don't we use it?

Is this change OK?

By the way, scsipi_size() used only by the sd driver,
this patch also moves its function to sd.c.

-- 
ITOH Yasufumi

Index: scsipi_base.c
===================================================================
RCS file: /cvsroot/src/sys/dev/scsipi/scsipi_base.c,v
retrieving revision 1.140
diff -u -p -r1.140 scsipi_base.c
--- scsipi_base.c	20 Oct 2006 07:11:50 -0000	1.140
+++ scsipi_base.c	24 Nov 2006 17:41:13 -0000
@@ -1021,100 +1021,6 @@ scsipi_interpret_sense(struct scsipi_xfe
 }
 
 /*
- * scsipi_validate_secsize:
- *
- *	Validate the sector size reported by READ_CAPACITY_1[06].
- *	Use the supplied default if the reported size looks wrong.
- */
-static int
-scsipi_validate_secsize(struct scsipi_periph *periph, const char *opcode,
-    int raw_len, int def_len)
-{
-
-	switch (raw_len) {
-	case 256:
-	case 512:
-	case 1024:
-	case 2048:
-	case 4096:
-		break;
-
-	default:
-		scsipi_printaddr(periph);
-		printf("%s returned %s sector size: 0x%x. Defaulting to %d "
-		    "bytes.\n", opcode, (raw_len ^ (1 << (ffs(raw_len) - 1))) ?
-		    "preposterous" : "unsupported", raw_len, def_len);
-		/*FALLTHROUGH*/
-	case 0:
-		raw_len = def_len;
-		break;
-	}
-
-	return (raw_len);
-}
-
-/*
- * scsipi_size:
- *
- *	Find out from the device what its capacity is.
- */
-u_int64_t
-scsipi_size(struct scsipi_periph *periph, int *secsize, int defsize, int flags)
-{
-	union {
-		struct scsipi_read_capacity_10 cmd;
-		struct scsipi_read_capacity_16 cmd16;
-	} cmd;
-	union {
-		struct scsipi_read_capacity_10_data data;
-		struct scsipi_read_capacity_16_data data16;
-	} data;
-
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.cmd.opcode = READ_CAPACITY_10;
-
-	/*
-	 * If the command works, interpret the result as a 4 byte
-	 * number of blocks
-	 */
-	if (scsipi_command(periph, (void *)&cmd.cmd, sizeof(cmd.cmd),
-	    (void *)&data.data, sizeof(data.data), SCSIPIRETRIES, 20000, NULL,
-	    flags | XS_CTL_DATA_IN | XS_CTL_DATA_ONSTACK | XS_CTL_SILENT) != 0)
-		return (0);
-
-	if (_4btol(data.data.addr) != 0xffffffff) {
-		if (secsize) {
-			*secsize = scsipi_validate_secsize(periph,
-			    "READ_CAPACITY_10", _4btol(data.data.length),
-			    defsize);
-		}
-		return (_4btol(data.data.addr) + 1);
-	}
-
-	/*
-	 * Device is larger than can be reflected by READ CAPACITY (10).
-	 * Try READ CAPACITY (16).
-	 */
-
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.cmd16.opcode = READ_CAPACITY_16;
-	cmd.cmd16.byte2 = SRC16_SERVICE_ACTION;
-	_lto4b(sizeof(data.data16), cmd.cmd16.len);
-
-	if (scsipi_command(periph, (void *)&cmd.cmd16, sizeof(cmd.cmd16),
-	    (void *)&data.data16, sizeof(data.data16), SCSIPIRETRIES, 20000,
-	    NULL,
-	    flags | XS_CTL_DATA_IN | XS_CTL_DATA_ONSTACK | XS_CTL_SILENT) != 0)
-		return (0);
-
-	if (secsize) {
-		*secsize = scsipi_validate_secsize(periph, "READ_CAPACITY_16",
-		    _4btol(data.data16.length), defsize);
-	}
-	return (_8btol(data.data16.addr) + 1);
-}
-
-/*
  * scsipi_test_unit_ready:
  *
  *	Issue a `test unit ready' request.
Index: scsipiconf.h
===================================================================
RCS file: /cvsroot/src/sys/dev/scsipi/scsipiconf.h,v
retrieving revision 1.106
diff -u -p -r1.106 scsipiconf.h
--- scsipiconf.h	30 Oct 2006 16:15:56 -0000	1.106
+++ scsipiconf.h	24 Nov 2006 17:41:13 -0000
@@ -633,7 +633,6 @@ const void *scsipi_inqmatch(struct scsip
 const char *scsipi_dtype(int);
 void	scsipi_strvis(u_char *, int, const u_char *, int);
 int	scsipi_execute_xs(struct scsipi_xfer *);
-u_int64_t scsipi_size(struct scsipi_periph *, int *, int, int);
 int	scsipi_test_unit_ready(struct scsipi_periph *, int);
 int	scsipi_prevent(struct scsipi_periph *, int, int);
 int	scsipi_inquire(struct scsipi_periph *,
Index: sd.c
===================================================================
RCS file: /cvsroot/src/sys/dev/scsipi/sd.c,v
retrieving revision 1.253
diff -u -p -r1.253 sd.c
--- sd.c	20 Oct 2006 07:11:50 -0000	1.253
+++ sd.c	24 Nov 2006 17:41:13 -0000
@@ -97,6 +97,8 @@ __KERNEL_RCSID(0, "$NetBSD: sd.c,v 1.253
 
 #define	SDLABELDEV(dev)	(MAKESDDEV(major(dev), SDUNIT(dev), RAW_PART))
 
+#define SD_DEFAULT_SECSIZE	512
+
 static void	sdminphys(struct buf *);
 static void	sdgetdefaultlabel(struct sd_softc *, struct disklabel *);
 static int	sdgetdisklabel(struct sd_softc *);
@@ -110,6 +112,8 @@ static int	sd_mode_sense(struct sd_softc
 		    int, int *);
 static int	sd_mode_select(struct sd_softc *, u_int8_t, void *, size_t, int,
 		    int);
+static int	sd_validate_secsize(struct scsipi_periph *, int);
+static u_int64_t sd_read_capacity(struct scsipi_periph *, int *, int flags);
 static int	sd_get_simplifiedparms(struct sd_softc *, struct disk_parms *,
 		    int);
 static int	sd_get_capacity(struct sd_softc *, struct disk_parms *, int);
@@ -1636,6 +1640,91 @@ sd_mode_select(struct sd_softc *sd, u_in
 	}
 }
 
+/*
+ * sd_validate_secsize:
+ *
+ *	Validate the sector size.
+ */
+static int
+sd_validate_secsize(struct scsipi_periph *periph, int len)
+{
+
+	switch (len) {
+	case 256:
+	case 512:
+	case 1024:
+	case 2048:
+	case 4096:
+		return 1;
+	}
+
+	if (periph) {
+		scsipi_printaddr(periph);
+		printf("%s sector size: 0x%x. Defaulting to %d bytes.\n",
+		    (len ^ (1 << (ffs(len) - 1))) ?
+		    "preposterous" : "unsupported",
+		    len, SD_DEFAULT_SECSIZE);
+	}
+
+	return 0;
+}
+
+/*
+ * sd_read_capacity:
+ *
+ *	Find out from the device what its capacity is.
+ */
+static u_int64_t
+sd_read_capacity(struct scsipi_periph *periph, int *secsize, int flags)
+{
+	union {
+		struct scsipi_read_capacity_10 cmd;
+		struct scsipi_read_capacity_16 cmd16;
+	} cmd;
+	union {
+		struct scsipi_read_capacity_10_data data;
+		struct scsipi_read_capacity_16_data data16;
+	} data;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.cmd.opcode = READ_CAPACITY_10;
+
+	/*
+	 * If the command works, interpret the result as a 4 byte
+	 * number of blocks
+	 */
+	memset(&data.data, 0, sizeof(data.data));
+	if (scsipi_command(periph, (void *)&cmd.cmd, sizeof(cmd.cmd),
+	    (void *)&data.data, sizeof(data.data), SCSIPIRETRIES, 20000, NULL,
+	    flags | XS_CTL_DATA_IN | XS_CTL_DATA_ONSTACK | XS_CTL_SILENT) != 0)
+		return (0);
+
+	if (_4btol(data.data.addr) != 0xffffffff) {
+		*secsize = _4btol(data.data.length);
+		return (_4btol(data.data.addr) + 1);
+	}
+
+	/*
+	 * Device is larger than can be reflected by READ CAPACITY (10).
+	 * Try READ CAPACITY (16).
+	 */
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.cmd16.opcode = READ_CAPACITY_16;
+	cmd.cmd16.byte2 = SRC16_SERVICE_ACTION;
+	_lto4b(sizeof(data.data16), cmd.cmd16.len);
+
+	memset(&data.data16, 0, sizeof(data.data16));
+	if (scsipi_command(periph, (void *)&cmd.cmd16, sizeof(cmd.cmd16),
+	    (void *)&data.data16, sizeof(data.data16), SCSIPIRETRIES, 20000,
+	    NULL,
+	    flags | XS_CTL_DATA_IN | XS_CTL_DATA_ONSTACK | XS_CTL_SILENT) != 0)
+		return (0);
+
+	*secsize = _4btol(data.data16.length);
+	return (_8btol(data.data16.addr) + 1);
+}
+
 static int
 sd_get_simplifiedparms(struct sd_softc *sd, struct disk_parms *dp, int flags)
 {
@@ -1655,15 +1744,17 @@ sd_get_simplifiedparms(struct sd_softc *
 	int error, blksize;
 
 	/*
-	 * scsipi_size (ie "read capacity") and mode sense page 6
+	 * sd_read_capacity (ie "read capacity") and mode sense page 6
 	 * give the same information. Do both for now, and check
 	 * for consistency.
 	 * XXX probably differs for removable media
 	 */
-	dp->blksize = 512;
-	if ((sectors = scsipi_size(sd->sc_periph, &blksize, 512, flags)) == 0)
+	dp->blksize = SD_DEFAULT_SECSIZE;
+	if ((sectors = sd_read_capacity(sd->sc_periph, &blksize, flags)) == 0)
 		return (SDGP_RESULT_OFFLINE);		/* XXX? */
 
+	dp->blksize = blksize;
+
 	error = scsipi_mode_sense(sd->sc_periph, SMS_DBD, 6,
 	    &scsipi_sense.header, sizeof(scsipi_sense),
 	    flags | XS_CTL_DATA_ONSTACK, SDRETRIES, 6000);
@@ -1671,9 +1762,10 @@ sd_get_simplifiedparms(struct sd_softc *
 	if (error != 0)
 		return (SDGP_RESULT_OFFLINE);		/* XXX? */
 
-	dp->blksize = _2btol(scsipi_sense.lbs);
-	if (dp->blksize == 0)
-		dp->blksize = blksize;
+	if (!sd_validate_secsize(NULL, dp->blksize))
+		dp->blksize = _2btol(scsipi_sense.lbs);
+	if (!sd_validate_secsize(sd->sc_periph, dp->blksize))
+		dp->blksize = SD_DEFAULT_SECSIZE;
 
 	/*
 	 * Create a pseudo-geometry.
@@ -1707,7 +1799,7 @@ sd_get_capacity(struct sd_softc *sd, str
 	u_int8_t *p;
 #endif
 
-	dp->disksize = sectors = scsipi_size(sd->sc_periph, &blksize, 512,
+	dp->disksize = sectors = sd_read_capacity(sd->sc_periph, &blksize,
 	    flags);
 	if (sectors == 0) {
 		struct scsipi_read_format_capacities cmd;
@@ -1752,18 +1844,16 @@ printf("rfc result:"); for (i = sizeof(s
 		if (sectors == 0)
 			return (SDGP_RESULT_OFFLINE);		/* XXX? */
 
-		dp->blksize = _3btol(data.desc.blklen);
-		if (dp->blksize == 0)
-			dp->blksize = 512;
-	} else {
+		blksize = _3btol(data.desc.blklen);
+
+	} else if (!sd_validate_secsize(NULL, blksize)) {
 		struct sd_mode_sense_data scsipi_sense;
 		int big, bsize;
 		struct scsi_general_block_descriptor *bdesc;
 
 		memset(&scsipi_sense, 0, sizeof(scsipi_sense));
 		error = sd_mode_sense(sd, 0, &scsipi_sense,
-		    sizeof(scsipi_sense.blk_desc), 0, flags | XS_CTL_SILENT, &big);
-		dp->blksize = blksize;
+		    sizeof(scsipi_sense.blk_desc), 0x3f, flags | XS_CTL_SILENT, &big);
 		if (!error) {
 			if (big) {
 				bdesc = (void *)(&scsipi_sense.header.big + 1);
@@ -1780,13 +1870,15 @@ printf("page 0 ok\n");
 #endif
 
 			if (bsize >= 8) {
-				dp->blksize = _3btol(bdesc->blklen);
-				if (dp->blksize == 0)
-					dp->blksize = blksize;
+				blksize = _3btol(bdesc->blklen);
 			}
 		}
 	}
 
+	if (!sd_validate_secsize(sd->sc_periph, blksize))
+		blksize = SD_DEFAULT_SECSIZE;
+
+	dp->blksize = blksize;
 	dp->disksize512 = (sectors * dp->blksize) / DEV_BSIZE;
 	return (0);
 }