Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 10 Dec 2010 21:45:10 +0000 (UTC)
From:      "Kenneth D. Merry" <ken@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r216363 - head/sys/dev/mps
Message-ID:  <201012102145.oBALjAv7086574@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: ken
Date: Fri Dec 10 21:45:10 2010
New Revision: 216363
URL: http://svn.freebsd.org/changeset/base/216363

Log:
  Fix an event handling bug with the mps(4) driver.
  
  This bug manifested itself after repeated device arrivals and
  departures.  The root of the problem was that the last entry in the
  reply array wasn't initialized/allocated.  So every time we got
  around to that event, we had a bogus address.
  
  There were a couple more problems with the code that are also fixed:
  
   - The reply mechanism was being treated as sequential (indexed by
     sc->replycurindex) even though the spec says that the driver
     should use the ReplyFrameAddress field of the post queue
     descriptor to figure out where the reply is.  There is no
     guarantee that the reply descriptors will be used in sequential
     order.
  
   - The second word of the reply post queue descriptor wasn't being
     checked in mps_intr_locked() to make sure that it wasn't
     0xffffffff.  So the driver could potentially come across a
     partially DMAed descriptor.
  
   - The number of replies allocated was one less than the actual
     size of the queue.  Instead, it was the size of the number of
     replies that can be used at one time.  (Which is one less than
     the size of the queue.)
  
  mps.c:		When initializing the entries in the reply free
  		queue, make sure we initialize the full number that
  		we tell the chip we have (sc->fqdepth), not the
  		number that can be used at any one time (sc->num_replies).
  
  		When allocating replies, make sure we allocate the
  		number of replies that we've told the chip exist,
  		not just the number that can be used simultaneously.
  
  		Use the ReplyFrameAddress field of the post queue
  		descriptor to figure out which reply is being
  		referenced.  This is what the spec says to do, and
  		the spec doesn't guarantee that the replies will be
  		used in order.
  
  		Put a check in to verify that the reply address passed
  		back from the card is valid.  (Panic if it isn't, we'll
  		panic when we try to deference the reply pointer in any
  		case.)
  
  		In mps_intr_locked(), verify that the second word of the
  		post queue descriptor is not 0xffffffff in addition to
  		verifying that the unused flag is not set, so we can
  		make sure we didn't get a partially DMAed descriptor.
  
  		Remove references to sc->replycurindex, it isn't needed
  		now.
  
  mpsvar.h:	Remove replycurindex from the softc, it isn't needed now.
  
  Reviewed by:	scottl

Modified:
  head/sys/dev/mps/mps.c
  head/sys/dev/mps/mpsvar.h

Modified: head/sys/dev/mps/mps.c
==============================================================================
--- head/sys/dev/mps/mps.c	Fri Dec 10 21:43:20 2010	(r216362)
+++ head/sys/dev/mps/mps.c	Fri Dec 10 21:45:10 2010	(r216363)
@@ -43,6 +43,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/malloc.h>
 #include <sys/uio.h>
 #include <sys/sysctl.h>
+#include <sys/endian.h>
 
 #include <machine/bus.h>
 #include <machine/resource.h>
