Change in coreboot[master]: soc/qualcomm/qcs405: Handle unknown QUP and BLSP

[Jacob Garber (Code Review)]

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

Change subject: soc/qualcomm/qcs405: Handle unknown QUP and BLSP
......................................................................

soc/qualcomm/qcs405: Handle unknown QUP and BLSP

Print an error message and return if an unknown QUP or BLSP is
encountered. This prevents a possible null dereference of spi_clk.

Change-Id: I374e15ce899c651df9c2d3e0f1ec646e33d4bdb2
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1401086
---
M src/soc/qualcomm/qcs405/clock.c
1 file changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/34523/1

diff --git a/src/soc/qualcomm/qcs405/clock.c b/src/soc/qualcomm/qcs405/clock.c
index de42147..eae1644 100644
--- a/src/soc/qualcomm/qcs405/clock.c
+++ b/src/soc/qualcomm/qcs405/clock.c
@@ -235,12 +235,16 @@
    		spi_clk = (struct qcs405_clock *)
    				&gcc->blsp1_qup4_spi_clk;
    		break;
+		default:
+			printk(BIOS_ERR, "QUP %d not supported\n", qup);
+			return;
    	}
-	} else if (blsp == 2)
+	} else if (blsp == 2) {
    	spi_clk = (struct qcs405_clock *)&gcc->blsp2_qup0_spi_clk;
-
-	else
-		printk(BIOS_ERR, "BLSP%d not supported\n", blsp);
+	} else {
+		printk(BIOS_ERR, "BLSP %d not supported\n", blsp);
+		return;
+	}

clock_configure(spi_clk, spi_cfg, hz, ARRAY_SIZE(spi_cfg));
 }

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34523 )

Change subject: soc/qualcomm/qcs405: Handle unknown QUP and BLSP
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34523/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34523/1//COMMIT_MSG@10 
PS1, Line 10: null
null pointer

[Jacob Garber (Code Review)]

Hello Julius Werner, Paul Menzel, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: soc/qualcomm/qcs405: Handle unknown QUP and BLSP
......................................................................

soc/qualcomm/qcs405: Handle unknown QUP and BLSP

Print an error message and return if an unknown QUP or BLSP is
encountered. This prevents a possible null pointer dereference
of spi_clk.

Change-Id: I374e15ce899c651df9c2d3e0f1ec646e33d4bdb2
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1401086
---
M src/soc/qualcomm/qcs405/clock.c
1 file changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/34523/2

[Jacob Garber (Code Review)]

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

Change subject: soc/qualcomm/qcs405: Handle unknown QUP and BLSP
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34523/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34523/1//COMMIT_MSG@10 
PS1, Line 10: null
> null pointer

Done

[Angel Pons (Code Review)]

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

Change subject: soc/qualcomm/qcs405: Handle unknown QUP and BLSP
......................................................................

Patch Set 2: Code-Review+2

(2 comments)

I found this: https://www.inforcecomputing.com/public_docs/BLSPs_on_Inforce_6540_6501_Snap...

Looks like QUP stands for "Qualcomm Universal Peripheral", and BLSP stands for "BAM Low Speed Peripheral", where BAM means "Bus Access Manager/Module". Maybe you want to update the "QUP not supported" message?

https://review.coreboot.org/c/coreboot/+/34523/2/src/soc/qualcomm/qcs405/clo... 
File src/soc/qualcomm/qcs405/clock.c:

https://review.coreboot.org/c/coreboot/+/34523/2/src/soc/qualcomm/qcs405/clo... 
PS2, Line 218: 	case 0:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup0_spi_clk;
             : 			break;
             : 		case 1:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup1_spi_clk;
             : 			break;
             : 		case 2:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup2_spi_clk;
             : 			break;
             : 		case 3:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup3_spi_clk;
             : 			break;
             : 		case 4:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup4_spi_clk;
Not on this patch, but I guess this could fit in one line with the new 96 character line length.

https://review.coreboot.org/c/coreboot/+/34523/2/src/soc/qualcomm/qcs405/clo... 
PS2, Line 239: pri
I guess "Invalid QUP %d\n" would be better

Change in coreboot[master]: soc/qualcomm/qcs405: Handle invalid QUP and BLSP

[Jacob Garber (Code Review)]

Hello Angel Pons, Julius Werner, Paul Menzel, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: soc/qualcomm/qcs405: Handle invalid QUP and BLSP
......................................................................

soc/qualcomm/qcs405: Handle invalid QUP and BLSP

Print an error message and return if an invalid QUP or BLSP is
encountered. This prevents a possible null pointer dereference
of spi_clk.

Change-Id: I374e15ce899c651df9c2d3e0f1ec646e33d4bdb2
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1401086
---
M src/soc/qualcomm/qcs405/clock.c
1 file changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/34523/3

Change in coreboot[master]: soc/qualcomm/qcs405: Handle invalid QUP and BLSP

[Jacob Garber (Code Review)]

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

Change subject: soc/qualcomm/qcs405: Handle invalid QUP and BLSP
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34523/2/src/soc/qualcomm/qcs405/clo... 
File src/soc/qualcomm/qcs405/clock.c:

https://review.coreboot.org/c/coreboot/+/34523/2/src/soc/qualcomm/qcs405/clo... 
PS2, Line 218: 	case 0:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup0_spi_clk;
             : 			break;
             : 		case 1:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup1_spi_clk;
             : 			break;
             : 		case 2:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup2_spi_clk;
             : 			break;
             : 		case 3:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup3_spi_clk;
             : 			break;
             : 		case 4:
             : 			spi_clk = (struct qcs405_clock *)
             : 					&gcc->blsp1_qup4_spi_clk;
> Not on this patch, but I guess this could fit in one line with the new 96 character line length.

Ack

https://review.coreboot.org/c/coreboot/+/34523/2/src/soc/qualcomm/qcs405/clo... 
PS2, Line 239: pri
> I guess "Invalid QUP %d\n" would be better

Done

Change in coreboot[master]: soc/qualcomm/qcs405: Handle invalid QUP and BLSP

[Patrick Georgi (Code Review)]

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

Change subject: soc/qualcomm/qcs405: Handle invalid QUP and BLSP
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: soc/qualcomm/qcs405: Handle invalid QUP and BLSP

[Patrick Georgi (Code Review)]

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

Change subject: soc/qualcomm/qcs405: Handle invalid QUP and BLSP
......................................................................

soc/qualcomm/qcs405: Handle invalid QUP and BLSP

Print an error message and return if an invalid QUP or BLSP is
encountered. This prevents a possible null pointer dereference
of spi_clk.

Change-Id: I374e15ce899c651df9c2d3e0f1ec646e33d4bdb2
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1401086
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34523
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
---
M src/soc/qualcomm/qcs405/clock.c
1 file changed, 8 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/src/soc/qualcomm/qcs405/clock.c b/src/soc/qualcomm/qcs405/clock.c
index de42147..b7dd51b 100644
--- a/src/soc/qualcomm/qcs405/clock.c
+++ b/src/soc/qualcomm/qcs405/clock.c
@@ -235,12 +235,16 @@
    		spi_clk = (struct qcs405_clock *)
    				&gcc->blsp1_qup4_spi_clk;
    		break;
+		default:
+			printk(BIOS_ERR, "Invalid QUP %d\n", qup);
+			return;
    	}
-	} else if (blsp == 2)
+	} else if (blsp == 2) {
    	spi_clk = (struct qcs405_clock *)&gcc->blsp2_qup0_spi_clk;
-
-	else
-		printk(BIOS_ERR, "BLSP%d not supported\n", blsp);
+	} else {
+		printk(BIOS_ERR, "BLSP %d not supported\n", blsp);
+		return;
+	}

clock_configure(spi_clk, spi_cfg, hz, ARRAY_SIZE(spi_cfg));
 }