>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.
It works correct, you can remove cdnsp_halt and move cndsp_reset
as the last instruction before return.
Probably cdnsp_reset also is not required but it's good to restore
the controller to the default state after error. Just in case.
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
