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

[Sahil Chandna <sahilchandna@linux.microsoft.com>]

On 03-09-2026 22:18, Michael Kelley wrote:
> 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
> 
Thanks for reviewing. Sure, this makes sense.I will share proposed
implementation in v3

Regards,
Sahil
>> +
>>  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