Re: [PATCH v9 11/23] x86/virt/seamldr: Allocate and populate a module update request

[Dave Hansen <dave.hansen@intel.com>]

On 5/13/26 08:09, Chao Gao wrote:
>  int seamldr_install_module(const u8 *data, u32 size)
>  {

Can we please do better than 'data' and 'size'? Even s/size/data_len/ is
better. This goes for the *entire* series.

Why not call it 'tdx_image' or '_tdx_image' or '_module_image'?

Second, I'm losing track of where these are in the process. Comments on
functions help. Here's how...

> +	struct seamldr_params *params;
> +	int ret;
> +
> +	params = kzalloc_obj(*params);
> +	if (!params)
> +		return -ENOMEM;

init_seamldr_params() should have a comment like this:

	/*
	 * @data points to a vmalloc()'d 'struct tdx_image'. Transform
	 * it into @params which is the P-SEAMLDR ABI format.
	 */

Then this site can say:

	/* Populate 'params' from 'data' */

To at least show the flow of the code. Gunk flows from data=>params.
It's *NOT* otherwise obvious what's goign on here.

Please go through the whole series and make sure you're doing stuff like
this. Comment functions. Say what they are doing at a high level.
Comment their call sites when what they are doing is not clear.

> +	ret = init_seamldr_params(params, data, size);
> +	if (ret)
> +		goto out;
> +
>  	/* TODO: Update TDX module here */
> -	return 0;
> +out:
> +	kfree(params);
> +	return ret;
>  }