Change in ...coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Marco Chen (Code Review)]

Hello Marco Chen,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

vendorcode/google: load sar config from CBFS first then VPD

SAR config provisioned in RO VPD can be done in the factory only. Once
it is wrong, we can override the SAR config by updating FW RW which can
carry new SAR config in CBFS. As a result, we should check CBFS first
then VPD.

Change-Id: I5aa6235fb7a6d0b2ed52893a42f7bd57806af6c1
Signed-off-by: Marco Chen 
---
M src/vendorcode/google/chromeos/sar.c
1 file changed, 23 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/33661/1

diff --git a/src/vendorcode/google/chromeos/sar.c b/src/vendorcode/google/chromeos/sar.c
index bbcb211..a5a2c3b 100644
--- a/src/vendorcode/google/chromeos/sar.c
+++ b/src/vendorcode/google/chromeos/sar.c
@@ -82,36 +82,37 @@
    				 sizeof(struct wifi_sar_delta_table);
    }

-	/* Try to read the SAR limit entry from VPD */
-	if (!vpd_gets(wifi_sar_limit_key, wifi_sar_limit_str,
-							 buffer_size, VPD_ANY)) {
-		printk(BIOS_ERR, "Error: Could not locate '%s' in VPD.\n",
-				wifi_sar_limit_key);
-
-		if (!CONFIG(WIFI_SAR_CBFS))
-			return -1;
-
+	if (CONFIG(WIFI_SAR_CBFS)) {
    	printk(BIOS_DEBUG, "Checking CBFS for default SAR values\n");

sar_cbfs_len = load_sar_file_from_cbfs(
    				(void *) wifi_sar_limit_str,
    					sar_expected_len);

-		if (sar_cbfs_len != sar_expected_len) {
-			printk(BIOS_ERR, "%s has bad len in CBFS\n",
-					WIFI_SAR_CBFS_FILENAME);
-			return -1;
-		}
-	} else {
-		/* VPD key "wifi_sar" found. strlen is checked with addition of
-		 * 1 as we have created buffer size 1 char larger for the reason
-		 * mentioned at start of this function itself */
-		if (strlen(wifi_sar_limit_str) + 1 != sar_expected_len) {
-			printk(BIOS_ERR, "WIFI SAR key has bad len in VPD\n");
-			return -1;
-		}
+		if (sar_cbfs_len == sar_expected_len)
+		    goto done;
+
+		printk(BIOS_ERR, "%s has bad len in CBFS\n",
+				WIFI_SAR_CBFS_FILENAME);
    }

+	/* Try to read the SAR limit entry from VPD */
+	if (!vpd_gets(wifi_sar_limit_key, wifi_sar_limit_str,
+							 buffer_size, VPD_ANY)) {
+		printk(BIOS_ERR, "Error: Could not locate '%s' in VPD.\n",
+				wifi_sar_limit_key);
+		return -1;
+	}
+
+	/* VPD key "wifi_sar" found. strlen is checked with addition of
+	 * 1 as we have created buffer size 1 char larger for the reason
+	 * mentioned at start of this function itself */
+	if (strlen(wifi_sar_limit_str) + 1 != sar_expected_len) {
+		printk(BIOS_ERR, "WIFI SAR key has bad len in VPD\n");
+		return -1;
+	}
+
+done:
    /* Decode the heximal encoded string to binary values */
    if (hexstrtobin(wifi_sar_limit_str, bin_buffer, bin_buff_adjusted_size)
    		< bin_buff_adjusted_size) {

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:

shouldn't this be some change only merged to specific firmware branches in cros repos?

cbfs first then vpd does not sound like a right/good behavior for upstream globally - people may want to use CBFS as default, then individual project set their VPD to override.

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> Patch Set 1:
>
> shouldn't this be some change only merged to specific firmware branches in cros repos?
>
> cbfs first then vpd does not sound like a right/good behavior for upstream globally - people may want to use CBFS as default, then individual project set their VPD to override.

If first project chooses CBFS approach then individual project can just load different SAR file from CBFS by SKU ID (shared the same firmware build target). Loading corresponding VBT by SKU IDs is the similar implementation now.

[Paul Menzel (Code Review)]

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

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1: Code-Review+1

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:

ping to reviewers, thanks.

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Justin TerAvest (Code Review)]

Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1: Code-Review+1

We need to document how SAR loading works (and what the data format is). I'm not sure if we should have a file here, or in docs/ in the chromeos repository.

I revisited the thread and the logic here looks good for what we agreed on.

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Furquan Shaikh (Code Review)]

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

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+1
>
> We need to document how SAR loading works (and what the data format is). I'm not sure if we should have a file here, or in docs/ in the chromeos repository.
>
> I revisited the thread and the logic here looks good for what we agreed on.

