RE: [PATCH v2] PCI: hv: warn when wait_for_response() waits indefinitely

[Michael Kelley <mhklinux@outlook.com>]

From: Sahil Chandna <sahilchandna@linux.microsoft.com> Sent: Wednesday, September 2, 2026 4:59 AM
> 
> A guest can wait indefinitely in wait_for_response() for the host to
> send either a rescind message or a packet completion. If the
> host does not send either, the guest can remain blocked with no
> diagnostic indicating a reason.
> This was observed during a guest kernel upgrade in which the
> host-side application handling the PCI channel faulted, causing the
> guest to never receive the completion request.
> Add a warning in wait_for_response() when the wait exceeds
> a timeout so that such a hang is visible in the guest's kernel log
> and can be correlated with host-side state.
> 
> Suggested-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>
> Suggested-by: Naman Jain <namjain@linux.microsoft.com>
> Suggested-by: Michael Kelley <mhklinux@outlook.com>
> Signed-off-by: Sahil Chandna <sahilchandna@linux.microsoft.com>
> ---
> Changes since v1:
> - Removed periodic warning to one time warning in 2 minutes
> - Include vmbus relid and stuck PCI msg.
> Link to v1: https://lore.kernel.org/all/20260825051850.2438816-1-sahilchandna@linux.microsoft.com/
> ---
>  drivers/pci/controller/pci-hyperv.c | 48 +++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 89816a2bd7cd..ca9d8efd748c 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1040,19 +1040,40 @@ static void put_pcichild(struct hv_pci_dev *hpdev)
> 
>  /*
>   * There is no good way to get notified from vmbus_onoffer_rescind(),
> - * so let's use polling here, since this is not a hot path.
> + * so let's use polling here, since this is not a hot path. If
> + * wait_for_response() has been polling for PCI_RESPONSE_HANG_TIMEOUT_SEC
> + * without either a rescind or completion, add a warning.
>   */
> +#define PCI_RESPONSE_HANG_TIMEOUT_SEC 120

This all looks good to me and it should work as written. But the code
seems more complex than it needs to be. There are two timers running in
parallel -- the 100 millisecond timer in wait_for_completion_timeout(),
and the larger 120 second timeout for printing the "stuck waiting" message.
A simpler approach would be to count iterations through the "while(true)"
loop. Print the "stuck warning" message when the count is exactly
1200. Print the "late completion" message if the completion occurs and
count is > 1200. Then there's no need to manipulate jiffies and the "warned"
flag isn't needed. Make the loop counter 64-bit so overflow isn't an issue.

Arguably this suggested approach is a little too clever, but it seems clear
enough to be easily understood.

Michael

> +
>  static int wait_for_response(struct hv_device *hdev,
> -			     struct completion *comp)
> +			     struct completion *comp,
> +			     const char *msg_type)
>  {
> +	unsigned long delay = secs_to_jiffies(PCI_RESPONSE_HANG_TIMEOUT_SEC);
> +	u64 timeout = get_jiffies_64() + delay;
> +	bool warned = false;
> +
>  	while (true) {
>  		if (hdev->channel->rescind) {
>  			dev_warn_once(&hdev->device, "The device is gone.\n");
>  			return -ENODEV;
>  		}
> 
> -		if (wait_for_completion_timeout(comp, HZ / 10))
> +		if (wait_for_completion_timeout(comp, HZ / 10)) {
> +			if (warned)
> +				dev_warn(&hdev->device,
> +					 "Late %s completion arrived.\n", msg_type);
>  			break;
> +		}
> +
> +		if (!warned && time_after64(get_jiffies_64(), timeout)) {
> +			dev_err(&hdev->device,
> +				"%s stuck waiting for response, relid = %u\n",
> +				msg_type, hdev->channel->offermsg.child_relid);
> +
> +			warned = true;
> +		}
>  	}
> 
>  	return 0;
> @@ -1518,7 +1539,8 @@ static int hv_read_config_block(struct pci_dev *pdev, void *buf,
>  	if (ret)
>  		return ret;
> 
> -	ret = wait_for_response(hbus->hdev, &comp_pkt.comp_pkt.host_event);
> +	ret = wait_for_response(hbus->hdev, &comp_pkt.comp_pkt.host_event,
> +				"PCI_READ_BLOCK");
>  	if (ret)
>  		return ret;
> 
> @@ -1607,7 +1629,8 @@ static int hv_write_config_block(struct pci_dev *pdev, void *buf,
>  	if (ret)
>  		return ret;
> 
> -	ret = wait_for_response(hbus->hdev, &comp_pkt.host_event);
> +	ret = wait_for_response(hbus->hdev, &comp_pkt.host_event,
> +				"PCI_WRITE_BLOCK");
>  	if (ret)
>  		return ret;
> 
> @@ -2624,7 +2647,8 @@ static struct hv_pci_dev *new_pcichild_device(struct
> hv_pcibus_device *hbus,
>  	if (ret)
>  		goto error;
> 
> -	if (wait_for_response(hbus->hdev, &comp_pkt.host_event))
> +	if (wait_for_response(hbus->hdev, &comp_pkt.host_event,
> +			      "PCI_QUERY_RESOURCE_REQUIREMENTS"))
>  		goto error;
> 
>  	hpdev->desc = *desc;
> @@ -3256,7 +3280,8 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev,
>  				(unsigned long)pkt, VM_PKT_DATA_INBAND,
> 
> 	VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>  		if (!ret)
> -			ret = wait_for_response(hdev, &comp_pkt.host_event);
> +			ret = wait_for_response(hdev, &comp_pkt.host_event,
> +						"PCI_QUERY_PROTOCOL_VERSION");
> 
>  		if (ret) {
>  			dev_err(&hdev->device,
> @@ -3476,7 +3501,8 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
>  			       (unsigned long)pkt, VM_PKT_DATA_INBAND,
>  			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>  	if (!ret)
> -		ret = wait_for_response(hdev, &comp_pkt.host_event);
> +		ret = wait_for_response(hdev, &comp_pkt.host_event,
> +					"PCI_BUS_D0ENTRY");
> 
>  	if (ret)
>  		goto exit;
> @@ -3553,7 +3579,8 @@ static int hv_pci_query_relations(struct hv_device *hdev)
>  	ret = vmbus_sendpacket(hdev->channel, &message, sizeof(message),
>  			       0, VM_PKT_DATA_INBAND, 0);
>  	if (!ret)
> -		ret = wait_for_response(hdev, &comp);
> +		ret = wait_for_response(hdev, &comp,
> +					"PCI_QUERY_BUS_RELATIONS");
> 
>  	/*
>  	 * In the case of fast device addition/removal, it's possible that
> @@ -3644,7 +3671,8 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
>  				VM_PKT_DATA_INBAND,
> 
> 	VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>  		if (!ret)
> -			ret = wait_for_response(hdev, &comp_pkt.host_event);
> +			ret = wait_for_response(hdev, &comp_pkt.host_event,
> +						"PCI_RESOURCE_ASSIGNED");
>  		if (ret)
>  			break;
> 
> --
> 2.53.0