git: 0f8a8416a4e0 - main - dwmmc: cleanup cmd and locking, consistency between mmc and mmccam

From: Bjoern A. Zeeb <bz_at_FreeBSD.org>
Date: Thu, 31 Jul 2025 17:39:02 UTC
The branch main has been updated by bz:

URL: https://cgit.FreeBSD.org/src/commit/?id=0f8a8416a4e07ddeaecc4eafd6f418da0f21efc7

commit 0f8a8416a4e07ddeaecc4eafd6f418da0f21efc7
Author:     Bjoern A. Zeeb <bz@FreeBSD.org>
AuthorDate: 2025-07-29 12:36:26 +0000
Commit:     Bjoern A. Zeeb <bz@FreeBSD.org>
CommitDate: 2025-07-31 17:37:54 +0000

    dwmmc: cleanup cmd and locking, consistency between mmc and mmccam
    
    In general sprinkle locking assertions and harmonized KASSERTs
    throughout the upper part of the driver to document expectations.
    
    In dwmmc_cmd_done() "cmd" should be set correctly and be used for
    both MMCCAM and classic mmc rather than special-casing mmccam.
    In dwmmc_next_operation() place variable declarations on the top
    for both cases before the first debug and lock assertion calls;
    then factor out common parts at the end and put both cases in the
    same order.
    By calling dwmmc_next_operation() directly from both dwmmc_request()
    in the mmc case, and dwmmc_cam_request() in the mmccase (rather than)
    chaining calls in the latter, we avoid unlocking the sc in the mmccam
    case and have a consistent call path from both; also removing the
    mmccam #ifdef from dwmmc_request() brings more clarity.
    In dwmmc_next_operation() enhance the panic/error messages with
    some extra information and assert that we come in with a cam pinfo
    on CAM_ACTIVE_INDEX.
    
    MFC after:      3 days
    Reviewed by:    manu
    Differential Revision: https://reviews.freebsd.org/D51628
---
 sys/dev/mmc/host/dwmmc.c | 83 +++++++++++++++++++++++++-----------------------
 1 file changed, 44 insertions(+), 39 deletions(-)