Quick question: Do we want to still continue supporting SAR in VPD?

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Justin TerAvest (Code Review)]

Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1: Code-Review+1
> >
> > We need to document how SAR loading works (and what the data format is). I'm not sure if we should have a file here, or in docs/ in the chromeos repository.
> >
> > I revisited the thread and the logic here looks good for what we agreed on.
>
> Quick question: Do we want to still continue supporting SAR in VPD?

It'd certainly be simpler to just require CBFS.

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> > Quick question: Do we want to still continue supporting SAR in VPD?
>
> It'd certainly be simpler to just require CBFS.

My suggestion is yes and the reason is to reduce the dependency to the FW (no necessary to up-rev FW). And after SAR value is reviewed, factory test can still verify the value.

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Furquan Shaikh (Code Review)]

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

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> Patch Set 1:
> > > Quick question: Do we want to still continue supporting SAR in VPD?
> >
> > It'd certainly be simpler to just require CBFS.
>
> My suggestion is yes and the reason is to reduce the dependency to the FW (no necessary to up-rev FW). And after SAR value is reviewed, factory test can still verify the value.

In my opinion, having 2 ways of doing things makes it confusing for the partners. If we are moving towards CBFS, ideally, it would be good to encourage partners to start using that. For the factory test, partners can still use CBFS SAR without having to rebuild the whole image. It would require few additional steps, but we can definitely come up with steps for them. What do you think?

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> In my opinion, having 2 ways of doing things makes it confusing for the partners.

It is impossible to update RO VPD if we did something wrong once it's written.
And there's no reliable way to build a factory "test" for verify the correctness of  that value unless if there's some one reviewing that manually.

So if there is only one way, reducing the dependency on factory provisioning would make more sense, i.e., CBFS.

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Furquan Shaikh (Code Review)]

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

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> Patch Set 1:
> > In my opinion, having 2 ways of doing things makes it confusing for the partners.
>
> It is impossible to update RO VPD if we did something wrong once it's written.
> And there's no reliable way to build a factory "test" for verify the correctness of  that value unless if there's some one reviewing that manually.
>
> So if there is only one way, reducing the dependency on factory provisioning would make more sense, i.e., CBFS.

+1. With support for CBFS SAR, I would like to completely deprecate the use of VPD for SAR and thus remove it from coreboot as well.

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:

reply to Hung-Te:
  The reason for this CL is to set WIFI SAR in CBFS to be higher priority then VPD in RO. As a result, we don't consider to update FW RO actually; we update CBFS in RW to override the SAR value instead if we need to.

Regarding review effort, no matter in factory test or CBFS we all need someone to review so there is no difference. There are others in VPD RO like region and MAC so ODM should take responsibility for them.

reply to Furquan:
  The main reason for me to keep VPD is to let ODM to do more and reduce the dependency to FW. For example, there is a next wave device like laser which is just new SKU IDs of phaser360. Since phaser360 was shipped, the cherry-pick policy to model.yaml of this device is strict which introduce troubles for development cycle if we can only set SAR in CBFS. Especially SAR value will be decided when ME is locked down (ex: DVT).

