This is just an RFC patch because I couldn't figure out why we were
calling halt and reset so I just deleted that.
cdnsp_halt(pdev);
cdnsp_reset(pdev);
This function uses "One Function Cleans up Everything" style and that's
basically impossible to do correctly. It's cleaner to write it with
"clean up the most recent allocation". It's simple to review because
you only have to remember the most recent successful allocation and
verify that the goto matches. You never free anything that wasn't
allocated so if avoids a lot of bugs. Also you can copy and paste the
error handling from here, remove the labels, and add a call to
cdnsp_free_priv_device(pdev) and it auto generates the cdnsp_mem_cleanup()
function.
There are two problems that I see with the original code. If
pdev->dcbaa = dma_alloc_coherent() fails then that leads to a NULL
dereference inside the cdnsp_free_priv_device() function. And if
cdnsp_alloc_priv_device() fails that leads to a double free because
we free pdev->out_ctx.bytes in several places.
---
drivers/usb/cdns3/cdnsp-mem.c | 36 ++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/cdns3/cdnsp-mem.c b/drivers/usb/cdns3/cdnsp-mem.c
index 6a0a12e1f54c..6d3fe72e920e 100644
--- a/drivers/usb/cdns3/cdnsp-mem.c
+++ b/drivers/usb/cdns3/cdnsp-mem.c
@@ -1229,7 +1229,7 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
pdev->dcbaa = dma_alloc_coherent(dev, sizeof(*pdev->dcbaa),
&dma, GFP_KERNEL);
if (!pdev->dcbaa)
- goto mem_init_fail;
+ return -ENOMEM;
memset(pdev->dcbaa, 0, sizeof(*pdev->dcbaa));
pdev->dcbaa->dma = dma;
@@ -1247,17 +1247,19 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
pdev->segment_pool = dma_pool_create("CDNSP ring segments", dev,
TRB_SEGMENT_SIZE, TRB_SEGMENT_SIZE,
page_size);
+ if (!pdev->segment_pool)
+ goto release_dcbaa;
pdev->device_pool = dma_pool_create("CDNSP input/output contexts", dev,
CDNSP_CTX_SIZE, 64, page_size);
+ if (!pdev->device_pool)
+ goto destroy_segment_pool;
- if (!pdev->segment_pool || !pdev->device_pool)
- goto mem_init_fail;
/* Set up the command ring to have one segments for now. */
pdev->cmd_ring = cdnsp_ring_alloc(pdev, 1, TYPE_COMMAND, 0, flags);
if (!pdev->cmd_ring)
- goto mem_init_fail;
+ goto destroy_device_pool;
/* Set the address in the Command Ring Control register */
val_64 = cdnsp_read_64(&pdev->op_regs->cmd_ring);
@@ -1280,11 +1282,11 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
pdev->event_ring = cdnsp_ring_alloc(pdev, ERST_NUM_SEGS, TYPE_EVENT,
0, flags);
if (!pdev->event_ring)
- goto mem_init_fail;
+ goto free_cmd_ring;
ret = cdnsp_alloc_erst(pdev, pdev->event_ring, &pdev->erst, flags);
if (ret)
- goto mem_init_fail;
+ goto free_event_ring;
/* Set ERST count with the number of entries in the segment table. */
val = readl(&pdev->ir_set->erst_size);
@@ -1303,22 +1305,30 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
ret = cdnsp_setup_port_arrays(pdev, flags);
if (ret)
- goto mem_init_fail;
+ goto free_erst;
ret = cdnsp_alloc_priv_device(pdev, GFP_ATOMIC);
if (ret) {
dev_err(pdev->dev,
"Could not allocate cdnsp_device data structures\n");
- goto mem_init_fail;
+ goto free_erst;
}
return 0;
-mem_init_fail:
- dev_err(pdev->dev, "Couldn't initialize memory\n");
- cdnsp_halt(pdev);
- cdnsp_reset(pdev);
- cdnsp_mem_cleanup(pdev);
+free_erst:
+ cdnsp_free_erst(pdev, &pdev->erst);
+free_event_ring:
+ cdnsp_ring_free(pdev, pdev->event_ring);
+free_cmd_ring:
+ cdnsp_ring_free(pdev, pdev->cmd_ring);
+destroy_device_pool:
+ dma_pool_destroy(pdev->device_pool);
+destroy_segment_pool:
+ dma_pool_destroy(pdev->segment_pool);
+release_dcbaa:
+ dma_free_coherent(dev, sizeof(*pdev->dcbaa), pdev->dcbaa,
+ pdev->dcbaa->dma);
return ret;
}
--
2.29.2
