Date: Thu, 21 Feb 2008 21:58:49 -0800 (PST) From: Matthew Jacob <mj@feral.com> To: Scott Long <scottl@samsco.org> Cc: scsi@freebsd.org Subject: Re: scsi_ses.c fixes Message-ID: <20080221215727.H40427@ns1.feral.com> In-Reply-To: <47BC8775.7090600@samsco.org> References: <20080220112921.Y20235@ns1.feral.com> <47BC832E.1040207@samsco.org> <20080220115532.T20410@ns1.feral.com> <47BC8775.7090600@samsco.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080221215727.H40427>