On Mon, 20 Jan 2025 11:09:46 +0800
Li Zhijian <lizhij...@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 <lizhij...@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);
