Change in ...coreboot[master]: libpayload: ahci: Prevent memory leaks when failing on init

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32778

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

libpayload: ahci: Prevent memory leaks when failing on init

Free several resources when AHCI initialization fails. Note that it is
only safe to free resources when the command engine has stopped, since
otherwise they may still be used for DMA.

Signed-off-by: Jacob Garber 
Change-Id: I6826d79338b26ff9696ab6ac9eb4c59f734687d8
---
M payloads/libpayload/drivers/storage/ahci.c
1 file changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/32778/1

diff --git a/payloads/libpayload/drivers/storage/ahci.c b/payloads/libpayload/drivers/storage/ahci.c
index 40e1008..0aa499f 100644
--- a/payloads/libpayload/drivers/storage/ahci.c
+++ b/payloads/libpayload/drivers/storage/ahci.c
@@ -108,6 +108,10 @@

const int ncs = HBA_CAPS_DECODE_NCS(ctrl->caps);

+	/* Set command list base and received FIS base. */
+	if (ahci_cmdengine_stop(port))
+		return 1;
+
    /* Allocate command list, one command table and received FIS. */
    cmd_t *const cmdlist = memalign(1024, ncs * sizeof(cmd_t));
    cmdtable_t *const cmdtable = memalign(128, sizeof(cmdtable_t));
@@ -120,13 +124,10 @@
    memset((void *)cmdtable, '\0', sizeof(*cmdtable));
    memset((void *)rcvd_fis, '\0', sizeof(*rcvd_fis));

-	/* Set command list base and received FIS base. */
-	if (ahci_cmdengine_stop(port))
-		return 1;
    port->cmdlist_base = virt_to_phys(cmdlist);
    port->frameinfo_base = virt_to_phys(rcvd_fis);
    if (ahci_cmdengine_start(port))
-		return 1;
+		goto _cleanup_ret;
    /* Put port into active state. */
    port->cmd_stat |= HBA_PxCMD_ICC_ACTIVE;

@@ -178,6 +179,8 @@
    /* Clean up (not reached for initialized devices). */
    if (dev)
    	free(dev);
+	/* Only free if stopping succeeds, since otherwise the controller may
+	 * still use the resources for DMA. */
    if (!ahci_cmdengine_stop(port)) {
    	port->cmdlist_base = 0;
    	port->frameinfo_base = 0;

[Jacob Garber (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32778

to look at the new patch set (#2).

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

libpayload: ahci: Prevent memory leaks when failing on init

Free several resources when AHCI initialization fails. Note that it is
only safe to free resources when the command engine has stopped, since
otherwise they may still be used for DMA.

Found-by: Coverity CID 1260719, 1260727, 1261090, 1261098
Signed-off-by: Jacob Garber 
Change-Id: I6826d79338b26ff9696ab6ac9eb4c59f734687d8
---
M payloads/libpayload/drivers/storage/ahci.c
1 file changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/32778/2

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32778 )

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

Patch Set 2:

This is a resurrection of https://review.coreboot.org/c/coreboot/+/18033. I'm not familiar with the details of how the AHCI command engine should run, so I would appreciate any comments or review.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32778 )

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

Patch Set 2: Code-Review+2

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32778 )

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

Patch Set 2: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32778 )

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/32778/2/payloads/libpayload/drivers/storage/... 
File payloads/libpayload/drivers/storage/ahci.c:

https://review.coreboot.org/#/c/32778/2/payloads/libpayload/drivers/storage/... 
PS2, Line 111: 	/* Set command list base and received FIS base. */
This comment applies more to the part of the block that
wasn't moved.

https://review.coreboot.org/#/c/32778/2/payloads/libpayload/drivers/storage/... 
PS2, Line 183: 	 * still use the resources for DMA. */
Please use one of the commenting styles documented here:
https://doc.coreboot.org/coding_style.html#commenting
e.g. drop the asterisk on the left

[Jacob Garber (Code Review)]

Hello Patrick Rudolph, David Hendricks, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32778

to look at the new patch set (#3).

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

libpayload: ahci: Prevent memory leaks when failing on init

Free several resources when AHCI initialization fails. Note that it is
only safe to free resources when the command engine has stopped, since
otherwise they may still be used for DMA.

Found-by: Coverity CID 1260719, 1260727, 1261090, 1261098
Signed-off-by: Jacob Garber 
Change-Id: I6826d79338b26ff9696ab6ac9eb4c59f734687d8
---
M payloads/libpayload/drivers/storage/ahci.c
1 file changed, 6 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/32778/3

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32778 )

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/32778/2/payloads/libpayload/drivers/storage/... 
File payloads/libpayload/drivers/storage/ahci.c:

https://review.coreboot.org/#/c/32778/2/payloads/libpayload/drivers/storage/... 
PS2, Line 111: 	/* Set command list base and received FIS base. */
> This comment applies more to the part of the block that […]

Done

https://review.coreboot.org/#/c/32778/2/payloads/libpayload/drivers/storage/... 
PS2, Line 183: 	 * still use the resources for DMA. */
> Please use one of the commenting styles documented here: […]

Done

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32778 )

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

Patch Set 3: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32778 )

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32778 )

Change subject: libpayload: ahci: Prevent memory leaks when failing on init
......................................................................

libpayload: ahci: Prevent memory leaks when failing on init

Free several resources when AHCI initialization fails. Note that it is
only safe to free resources when the command engine has stopped, since
otherwise they may still be used for DMA.

Found-by: Coverity CID 1260719, 1260727, 1261090, 1261098
Signed-off-by: Jacob Garber 
Change-Id: I6826d79338b26ff9696ab6ac9eb4c59f734687d8
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32778
Tested-by: build bot (Jenkins) 
Reviewed-by: HAOUAS Elyes 
Reviewed-by: Nico Huber 
---
M payloads/libpayload/drivers/storage/ahci.c
1 file changed, 6 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  HAOUAS Elyes: Looks good to me, approved

diff --git a/payloads/libpayload/drivers/storage/ahci.c b/payloads/libpayload/drivers/storage/ahci.c
index 40e1008..f74adaf 100644
--- a/payloads/libpayload/drivers/storage/ahci.c
+++ b/payloads/libpayload/drivers/storage/ahci.c
@@ -108,6 +108,9 @@

const int ncs = HBA_CAPS_DECODE_NCS(ctrl->caps);

+	if (ahci_cmdengine_stop(port))
+		return 1;
+
    /* Allocate command list, one command table and received FIS. */
    cmd_t *const cmdlist = memalign(1024, ncs * sizeof(cmd_t));
    cmdtable_t *const cmdtable = memalign(128, sizeof(cmdtable_t));
@@ -121,12 +124,10 @@
    memset((void *)rcvd_fis, '\0', sizeof(*rcvd_fis));

/* Set command list base and received FIS base. */
-	if (ahci_cmdengine_stop(port))
-		return 1;
    port->cmdlist_base = virt_to_phys(cmdlist);
    port->frameinfo_base = virt_to_phys(rcvd_fis);
    if (ahci_cmdengine_start(port))
-		return 1;
+		goto _cleanup_ret;
    /* Put port into active state. */
    port->cmd_stat |= HBA_PxCMD_ICC_ACTIVE;

@@ -178,6 +179,8 @@
    /* Clean up (not reached for initialized devices). */
    if (dev)
    	free(dev);
+	/* Only free if stopping succeeds, since otherwise the controller may
+	   still use the resources for DMA. */
    if (!ahci_cmdengine_stop(port)) {
    	port->cmdlist_base = 0;
    	port->frameinfo_base = 0;