Re: [PATCHv2 04/12] x86/virt/tdx: Add tdx_alloc/free_page() helpers

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

On 6/9/25 19:36, Chao Gao wrote:
>> +static int tdx_alloc_pamt_pages(struct list_head *pamt_pages)
>> +{
>> +	for (int i = 0; i < tdx_nr_pamt_pages(); i++) {
>> +		struct page *page = alloc_page(GFP_KERNEL);
>> +		if (!page)
>> +			goto fail;
> 
> this goto isn't needed. it is used only once. so we can just free the pages and
> return -ENOMEM here.

<shrug>

There's no rule saying that gotos need to be used more than once. It's
idiomatic kernel C to use a goto as an error landing site. In fact, I
*prefer* this because it lets me read the main, non-error-case flow
through the function. Then, at my leisure, I can review the error handling.

This is also, IMNHO, less error-prone to someone adding code and doing a
plain return without freeing the pages.

Third, the goto keeps the indentation down.

So, the suggestion here is well intended, but I think it's flawed in
multiple ways. If you write your code this way (free of one-use gotos),
I won't complain too much. But if you suggest other folks get rid of the
gotos, I'm not super happy.

So, Kirill, do it whatever way you want.

But, Chao, please don't keep suggesting things like this at least in
junk I've got to merge.

>> +	if (tdx_hpa_range_not_free(err))
>> +		return 1;
> 
> I think this needs a comment for the return values 0/1/-EIO above the function.

You and I are in full agreement that this series is gloriously
unencumbered by comments at this point.