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

Chao Gao
2026-04-01T07:50:22Z

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.
>
>
>