Change in flashrom[master]: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/45944 )

Change subject: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow
......................................................................

realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow

Ensure that when bus number and reset params are specified
at the same time are both correctly parsed by get_params().
Also renames the goto err cleanup path to make it clear.

Change-Id: Icb45b1ab39181b0f1a2dec1cce549d30db984936
Signed-off-by: Edward O'Callaghan 
---
M realtek_mst_i2c_spi.c
1 file changed, 4 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/45944/1

diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index e975290..d1f7227 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -416,23 +416,22 @@
    	int bus = (int)strtol(bus_str, &bus_suffix, 10);
    	if (errno != 0 || bus_str == bus_suffix) {
    		msg_perr("%s: Could not convert 'bus'.\n", __func__);
-			goto get_params_done;
+			goto _get_params_failed;
    	}

if (bus < 0 || bus > 255) {
    		msg_perr("%s: Value for 'bus' is out of range(0-255).\n", __func__);
-			goto get_params_done;
+			goto _get_params_failed;
    	}

if (strlen(bus_suffix) > 0) {
    		msg_perr("%s: Garbage following 'bus' value.\n", __func__);
-			goto get_params_done;
+			goto _get_params_failed;
    	}

msg_pinfo("Using i2c bus %i.\n", bus);
    	*i2c_bus = bus;
    	ret = 0;
-		goto get_params_done;
    } else {
    	msg_perr("%s: Bus number not specified.\n", __func__);
    }
@@ -451,7 +450,7 @@
    	*reset = 0; /* Default behaviour is no MCU reset on tear-down. */
    free(reset_str);

-get_params_done:
+_get_params_failed:
    if (bus_str)
    	free(bus_str);

[Edward O'Callaghan (Code Review)]

Hello Sam McNally, build bot (Jenkins), Shiyu Sun,

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

https://review.coreboot.org/c/flashrom/+/45944

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

Change subject: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow
......................................................................

realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow

Ensure that when bus number and reset params are specified
at the same time are both correctly parsed by get_params().
Also renames the goto err cleanup path to make it clear.

Change-Id: Icb45b1ab39181b0f1a2dec1cce549d30db984936
Signed-off-by: Edward O'Callaghan 
Spotted-by: Shiyu Sun 
---
M realtek_mst_i2c_spi.c
1 file changed, 4 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/45944/2

[Sam McNally (Code Review)]

Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45944 )

Change subject: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow
......................................................................

Patch Set 2: Code-Review+2

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/45944 )

Change subject: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow
......................................................................

realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow

Ensure that when bus number and reset params are specified
at the same time are both correctly parsed by get_params().
Also renames the goto err cleanup path to make it clear.

Change-Id: Icb45b1ab39181b0f1a2dec1cce549d30db984936
Signed-off-by: Edward O'Callaghan 
Spotted-by: Shiyu Sun 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/45944
Reviewed-by: Sam McNally 
Tested-by: build bot (Jenkins) 
---
M realtek_mst_i2c_spi.c
1 file changed, 4 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Sam McNally: Looks good to me, approved

diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c
index e975290..d1f7227 100644
--- a/realtek_mst_i2c_spi.c
+++ b/realtek_mst_i2c_spi.c
@@ -416,23 +416,22 @@
    	int bus = (int)strtol(bus_str, &bus_suffix, 10);
    	if (errno != 0 || bus_str == bus_suffix) {
    		msg_perr("%s: Could not convert 'bus'.\n", __func__);
-			goto get_params_done;
+			goto _get_params_failed;
    	}

if (bus < 0 || bus > 255) {
    		msg_perr("%s: Value for 'bus' is out of range(0-255).\n", __func__);
-			goto get_params_done;
+			goto _get_params_failed;
    	}

if (strlen(bus_suffix) > 0) {
    		msg_perr("%s: Garbage following 'bus' value.\n", __func__);
-			goto get_params_done;
+			goto _get_params_failed;
    	}

msg_pinfo("Using i2c bus %i.\n", bus);
    	*i2c_bus = bus;
    	ret = 0;
-		goto get_params_done;
    } else {
    	msg_perr("%s: Bus number not specified.\n", __func__);
    }
@@ -451,7 +450,7 @@
    	*reset = 0; /* Default behaviour is no MCU reset on tear-down. */
    free(reset_str);

-get_params_done:
+_get_params_failed:
    if (bus_str)
    	free(bus_str);

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45944 )

Change subject: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow
......................................................................

Patch Set 3:

Why on Earth was this fast-tracked? I was sleeping when this patch was created and submitted...

https://www.flashrom.org/Development_Guidelines#Merging_to_branches

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45944 )

Change subject: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45944/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/45944/3//COMMIT_MSG@11 
PS3, Line 11: Also renames the goto err cleanup path to make it clear.
It would have been much better to not rename the label in this patch.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45944 )

Change subject: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow
......................................................................

Patch Set 3:

(1 comment)
> Patch Set 3:
>
> Why on Earth was this fast-tracked? I was sleeping when this patch was created and submitted...
>
> https://www.flashrom.org/Development_Guidelines#Merging_to_branches

Sorry that is my bad and responsibility of assuming the triviality of it.

https://review.coreboot.org/c/flashrom/+/45944/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/45944/3//COMMIT_MSG@11 
PS3, Line 11: Also renames the goto err cleanup path to make it clear.
> It would have been much better to not rename the label in this patch.

What was the reasoning of not to rename it if the purpose changed in meaning?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45944 )

Change subject: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45944/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/45944/3//COMMIT_MSG@11 
PS3, Line 11: Also renames the goto err cleanup path to make it clear.
> What was the reasoning of not to rename it if the purpose changed in meaning?

It would have been a single-line fix, which would have been trivial.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/45944 )

Change subject: realtek_mst_i2c_spi.c: Fixup get_params() err ctrl flow
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/flashrom/+/45944/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/45944/3//COMMIT_MSG@11 
PS3, Line 11: Also renames the goto err cleanup path to make it clear.
> It would have been a single-line fix, which would have been trivial.

That was my initial patch but then I realised the goto path didn't make any sense once the goto was dropped in one path and hence the rename together.

tbh, all my attention at the moment has been drawn to figuring out a spi jedec abstraction mechanism for wp to be well designed. So I just wanted to get it fixed for Shiyu to continue to make progress with Realtek and move back to that. Once again sorry it made it in too quickly, it was not my intention more I just didn't pay enough mental attention to it.