From: Dave Hansen <dave.hansen@linux.intel.com>
To: dave.hansen@linux.intel.com
Subject: Re: [PATCH v9 10/23] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
In-Reply-To: <20260513151045.1420990-11-chao.gao@intel.com>
References: <20260513151045.1420990-11-chao.gao@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

[Draft review generated by x86-maintainer-agent applying Dave_Hansen.txt
 rules to the v9 series.  Strip this banner before sending.]

> +static enum fw_upload_err tdx_fw_write(struct fw_upload *fwl, const u8 *data,
> +				       u32 offset, u32 size, u32 *written)
> +{
> +	int ret;
> +
> +	ret = seamldr_install_module(data, size);
> +	switch (ret) {
> +	case 0:
> +		*written = size;
> +		return FW_UPLOAD_ERR_NONE;
> +	default:
> +		return FW_UPLOAD_ERR_FW_INVALID;
> +	}
> +}

A switch with one case and a default is just an if/else with extra
syntax.  Either keep it as an if/else here, or note in a comment that
later patches in the series add more cases (patch 20 adds -EBUSY ->
FW_UPLOAD_ERR_BUSY).  Pre-emptively answering "why is this a switch"
is exactly Dave_Hansen.txt RULE 17.

> +/**
> + * seamldr_install_module - Install a new TDX module.
> + ...
> + */
> +int seamldr_install_module(const u8 *data, u32 size)
> +{
> +	/* TODO: Update TDX module here */
> +	return 0;
> +}

Subject line says "Implement firmware upload sysfs ABI for TDX module
updates" and the body of seamldr_install_module() is a /* TODO */ that
returns success.  The ABI side is honest -- the upload framework is
wired up -- but the install side here is a stub.  Tightening the
subject ("Wire up fw_upload framework for TDX module updates" or
"Plumb fw_upload to seamldr_install_module()") would let a reader
glance at the commit and know what's actually inside.
Dave_Hansen.txt RULE 1.

The TL;DR + Long Version structure of the commit message is the right
shape and addresses what I asked for on v8 -- thanks.

The follow-up note about .poll_complete() optional (handled
separately) is the right way to do it -- park the follow-up and
keep this patch focused.

The select FW_LOADER / FW_UPLOAD in Kconfig is the right thing here
(building this without those would silently skip the upload glue
and the user would see no /sys/.../firmware/tdx_module directory).