coreboot-gerrit February 2024

coreboot-gerrit@coreboot.org

1 participants
3067 discussions

[XS] Change in coreboot[main]: drivers/wifi: Use depends instead of if in Kconfig
by Subrata Banik (Code Review) 02 Feb '24

02 Feb '24

Attention is currently required from: David Ruth, David Ruth, Eric Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80307?usp=email ) Change subject: drivers/wifi: Use depends instead of if in Kconfig ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/80307?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I3604b394f999b28de4723337b3b6b4e21139c83b Gerrit-Change-Number: 80307 Gerrit-PatchSet: 2 Gerrit-Owner: David Ruth <druth(a)chromium.org> Gerrit-Reviewer: David Ruth <druth(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Eric Lai <ericllai(a)google.com> Gerrit-Attention: David Ruth <druth(a)chromium.org> Gerrit-Attention: David Ruth <druth(a)google.com> Gerrit-Comment-Date: Fri, 02 Feb 2024 16:52:24 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Add limited to 32-bits FSP 2.4 support
by Jérémy Compostella (Code Review) 02 Feb '24

02 Feb '24

Attention is currently required from: Andrey Petrov, Bora Guvendik, Chen, Gang C, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Nick Vaccaro, Ronak Kanabar, Shuo Liu, Subrata Banik, Tarun, Wonkyu Kim. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80275?usp=email ) Change subject: drivers/intel/fsp2_0: Add limited to 32-bits FSP 2.4 support ...................................................................... Patch Set 8: (1 comment) Patchset: PS8: > Will this be merged recently? I would like this FSP 2.4 support to merge as soon as possible. -- To view, visit https://review.coreboot.org/c/coreboot/+/80275?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I1c24d26e105c3dcbd9cca0e7197ab1362344aa97 Gerrit-Change-Number: 80275 Gerrit-PatchSet: 8 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Tarun <tstuli(a)gmail.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Eran Mitrani <mitrani(a)google.com> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Tarun <tstuli(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Comment-Date: Fri, 02 Feb 2024 16:45:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Support FSP 2.4 64-bits
by Jérémy Compostella (Code Review) 02 Feb '24

02 Feb '24

Attention is currently required from: Andrey Petrov, Arthur Heymans, Bora Guvendik, Christian Walter, Felix Held, Fred Reitberger, Jason Glenesk, Johnny Lin, Lean Sheng Tan, Matt DeVillier, Patrick Rudolph, Paul Menzel, Ronak Kanabar, Shuo Liu, Subrata Banik, Tim Chu, Wonkyu Kim. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80277?usp=email ) Change subject: drivers/intel/fsp2_0: Support FSP 2.4 64-bits ...................................................................... Patch Set 9: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/80277/comment/455b695a_584d8b6a : PS8, Line 28: Fortunately, EDK2 header allows to override the EFIAPI : definition. The __ms_abi__ attribute works for both i386 and x86_64. : > Not true on clang and it also does not match what the code is doing. Indeed I ran into this issue yesterday. I fixed that in the code already but needed to update the commit message. Thanks for the reminder. File src/drivers/intel/fsp2_0/memory_init.c: https://review.coreboot.org/c/coreboot/+/80277/comment/39aef609_d84f6271 : PS7, Line 42: > can we simply align 16 to cover both case? Absolutely ! I just wanted the decision to be collegial. **@Subrata** and **@Arthur** what do you think. I did not want to disrupt existing devices but the impact should be very minimal, should I just proceed with a general 16-bytes alignment ? File src/include/efi/efi_datatype.h: https://review.coreboot.org/c/coreboot/+/80277/comment/6bd233c4_800c4d59 : PS8, Line 65: /* Status codes common to all execution phases */ > duplicated comment. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/80277?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec99 Gerrit-Change-Number: 80277 Gerrit-PatchSet: 9 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 02 Feb 2024 16:44:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: drivers/intel/fsp2_0: Support FSP 2.4 64-bits
by Jérémy Compostella (Code Review) 02 Feb '24

02 Feb '24

