On 03-09-2026 22:18, Michael Kelley wrote:
> From: Sahil Chandna <[email protected]> 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 <[email protected]>
>> Suggested-by: Naman Jain <[email protected]>
>> Suggested-by: Michael Kelley <[email protected]>
>> Signed-off-by: Sahil Chandna <[email protected]>
>> ---
>> 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/[email protected]/
>> ---
>> 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