@@ -607,9 +608,16 @@ mps_alloc_queues(struct mps_softc *sc)
 static int
 mps_alloc_replies(struct mps_softc *sc)
 {
-	int rsize;
+	int rsize, num_replies;
 
-	rsize = sc->facts->ReplyFrameSize * sc->num_replies * 4; 
+	/*
+	 * sc->num_replies should be one less than sc->fqdepth.  We need to
+	 * allocate space for sc->fqdepth replies, but only sc->num_replies
+	 * replies can be used at once.
+	 */
+	num_replies = max(sc->fqdepth, sc->num_replies);
+
+	rsize = sc->facts->ReplyFrameSize * num_replies * 4; 
         if (bus_dma_tag_create( sc->mps_parent_dmat,    /* parent */
 				4, 0,			/* algnmnt, boundary */
 				BUS_SPACE_MAXADDR_32BIT,/* lowaddr */
@@ -782,11 +790,19 @@ mps_init_queues(struct mps_softc *sc)
 
 	memset((uint8_t *)sc->post_queue, 0xff, sc->pqdepth * 8);
 
+	/*
+	 * According to the spec, we need to use one less reply than we
+	 * have space for on the queue.  So sc->num_replies (the number we
+	 * use) should be less than sc->fqdepth (allocated size).
+	 */
 	if (sc->num_replies >= sc->fqdepth)
 		return (EINVAL);
 
-	for (i = 0; i < sc->num_replies; i++)
-		sc->free_queue[i] = sc->reply_busaddr + i * sc->facts->ReplyFrameSize * 4;
+	/*
+	 * Initialize all of the free queue entries.
+	 */
+	for (i = 0; i < sc->fqdepth; i++)
+		sc->free_queue[i] = sc->reply_busaddr + (i * sc->facts->ReplyFrameSize * 4);
 	sc->replyfreeindex = sc->num_replies;
 
 	return (0);
@@ -907,7 +923,6 @@ mps_attach(struct mps_softc *sc)
 	 * replies.
 	 */
 	sc->replypostindex = 0;
-	sc->replycurindex = 0;
 	mps_regwrite(sc, MPI2_REPLY_FREE_HOST_INDEX_OFFSET, sc->replyfreeindex);
 	mps_regwrite(sc, MPI2_REPLY_POST_HOST_INDEX_OFFSET, 0);
 
@@ -1211,7 +1226,8 @@ mps_intr_locked(void *data)
 		desc = &sc->post_queue[pq];
 		flags = desc->Default.ReplyFlags &
 		    MPI2_RPY_DESCRIPT_FLAGS_TYPE_MASK;
-		if (flags == MPI2_RPY_DESCRIPT_FLAGS_UNUSED)
+		if ((flags == MPI2_RPY_DESCRIPT_FLAGS_UNUSED)
+		 || (desc->Words.High == 0xffffffff))
 			break;
 
 		switch (flags) {
@@ -1224,9 +1240,36 @@ mps_intr_locked(void *data)
 			uint32_t baddr;
 			uint8_t *reply;
 
+			/*
+			 * Re-compose the reply address from the address
+			 * sent back from the chip.  The ReplyFrameAddress
+			 * is the lower 32 bits of the physical address of
+			 * particular reply frame.  Convert that address to
+			 * host format, and then use that to provide the
+			 * offset against the virtual address base
+			 * (sc->reply_frames).
+			 */
+			baddr = le32toh(desc->AddressReply.ReplyFrameAddress);
 			reply = sc->reply_frames +
-			    sc->replycurindex * sc->facts->ReplyFrameSize * 4;
-			baddr = desc->AddressReply.ReplyFrameAddress;
+				(baddr - ((uint32_t)sc->reply_busaddr));
+			/*
+			 * Make sure the reply we got back is in a valid
+			 * range.  If not, go ahead and panic here, since
+			 * we'll probably panic as soon as we deference the
+			 * reply pointer anyway.
+			 */
+			if ((reply < sc->reply_frames)
+			 || (reply > (sc->reply_frames +
+			     (sc->fqdepth * sc->facts->ReplyFrameSize * 4)))) {
+				printf("%s: WARNING: reply %p out of range!\n",
+				       __func__, reply);
+				printf("%s: reply_frames %p, fqdepth %d, "
+				       "frame size %d\n", __func__,
+				       sc->reply_frames, sc->fqdepth,
+				       sc->facts->ReplyFrameSize * 4);
+				printf("%s: baddr %#x,\n", __func__, baddr);
+				panic("Reply address out of range");
+			}
 			if (desc->AddressReply.SMID == 0) {
 				mps_dispatch_event(sc, baddr,
 				   (MPI2_EVENT_NOTIFICATION_REPLY *) reply);
@@ -1236,8 +1279,6 @@ mps_intr_locked(void *data)
 				cm->cm_reply_data =
 				    desc->AddressReply.ReplyFrameAddress;
 			}
-			if (++sc->replycurindex >= sc->fqdepth)
-				sc->replycurindex = 0;
 			break;
 		}
 		case MPI2_RPY_DESCRIPT_FLAGS_TARGETASSIST_SUCCESS:

Modified: head/sys/dev/mps/mpsvar.h
==============================================================================
--- head/sys/dev/mps/mpsvar.h	Fri Dec 10 21:43:20 2010	(r216362)
+++ head/sys/dev/mps/mpsvar.h	Fri Dec 10 21:45:10 2010	(r216363)
@@ -135,7 +135,6 @@ struct mps_softc {
 	TAILQ_HEAD(, mps_command)	tm_list;
 	int				replypostindex;
 	int				replyfreeindex;
-	int				replycurindex;
 
 	struct resource			*mps_regs_resource;
 	bus_space_handle_t		mps_bhandle;

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201012102145.oBALjAv7086574>