diff mbox series

[v1] firmware: microchip: auto-update: fix poll_complete() to not report spurious timeout errors

Message ID 20241016-slouching-excess-d7a62fc926a1@spud (mailing list archive)
State Accepted
Headers show
Series [v1] firmware: microchip: auto-update: fix poll_complete() to not report spurious timeout errors | expand

Checks

Context Check Description
conchuod/vmtest-for-next-PR success PR summary
conchuod/patch-1-test-1 success .github/scripts/patches/tests/build_rv32_defconfig.sh took 144.07s
conchuod/patch-1-test-2 success .github/scripts/patches/tests/build_rv64_clang_allmodconfig.sh took 1338.22s
conchuod/patch-1-test-3 success .github/scripts/patches/tests/build_rv64_gcc_allmodconfig.sh took 1549.35s
conchuod/patch-1-test-4 success .github/scripts/patches/tests/build_rv64_nommu_k210_defconfig.sh took 20.98s
conchuod/patch-1-test-5 success .github/scripts/patches/tests/build_rv64_nommu_virt_defconfig.sh took 23.11s
conchuod/patch-1-test-6 warning .github/scripts/patches/tests/checkpatch.sh took 0.46s
conchuod/patch-1-test-7 success .github/scripts/patches/tests/dtb_warn_rv64.sh took 42.14s
conchuod/patch-1-test-8 success .github/scripts/patches/tests/header_inline.sh took 0.00s
conchuod/patch-1-test-9 success .github/scripts/patches/tests/kdoc.sh took 0.51s
conchuod/patch-1-test-10 success .github/scripts/patches/tests/module_param.sh took 0.01s
conchuod/patch-1-test-11 success .github/scripts/patches/tests/verify_fixes.sh took 0.02s
conchuod/patch-1-test-12 success .github/scripts/patches/tests/verify_signedoff.sh took 0.03s

Commit Message

Conor Dooley Oct. 16, 2024, 4:35 p.m. UTC
From: Conor Dooley <conor.dooley@microchip.com>

fw_upload's poll_complete() is really intended for use with
asynchronous write() implementations - or at least those where the
write() loop may terminate without the kernel yet being aware of whether
or not the firmware upload has succeeded. For auto-update, write() is
only ever called once and will only return when uploading has completed,
be that by passing or failing. The core fw_upload code only calls
poll_complete() after the final call to write() has returned.

However, the poll_complete() implementation in the auto-update driver
was written to expect poll_complete() to be called from another context,
and it waits for a completion signalled from write(). Since
poll_complete() is actually called from the same context, after the
write() loop has terminated, wait_for_completion() never sees the
completion get signalled and always times out, causing programming to
always report a failing.

Since write() is full synchronous, and its return value will indicate
whether or not programming passed or failed, poll_complete() serves no
purpose and can be cut down to simply return FW_UPLOAD_ERR_NONE.

Fixes: ec5b0f1193ad4 ("firmware: microchip: add PolarFire SoC Auto Update support")
Reported-by: Jamie Gibbons <jamie.gibbons@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
CC: Conor Dooley <conor.dooley@microchip.com>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: linux-riscv@lists.infradead.org
CC: linux-kernel@vger.kernel.org
---
 drivers/firmware/microchip/mpfs-auto-update.c | 42 ++++---------------
 1 file changed, 7 insertions(+), 35 deletions(-)

Comments

Conor Dooley Oct. 17, 2024, 4:41 p.m. UTC | #1
From: Conor Dooley <conor.dooley@microchip.com>

On Wed, 16 Oct 2024 17:35:06 +0100, Conor Dooley wrote:
> fw_upload's poll_complete() is really intended for use with
> asynchronous write() implementations - or at least those where the
> write() loop may terminate without the kernel yet being aware of whether
> or not the firmware upload has succeeded. For auto-update, write() is
> only ever called once and will only return when uploading has completed,
> be that by passing or failing. The core fw_upload code only calls
> poll_complete() after the final call to write() has returned.
> 
> [...]

Applied to riscv-soc-fixes, thanks!

