| Message ID | 20200507050211.10923-1-weh@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PCI HyperV device error handling | expand |
From: Wei Hu <weh@microsoft.com> Sent: Wednesday, May 6, 2020 10:02 PM > > In some error cases in hv_pci_probe(), allocated resources are not freed. > Fix this by adding a field to keep track of the high water mark for slots > that have resources allocated to them. In case of an error, this high > water mark is used to know which slots have resources that must be released. > Since slots are numbered starting with zero, a value of -1 indicates no > slots have been allocated resources. There may be unused slots in the range > between slot 0 and the high water mark slot, but these slots are already > ignored by the existing code in the allocate and release loops with the call > to get_pcichild_wslot(). > > Signed-off-by: Wei Hu <weh@microsoft.com> > --- > v3: Add detailed explanation of this patch in commit log per Lorenzo > Pieralisi's suggestions. > > drivers/pci/controller/pci-hyperv.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index e15022ff63e3..e6fac0187722 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -480,6 +480,9 @@ struct hv_pcibus_device { > > struct workqueue_struct *wq; > > + /* Highest slot of child device with resources allocated */ > + int wslot_res_allocated; > + > /* hypercall arg, must not cross page boundary */ > struct hv_retarget_device_interrupt retarget_msi_interrupt_params; > > @@ -2847,7 +2850,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev) > struct hv_pci_dev *hpdev; > struct pci_packet *pkt; > size_t size_res; > - u32 wslot; > + int wslot; > int ret; > > size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2) > @@ -2900,6 +2903,8 @@ static int hv_send_resources_allocated(struct hv_device *hdev) > comp_pkt.completion_status); > break; > } > + > + hbus->wslot_res_allocated = wslot; > } > > kfree(pkt); > @@ -2918,10 +2923,10 @@ static int hv_send_resources_released(struct hv_device *hdev) > struct hv_pcibus_device *hbus = hv_get_drvdata(hdev); > struct pci_child_message pkt; > struct hv_pci_dev *hpdev; > - u32 wslot; > + int wslot; > int ret; > > - for (wslot = 0; wslot < 256; wslot++) { > + for (wslot = hbus->wslot_res_allocated; wslot >= 0; wslot--) { > hpdev = get_pcichild_wslot(hbus, wslot); > if (!hpdev) > continue; > @@ -2936,8 +2941,12 @@ static int hv_send_resources_released(struct hv_device *hdev) > VM_PKT_DATA_INBAND, 0); > if (ret) > return ret; > + > + hbus->wslot_res_allocated = wslot - 1; > } > > + hbus->wslot_res_allocated = -1; > + > return 0; > } > > @@ -3037,6 +3046,7 @@ static int hv_pci_probe(struct hv_device *hdev, > if (!hbus) > return -ENOMEM; > hbus->state = hv_pcibus_init; > + hbus->wslot_res_allocated = -1; > > /* > * The PCI bus "domain" is what is called "segment" in ACPI and other > @@ -3136,7 +3146,7 @@ static int hv_pci_probe(struct hv_device *hdev, > > ret = hv_pci_allocate_bridge_windows(hbus); > if (ret) > - goto free_irq_domain; > + goto exit_d0; > > ret = hv_send_resources_allocated(hdev); > if (ret) > @@ -3154,6 +3164,8 @@ static int hv_pci_probe(struct hv_device *hdev, > > free_windows: > hv_pci_free_bridge_windows(hbus); > +exit_d0: > + (void) hv_pci_bus_exit(hdev, true); > free_irq_domain: > irq_domain_remove(hbus->irq_domain); > free_fwnode: > -- > 2.20.1 Reviewed-by: Michael Kelley <mikelley@microsoft.com>
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index e15022ff63e3..e6fac0187722 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -480,6 +480,9 @@ struct hv_pcibus_device { struct workqueue_struct *wq; + /* Highest slot of child device with resources allocated */ + int wslot_res_allocated; + /* hypercall arg, must not cross page boundary */ struct hv_retarget_device_interrupt retarget_msi_interrupt_params; @@ -2847,7 +2850,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev) struct hv_pci_dev *hpdev; struct pci_packet *pkt; size_t size_res; - u32 wslot; + int wslot; int ret; size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2) @@ -2900,6 +2903,8 @@ static int hv_send_resources_allocated(struct hv_device *hdev) comp_pkt.completion_status); break; } + + hbus->wslot_res_allocated = wslot; } kfree(pkt); @@ -2918,10 +2923,10 @@ static int hv_send_resources_released(struct hv_device *hdev) struct hv_pcibus_device *hbus = hv_get_drvdata(hdev); struct pci_child_message pkt; struct hv_pci_dev *hpdev; - u32 wslot; + int wslot; int ret; - for (wslot = 0; wslot < 256; wslot++) { + for (wslot = hbus->wslot_res_allocated; wslot >= 0; wslot--) { hpdev = get_pcichild_wslot(hbus, wslot); if (!hpdev) continue; @@ -2936,8 +2941,12 @@ static int hv_send_resources_released(struct hv_device *hdev) VM_PKT_DATA_INBAND, 0); if (ret) return ret; + + hbus->wslot_res_allocated = wslot - 1; } + hbus->wslot_res_allocated = -1; + return 0; } @@ -3037,6 +3046,7 @@ static int hv_pci_probe(struct hv_device *hdev, if (!hbus) return -ENOMEM; hbus->state = hv_pcibus_init; + hbus->wslot_res_allocated = -1; /* * The PCI bus "domain" is what is called "segment" in ACPI and other @@ -3136,7 +3146,7 @@ static int hv_pci_probe(struct hv_device *hdev, ret = hv_pci_allocate_bridge_windows(hbus); if (ret) - goto free_irq_domain; + goto exit_d0; ret = hv_send_resources_allocated(hdev); if (ret) @@ -3154,6 +3164,8 @@ static int hv_pci_probe(struct hv_device *hdev, free_windows: hv_pci_free_bridge_windows(hbus); +exit_d0: + (void) hv_pci_bus_exit(hdev, true); free_irq_domain: irq_domain_remove(hbus->irq_domain); free_fwnode:
In some error cases in hv_pci_probe(), allocated resources are not freed. Fix this by adding a field to keep track of the high water mark for slots that have resources allocated to them. In case of an error, this high water mark is used to know which slots have resources that must be released. Since slots are numbered starting with zero, a value of -1 indicates no slots have been allocated resources. There may be unused slots in the range between slot 0 and the high water mark slot, but these slots are already ignored by the existing code in the allocate and release loops with the call to get_pcichild_wslot(). Signed-off-by: Wei Hu <weh@microsoft.com> --- v3: Add detailed explanation of this patch in commit log per Lorenzo Pieralisi's suggestions. drivers/pci/controller/pci-hyperv.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)