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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The module pins itself in memory and becomes permanently un-unloadable due to a reference cycle.
- [Medium] seamldr_init() failure propagates to tdx_host_probe() and fails the whole tdx-host module.
--

--- Patch [7]: [PATCH v6 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates ---
commit 805f010d78ca6bd2ca042a5f57d4d4c84f3c2978
Author: Chao Gao <[email protected]>

coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates

This patch introduces a sysfs ABI using the fw_upload framework to allow 
userspace to provide TDX module updates. It explicitly chooses fw_upload
over request_firmware() to give userspace full control over module
versioning and non-linear update policies.

> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index 01f68c390a49e..d4a5528530211 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
[ ... ]
> +static int seamldr_init(struct device *dev)
> +{
> +	struct fw_upload *tdx_fwl;
> +
> +	if (!can_expose_seamldr())
> +		return 0;
> +
> +	tdx_fwl = firmware_upload_register(THIS_MODULE, dev, "tdx_module",
> +					   &tdx_fw_ops, NULL);

[Severity: High]
Passing THIS_MODULE to firmware_upload_register() increments the module's 
reference count. 

Since tdx-host uses a root faux_device without a sysfs unbind mechanism, 
the elevated reference count will cause rmmod to always fail with -EBUSY. 

Does this effectively pin the module in memory permanently and prevent updates 
without rebooting? Should this pass NULL instead of THIS_MODULE to avoid the 
reference cycle?

> +	if (IS_ERR(tdx_fwl))
> +		return PTR_ERR(tdx_fwl);
> +
> +	return devm_add_action_or_reset(dev, seamldr_deinit, tdx_fwl);
> +}

[Severity: Medium]
The v3 changelog mentions "Don't fail the whole tdx-host device if 
seamldr_init() met an error", but returning the errors here propagates them 
back to tdx_host_probe(). 

This causes the entire tdx-host module initialization to fail if the optional 
firmware upload interface fails to register. 

Did this inadvertently revert the v3 design intent? Should this function
ignore these errors and return 0 instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7