Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
Re: qwx(4) suspend/hibernate + resume
To:
Theo de Raadt <deraadt@openbsd.org>
Cc:
tech@openbsd.org
Date:
Wed, 21 Feb 2024 22:28:05 +0100

Download raw body.

Thread
On Wed, Feb 21, 2024 at 10:18:25AM -0700, Theo de Raadt wrote:
> Works for multiple suspends on a x13s.

In addition to the previous diff, I propose the following to
uncouple qwx(4) resume from disk-device resume.

I recall us going through similarly careful steps in iwx(4).

Successfully tested with hibernate.

-----------------------------------------------
 cache qwx(4) firmware images in memory across suspend/resume cycles
 
diff 6dc955176d769c398ad467a99b6434d9c54d57b8 2ccfb7ca303c6e380a8a8790fa14b1556eba4167
commit - 6dc955176d769c398ad467a99b6434d9c54d57b8
commit + 2ccfb7ca303c6e380a8a8790fa14b1556eba4167
blob - b1c0c1a5f3627951ac086c6d403fbbd57d8f8b1b
blob + 43753d3d31405116391a7f706fb0735162b062b5
--- sys/dev/ic/qwx.c
+++ sys/dev/ic/qwx.c
@@ -299,6 +299,18 @@ qwx_stop(struct ifnet *ifp)
 	splx(s);
 }
 
