ufshci: tolerate partially constructed queues in SDB teardown

When attach fails, ufshci_req_sdb_destroy() runs on a partially
constructed queue, and it runs twice: once from the construct error
path and once from the controller destructor.

Make that safe: NULL-check each resource before freeing it and clear
the pointer afterwards, so a second call finds nothing to do. The
construct error label no longer frees the command descriptors itself,
which fixes a double free of ucd_bus_addr. Also destroy the payload
DMA tag, which was previously leaked. Drop the mtx_initialized()
checks: the locks are always set up before any failure path can reach
the destroy.

Attach can also fail before the queues were constructed at all. The
destructor would then call a NULL qops.destroy pointer, so skip the
destroy when the queue was never set up.

Sponsored by:           Samsung Electronics
Reviewed by:		imp (mentor)
Differential Revision:	https://reviews.freebsd.org/D58660

2 files changed, 34 insertions, 18 deletions

diff --git a/sys/dev/ufshci/ufshci_req_queue.c b/sys/dev/ufshci/ufshci_req_queue.c
index 371b9c520979..d8d86070c18c 100644
--- a/sys/dev/ufshci/ufshci_req_queue.c
+++ b/sys/dev/ufshci/ufshci_req_queue.c
@@ -72,6 +72,10 @@ ufshci_utmr_req_queue_construct(struct ufshci_controller *ctrlr)
 void
 ufshci_utmr_req_queue_destroy(struct ufshci_controller *ctrlr)
 {
+	/* Attach may fail before the queue ops are set up. */
+	if (ctrlr->task_mgmt_req_queue.qops.destroy == NULL)
+		return;
+
 	ctrlr->task_mgmt_req_queue.qops.destroy(ctrlr,
 	    &ctrlr->task_mgmt_req_queue);
 }
@@ -114,6 +118,10 @@ ufshci_utr_req_queue_construct(struct ufshci_controller *ctrlr)
 void
 ufshci_utr_req_queue_destroy(struct ufshci_controller *ctrlr)
 {
+	/* Attach may fail before the queue ops are set up. */
+	if (ctrlr->transfer_req_queue.qops.destroy == NULL)
+		return;
+
 	ctrlr->transfer_req_queue.qops.destroy(ctrlr,
 	    &ctrlr->transfer_req_queue);
 }
diff --git a/sys/dev/ufshci/ufshci_req_sdb.c b/sys/dev/ufshci/ufshci_req_sdb.c
index cc99b645331c..c7bf033ddc37 100644
--- a/sys/dev/ufshci/ufshci_req_sdb.c
+++ b/sys/dev/ufshci/ufshci_req_sdb.c
@@ -22,10 +22,16 @@ ufshci_req_sdb_cmd_desc_destroy(struct ufshci_req_queue *req_queue)
 	struct ufshci_tracker *tr;
 	int i;
 
-	for (i = 0; i < req_queue->num_trackers; i++) {
-		tr = hwq->act_tr[i];
-		bus_dmamap_destroy(req_queue->dma_tag_payload,
-		    tr->payload_dma_map);
+	if (req_queue->dma_tag_payload != NULL) {
+		for (i = 0; i < req_queue->num_trackers; i++) {
+			tr = hwq->act_tr[i];
+			if (tr->payload_dma_map != NULL)
+				bus_dmamap_destroy(req_queue->dma_tag_payload,
+				    tr->payload_dma_map);
+		}
+
+		bus_dma_tag_destroy(req_queue->dma_tag_payload);
+		req_queue->dma_tag_payload = NULL;
 	}
 
 	if (req_queue->ucd) {
@@ -42,6 +48,7 @@ ufshci_req_sdb_cmd_desc_destroy(struct ufshci_req_queue *req_queue)
 	}
 
 	free(req_queue->hwq->ucd_bus_addr, M_UFSHCI);
+	req_queue->hwq->ucd_bus_addr = NULL;
 }
 
 static void
@@ -145,7 +152,6 @@ ufshci_req_sdb_cmd_desc_construct(struct ufshci_req_queue *req_queue,
 
 	return (0);
 out:
-	ufshci_req_sdb_cmd_desc_destroy(req_queue);
 	return (ENOMEM);
 }
 
@@ -297,24 +303,27 @@ void
 ufshci_req_sdb_destroy(struct ufshci_controller *ctrlr,
     struct ufshci_req_queue *req_queue)
 {
-	struct ufshci_hw_queue *hwq = &req_queue->hwq[UFSHCI_SDB_Q];
-	struct ufshci_tracker *tr;
+	struct ufshci_hw_queue *hwq;
 	int i;
 
+	if (req_queue->hwq == NULL)
+		return;
+
+	hwq = &req_queue->hwq[UFSHCI_SDB_Q];
+
 	mtx_lock(&hwq->recovery_lock);
 	hwq->timer_armed = false;
 	mtx_unlock(&hwq->recovery_lock);
 	callout_drain(&hwq->timer);
 
-	if (!req_queue->is_task_mgmt)
-		ufshci_req_sdb_cmd_desc_destroy(&ctrlr->transfer_req_queue);
+	if (hwq->act_tr != NULL) {
+		if (!req_queue->is_task_mgmt)
+			ufshci_req_sdb_cmd_desc_destroy(
+			    &ctrlr->transfer_req_queue);
 
-	for (i = 0; i < req_queue->num_trackers; i++) {
-		tr = hwq->act_tr[i];
-		free(tr, M_UFSHCI);
-	}
+		for (i = 0; i < req_queue->num_trackers; i++)
+			free(hwq->act_tr[i], M_UFSHCI);
 
-	if (hwq->act_tr) {
 		free(hwq->act_tr, M_UFSHCI);
 		hwq->act_tr = NULL;
 	}
@@ -331,12 +340,11 @@ ufshci_req_sdb_destroy(struct ufshci_controller *ctrlr,
 		hwq->dma_tag_queue = NULL;
 	}
 
-	if (mtx_initialized(&hwq->recovery_lock))
-		mtx_destroy(&hwq->recovery_lock);
-	if (mtx_initialized(&hwq->qlock))
-		mtx_destroy(&hwq->qlock);
+	mtx_destroy(&hwq->recovery_lock);
+	mtx_destroy(&hwq->qlock);
 
 	free(req_queue->hwq, M_UFSHCI);
+	req_queue->hwq = NULL;
 }
 
 struct ufshci_hw_queue *