Attention is currently required from: Andrey Petrov, Bora Guvendik, Christian Walter, Felix Held, Fred Reitberger, Jason Glenesk, Johnny Lin, Jérémy Compostella, Lean Sheng Tan, Matt DeVillier, Patrick Rudolph, Paul Menzel, Ronak Kanabar, Subrata Banik, Tim Chu, Wonkyu Kim. Hello Andrey Petrov, Arthur Heymans, Bora Guvendik, Christian Walter, Felix Held, Fred Reitberger, Jason Glenesk, Johnny Lin, Lean Sheng Tan, Matt DeVillier, Patrick Rudolph, Ronak Kanabar, Shuo Liu, Tim Chu, Wonkyu Kim, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80277?usp=email to look at the new patch set (#9). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: drivers/intel/fsp2_0: Support FSP 2.4 64-bits ...................................................................... drivers/intel/fsp2_0: Support FSP 2.4 64-bits FSP 2.4 brings FSP 64-bits support which requires some adjustments in coreboot: - Stack alignment: 1. FSP functions must be called with the stack 16-bytes aligned. This is already setup properly with the default value of the `mpreferred-stack-boundary' compiler option (4). 2. The FSP stack buffer supplied by coreboot through the `StackBase' UPD must be 16-bytes aligned. - The EDK2 EFIAPI macro definition relies on compiler flags such as __GNUC__ which is not working well when included by coreboot. While it has no side-effect on i386 because the C calling convention used by coreboot and FSP are the same, it breaks on x86_64 because FSP/UEFI uses the Microsoft x64 calling convention while coreboot uses the System V AMD64 ABI. Fortunately, EDK2 header allows to override the EFIAPI definition. This appropriate attribute has to be set to all functions calling or called by the FSP. - Add fsp print helper macros to print `efi_return_status_t' with the appropriate format Change-Id: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec99 Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com> --- M src/drivers/intel/fsp2_0/debug.c M src/drivers/intel/fsp2_0/fsp_debug_event.c M src/drivers/intel/fsp2_0/include/fsp/debug.h M src/drivers/intel/fsp2_0/include/fsp/fsp_debug_event.h M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/intel/fsp2_0/include/fsp/soc_binding.h M src/drivers/intel/fsp2_0/include/fsp/util.h M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/ppi/mp_service1.c M src/drivers/intel/fsp2_0/ppi/mp_service2.c M src/drivers/intel/fsp2_0/silicon_init.c M src/drivers/intel/fsp2_0/util.c M src/include/efi/efi_datatype.h M src/soc/amd/common/fsp/fsp_reset.c M src/soc/intel/common/fsp_reset.c 15 files changed, 96 insertions(+), 62 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/80277/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/80277?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: If0397f5cc8d0f4f1872bd37a001fe42e0c37ec99 Gerrit-Change-Number: 80277 Gerrit-PatchSet: 9 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Appukuttan V K <appukuttan.vk(a)intel.com> Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-MessageType: newpatchset

1 0

[XS] Change in coreboot[main]: mb/amd/birman/Kconfig: fix comment on endif
by Varshit Pandya (Code Review) 02 Feb '24

02 Feb '24

Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk. Varshit Pandya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80289?usp=email ) Change subject: mb/amd/birman/Kconfig: fix comment on endif ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/80289?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I986e5a456e8f9fd92aacd007479c861feea06199 Gerrit-Change-Number: 80289 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 02 Feb 2024 16:25:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: soc/amd/phoenix/Makefile: only include FSP folder conditionally
by Varshit Pandya (Code Review) 02 Feb '24

02 Feb '24

Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk. Varshit Pandya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80288?usp=email ) Change subject: soc/amd/phoenix/Makefile: only include FSP folder conditionally ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/80288?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I18668ab8578b297c328fdc647c8a95f540ac6272 Gerrit-Change-Number: 80288 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 02 Feb 2024 16:25:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: [WIP,UNTESTED] arch/x86/ioapic: always write IOAPIC ID in set_ioapic_id
by Nico Huber (Code Review) 02 Feb '24

02 Feb '24

Attention is currently required from: Arthur Heymans, Felix Held, Jérémy Compostella. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80330?usp=email ) Change subject: [WIP,UNTESTED] arch/x86/ioapic: always write IOAPIC ID in set_ioapic_id ...................................................................... Patch Set 1: Code-Review+1 (1 comment) Patchset: PS1: > haven't looked at all the details yet, but at least for the socs i work on the old behavior seems to […] What I remember from the cleanup times is that today's IOAPICs probably don't care about their ID. Historically, an ID of 0 would have been wrong, because that was usually the BSP LAPIC (and they needed a unique ID on the APIC bus, which is gone). Not sure if anything still cares about the ID, though we have to put it into ACPI tables (I just saw). We read the ID from hardware for this, so we should also write it to hw I guess. -- To view, visit https://review.coreboot.org/c/coreboot/+/80330?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ic8538f82a6b10f16eeb228669db197dc8e326ffd Gerrit-Change-Number: 80330 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 02 Feb 2024 16:15:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

[XS] Change in em100[main]: em100: return an error if a command fails
by Stefan Reinauer (Code Review) 02 Feb '24

02 Feb '24

