Re: [PATCH v2 2/3] hyperv: Change hv_root_partition into a function

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 2/20/2025 2:59 PM, MUKESH RATHOR wrote:
> 
> 
> On 2/20/25 14:56, Easwar Hariharan wrote:
>  > On 2/20/2025 1:59 PM, MUKESH RATHOR wrote:
>  >>
>  >>
>  >> On 2/20/25 10:33, Nuno Das Neves wrote:
>  >>   > Introduce hv_current_partition_type to store the partition type
>  >>   > as an enum.
>  >>   >
>  >>   > Right now this is limited to guest or root partition, but there will
>  >>   > be other kinds in future and the enum is easily extensible.
>  >>   >
>  >>   > Set up hv_current_partition_type early in Hyper-V initialization
> with
>  >>   > hv_identify_partition_type(). hv_root_partition() just queries this
>  >>   > value, and shouldn't be called before that.
>  >>   >
>  >>   > Making this check into a function sets the stage for adding a config
>  >>   > option to gate the compilation of root partition code. In
> particular,
>  >>   > hv_root_partition() can be stubbed out always be false if root
>  >>   > partition support isn't desired.
>  >>   >
>  >>   > Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>  >>   > ---
>  >>   >   arch/arm64/hyperv/mshyperv.c       |  2 ++
>  >>   >   arch/x86/hyperv/hv_init.c          | 10 ++++-----
>  >>   >   arch/x86/kernel/cpu/mshyperv.c     | 24 ++------------------
>  >>   >   drivers/clocksource/hyperv_timer.c |  4 ++--
>  >>   >   drivers/hv/hv.c                    | 10 ++++-----
>  >>   >   drivers/hv/hv_common.c             | 35
> +++++++++++++++++++++++++-----
>  >>   >   drivers/hv/vmbus_drv.c             |  2 +-
>  >>   >   drivers/iommu/hyperv-iommu.c       |  4 ++--
>  >>   >   include/asm-generic/mshyperv.h     | 15 +++++++++++--
>  >>   >   9 files changed, 61 insertions(+), 45 deletions(-)
>  >>   >
>  >
>  > <snip>
>  >
>  >>   > @@ -34,8 +34,11 @@
>  >>   >   u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
>  >>   >   EXPORT_SYMBOL_GPL(hv_current_partition_id);
>  >>   >
>  >>   > +enum hv_partition_type hv_current_partition_type;
>  >>   > +EXPORT_SYMBOL_GPL(hv_current_partition_type);
>  >>   > +
>  >>
>  >> nit: if possible and not too late, can we please use more Unix
>  >> style naming, eg, hv_curr_ptid and hv_curr_pt_type rather than this
>  >> long windows style names that causes unnecessary line wraps/splits.
>  >>
>  >> Thanks,
>  >> -Mukesh
>  >>
>  >
>  > Per
> https://docs.kernel.org/process/coding-style.html#naming
>  >
>  > GLOBAL variables (to be used only if you really need them) need to
> have descriptive names,
>  > as do global functions. If you have a function that counts the number
> of active users,
>  > you should call that count_active_users() or similar, you should not
> call it cntusr().
> 
> Thant's hardly a fair comparison. Suggestion was NOT hvptid.
> 
I'm in favor of shortening the names when the abbreviation is common and
therefore still perfectly clear to anyone reading it - e.g. "curr" is
a perfectly acceptable abbreviation of "current", in my view.

I think abbreviating "partition" to "pt" is probably not a good fit for
global variables. Anyone seeing a variable with the word "partition"
(and hv_ prefix) can go look up what a Hyper-V partition is if they don't
know, but "pt" would be completely impenetrable without reading through a
fair amount of the code that uses it to figure out what it refers to.

I think even slightly longer abbreviations like "part", "ptn", "prt", or
"prtn" are not good enough unfortunately... the word "partition" just
doesn't lend itself to abbreviation in an obvious way.

So, for this patch I'm fine with changing it to "hv_curr_partition_type"
which saves a few characters.

Feel free to post a followup for "hv_curr_partition_id" if you like.

Note - For the driver code which isn't as exposed to the rest of the
kernel, I think we can continue to use "pt" or similar to keep the names
shorter.

Thanks
Nuno