Re: [PATCH v1 0/9] drivers: hv: dxgkrnl: Driver overview

From: Nathan Chancellor <nathan@kernel.org>
To: Iouri Tarassov <iourit@linux.microsoft.com>
Cc: kys@microsoft.com, haiyangz@microsoft.com,
	sthemmin@microsoft.com, wei.liu@kernel.org,
	linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
	spronovo@microsoft.com, gregkh@linuxfoundation.org
Subject: Re: [PATCH v1 0/9] drivers: hv: dxgkrnl: Driver overview
Date: Wed, 12 Jan 2022 15:12:19 -0700	[thread overview]
Message-ID: <Yd9SQxTznfHGjuDD@archlinux-ax161> (raw)
In-Reply-To: <cover.1641937419.git.iourit@linux.microsoft.com>

Hi Iouri,

On Wed, Jan 12, 2022 at 11:55:05AM -0800, Iouri Tarassov wrote:
> This is a follow-up on the changes we sent a few months back[1].
> 
> [1] https://lore.kernel.org/lkml/20200814123856.3880009-1-sashal@kernel.org/
> 
> The patches address the feedback, given by Greg KH and other reviewers, contain
> bug fixes, the implementation of asynchronous VM bus messages to the host
> and contain the remaining implementation of our vGPU / Compute hardware
> virtualization support that powers the Windows Subsystem for Linux (WSL) and
> will soon power the Windows Subsystem for Android (WSA).

<snip>

> We're looking forward additional feedback.

I have been including this patch set into my downstream WSL2 kernel that
I build with clang and I noticed an instance of -Wenum-conversion that
is still present in this revision:

In file included from drivers/hv/dxgkrnl/dxgsyncfile.c:21:
drivers/hv/dxgkrnl/dxgvmbus.h:894:26: warning: implicit conversion from enumeration type 'enum dxgkvmb_commandtype' to different enumeration type 'enum dxgkvmb_commandtype_global' [-Wenum-conversion]
        command->command_type   = DXGK_VMBCOMMAND_INVALID;
                                ~ ^~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from drivers/hv/dxgkrnl/dxgvmbus.c:23:
drivers/hv/dxgkrnl/dxgvmbus.h:894:26: warning: implicit conversion from enumeration type 'enum dxgkvmb_commandtype' to different enumeration type 'enum dxgkvmb_commandtype_global' [-Wenum-conversion]
        command->command_type   = DXGK_VMBCOMMAND_INVALID;
                                ~ ^~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from drivers/hv/dxgkrnl/ioctl.c:21:
drivers/hv/dxgkrnl/dxgvmbus.h:894:26: warning: implicit conversion from enumeration type 'enum dxgkvmb_commandtype' to different enumeration type 'enum dxgkvmb_commandtype_global' [-Wenum-conversion]
        command->command_type   = DXGK_VMBCOMMAND_INVALID;
                                ~ ^~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

This comes from command_vgpu_to_host_init0(), which does not actually
appear to be used anywhere:

$ rg command_vgpu_to_host_init0
drivers/hv/dxgkrnl/dxgvmbus.h
891:static inline void command_vgpu_to_host_init0(struct dxgkvmb_command_vm_to_host

Please consider cleaning this up in a future revision so that clang
builds stay clean :)

I happened to notice there was another function that looks very similar
to command_vgpu_to_host_init0(), command_vm_to_host_init0(), which is
also unused.  This was hidden because it is marked as "static inline" in
a .c file, which should generally be avoided; I would recommend
replacing all instances of "static inline" with just "static". The
compiler will still inline it if it feels it is worthwhile. Doing this
reveals one other unused function, is_empty():

$ sed -i 's/static inline /static /g' drivers/hv/dxgkrnl/*.c

$ make -skj"$(nproc)" LLVM=1 drivers/hv/dxgkrnl/
drivers/hv/dxgkrnl/hmgr.c:167:13: warning: unused function 'is_empty' [-Wunused-function]
static bool is_empty(struct hmgrtable *table)
            ^
1 warning generated.
drivers/hv/dxgkrnl/dxgvmbus.c:234:13: warning: unused function 'command_vm_to_host_init0' [-Wunused-function]
static void command_vm_to_host_init0(struct dxgkvmb_command_vm_to_host
            ^
1 warning generated.

Cheers,
Nathan