Re: [RESEND PATCH 4/6] drm_hyperv: Remove support for synth video protocol of old Hyper-V hosts

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The static inline function `hv_drm_version_ge()` and the `synthvid_version` field in `struct hv_drm_device` are left behind as dead code.

Pre-existing issues:
- [High] Timeout of VMBus resolution request leaves the channel open, allowing delayed responses to corrupt memory and desynchronize wait completions.
--

--- Patch [4]: [RESEND PATCH 4/6] drm_hyperv: Remove support for synth video protocol of old Hyper-V hosts ---
commit 942daa19258c15938630a14160717e1dd6e0fbc1
Author: Michael Kelley <[email protected]>

drm_hyperv: Remove support for synth video protocol of old Hyper-V hosts

This commit removes support for older synthetic video protocols, as running
Linux on Hyper-V hosts earlier than WS2016 and Windows 10 is no longer
supported. It simplifies the protocol version negotiation to only request
the WIN10 version.

> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> index f0ef627b4898a..cea1ea2595cd6 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c

[ ... ]

> @@ -581,11 +570,9 @@ int hv_drm_connect_vsp(struct hv_device *hdev)
>  
>  	hv->screen_depth = SYNTHVID_DEPTH_WIN8;
>  
> -	if (hv_drm_version_ge(hv->synthvid_version, SYNTHVID_VERSION_WIN10)) {
> -		ret = hv_drm_get_supported_resolution(hdev);
> -		if (ret)
> -			drm_err(dev, "Failed to get supported resolution from host, use default\n");
> -	}
> +	ret = hv_drm_get_supported_resolution(hdev);

[Severity: Low]
Since the only caller to hv_drm_version_ge() and the only reader of 
hv->synthvid_version are removed by this patch, should both the static 
inline function in hyperv_drm_proto.c and the struct field in 
hyperv_drm.h be removed as well to avoid leaving dead code behind?

> +	if (ret)
> +		drm_err(dev, "Failed to get supported resolution from host, use default\n");

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the error
handling here.

If hv_drm_get_supported_resolution() times out waiting for a response,
the code logs an error but execution continues without closing the
channel or returning an error.

If the host sends a delayed response later, wouldn't it trigger
hv_drm_receive_sub() and asynchronously overwrite hv->init_buf while
subsequent requests are actively building a new request in that shared
memory? It looks like this might also spuriously trigger complete(&hv->wait)
and desynchronize the state.

Could this timeout be handled by returning an error or resetting the
channel to prevent data corruption?

>  
>  	if (!hv->screen_width_max) {
>  		hv->screen_width_max = SYNTHVID_WIDTH_WIN8;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4