Hi Dan,
>
>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);
> v(pdev);
At first glance it looks like cdnsp_halt can be removed.
I little afraid about cdnsp_reset because it reset some internal
state that could be changed during initialization.
I think that you could move cdnsp_reset just before return instruction.
I will make some test to confirm it.
Thanks,
Pawel
>
>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
