From: Dave Hansen <dave.hansen@linux.intel.com>
To: dave.hansen@linux.intel.com
Subject: Re: [PATCH v9 01/23] x86/virt/tdx: Consolidate TDX global initialization states
In-Reply-To: <20260513151045.1420990-2-chao.gao@intel.com>
References: <20260513151045.1420990-2-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.]

> +struct tdx_module_state {
> +	bool initialized;
> +	bool sysinit_done;
> +	int sysinit_ret;
> +};
> +
> +static struct tdx_module_state tdx_module_state;

Two small things on this one.

1. The name (Dave_Hansen.txt RULE 10).  These three fields are
   bookkeeping for the *initialization* flow, not "the state of the
   module".  Calling it `tdx_module_state` invites a future reader (or
   developer) to dump the module's runtime status into it, which is
   not what you want.  Suggest `tdx_init_state` or
   `tdx_module_init_state`.

2. The struct and the variable share an identifier.  Kernel-legal, but
   when you grep tdx_module_state you can no longer tell type uses
   from instance uses.  Renaming the variable
   (e.g. tdx_module_init_state.{initialized,sysinit_done,sysinit_ret})
   reads better and pairs with point (1).

The changelog is clear about *why* this consolidation is happening
(RULE 1) -- specifically the "easy to miss when a new state is added"
sentence is the whole justification and it's exactly the right place.

Drop of __ro_after_init: also called out in the changelog.  Good.

With the naming addressed:

Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>  [conditional]