Re: [PATCH v7 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates

[Chao Gao <chao.gao@intel.com>]

On Tue, Mar 31, 2026 at 08:11:22AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> +static enum fw_upload_err tdx_fw_poll_complete(struct fw_upload *fwl)
>> +{
>> +	/*
>> +	 * TDX module updates are completed in the previous phase
>> +	 * (tdx_fw_write()). If any error occurred, the previous phase
>> +	 * would return an error code to abort the update process. In
>> +	 * other words, reaching this point means the update succeeded.
>> +	 */
>> +	return FW_UPLOAD_ERR_NONE;
>> +}
>> +
>
>This will be the seventh 'fw_upload_ops' and the third that has this
>pattern of not needing a ->poll_complete() implementation. It seems like
>allowing a NULL ->poll_complete or having a common stub for this pattern
>would be worthwhile.

Thanks for this suggestion. allowing a NULL ->poll_complete looks good.

I will post a separate series for this. the core change would be:

From b0d02c337db9df040b702f57a28e960da04bd9e3 Mon Sep 17 00:00:00 2001
From: Chao Gao <chao.gao@intel.com>
Date: Tue, 31 Mar 2026 22:17:36 -0700
Subject: [PATCH] firmware_loader: Make fw_upload_ops::poll_complete() optional

mpfs-auto-update.c and thp7312.c implement poll_complete() as a trivial
stub that simply returns FW_UPLOAD_ERR_NONE, because their write() is
synchronous and the upload completes before fw_upload_ops::write() returns.

Make poll_complete() optional so that drivers with synchronous write()
don't need to duplicate this trivial stub. When the callback is not
provided, assume the upload has completed and simply return
FW_UPLOAD_ERR_NONE.

An alternative would be defining and exporting a common stub, but
exporting a stub seems unnecessary and differs from the optional
fw_upload_ops::cleanup() handling.

Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Link: https://lore.kernel.org/kvm/01fc8946-eb84-46fa-9458-f345dd3f6033@intel.com/
---
 drivers/base/firmware_loader/sysfs_upload.c | 12 +++++++++---
 include/linux/firmware.h                    |  2 +-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
index f59a7856934c..06788a6d7227 100644
--- a/drivers/base/firmware_loader/sysfs_upload.c
+++ b/drivers/base/firmware_loader/sysfs_upload.c
@@ -158,6 +158,13 @@ static void fw_upload_prog_complete(struct fw_upload_priv *fwlp)
	mutex_unlock(&fwlp->lock);
 }
 
+static enum fw_upload_err fw_upload_poll_complete(struct fw_upload_priv *fwlp)
+{
+	if (!fwlp->ops->poll_complete)
+		return FW_UPLOAD_ERR_NONE;
+	return fwlp->ops->poll_complete(fwlp->fw_upload);
+}
+
 static void fw_upload_main(struct work_struct *work)
 {
	struct fw_upload_priv *fwlp;
@@ -197,7 +204,7 @@ static void fw_upload_main(struct work_struct *work)
	}
 
	fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PROGRAMMING);
-	ret = fwlp->ops->poll_complete(fwl);
+	ret = fw_upload_poll_complete(fwlp);
	if (ret != FW_UPLOAD_ERR_NONE)
		fw_upload_set_error(fwlp, ret);
 
@@ -306,8 +313,7 @@ firmware_upload_register(struct module *module, struct device *parent,
	if (!name || name[0] == '\0')
		return ERR_PTR(-EINVAL);
 
-	if (!ops || !ops->cancel || !ops->prepare ||
-	    !ops->write || !ops->poll_complete) {
+	if (!ops || !ops->cancel || !ops->prepare || !ops->write) {
		dev_err(parent, "Attempt to register without all required ops\n");
		return ERR_PTR(-EINVAL);
	}
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index aae1b85ffc10..c320e8b3e708 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -57,7 +57,7 @@ struct fw_upload {
  *			  size written or a negative error code. The write()
  *			  op will be called repeatedly until all data is
  *			  written.
- * @poll_complete:	  Required: Check for the completion of the
+ * @poll_complete:	  Optional: Check for the completion of the
  *			  HW authentication/programming process.
  * @cancel:		  Required: Request cancellation of update. This op
  *			  is called from the context of a different kernel
-- 
2.47.3


>
>I also don't think you need to be that verbose. This would be fine:
>
>	/*
>	 * The upload completed during tdx_fw_write().
>	 * Never poll for completion.
>	 */

Sure. thanks.
>
>
>