diff --git a/sys/dev/mmc/host/dwmmc.c b/sys/dev/mmc/host/dwmmc.c
index 57992571982c..a422d86d6034 100644
--- a/sys/dev/mmc/host/dwmmc.c
+++ b/sys/dev/mmc/host/dwmmc.c
@@ -315,20 +315,11 @@ static void
 dwmmc_cmd_done(struct dwmmc_softc *sc)
 {
 	struct mmc_command *cmd;
-#ifdef MMCCAM
-	union ccb *ccb;
-#endif
 
-#ifdef MMCCAM
-	ccb = sc->ccb;
-	if (ccb == NULL)
-		return;
-	cmd = &ccb->mmcio.cmd;
-#else
+	DWMMC_ASSERT_LOCKED(sc);
+
 	cmd = sc->curcmd;
-#endif
-	if (cmd == NULL)
-		return;
+	KASSERT(cmd != NULL, ("%s: sc %p curcmd %p == NULL", __func__, sc, cmd));
 
 	if (cmd->flags & MMC_RSP_PRESENT) {
 		if (cmd->flags & MMC_RSP_136) {
@@ -350,15 +341,17 @@ dwmmc_tasklet(struct dwmmc_softc *sc)
 {
 	struct mmc_command *cmd;
 
+	DWMMC_ASSERT_LOCKED(sc);
+
 	cmd = sc->curcmd;
-	if (cmd == NULL)
-		return;
+	KASSERT(cmd != NULL, ("%s: sc %p curcmd %p == NULL", __func__, sc, cmd));
 
 	if (!sc->cmd_done)
 		return;
 
 	if (cmd->error != MMC_ERR_NONE || !cmd->data) {
 		dwmmc_next_operation(sc);
+
 	} else if (cmd->data && sc->dto_rcvd) {
 		if ((cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK ||
 		     cmd->opcode == MMC_READ_MULTIPLE_BLOCK) &&
@@ -383,6 +376,7 @@ dwmmc_intr(void *arg)
 	DWMMC_LOCK(sc);
 
 	cmd = sc->curcmd;
+	KASSERT(cmd != NULL, ("%s: sc %p curcmd %p == NULL", __func__, sc, cmd));
 
 	/* First handle SDMMC controller interrupts */
 	reg = READ4(sc, SDMMC_MINTSTS);
@@ -1093,6 +1087,9 @@ dwmmc_start_cmd(struct dwmmc_softc *sc, struct mmc_command *cmd)
 	uint32_t cmdr;
 
 	dprintf("%s\n", __func__);
+
+	DWMMC_ASSERT_LOCKED(sc);
+
 	sc->curcmd = cmd;
 	data = cmd->data;
 
@@ -1177,18 +1174,22 @@ dwmmc_start_cmd(struct dwmmc_softc *sc, struct mmc_command *cmd)
 static void
 dwmmc_next_operation(struct dwmmc_softc *sc)
 {
-	struct mmc_command *cmd;
-	dprintf("%s\n", __func__);
 #ifdef MMCCAM
 	union ccb *ccb;
+#else
+	struct mmc_request *req;
+#endif
+	struct mmc_command *cmd;
 
+	dprintf("%s\n", __func__);
+	DWMMC_ASSERT_LOCKED(sc);
+
+#ifdef MMCCAM
 	ccb = sc->ccb;
 	if (ccb == NULL)
 		return;
 	cmd = &ccb->mmcio.cmd;
 #else
-	struct mmc_request *req;
-
 	req = sc->req;
 	if (req == NULL)
 		return;
@@ -1205,7 +1206,7 @@ dwmmc_next_operation(struct dwmmc_softc *sc)
 	 * mostly caused by multi-block write command
 	 * followed by single-read.
 	 */
-	while(READ4(sc, SDMMC_STATUS) & (SDMMC_STATUS_DATA_BUSY))
+	while (READ4(sc, SDMMC_STATUS) & (SDMMC_STATUS_DATA_BUSY))
 		continue;
 
 	if (sc->flags & PENDING_CMD) {
@@ -1219,50 +1220,44 @@ dwmmc_next_operation(struct dwmmc_softc *sc)
 		return;
 	}
 
-#ifdef MMCCAM
-	sc->ccb = NULL;
 	sc->curcmd = NULL;
+#ifdef MMCCAM
 	ccb->ccb_h.status =
 		(ccb->mmcio.cmd.error == 0 ? CAM_REQ_CMP : CAM_REQ_CMP_ERR);
 	xpt_done(ccb);
+	sc->ccb = NULL;
 #else
-	sc->req = NULL;
-	sc->curcmd = NULL;
 	req->done(req);
+	sc->req = NULL;
 #endif
 }
 
+#ifndef MMCCAM
 static int
 dwmmc_request(device_t brdev, device_t reqdev, struct mmc_request *req)
 {
 	struct dwmmc_softc *sc;
 
-	sc = device_get_softc(brdev);
-
 	dprintf("%s\n", __func__);
 
-	DWMMC_LOCK(sc);
+	sc = device_get_softc(brdev);
 
-#ifdef MMCCAM
-	sc->flags |= PENDING_CMD;
-#else
+	DWMMC_LOCK(sc);
 	if (sc->req != NULL) {
 		DWMMC_UNLOCK(sc);
 		return (EBUSY);
 	}
-
 	sc->req = req;
 	sc->flags |= PENDING_CMD;
 	if (sc->req->stop)
 		sc->flags |= PENDING_STOP;
-#endif
-	dwmmc_next_operation(sc);
 
+	dwmmc_next_operation(sc);
 	DWMMC_UNLOCK(sc);
+
 	return (0);
 }
 
-#ifndef MMCCAM
 static int
 dwmmc_get_ro(device_t brdev, device_t reqdev)
 {
@@ -1505,10 +1500,15 @@ dwmmc_cam_request(device_t dev, union ccb *ccb)
 	struct ccb_mmcio *mmcio;
 
 	sc = device_get_softc(dev);
-	mmcio = &ccb->mmcio;
-
 	DWMMC_LOCK(sc);
 
+	KASSERT(ccb->ccb_h.pinfo.index == CAM_ACTIVE_INDEX,
+	    ("%s: ccb %p index %d != CAM_ACTIVE_INDEX: func=%#x %s status %#x\n",
+	    __func__, ccb, ccb->ccb_h.pinfo.index, ccb->ccb_h.func_code,
+	    xpt_action_name(ccb->ccb_h.func_code), ccb->ccb_h.status));
+
+	mmcio = &ccb->mmcio;
+
 #ifdef DEBUG
 	if (__predict_false(bootverbose)) {
 		device_printf(sc->dev, "CMD%u arg %#x flags %#x dlen %u dflags %#x\n",
@@ -1519,16 +1519,21 @@ dwmmc_cam_request(device_t dev, union ccb *ccb)
 #endif
 	if (mmcio->cmd.data != NULL) {
 		if (mmcio->cmd.data->len == 0 || mmcio->cmd.data->flags == 0)
-			panic("data->len = %d, data->flags = %d -- something is b0rked",
-			      (int)mmcio->cmd.data->len, mmcio->cmd.data->flags);
+			panic("%s: data %p data->len = %d, data->flags = %d -- something is b0rked",
+			      __func__, mmcio->cmd.data, (int)mmcio->cmd.data->len, mmcio->cmd.data->flags);
 	}
+
 	if (sc->ccb != NULL) {
-		device_printf(sc->dev, "Controller still has an active command\n");
+		device_printf(sc->dev, "%s: Controller still has an active command: "
+		    "sc->ccb %p new ccb %p\n", __func__, sc->ccb, ccb);
+		DWMMC_UNLOCK(sc);
 		return (EBUSY);
 	}
 	sc->ccb = ccb;
+	sc->flags |= PENDING_CMD;
+
+	dwmmc_next_operation(sc);
 	DWMMC_UNLOCK(sc);
-	dwmmc_request(sc->dev, NULL, NULL);
 
 	return (0);
 }