Stefan Reinauer has submitted this change. ( https://review.coreboot.org/c/em100/+/80250?usp=email ) Change subject: em100: return an error if a command fails ...................................................................... em100: return an error if a command fails We don't properly return an error in every code path when a command fails. This makes it hard to use this tool in a shell script because you can't be sure that your command was successful. Change-Id: I2554252d007a92e939088389d67f9d4cede837ee Signed-off-by: Corvin Köhne <c.koehne(a)beckhoff.com> Reviewed-on: https://review.coreboot.org/c/em100/+/80250 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Reviewed-by: Jakub Czapiga <czapiga(a)google.com> --- M em100.c 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: Jakub Czapiga: Looks good to me, approved build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved diff --git a/em100.c b/em100.c index 666cef9..68407c3 100644 --- a/em100.c +++ b/em100.c @@ -1068,7 +1068,7 @@ if (!set_chip_type(em100, chip)) { printf("Failed configuring chip type.\n"); em100_detach(em100); - return 0; + return 1; } printf("Chip set to %s %s.\n", chip->vendor, chip->name); @@ -1099,7 +1099,7 @@ if (!set_hold_pin_state_from_str(em100, holdpin)) { printf("Failed configuring hold pin state.\n"); em100_detach(em100); - return 0; + return 1; } } -- To view, visit https://review.coreboot.org/c/em100/+/80250?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: em100 Gerrit-Branch: main Gerrit-Change-Id: I2554252d007a92e939088389d67f9d4cede837ee Gerrit-Change-Number: 80250 Gerrit-PatchSet: 3 Gerrit-Owner: c.koehne(a)beckhoff.com Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in em100[main]: em100: return an error if a command fails
by Stefan Reinauer (Code Review) 02 Feb '24

02 Feb '24

Attention is currently required from: c.koehne(a)beckhoff.com. Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/80250?usp=email ) Change subject: em100: return an error if a command fails ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/em100/+/80250?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: em100 Gerrit-Branch: main Gerrit-Change-Id: I2554252d007a92e939088389d67f9d4cede837ee Gerrit-Change-Number: 80250 Gerrit-PatchSet: 2 Gerrit-Owner: c.koehne(a)beckhoff.com Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: c.koehne(a)beckhoff.com Gerrit-Comment-Date: Fri, 02 Feb 2024 16:14:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in em100[main]: em100: exit when using an invalid device name
by Stefan Reinauer (Code Review) 02 Feb '24

02 Feb '24

Stefan Reinauer has submitted this change. ( https://review.coreboot.org/c/em100/+/80249?usp=email ) Change subject: em100: exit when using an invalid device name ...................................................................... em100: exit when using an invalid device name If you use a stupid device name like `em100 --device bla`, we won't exit with an error right now. It just ignores the devices and uses the first device it can find. This could lead to unexpected issues when connecting more than one EM100 device to your machine. It's much better to exit if we're unable to parse the device name. Change-Id: If0f7b8b576224a9db6ab319c2212bc245110c5c1 Signed-off-by: Corvin Köhne <c.koehne(a)beckhoff.com> Reviewed-on: https://review.coreboot.org/c/em100/+/80249 Reviewed-by: Jakub Czapiga <czapiga(a)google.com> Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M em100.c 1 file changed, 11 insertions(+), 4 deletions(-) Approvals: Stefan Reinauer: Looks good to me, approved build bot (Jenkins): Verified Jakub Czapiga: Looks good to me, approved diff --git a/em100.c b/em100.c index cfcecb9..666cef9 100644 --- a/em100.c +++ b/em100.c @@ -930,10 +930,17 @@ break; case 'x': if ((toupper(optarg[0]) == 'D' && toupper(optarg[1]) == 'P') || - (toupper(optarg[0]) == 'E' && toupper(optarg[1]) == 'M')) - sscanf(optarg + 2, "%u", &serial_number); - else - sscanf(optarg, "%d:%d", &bus, &device); + (toupper(optarg[0]) == 'E' && toupper(optarg[1]) == 'M')) { + if (sscanf(optarg + 2, "%u", &serial_number) != 1) { + printf("Invalid device name: %s\n", optarg); + return 1; + } + } else { + if (sscanf(optarg, "%d:%d", &bus, &device) != 2) { + printf("Invalid device name: %s\n", optarg); + return 1; + } + } break; case 'l': em100_list(); -- To view, visit https://review.coreboot.org/c/em100/+/80249?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: em100 Gerrit-Branch: main Gerrit-Change-Id: If0f7b8b576224a9db6ab319c2212bc245110c5c1 Gerrit-Change-Number: 80249 Gerrit-PatchSet: 3 Gerrit-Owner: c.koehne(a)beckhoff.com Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2024