scsi_ses.c fixes

[Matthew Jacob]

On Wed, 20 Feb 2008, Scott Long wrote:

> Matthew Jacob wrote:
>> 
>> Well, no need to be embarassed. It's embarassing to not hear anything about 
>> for months- and this for something which as a driver configures with just 
>> about every backplane and storage unit out there. Nobody apparently 
>> installs and runs the examples ses code - probably they should be promoted 
>> to real modules.
>> 
>
> Yeah, I thought that I had tested against the example ses code at one 
> point, but apparently not.  While we're here, note the comment about 
> dropping the lock multiple times to do copyout.  That can probably be 
> easily fixed by allocating a temporary buffer to copy everything into 
> first, but I was hoping to find a more elegant solution.  If you have 
> any ideas, feel free to try them out.

You don't really need to lock the peripheral for some of the cases- the 
data is stable as long as periph doesn't go away, which it shouldn't as 
long as the user app had the device open. This plus u_int ->uint.

Index: scsi_ses.c
===================================================================
RCS file: /home/ncvs/src/sys/cam/scsi/scsi_ses.c,v
retrieving revision 1.36
diff -u -r1.36 scsi_ses.c
--- scsi_ses.c	20 Feb 2008 19:49:46 -0000	1.36
+++ scsi_ses.c	22 Feb 2008 05:56:32 -0000
@@ -144,9 +144,9 @@
  	encvec		ses_vec;	/* vector to handlers */
  	void *		ses_private;	/* per-type private data */
  	encobj *	ses_objmap;	/* objects */
-	u_int32_t	ses_nobjects;	/* number of objects */
+	uint32_t	ses_nobjects;	/* number of objects */
  	ses_encstat	ses_encstat;	/* overall status */
-	u_int8_t	ses_flags;
+	uint8_t		ses_flags;
  	union ccb	ses_saved_ccb;
  	struct cdev *ses_dev;
  	struct cam_periph *periph;
@@ -166,9 +166,9 @@
  static  periph_dtor_t   sescleanup;
  static  periph_start_t  sesstart;

-static void sesasync(void *, u_int32_t, struct cam_path *, void *);
+static void sesasync(void *, uint32_t, struct cam_path *, void *);
  static void sesdone(struct cam_periph *, union ccb *);
-static int seserror(union ccb *, u_int32_t, u_int32_t);
+static int seserror(union ccb *, uint32_t, uint32_t);

  static struct periph_driver sesdriver = {
  	sesinit, "ses",
@@ -234,7 +234,7 @@
  }

  static void
-sesasync(void *callback_arg, u_int32_t code, struct cam_path *path, void *arg)
+sesasync(void *callback_arg, uint32_t code, struct cam_path *path, void *arg)
  {
  	struct cam_periph *periph;

@@ -303,7 +303,7 @@
  		return (CAM_REQ_CMP_ERR);
  	}

-	softc = malloc(sizeof (struct ses_softc), M_SCSISES, M_NOWAIT);
+	softc = SES_MALLOC(sizeof (struct ses_softc));
  	if (softc == NULL) {
  		printf("sesregister: Unable to probe new device. "
  		       "Unable to allocate softc\n"); 
@@ -472,7 +472,7 @@
  }

  static int
-seserror(union ccb *ccb, u_int32_t cflags, u_int32_t sflags)
+seserror(union ccb *ccb, uint32_t cflags, uint32_t sflags)
  {
  	struct ses_softc *softc;
  	struct cam_periph *periph;
@@ -489,7 +489,7 @@
  	struct cam_periph *periph;
  	ses_encstat tmp;
  	ses_objstat objs;
-	ses_object obj, *uobj;
+	ses_object *uobj;
  	struct ses_softc *ssc;
  	void *addr;
  	int error, i;
@@ -511,6 +511,9 @@

  	/*
  	 * Now check to see whether we're initialized or not.
+	 * This actually should never fail as we're not supposed
+	 * to get past ses_open w/o successfully initializing
+	 * things.
  	 */
  	if ((ssc->ses_flags & SES_FLAG_INITIALIZED) == 0) {
  		cam_periph_unlock(periph);
@@ -526,6 +529,14 @@
  	/*
  	 * If this command can change the device's state,
  	 * we must have the device open for writing.
+	 *
+	 * For commands that get information about the
+	 * device- we don't need to lock the peripheral
+	 * if we aren't running a command. The number
+	 * of objects and the contents will stay stable
+	 * after the first open that does initialization.
+	 * The periph also can't go away while a user
+	 * process has it open.
  	 */
  	switch (cmd) {
  	case SESIOC_GETNOBJ:
@@ -546,23 +557,16 @@
  		break;

  	case SESIOC_GETOBJMAP:
-		/*
-		 * XXX Dropping the lock while copying multiple segments is
-		 * bogus.
-		 */
-		cam_periph_lock(periph);
-		for (uobj = addr, i = 0; i != ssc->ses_nobjects; i++, uobj++) {
-			obj.obj_id = i;
-			obj.subencid = ssc->ses_objmap[i].subenclosure;
-			obj.object_type = ssc->ses_objmap[i].enctype;
-			cam_periph_unlock(periph);
-			error = copyout(&obj, uobj, sizeof (ses_object));
-			cam_periph_lock(periph);
+		for (uobj = addr, i = 0; i != ssc->ses_nobjects; i++) {
+			ses_object kobj;
+			kobj.obj_id = i;
+			kobj.subencid = ssc->ses_objmap[i].subenclosure;
+			kobj.object_type = ssc->ses_objmap[i].enctype;
+			error = copyout(&kobj, &uobj[i], sizeof (ses_object));
  			if (error) {
  				break;
  			}
  		}
-		cam_periph_unlock(periph);
  		break;

  	case SESIOC_GETENCSTAT: