[RFC] usb: cdnsp: fix error handling in cdnsp_mem_init()

[RFC] usb: cdnsp: fix error handling in cdnsp_mem_init()

[Dan Carpenter]

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

RE: [RFC] usb: cdnsp: fix error handling in cdnsp_mem_init()

[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);
>	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

RE: [RFC] usb: cdnsp: fix error handling in cdnsp_mem_init()

[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);
>>	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

RE: [RFC] usb: cdnsp: fix error handling in cdnsp_mem_init()

[Pawel Laszczak]

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