Hi Dan,
Can you send this patch or can I do it for you.
It has to be generated one again on top of Peter Chen for-usb-next branch.
Regards,
Pawel Laszczak
>>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);
>>> cdnsp_reset(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
