| Message ID | 20250120030947.254930-2-lizhijian@fujitsu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] hw/mem/cxl_type3: Add paired msix_uninit_exclusive_bar() call | expand |
On Mon, 20 Jan 2025 11:09:46 +0800 Li Zhijian <lizhijian@fujitsu.com> wrote: > Address a memory leak issue by ensuring `regs->special_ops` is freed when > `msix_init_exclusive_bar()` encounters an error during CXL Type3 device > initialization. > > Additionally, this patch renames err_address_space_free to err_msix_uninit > for better clarity and logical flow > > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> Hi. The need to reorder the cleanup calls made me look more closely at this patch. Either the order was wrong here or in the previous patch. I think the issue is here. Jonathan > --- > hw/mem/cxl_type3.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 9dad250f56d5..9eb3d0979cf5 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -885,7 +885,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) > /* MSI(-X) Initialization */ > rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL); > if (rc) { > - goto err_address_space_free; > + goto err_free_special_ops; > } > for (i = 0; i < CXL_T3_MSIX_VECTOR_NR; i++) { > msix_vector_use(pci_dev, i); > @@ -899,7 +899,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) > cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table; > cxl_cstate->cdat.private = ct3d; > if (!cxl_doe_cdat_init(cxl_cstate, errp)) { > - goto err_free_special_ops; > + goto err_msix_uninit; > } > > pcie_cap_deverr_init(pci_dev); > @@ -936,9 +936,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) > err_release_cdat: > cxl_doe_cdat_release(cxl_cstate); > err_free_special_ops: > - msix_uninit_exclusive_bar(pci_dev); > g_free(regs->special_ops); > -err_address_space_free: > +err_msix_uninit: > + msix_uninit_exclusive_bar(pci_dev); This reorder doesn't look correct. Should end up I think as err_release_cdata: cxl_doe_cdata_release(cxl_cstate); err_msix_uninit: msix_uninit_eclusive_bar(pci_dev); err_free_special_ops: g_free(regs->special_ops) err_address_space_free: etc. > if (ct3d->dc.host_dc) { > cxl_destroy_dc_regions(ct3d); > address_space_destroy(&ct3d->dc.host_dc_as);
On Tue, 21 Jan 2025 14:58:12 +0000 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Mon, 20 Jan 2025 11:09:46 +0800 > Li Zhijian <lizhijian@fujitsu.com> wrote: > > > Address a memory leak issue by ensuring `regs->special_ops` is freed when > > `msix_init_exclusive_bar()` encounters an error during CXL Type3 device > > initialization. > > > > Additionally, this patch renames err_address_space_free to err_msix_uninit > > for better clarity and logical flow > > > > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> > Hi. > > The need to reorder the cleanup calls made me look more > closely at this patch. Either the order was wrong here or in the previous > patch. I think the issue is here. > > Jonathan > > > --- > > hw/mem/cxl_type3.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > index 9dad250f56d5..9eb3d0979cf5 100644 > > --- a/hw/mem/cxl_type3.c > > +++ b/hw/mem/cxl_type3.c > > @@ -885,7 +885,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) > > /* MSI(-X) Initialization */ > > rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL); > > if (rc) { > > - goto err_address_space_free; > > + goto err_free_special_ops; > > } > > for (i = 0; i < CXL_T3_MSIX_VECTOR_NR; i++) { > > msix_vector_use(pci_dev, i); > > @@ -899,7 +899,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) > > cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table; > > cxl_cstate->cdat.private = ct3d; > > if (!cxl_doe_cdat_init(cxl_cstate, errp)) { > > - goto err_free_special_ops; > > + goto err_msix_uninit; > > } > > > > pcie_cap_deverr_init(pci_dev); > > @@ -936,9 +936,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) > > err_release_cdat: > > cxl_doe_cdat_release(cxl_cstate); > > err_free_special_ops: > > - msix_uninit_exclusive_bar(pci_dev); > > g_free(regs->special_ops); > > -err_address_space_free: > > +err_msix_uninit: > > + msix_uninit_exclusive_bar(pci_dev); > > This reorder doesn't look correct. > > Should end up I think as > err_release_cdata: > cxl_doe_cdata_release(cxl_cstate); > err_msix_uninit: > msix_uninit_eclusive_bar(pci_dev); > err_free_special_ops: > g_free(regs->special_ops) > err_address_space_free: This last label can go away. I've applied your series with order modified as here to my CXL staging git tree. I'll push out later today hopefully. Thanks, Jonathan > etc. > > > if (ct3d->dc.host_dc) { > > cxl_destroy_dc_regions(ct3d); > > address_space_destroy(&ct3d->dc.host_dc_as); > >
On 21/01/2025 23:19, Jonathan Cameron wrote: >>> - msix_uninit_exclusive_bar(pci_dev); >>> g_free(regs->special_ops); >>> -err_address_space_free: >>> +err_msix_uninit: >>> + msix_uninit_exclusive_bar(pci_dev); >> This reorder doesn't look correct. >> >> Should end up I think as >> err_release_cdata: >> cxl_doe_cdata_release(cxl_cstate); >> err_msix_uninit: >> msix_uninit_eclusive_bar(pci_dev); >> err_free_special_ops: >> g_free(regs->special_ops) >> err_address_space_free: > This last label can go away. > > I've applied your series with order modified as here to my CXL staging git > tree. Thank you very much for your assistance. Indeed, their order should be rearranged (I submitted them in the sequence in which I discovered the issues). I'll push out later today hopefully. > > Thanks, > > Jonathan > >> etc.
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 9dad250f56d5..9eb3d0979cf5 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -885,7 +885,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) /* MSI(-X) Initialization */ rc = msix_init_exclusive_bar(pci_dev, CXL_T3_MSIX_VECTOR_NR, 4, NULL); if (rc) { - goto err_address_space_free; + goto err_free_special_ops; } for (i = 0; i < CXL_T3_MSIX_VECTOR_NR; i++) { msix_vector_use(pci_dev, i); @@ -899,7 +899,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table; cxl_cstate->cdat.private = ct3d; if (!cxl_doe_cdat_init(cxl_cstate, errp)) { - goto err_free_special_ops; + goto err_msix_uninit; } pcie_cap_deverr_init(pci_dev); @@ -936,9 +936,9 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp) err_release_cdat: cxl_doe_cdat_release(cxl_cstate); err_free_special_ops: - msix_uninit_exclusive_bar(pci_dev); g_free(regs->special_ops); -err_address_space_free: +err_msix_uninit: + msix_uninit_exclusive_bar(pci_dev); if (ct3d->dc.host_dc) { cxl_destroy_dc_regions(ct3d); address_space_destroy(&ct3d->dc.host_dc_as);
Address a memory leak issue by ensuring `regs->special_ops` is freed when `msix_init_exclusive_bar()` encounters an error during CXL Type3 device initialization. Additionally, this patch renames err_address_space_free to err_msix_uninit for better clarity and logical flow Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> --- hw/mem/cxl_type3.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)