>Number:         12501
>Category:       kern
>Synopsis:       use READ_CAPACITY v. ATAPI_READ_FORMAT_CAPACITIES in sd_atapi.c
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Thu Mar 29 09:49:01 PST 2001
>Closed-Date:
>Last-Modified:
>Originator:     Brett McCoy
>Release:        2001-03-29
>Organization:
	
>Environment:
	
System: NetBSD morte 1.5T NetBSD 1.5T (MORTEfw) #71: Thu Mar 29 11:33:24 EST 2001 bmccoy@morte:/usr/src/sys/arch/i386/compile/MORTEfw i386
Architecture: i386
Machine: i386
>Description:
	sd_atapibus_get_parms in sd_atapi.c uses the
	ATAPI_READ_FORMAT_CAPACITIES command to get the number of
	sectors and block size from a device.  This command is not
	well supported on all ATAPI drives, especially flash card
	readers.  The READ_CAPACITY command is much better supported
	and is a much simpler command.  It's already used in the
	scsipi_size function (in scsipi_base.c).  The problem is that
	that function only returns the number of sectors and not the
	block size.

	My request is to modify sd_atapibus_get_parms to use the
	READ_CAPCITY command to get the sector count and block size
	from the device.  I've implemented this by creating a
	sd_atapi_size function in sd_atapi.c and calling it from
	sd_atapibus_get_parms to set dp->disksize and dp->blksize.

	I think it would be better to generalize the scsipi_size
	function to return block size as well as sector count.  But
	this works fine and doesn't add too much extra code.
>How-To-Repeat:
	
>Fix:
Index: sd_atapi.c
===================================================================
RCS file: /cvsroot/syssrc/sys/dev/scsipi/sd_atapi.c,v
retrieving revision 1.8
diff -u -r1.8 sd_atapi.c
--- sd_atapi.c  2001/03/20 22:39:08     1.8
+++ sd_atapi.c  2001/03/29 17:45:39
@@ -75,6 +75,8 @@
         "",         "",                 ""},
 };
 
+static u_long   sd_atapi_size __P((struct scsipi_link *,
+                                  int, u_long *, u_long *));
 static int     sd_atapibus_get_parms __P((struct sd_softc *,
                    struct disk_parms *, int));
 
@@ -122,58 +124,53 @@
        sdattach(parent, sd, sc_link, &sd_atapibus_ops);
 }
 
+/*
+ * Find out from the device what its capacity is.
+ * XXX - this should really use atapi_size, but it
+ *       doesn't return block size information.
+ */
+u_long
+sd_atapi_size(sc_link, flags, disksize, blksize)
+       struct scsipi_link *sc_link;
+       int flags;
+       u_long *disksize;
+       u_long *blksize;
+{
+       struct scsipi_read_cap_data rdcap;
+       struct scsipi_read_capacity scsipi_cmd;
+
+       /*
+        * make up a scsipi command and ask the scsipi driver to do
+        * it for you.
+        */
+       bzero(&scsipi_cmd, sizeof(scsipi_cmd));
+       scsipi_cmd.opcode = READ_CAPACITY;
+
+       if (scsipi_command(sc_link, (struct scsipi_generic *)&scsipi_cmd,
+           sizeof(scsipi_cmd), (u_char *)&rdcap, sizeof(rdcap),
+           SCSIPIRETRIES, 20000, NULL,
+           flags | XS_CTL_DATA_IN | XS_CTL_DATA_ONSTACK) != 0) {
+               sc_link->sc_print_addr(sc_link);
+               printf("could not get size\n");
+               return (0);
+       }
+
+       *disksize = _4btol(rdcap.addr) + 1;
+       *blksize = _4btol(rdcap.length);
+       return (*disksize);
+}
+
 static int
 sd_atapibus_get_parms(sd, dp, flags)
        struct sd_softc *sd;
        struct disk_parms *dp;
        int flags;
 {
-       struct atapi_read_format_capacities scsipi_cmd;
-       struct atapi_capacity_descriptor *descp;
        struct atapi_sd_mode_data sense_data;
-       char capacity_data[ATAPI_CAP_DESC_SIZE(1)];
        int error;
-
-       bzero(&scsipi_cmd, sizeof scsipi_cmd);
-       scsipi_cmd.opcode = ATAPI_READ_FORMAT_CAPACITIES;
-       _lto2b(ATAPI_CAP_DESC_SIZE(1), scsipi_cmd.length);
-
-       error = scsipi_command(sd->sc_link,
-           (struct scsipi_generic *)&scsipi_cmd, sizeof(scsipi_cmd),
-           (void *)capacity_data, ATAPI_CAP_DESC_SIZE(1), SDRETRIES, 20000,
-           NULL, flags | XS_CTL_DATA_IN | XS_CTL_DATA_ONSTACK);
-       SC_DEBUG(sd->sc_link, SDEV_DB2,
-           ("sd_atapibus_get_parms: read format capacities error=%d\n",
-           error));
-       if (error != 0)
-               return (SDGP_RESULT_OFFLINE);
-
-       descp = (struct atapi_capacity_descriptor *)
-           &capacity_data[ATAPI_CAP_DESC_OFFSET_DESC(0)];
-
-       switch (descp->byte5 & ATAPI_CAP_DESC_CODE_MASK) {
-       case ATAPI_CAP_DESC_CODE_UNFORMATTED:
-               return SDGP_RESULT_UNFORMATTED;
-
-       case ATAPI_CAP_DESC_CODE_FORMATTED:
-               break;
-
-       case 0:
-               if (sd->sc_link->quirks & ADEV_BYTE5_ZERO)
-                       break;
-
-       default:
-#ifdef DIAGNOSTIC
-               printf("%s: strange capacity descriptor byte5 0x%x\n",
-                   sd->sc_dev.dv_xname, (u_int)descp->byte5);
-#endif
-               /* FALLTHROUGH */
-       case ATAPI_CAP_DESC_CODE_NONE:
-               return SDGP_RESULT_OFFLINE;
-       }
 
-       dp->disksize = _4btol(descp->nblks);
-       dp->blksize = _3btol(descp->blklen);
+       if (! sd_atapi_size(sd->sc_link, flags, &dp->disksize, &dp->blksize))
+         return (SDGP_RESULT_OFFLINE);
 
        /*
         * First, set up standard fictitious geometry, a la sd_scsi.c.

>Release-Note:
>Audit-Trail:
>Unformatted: