Change in coreboot[master]: soc/intel/tigerlake: Fix stale device pointer usage

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

soc/intel/tigerlake: Fix stale device pointer usage

TEST=Build the mainboard.

Signed-off-by: Karthikeyan Ramasubramanian 
Change-Id: I43cccd32589d75a9b0c7e60f8c82b19bbe6b69a7
---
M src/soc/intel/tigerlake/fsp_params_jsl.c
1 file changed, 2 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/39405/1

diff --git a/src/soc/intel/tigerlake/fsp_params_jsl.c b/src/soc/intel/tigerlake/fsp_params_jsl.c
index 6cb3b67..fa36e81 100644
--- a/src/soc/intel/tigerlake/fsp_params_jsl.c
+++ b/src/soc/intel/tigerlake/fsp_params_jsl.c
@@ -143,7 +143,8 @@
    }

/* SDCard related configuration */
-	params->ScsSdCardEnabled = pcidev_path_on_root(PCH_DEVFN_SDCARD) ? dev->enabled : 0;
+	params->ScsSdCardEnabled = (dev = pcidev_path_on_root(PCH_DEVFN_SDCARD)) ?
+									dev->enabled : 0;

params->Device4Enable = config->Device4Enable;

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 1: Code-Review+2

Thanks, there's one in romstage fsp_params too:
/* Audio */
m_cfg->PchHdaEnable = pcidev_path_on_root(PCH_DEVFN_HDA) ? dev->enabled : 0;

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+2
>
> Thanks, there's one in romstage fsp_params too:
> /* Audio */
> m_cfg->PchHdaEnable = pcidev_path_on_root(PCH_DEVFN_HDA) ? dev->enabled : 0;

That has been fixed already by this CL - https://review.coreboot.org/c/coreboot/+/39261

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 1:

Would it be easier to avoid this mistake in the future, if we split the logic into two lines.
One to assign dev to the correct device, and then the next one to set the UPD?

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Would it be easier to avoid this mistake in the future, if we split the logic into two lines.
> One to assign dev to the correct device, and then the next one to set the UPD?

In the original CL, the code was written in the way you mentioned. But there was a comment to use the ternary operator instead.

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > Would it be easier to avoid this mistake in the future, if we split the logic into two lines.
> > One to assign dev to the correct device, and then the next one to set the UPD?
>
> In the original CL, the code was written in the way you mentioned. But there was a comment to use the ternary operator instead.

i'd also prefer to read the i/t/e implementation.
note that there are 2 more cases like this just a few lines down
that use the i/t/e version.

[Karthik Ramasubramanian (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Justin TerAvest, Tim Wawrzynczak, Nick Vaccaro, Aamir Bohra, Patrick Rudolph, Karthikeyan Ramasubramanian,

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

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

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

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

soc/intel/tigerlake: Fix stale device pointer usage

TEST=Build the mainboard.

Signed-off-by: Karthikeyan Ramasubramanian 
Change-Id: I43cccd32589d75a9b0c7e60f8c82b19bbe6b69a7
---
M src/soc/intel/tigerlake/fsp_params_jsl.c
1 file changed, 5 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/39405/2

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 2:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > >
> > > Would it be easier to avoid this mistake in the future, if we split the logic into two lines.
> > > One to assign dev to the correct device, and then the next one to set the UPD?
> >
> > In the original CL, the code was written in the way you mentioned. But there was a comment to use the ternary operator instead.
>
> i'd also prefer to read the i/t/e implementation.
> note that there are 2 more cases like this just a few lines down
> that use the i/t/e version.

Done

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 2: Code-Review+1

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 2: Code-Review+2

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 2: Code-Review+2

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39405 )

Change subject: soc/intel/tigerlake: Fix stale device pointer usage
......................................................................

soc/intel/tigerlake: Fix stale device pointer usage

TEST=Build the mainboard.

Signed-off-by: Karthikeyan Ramasubramanian 
Change-Id: I43cccd32589d75a9b0c7e60f8c82b19bbe6b69a7
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39405
Tested-by: build bot (Jenkins) 
Reviewed-by: Caveh Jalali 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Nick Vaccaro 
Reviewed-by: Aamir Bohra 
---
M src/soc/intel/tigerlake/fsp_params_jsl.c
1 file changed, 5 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Aamir Bohra: Looks good to me, approved
  Nick Vaccaro: Looks good to me, approved
  Caveh Jalali: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/tigerlake/fsp_params_jsl.c b/src/soc/intel/tigerlake/fsp_params_jsl.c
index 6cb3b67..96aa7ba 100644
--- a/src/soc/intel/tigerlake/fsp_params_jsl.c
+++ b/src/soc/intel/tigerlake/fsp_params_jsl.c
@@ -143,7 +143,11 @@
    }

/* SDCard related configuration */
-	params->ScsSdCardEnabled = pcidev_path_on_root(PCH_DEVFN_SDCARD) ? dev->enabled : 0;
+	dev = pcidev_path_on_root(PCH_DEVFN_SDCARD);
+	if (!dev)
+		params->ScsSdCardEnabled = 0;
+	else
+		params->ScsSdCardEnabled = dev->enabled;

params->Device4Enable = config->Device4Enable;