From owner-svn-src-head@FreeBSD.ORG Fri Dec 10 21:45:10 2010 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id AB963106567A; Fri, 10 Dec 2010 21:45:10 +0000 (UTC) (envelope-from ken@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id 9A11F8FC21; Fri, 10 Dec 2010 21:45:10 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.3/8.14.3) with ESMTP id oBALjADJ086577; Fri, 10 Dec 2010 21:45:10 GMT (envelope-from ken@svn.freebsd.org) Received: (from ken@localhost) by svn.freebsd.org (8.14.3/8.14.3/Submit) id oBALjAv7086574; Fri, 10 Dec 2010 21:45:10 GMT (envelope-from ken@svn.freebsd.org) Message-Id: <201012102145.oBALjAv7086574@svn.freebsd.org> From: "Kenneth D. Merry" Date: Fri, 10 Dec 2010 21:45:10 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r216363 - head/sys/dev/mps X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Dec 2010 21:45:10 -0000 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 #include #include +#include #include #include @@ -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;