[1/1] firmware: microchip: auto-update: fix poll_complete() to not report spurious timeout errors
      https://git.kernel.org/conor/c/83beece5aff7

Thanks,
Conor.
diff mbox series

Patch

diff --git a/drivers/firmware/microchip/mpfs-auto-update.c b/drivers/firmware/microchip/mpfs-auto-update.c
index 9ca5ee58edbdf..0f7ec88482022 100644
--- a/drivers/firmware/microchip/mpfs-auto-update.c
+++ b/drivers/firmware/microchip/mpfs-auto-update.c
@@ -76,14 +76,11 @@ 
 #define AUTO_UPDATE_INFO_SIZE		SZ_1M
 #define AUTO_UPDATE_BITSTREAM_BASE	(AUTO_UPDATE_DIRECTORY_SIZE + AUTO_UPDATE_INFO_SIZE)
 
-#define AUTO_UPDATE_TIMEOUT_MS		60000
-
 struct mpfs_auto_update_priv {
 	struct mpfs_sys_controller *sys_controller;
 	struct device *dev;
 	struct mtd_info *flash;
 	struct fw_upload *fw_uploader;
-	struct completion programming_complete;
 	size_t size_per_bitstream;
 	bool cancel_request;
 };
@@ -156,19 +153,6 @@  static void mpfs_auto_update_cancel(struct fw_upload *fw_uploader)
 
 static enum fw_upload_err mpfs_auto_update_poll_complete(struct fw_upload *fw_uploader)
 {
-	struct mpfs_auto_update_priv *priv = fw_uploader->dd_handle;
-	int ret;
-
-	/*
-	 * There is no meaningful way to get the status of the programming while
-	 * it is in progress, so attempting anything other than waiting for it
-	 * to complete would be misplaced.
-	 */
-	ret = wait_for_completion_timeout(&priv->programming_complete,
-					  msecs_to_jiffies(AUTO_UPDATE_TIMEOUT_MS));
-	if (!ret)
-		return FW_UPLOAD_ERR_TIMEOUT;
-
 	return FW_UPLOAD_ERR_NONE;
 }
 
@@ -349,33 +333,23 @@  static enum fw_upload_err mpfs_auto_update_write(struct fw_upload *fw_uploader,
 						 u32 offset, u32 size, u32 *written)
 {
 	struct mpfs_auto_update_priv *priv = fw_uploader->dd_handle;
-	enum fw_upload_err err = FW_UPLOAD_ERR_NONE;
 	int ret;
 
-	reinit_completion(&priv->programming_complete);
-
 	ret = mpfs_auto_update_write_bitstream(fw_uploader, data, offset, size, written);
-	if (ret) {
-		err = FW_UPLOAD_ERR_RW_ERROR;
-		goto out;
-	}
+	if (ret)
+		return FW_UPLOAD_ERR_RW_ERROR;
 
-	if (priv->cancel_request) {
-		err = FW_UPLOAD_ERR_CANCELED;
-		goto out;
-	}
+	if (priv->cancel_request)
+		return FW_UPLOAD_ERR_CANCELED;
 
 	if (mpfs_auto_update_is_bitstream_info(data, size))
-		goto out;
+		return FW_UPLOAD_ERR_NONE;
 
 	ret = mpfs_auto_update_verify_image(fw_uploader);
 	if (ret)
-		err = FW_UPLOAD_ERR_FW_INVALID;
+		return FW_UPLOAD_ERR_FW_INVALID;
 
-out:
-	complete(&priv->programming_complete);
-
-	return err;
+	return FW_UPLOAD_ERR_NONE;
 }
 
 static const struct fw_upload_ops mpfs_auto_update_ops = {
@@ -461,8 +435,6 @@  static int mpfs_auto_update_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, ret,
 				     "The current bitstream does not support auto-update\n");
 
-	init_completion(&priv->programming_complete);
-
 	fw_uploader = firmware_upload_register(THIS_MODULE, dev, "mpfs-auto-update",
 					       &mpfs_auto_update_ops, priv);
 	if (IS_ERR(fw_uploader))