PCI: hv: Fix a timing issue which causes kdump to fail occasionally
Subsystems (4)
Wei Hu
maintainerPATCH v3 1/1 PCI: hv: Fix a timing issue which causes kdump to fail occasionally
Kdump could fail sometime on Hyper-V guest over Accelerated Network interface. This is because the retry in hv_pci_enter_d0() relies on an asynchronous host event arriving before the guest calls hv_send_resources_allocated(). Fix the problem by moving retry to hv_pci_probe(), removing this dependency and making the calling sequence synchronous.
---
v2: Adding Fixes tag according to Michael Kelley's review comment.
v3: Fix couple typos and reword commit message to make it clearer. Thanks the comments from Bjorn Helgaas.
drivers/pci/controller/pci-hyperv.c | 66 ++++++++++++++--------------- 1 file changed, 32 insertions(+), 34 deletions(-)
drivers/pci/controller/pci-hyperv.c +32 -34
@@ -2759,10 +2759,8 @@ static int hv_pci_enter_d0(struct hv_device *hdev)struct pci_bus_d0_entry *d0_entry;struct hv_pci_compl comp_pkt;struct pci_packet *pkt; -bool retry = true;int ret; -enter_d0_retry:/* * Tell the host that the bus is ready to use, and moved into the * powered-on state. This includes telling the host which region
@@ -2789,38 +2787,6 @@ static int hv_pci_enter_d0(struct hv_device *hdev)if (ret) goto exit; -/* -* In certain case (Kdump) the pci device of interest was -* not cleanly shut down and resource is still held on host -* side, the host could return invalid device status. -* We need to explicitly request host to release the resource -* and try to enter D0 again. -*/ -if (comp_pkt.completion_status <0 && retry) { - retry = false; - -dev_err (&hdev->device,"Retrying D0 Entry\n" ); - -/* -* Hv_pci_bus_exit() calls hv_send_resource_released() -* to free up resources of its child devices. -* In the kdump kernel we need to set the -* wslot_res_allocated to 255 so it scans all child -* devices to release resources allocated in the -* normal kernel before panic happened. -*/ - hbus->wslot_res_allocated =255 ; - - ret =hv_pci_bus_exit (hdev, true); - -if (ret ==0 ) { -kfree (pkt); - goto enter_d0_retry; - } -dev_err (&hdev->device, -"Retrying D0 failed with ret %d\n" , ret); - } -if (comp_pkt.completion_status <0 ) { dev_err(&hdev->device, "PCI Pass-through VSP failed D0 Entry with status %x\n",
@@ -3058,6 +3024,7 @@ static int hv_pci_probe(struct hv_device *hdev,struct hv_pcibus_device *hbus;u16 dom_req, dom;char *name; +bool enter_d0_retry = true;int ret;/*
@@ -3178,11 +3145,42 @@ static int hv_pci_probe(struct hv_device *hdev,if (ret) goto free_fwnode; +retry: ret =hv_pci_query_relations (hdev);if (ret) goto free_irq_domain; ret =hv_pci_enter_d0 (hdev); +/* +* In certain case (Kdump) the pci device of interest was +* not cleanly shut down and resource is still held on host +* side, the host could return invalid device status. +* We need to explicitly request host to release the resource +* and try to enter D0 again. +* The retry should start from hv_pci_query_relations() call. +*/ +if (ret == -EPROTO && enter_d0_retry) { + enter_d0_retry = false; + +dev_err (&hdev->device,"Retrying D0 Entry\n" ); + +/* +* Hv_pci_bus_exit() calls hv_send_resources_released() +* to free up resources of its child devices. +* In the kdump kernel we need to set the +* wslot_res_allocated to 255 so it scans all child +* devices to release resources allocated in the +* normal kernel before panic happened. +*/ + hbus->wslot_res_allocated =255 ; + ret =hv_pci_bus_exit (hdev, true); + +if (ret ==0 ) + goto retry; + +dev_err (&hdev->device, +"Retrying D0 failed with ret %d\n" , ret); + }if (ret) goto free_irq_domain;
Signature · 1 line
2.20.1
From: Wei Hu <[email protected]> Sent: Friday, July 17, 2020 8:48 PM
Quoted 17 lines
To: KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; Dexuan Cui <[email protected]>; Michael Kelley <[email protected]>
Cc: Wei Hu <[email protected]>
Subject: [PATCH v3] PCI: hv: Fix a timing issue which causes kdump to fail occasionallyKdump could fail sometime on Hyper-V guest over Accelerated Network interface. This is because the retry in
hv_pci_enter_d0()relies on an asynchronous host event arriving before the guest callshv_send_resources_allocated(). Fix the problem by moving retry tohv_pci_probe(), removing this dependency and making the calling sequence synchronous.Fixes c81992e7f4aa ("PCI: hv: Retry PCI bus D0 entry on invalid device state")Signed-off-by Wei Hu <[email protected]>
Quoted 8 lines
---
v2: Adding Fixes tag according to Michael Kelley's review comment.
v3: Fix couple typos and reword commit message to make it clearer. Thanks the comments from Bjorn Helgaas.drivers/pci/controller/pci-hyperv.c | 66 ++++++++++++++--------------- 1 file changed, 32 insertions(+), 34 deletions(-)
I gave my Reviewed-by on the earlier v1 of this patch, but reiterating it here:
Quoted 6 lines · Wei Hu
Kdump could fail sometime on Hyper-V guest over Accelerated Network interface. This is because the retry in
hv_pci_enter_d0()relies on an asynchronous host event arriving before the guest callshv_send_resources_allocated(). Fix the problem by moving retry tohv_pci_probe(), removing this dependency and making the calling sequence synchronous.
You have to explain why this code move fixes the problem and you also have to add a comment to the code so that anyone who has to fix it in the future can understand why the code is where you are moving it to and why that's a solution.
Quoted 2 lines
Fixes c81992e7f4aa ("PCI: hv: Retry PCI bus D0 entry on invalid device state")Signed-off-by Wei Hu <[email protected]>
Please carry tags and send patches -in-reply-to the previous version to allow threading.
Thanks,
Lorenzo
Quoted 116 lines
---
v2: Adding Fixes tag according to Michael Kelley's review comment.
v3: Fix couple typos and reword commit message to make it clearer. Thanks the comments from Bjorn Helgaas.drivers/pci/controller/pci-hyperv.c | 66 ++++++++++++++--------------- 1 file changed, 32 insertions(+), 34 deletions(-)drivers/pci/controller/pci-hyperv.c +32
-34@@ -2759,10 +2759,8 @@ static int hv_pci_enter_d0(struct hv_device *hdev)struct pci_bus_d0_entry *d0_entry;struct hv_pci_compl comp_pkt;struct pci_packet *pkt; -bool retry = true;int ret; -enter_d0_retry:/* * Tell the host that the bus is ready to use, and moved into the * powered-on state. This includes telling the host which region @@ -2789,38 +2787,6 @@ static int hv_pci_enter_d0(struct hv_device *hdev)if (ret) goto exit; -/* -* In certain case (Kdump) the pci device of interest was -* not cleanly shut down and resource is still held on host -* side, the host could return invalid device status. -* We need to explicitly request host to release the resource -* and try to enter D0 again. -*/ -if (comp_pkt.completion_status <0 && retry) { - retry = false; - -dev_err (&hdev->device,"Retrying D0 Entry\n" ); - -/* -* Hv_pci_bus_exit() calls hv_send_resource_released() -* to free up resources of its child devices. -* In the kdump kernel we need to set the -* wslot_res_allocated to 255 so it scans all child -* devices to release resources allocated in the -* normal kernel before panic happened. -*/ - hbus->wslot_res_allocated =255 ; - - ret =hv_pci_bus_exit (hdev, true); - -if (ret ==0 ) { -kfree (pkt); - goto enter_d0_retry; - } -dev_err (&hdev->device, -"Retrying D0 failed with ret %d\n" , ret); - } -if (comp_pkt.completion_status <0 ) { dev_err(&hdev->device, "PCI Pass-through VSP failed D0 Entry with status %x\n",@@ -3058,6 +3024,7 @@ static int hv_pci_probe(struct hv_device *hdev,struct hv_pcibus_device *hbus;u16 dom_req, dom;char *name; +bool enter_d0_retry = true;int ret;/* @@ -3178,11 +3145,42 @@ static int hv_pci_probe(struct hv_device *hdev,if (ret) goto free_fwnode; +retry: ret =hv_pci_query_relations (hdev);if (ret) goto free_irq_domain; ret =hv_pci_enter_d0 (hdev); +/* +* In certain case (Kdump) the pci device of interest was +* not cleanly shut down and resource is still held on host +* side, the host could return invalid device status. +* We need to explicitly request host to release the resource +* and try to enter D0 again. +* The retry should start from hv_pci_query_relations() call. +*/ +if (ret == -EPROTO && enter_d0_retry) { + enter_d0_retry = false; + +dev_err (&hdev->device,"Retrying D0 Entry\n" ); + +/* +* Hv_pci_bus_exit() calls hv_send_resources_released() +* to free up resources of its child devices. +* In the kdump kernel we need to set the +* wslot_res_allocated to 255 so it scans all child +* devices to release resources allocated in the +* normal kernel before panic happened. +*/ + hbus->wslot_res_allocated =255 ; + ret =hv_pci_bus_exit (hdev, true); + +if (ret ==0 ) + goto retry; + +dev_err (&hdev->device, +"Retrying D0 failed with ret %d\n" , ret); + }if (ret) goto free_irq_domain;Signature · 1 line
2.20.1
Wei Hu
maintainerQuoted 10 lines
-----Original Message-----
From: Lorenzo Pieralisi <[email protected]>Quoted 6 lines
Kdump could fail sometime on Hyper-V guest over Accelerated Network interface. This is because the retry in
hv_pci_enter_d0()relies on an asynchronous host event arriving before the guest callshv_send_resources_allocated(). Fix the problem by moving retry tohv_pci_probe(), removing this dependency and making the calling sequence synchronous.You have to explain why this code move fixes the problem
How about following commit message:
Kdump could fail sometime on Hyper-V guest over Accelerated Network nterface. This is because the retry in hv_pci_enter_d0() relies on an asynchronous host event arriving before the guest calls hv_send_resources_allocated(). Fix the problem by moving retry to hv_pci_probe() and start the retry from hv_pci_query_relations() call. This causes the host to generate an synchronous event, hence removing this unreliable dependency.
Quoted 4 lines
and you also have to
add a comment to the code so that anyone who has to fix it in the future can understand why the code is where you are moving it to and why that's a solution.
It already has the comment in the code as:
+ * The retry should start from hv_pci_query_relations() call.
It would be a bug if the retry does not start from this according to the host Behaviour. So I think no further explanation would be needed.
Quoted 8 lines
Quoted 4 lines
Fixes c81992e7f4aa ("PCI: hv: Retry PCI bus D0 entry on invaliddevice state")
Signed-off-by Wei Hu <[email protected]>Please carry tags and send patches -in-reply-to the previous version to allow threading.
Do you mean to add review-by tags? If not would you please elaborate what 'carry tags' means? Sorry I am not familiar to this term.
Thanks,
Wei
Quoted 123 lines
Thanks,
Lorenzodrivers/pci/controller/pci-hyperv.c static int hv_pci_enter_d0(struct hv_device
Quoted 15 lines
---
v2: Adding Fixes tag according to Michael Kelley's review comment.
v3: Fix couple typos and reword commit message to make it clearer. Thanks the comments from Bjorn Helgaas.drivers/pci/controller/pci-hyperv.c | 66 ++++++++++++++--------------- 1 file changed, 32 insertions(+), 34 deletions(-)drivers/pci/controller/pci-hyperv.c +0
-0@@ -2759,10 +2759,8 @@ static int hv_pci_enter_d0(struct hv_device*hdev)
Quoted 39 lines
drivers/pci/controller/pci-hyperv.c +0
-28struct pci_bus_d0_entry *d0_entry;struct hv_pci_compl comp_pkt;struct pci_packet *pkt; -bool retry = true;int ret; -enter_d0_retry:/* * Tell the host that the bus is ready to use, and moved into the * powered-on state. This includes telling the host which region @@ -2789,38 +2787,6 @@ static int hv_pci_enter_d0(struct hv_device *hdev) if (ret) goto exit; -/* -* In certain case (Kdump) the pci device of interest was -* not cleanly shut down and resource is still held on host -* side, the host could return invalid device status. -* We need to explicitly request host to release the resource -* and try to enter D0 again. -*/ -if (comp_pkt.completion_status <0 && retry) { - retry = false; - -dev_err (&hdev->device,"Retrying D0 Entry\n" ); - -/* -* Hv_pci_bus_exit() calls hv_send_resource_released() -* to free up resources of its child devices. -* In the kdump kernel we need to set the -* wslot_res_allocated to 255 so it scans all child -* devices to release resources allocated in the -* normal kernel before panic happened. -*/ - hbus->wslot_res_allocated =255 ; - - ret =hv_pci_bus_exit (hdev, true); - -if (ret ==0 ) {drivers/pci/controller/pci-hyperv.c static int hv_pci_enter_d0(struct hv_device
Quoted 10 lines
drivers/pci/controller/pci-hyperv.c +0
-7-kfree (pkt); - goto enter_d0_retry; - } -dev_err (&hdev->device, -"Retrying D0 failed with ret %d\n" , ret); - } -if (comp_pkt.completion_status <0 ) { dev_err(&hdev->device, "PCI Pass-through VSP failed D0 Entry withstatus %x\n", @@
Quoted 54 lines
-3058,6 +3024,7 @@ static int hv_pci_probe(struct
hv_device*hdev,drivers/pci/controller/pci-hyperv.c +32
-1struct hv_pcibus_device *hbus;u16 dom_req, dom;char *name; +bool enter_d0_retry = true;int ret;/* @@ -3178,11 +3145,42 @@ static int hv_pci_probe(struct hv_device *hdev,if (ret) goto free_fwnode; +retry: ret =hv_pci_query_relations (hdev);if (ret) goto free_irq_domain; ret =hv_pci_enter_d0 (hdev); +/* +* In certain case (Kdump) the pci device of interest was +* not cleanly shut down and resource is still held on host +* side, the host could return invalid device status. +* We need to explicitly request host to release the resource +* and try to enter D0 again. +* The retry should start from hv_pci_query_relations() call. +*/ +if (ret == -EPROTO && enter_d0_retry) { + enter_d0_retry = false; + +dev_err (&hdev->device,"Retrying D0 Entry\n" ); + +/* +* Hv_pci_bus_exit() calls hv_send_resources_released() +* to free up resources of its child devices. +* In the kdump kernel we need to set the +* wslot_res_allocated to 255 so it scans all child +* devices to release resources allocated in the +* normal kernel before panic happened. +*/ + hbus->wslot_res_allocated =255 ; + ret =hv_pci_bus_exit (hdev, true); + +if (ret ==0 ) + goto retry; + +dev_err (&hdev->device, +"Retrying D0 failed with ret %d\n" , ret); + }if (ret) goto free_irq_domain; --2.20.1
Quoted 9 lines · Wei Hu
Quoted 9 lines
-----Original Message-----
From: Lorenzo Pieralisi <[email protected]>Quoted 6 lines
Kdump could fail sometime on Hyper-V guest over Accelerated Network interface. This is because the retry in
hv_pci_enter_d0()relies on an asynchronous host event arriving before the guest callshv_send_resources_allocated(). Fix the problem by moving retry tohv_pci_probe(), removing this dependency and making the calling sequence synchronous.
Quoted 6 lines
Quoted 1 line
You have to explain why this code move fixes the problem
How about following commit message:
Kdump could fail sometime on Hyper-V guest over Accelerated Network nterface.
"Interface" and this is irrelevant if you don't explain why the failure is caused by it and not any other piece of software, ie why the failure happens explicitly on the Accelerated Network Interface only.
Quoted 6 lines
This is because the retry in
hv_pci_enter_d0()relies on an asynchronous host event arriving before the guest callshv_send_resources_allocated(). Fix the problem by moving retry tohv_pci_probe()and start the retry fromhv_pci_query_relations()call. This causes the host to generate an synchronous event, hence removing this unreliable dependency.
It is a bit better but I am pretty sure it can be improved, for instance by describing the failure path in relation to the asynchronous notification, in other words what happens when things go wrong.
drivers/pci/controller/pci-hyperv.c static int hv_pci_probe(struct hv_device *hdev, · line 3160
Quoted 7 lines
Quoted 4 lines
and you also have to
add a comment to the code so that anyone who has to fix it in the future can understand why the code is where you are moving it to and why that's a solution.It already has the comment in the code as:
drivers/pci/controller/pci-hyperv.c +1
-0+* The retry should start from hv_pci_query_relations() call.
Explain *why* it should restart from there, it is not that complicated.
Quoted 2 lines
It would be a bug if the retry does not start from this according to the host Behaviour. So I think no further explanation would be needed.
It is needed, add it please.
Quoted 10 lines
Quoted 7 lines
Quoted 3 lines
Fixes c81992e7f4aa ("PCI: hv: Retry PCI bus D0 entry on invaliddevice state")
Signed-off-by Wei Hu <[email protected]>Please carry tags and send patches -in-reply-to the previous version to allow threading.
Do you mean to add review-by tags? If not would you please elaborate what 'carry tags' means? Sorry I am not familiar to this term.
It should not be me who applies Reviewed-by tags added on previous patch versions, it should be you who add it to newer versions.
AKA Michael already reviewed it and I need to see his reviewed-by tag to apply it.
Thanks,
Lorenzo