+void
+qwx_free_firmware(struct qwx_softc *sc)
+{
+	int i;
+
+	for (i = 0; i < nitems(sc->fw_img); i++) {
+		free(sc->fw_img[i].data, M_DEVBUF, sc->fw_img[i].size);
+		sc->fw_img[i].data = NULL;
+		sc->fw_img[i].size = 0;
+	}
+}
+
 int
 qwx_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 {
@@ -322,7 +334,7 @@ qwx_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 		if (ifp->if_flags & IFF_UP) {
 			if (!(ifp->if_flags & IFF_RUNNING)) {
 				/* Force reload of firmware image from disk. */
-				sc->have_firmware = 0;
+				qwx_free_firmware(sc);
 				err = qwx_init(ifp);
 			}
 		} else {
@@ -8374,21 +8386,35 @@ err_free_req:
 int
 qwx_qmi_load_bdf_qmi(struct qwx_softc *sc, int regdb)
 {
-	u_char *data;
+	u_char *data = NULL;
 	const u_char *boardfw;
-	size_t len, boardfw_len;
+	size_t len = 0, boardfw_len;
 	uint32_t fw_size;
 	int ret = 0, bdf_type;
 #ifdef notyet
 	const uint8_t *tmp;
 	uint32_t file_type;
 #endif
+	int fw_idx = regdb ? QWX_FW_REGDB : QWX_FW_BOARD;
 
-	ret = qwx_core_fetch_bdf(sc, &data, &len, &boardfw, &boardfw_len,
-	    regdb ? ATH11K_REGDB_FILE : ATH11K_BOARD_API2_FILE);
-	if (ret)
-		return ret;
+	if (sc->fw_img[fw_idx].data) {
+		boardfw = sc->fw_img[fw_idx].data;
+		boardfw_len = sc->fw_img[fw_idx].size;
+	} else {
+		ret = qwx_core_fetch_bdf(sc, &data, &len,
+		    &boardfw, &boardfw_len,
+		    regdb ? ATH11K_REGDB_FILE : ATH11K_BOARD_API2_FILE);
+		if (ret)
+			return ret;
 
+		sc->fw_img[fw_idx].data = malloc(boardfw_len, M_DEVBUF,
+		    M_NOWAIT);
+		if (sc->fw_img[fw_idx].data) {
+			memcpy(sc->fw_img[fw_idx].data, boardfw, boardfw_len);
+			sc->fw_img[fw_idx].size = boardfw_len;
+		}
+	}
+
 	if (regdb)
 		bdf_type = ATH11K_QMI_BDF_TYPE_REGDB;
 	else if (boardfw_len >= QWX_SELFMAG &&
@@ -8506,16 +8532,24 @@ qwx_qmi_m3_load(struct qwx_softc *sc)
 	char path[PATH_MAX];
 	int ret;
 
-	ret = snprintf(path, sizeof(path), "%s-%s-%s",
-	    ATH11K_FW_DIR, sc->hw_params.fw.dir, ATH11K_M3_FILE);
-	if (ret < 0 || ret >= sizeof(path))
-		return ENOSPC;
+	if (sc->fw_img[QWX_FW_M3].data) {
+		data = sc->fw_img[QWX_FW_M3].data;
+		len = sc->fw_img[QWX_FW_M3].size;
+	} else {
+		ret = snprintf(path, sizeof(path), "%s-%s-%s",
+		    ATH11K_FW_DIR, sc->hw_params.fw.dir, ATH11K_M3_FILE);
+		if (ret < 0 || ret >= sizeof(path))
+			return ENOSPC;
 
-	ret = loadfirmware(path, &data, &len);
-	if (ret) {
-		printf("%s: could not read %s (error %d)\n",
-		    sc->sc_dev.dv_xname, path, ret);
-		return ret;
+		ret = loadfirmware(path, &data, &len);
+		if (ret) {
+			printf("%s: could not read %s (error %d)\n",
+			    sc->sc_dev.dv_xname, path, ret);
+			return ret;
+		}
+
+		sc->fw_img[QWX_FW_M3].data = data;
+		sc->fw_img[QWX_FW_M3].size = len;
 	}
 
 	if (sc->m3_mem == NULL || QWX_DMA_LEN(sc->m3_mem) < len) {
@@ -8531,7 +8565,6 @@ qwx_qmi_m3_load(struct qwx_softc *sc)
 	}
 
 	memcpy(QWX_DMA_KVA(sc->m3_mem), data, len);
-	free(data, M_DEVBUF, len);
 	return 0;
 }
 
@@ -24709,6 +24742,8 @@ qwx_detach(struct qwx_softc *sc)
 		qwx_dmamem_free(sc->sc_dmat, sc->m3_mem);
 		sc->m3_mem = NULL;
 	}
+
+	qwx_free_firmware(sc);
 }
 
 struct qwx_dmamem *
blob - f7f429fa27e3fc5f63cb9345a9e90f46d5fbf15d
blob + bc97e1e56f9ff5873d23b3782193a2af97f76e00
--- sys/dev/ic/qwxvar.h
+++ sys/dev/ic/qwxvar.h
@@ -1782,7 +1782,14 @@ struct qwx_softc {
 	struct qwx_survey_info	survey[IEEE80211_CHAN_MAX];
 
 	int			attached;
-	int			have_firmware;
+	struct {
+		u_char *data;
+		size_t size;
+	} fw_img[4];
+#define QWX_FW_AMSS	0
+#define QWX_FW_BOARD	1
+#define QWX_FW_M3	2
+#define QWX_FW_REGDB	3
 
 	int			sc_tx_timer;
 	uint32_t		qfullmsk;
blob - 9fd14232402e064ea2c520f63ba8d6afd0b36431
blob + d09d91a129077f0a85e43035530676df61c3afd6
--- sys/dev/pci/if_qwx_pci.c
+++ sys/dev/pci/if_qwx_pci.c
@@ -371,7 +371,7 @@ struct qwx_pci_softc {
 	struct taskq		*mhi_taskq;
 
 	/*
-	 * DMA memory for AMMS.bin firmware image.
+	 * DMA memory for AMSS.bin firmware image.
 	 * This memory must remain available to the device until
 	 * the device is powered down.
 	 */
@@ -3032,23 +3032,31 @@ qwx_mhi_fw_load_handler(struct qwx_pci_softc *psc)
 	u_char *data;
 	size_t len;
 
-	ret = snprintf(amss_path, sizeof(amss_path), "%s-%s-%s",
-	    ATH11K_FW_DIR, sc->hw_params.fw.dir, ATH11K_AMSS_FILE);
-	if (ret < 0 || ret >= sizeof(amss_path))
-		return ENOSPC;
+	if (sc->fw_img[QWX_FW_AMSS].data) {
+		data = sc->fw_img[QWX_FW_AMSS].data;
+		len = sc->fw_img[QWX_FW_AMSS].size;
+	} else {
+		ret = snprintf(amss_path, sizeof(amss_path), "%s-%s-%s",
+		    ATH11K_FW_DIR, sc->hw_params.fw.dir, ATH11K_AMSS_FILE);
+		if (ret < 0 || ret >= sizeof(amss_path))
+			return ENOSPC;
 
-	ret = loadfirmware(amss_path, &data, &len);
-	if (ret) {
-		printf("%s: could not read %s (error %d)\n",
-		    sc->sc_dev.dv_xname, amss_path, ret);
-		return ret;
-	}
+		ret = loadfirmware(amss_path, &data, &len);
+		if (ret) {
+			printf("%s: could not read %s (error %d)\n",
+			    sc->sc_dev.dv_xname, amss_path, ret);
+			return ret;
+		}
 
-	if (len < MHI_DMA_VEC_CHUNK_SIZE) {
-		printf("%s: %s is too short, have only %zu bytes\n",
-		    sc->sc_dev.dv_xname, amss_path, len);
-		free(data, M_DEVBUF, len);
-		return EINVAL;
+		if (len < MHI_DMA_VEC_CHUNK_SIZE) {
+			printf("%s: %s is too short, have only %zu bytes\n",
+			    sc->sc_dev.dv_xname, amss_path, len);
+			free(data, M_DEVBUF, len);
+			return EINVAL;
+		}
+
+		sc->fw_img[QWX_FW_AMSS].data = data;
+		sc->fw_img[QWX_FW_AMSS].size = len;
 	}
 
 	/* Second-stage boot loader sits in the first 512 KB of image. */
@@ -3056,7 +3064,6 @@ qwx_mhi_fw_load_handler(struct qwx_pci_softc *psc)
 	if (ret != 0) {
 		printf("%s: could not load firmware %s\n",
 		    sc->sc_dev.dv_xname, amss_path);
-		free(data, M_DEVBUF, len);
 		return ret;
 	}
 
@@ -3065,6 +3072,7 @@ qwx_mhi_fw_load_handler(struct qwx_pci_softc *psc)
 	if (ret != 0) {
 		printf("%s: could not load firmware %s\n",
 		    sc->sc_dev.dv_xname, amss_path);
+		return ret;
 	}
 
 	while (psc->bhi_ee < MHI_EE_AMSS) {
@@ -3076,11 +3084,8 @@ qwx_mhi_fw_load_handler(struct qwx_pci_softc *psc)
 	if (ret != 0) {
 		printf("%s: device failed to enter AMSS EE\n",
 		    sc->sc_dev.dv_xname);
-		free(data, M_DEVBUF, len);
-		return ret;
 	}
 
-	free(data, M_DEVBUF, len);
 	return ret;
 }