No strong insistence but would like to share why let ODM team to control is good for next wave projects.

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> The reason for this CL is to set WIFI SAR in CBFS to be higher priority then VPD in RO. As a result, we don't consider to update FW RO actually; we update CBFS in RW to override the SAR value instead if we need to.

This CL is a special case that people have VPD incorrectly set. And this is an example how communication with partner would lead to error, and how complicated the whole chromebook manufacturing is that you can't expect everyone to know every details - and things will go wrong sometimes.

With VPD, there's no way to fix (except special workarounds like this CL) it properly. But if it's CBFS, just one RW update would solve it.
> Regarding review effort, no matter in factory test or CBFS we all need someone to review so there is no difference. There are others in VPD RO like region and MAC so ODM should take responsibility for them.

It's true there are other VPDs, and we've seen them went wrong - for example the keyboard layout before we deprecate it with region. So the goal should be to minimize things we provisioned, just like how we abstracted all regional fields into single 'region' and have a clear way to make sure it won't go out of range (so even if partner set it wrong, it'll be very easy to catch, or has very limited impact). MAC and SNs are in the case that there's no better way to handle them. But SAR can be easily handled inside CBFS.

Regarding review, if it's in CBFS then we only need the RF engineer to review what's written in firmware source is correct. And it's in Google source repo, easily checked, one time effort. If it's a VPD process, you'll need: (1) ODM or RF eng to propose value, (2) communication to partner factory shopfloor eng to set this value in their backend, (3) write a test or something to validate if the value is correct. The value may go wrong any time in these steps, even in copy-paste. Also, (2) and (3) may be only in partner's local source folder, never goes back to Google to review. RF engineer will only see the failure when they received devices (or never).
> For example, there is a next wave device like laser which is just new SKU IDs of phaser360. Since phaser360 was shipped, the cherry-pick policy to model.yaml of this device is strict which introduce troubles for development cycle if we can only set SAR in CBFS.

That's the nature of device management. If this is a new device, then it should be a new model - which can have its own set of firmware (and RO).
And we can easily create a per-SKU SAR table in new firmware, and select it accordingly. Or am I missing anything? (Not quite following why model.yaml is mentioned here)

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Brian Norris (Code Review)]

Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> +1. With support for CBFS SAR, I would like to completely deprecate the use of VPD for SAR and thus remove it from coreboot as well.

+1000, and that's exactly what I proposed in email. In fact, I thought (or hoped, since it seemed like the sane option) we already *had* removed support until that thread came up.

I see the rest as a feature, not a bug. A new model is a new model. We don't try to pretend it's not just to make it easier for the ODM to screw everything up^W^W^Witerate more quickly. (Or IOW: what Hung-Te said.)

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Justin TerAvest (Code Review)]

Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33661 )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1:
> Patch Set 1:
> > +1. With support for CBFS SAR, I would like to completely deprecate the use of VPD for SAR and thus remove it from coreboot as well.
>
> +1000, and that's exactly what I proposed in email. In fact, I thought (or hoped, since it seemed like the sane option) we already *had* removed support until that thread came up.
>
> I see the rest as a feature, not a bug. A new model is a new model. We don't try to pretend it's not just to make it easier for the ODM to screw everything up^W^W^Witerate more quickly. (Or IOW: what Hung-Te said.)

We don't want to define new models just to allow for differences in SAR values.
We do want to allow for flexibility in devices without incurring tons of deployment and testing overhead.

Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Paul Menzel (Code Review)]

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

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Patch Set 1: -Code-Review

What is the status of this change-set?

[S] Change in coreboot[master]: vendorcode/google: load sar config from CBFS first then VPD

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33661?usp=email )

Change subject: vendorcode/google: load sar config from CBFS first then VPD
......................................................................

Abandoned