Hello Jason Glenesk,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/44086
to review the following change.
Change subject: /amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
/amd/fsp/picasso Fix type 17 smbios misalignment
Add __packed to TYPE17_DMI_INFO structure to remove padding. Remove reserved fields that are no longer required. Corresponding change will also be made within fsp to pack the structure.
BUG=b:154046847 TEST=Boot a trembyle with and without the reserved fields and confirm type 17 table is unchanged.
Change-Id: I9ba7e2a4fb82c7b0b77ee7c6c075e6211d4f6adf Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/vendorcode/amd/fsp/picasso/dmi_info.h 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44086/1
diff --git a/src/vendorcode/amd/fsp/picasso/dmi_info.h b/src/vendorcode/amd/fsp/picasso/dmi_info.h index adab2f9..36ca719 100644 --- a/src/vendorcode/amd/fsp/picasso/dmi_info.h +++ b/src/vendorcode/amd/fsp/picasso/dmi_info.h @@ -178,7 +178,6 @@ OUT DMI_T17_MEMORY_TYPE MemoryType; ///< The type of memory used in this device. OUT DMI_T17_TYPE_DETAIL TypeDetail; ///< Additional detail on the memory device type OUT UINT16 Speed; ///< Identifies the speed of the device, in megahertz (MHz). - OUT UINT32 _Reserved1_; OUT UINT64 ManufacturerIdCode; ///< Manufacturer ID code. OUT CHAR8 SerialNumber[9]; ///< Serial Number. OUT CHAR8 PartNumber[21]; ///< Part Number. @@ -188,8 +187,7 @@ OUT UINT16 MinimumVoltage; ///< Minimum operating voltage for this device, in millivolts OUT UINT16 MaximumVoltage; ///< Maximum operating voltage for this device, in millivolts OUT UINT16 ConfiguredVoltage; ///< Configured voltage for this device, in millivolts - OUT UINT32 _Reserved2_; -} TYPE17_DMI_INFO; +}__packed TYPE17_DMI_INFO;
/// Collection of pointers to the DMI records typedef struct {
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: /amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 1:
Please help to review this change.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: /amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 1: Code-Review+1
Good catch.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: /amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44086/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44086/1//COMMIT_MSG@7 PS1, Line 7: /amd/fsp/picasso nit: `vc/amd/fsp/picasso:`
Hello Jason Glenesk, Angel Pons, Rob Barnes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44086
to look at the new patch set (#2).
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment
Add __packed to TYPE17_DMI_INFO structure to remove padding. Remove reserved fields that are no longer required. Corresponding change will also be made within fsp to pack the structure.
BUG=b:154046847 TEST=Boot a trembyle with and without the reserved fields and confirm type 17 table is unchanged.
Change-Id: I9ba7e2a4fb82c7b0b77ee7c6c075e6211d4f6adf Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/vendorcode/amd/fsp/picasso/dmi_info.h 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44086/2
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44086/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44086/1//COMMIT_MSG@7 PS1, Line 7: /amd/fsp/picasso
nit: `vc/amd/fsp/picasso:`
No problem. Please keep these coming while i'm learning the processes and format.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44086/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44086/1//COMMIT_MSG@7 PS1, Line 7: /amd/fsp/picasso
No problem. Please keep these coming while i'm learning the processes and format.
It's usually of the form:
path/to/folder: Commit summary
Where "path/to/folder" is often abbreviated. We usually drop the "src/" prefix, and abbreviate mainboard, vendorcode, northbridge, southbridge to mb, vc, nb, sb respectively
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44086/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44086/1//COMMIT_MSG@7 PS1, Line 7: /amd/fsp/picasso
It's usually of the form: […]
https://doc.coreboot.org/tutorial/part2.html#step-4-submit-a-commit
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 2: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 2:
Is there a dependency on chrome-internal repo? If so, you need to add Cq-Depend=:
https://chromium.googlesource.com/chromiumos/docs/+/master/contributing.md#C...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 2:
Patch Set 2:
Is there a dependency on chrome-internal repo? If so, you need to add Cq-Depend=:
https://chromium.googlesource.com/chromiumos/docs/+/master/contributing.md#C...
crrev.com/i/3194239
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 2:
can i submit this patch or do we still need to wait on something here?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 2: Code-Review+2
Jason Glenesk has uploaded a new patch set (#3) to the change originally created by Jason Glenesk. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment
Add __packed to TYPE17_DMI_INFO structure to remove padding. Remove reserved fields that are no longer required. Corresponding change will also be made within fsp to pack the structure.
BUG=b:154046847 TEST=Boot a trembyle with and without the reserved fields and confirm type 17 table is unchanged.
Cq-Depend: chrome-internal:3194239 Change-Id: I9ba7e2a4fb82c7b0b77ee7c6c075e6211d4f6adf Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com --- M src/vendorcode/amd/fsp/picasso/dmi_info.h 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/44086/3
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
Patch Set 3:
Patch Set 2:
Is there a dependency on chrome-internal repo? If so, you need to add Cq-Depend=:
https://chromium.googlesource.com/chromiumos/docs/+/master/contributing.md#C...
Added the Cq on the internal fsp review. Thanks!
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44086 )
Change subject: vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment ......................................................................
vendorcode/amd/fsp/picasso Fix type 17 smbios misalignment
Add __packed to TYPE17_DMI_INFO structure to remove padding. Remove reserved fields that are no longer required. Corresponding change will also be made within fsp to pack the structure.
BUG=b:154046847 TEST=Boot a trembyle with and without the reserved fields and confirm type 17 table is unchanged.
Cq-Depend: chrome-internal:3194239 Change-Id: I9ba7e2a4fb82c7b0b77ee7c6c075e6211d4f6adf Signed-off-by: Jason Glenesk jason.glenesk@amd.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44086 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Rob Barnes robbarnes@google.com --- M src/vendorcode/amd/fsp/picasso/dmi_info.h 1 file changed, 1 insertion(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Angel Pons: Looks good to me, approved Rob Barnes: Looks good to me, but someone else must approve
diff --git a/src/vendorcode/amd/fsp/picasso/dmi_info.h b/src/vendorcode/amd/fsp/picasso/dmi_info.h index adab2f9..36ca719 100644 --- a/src/vendorcode/amd/fsp/picasso/dmi_info.h +++ b/src/vendorcode/amd/fsp/picasso/dmi_info.h @@ -178,7 +178,6 @@ OUT DMI_T17_MEMORY_TYPE MemoryType; ///< The type of memory used in this device. OUT DMI_T17_TYPE_DETAIL TypeDetail; ///< Additional detail on the memory device type OUT UINT16 Speed; ///< Identifies the speed of the device, in megahertz (MHz). - OUT UINT32 _Reserved1_; OUT UINT64 ManufacturerIdCode; ///< Manufacturer ID code. OUT CHAR8 SerialNumber[9]; ///< Serial Number. OUT CHAR8 PartNumber[21]; ///< Part Number. @@ -188,8 +187,7 @@ OUT UINT16 MinimumVoltage; ///< Minimum operating voltage for this device, in millivolts OUT UINT16 MaximumVoltage; ///< Maximum operating voltage for this device, in millivolts OUT UINT16 ConfiguredVoltage; ///< Configured voltage for this device, in millivolts - OUT UINT32 _Reserved2_; -} TYPE17_DMI_INFO; +}__packed TYPE17_DMI_INFO;
/// Collection of pointers to the DMI records typedef struct {
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Jason Glenesk (Code Review)
-
Rob Barnes (Code Review)