coreboot-gerrit August 2019

coreboot-gerrit@coreboot.org

1 participants
1942 discussions

Change in coreboot[master]: vc/amd/cimx/sb800: Remove old strict-aliasing workaround
by Jacob Garber (Code Review) 20 Aug '19

20 Aug '19

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34783 ) Change subject: vc/amd/cimx/sb800: Remove old strict-aliasing workaround ...................................................................... vc/amd/cimx/sb800: Remove old strict-aliasing workaround C strict aliasing rules state that it is undefined behaviour to access any pointer using another pointer of a different type (with several small exceptions). Eg. uint64_t x = 3; uint16_t y = *((uint16_t *)&x); // undefined behaviour From an architectural point of view there is often nothing wrong with pointer aliasing - the problem is that since it is undefined behaviour, the compiler will often use this as a cop-out to perform unintended or unsafe optimizations. The "safe" way to perfom the above assignment is to cast the pointers to a uint8_t * first (which is allowed to alias anything), and then work on a byte level: *((uint8_t *)&y) = *((uint8_t *)&x); *((uint8_t *)&y + 1) = *((uint8_t *)&x + 1); Horribly ugly, but there you go. Anyway, in an attempt to follow these strict aliasing rules, the ReadMEM() function in SB800 does the above operation when reading a uint16_t. While perfectly fine, however, it doesn't have to - all calls to ReadMEM() that read a uint16_t are passed a uint16_t pointer, so there are no strict aliasing violations to worry about (the WriteMEM() function is exactly similar). The problem is that using this unnecessary workaround generates almost 50 false positive warnings in Coverity. Rather than manually ignore them one-by-one, let's just remove the workaround entirely. As a side note, this change makes ReadMEM() and WriteMEM() now match their definitions in the SB900 code. Change-Id: Ia7e3a1eff88b855a05b33c7dafba16ed23784e43 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> --- M src/vendorcode/amd/cimx/sb800/MEMLIB.c 1 file changed, 2 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/34783/1 diff --git a/src/vendorcode/amd/cimx/sb800/MEMLIB.c b/src/vendorcode/amd/cimx/sb800/MEMLIB.c index 5531c62..d9eb8ff 100644 --- a/src/vendorcode/amd/cimx/sb800/MEMLIB.c +++ b/src/vendorcode/amd/cimx/sb800/MEMLIB.c @@ -46,9 +46,7 @@ *((UINT8*)Value) = *((UINT8*) ((UINTN)Address)); break; case AccWidthUint16: - //*((UINT16*)Value) = *((UINT16*) ((UINTN)Address)); //gcc break strict-aliasing rules - *((UINT8*)Value) = *((UINT8*) ((UINTN)Address)); - *((UINT8*)Value + 1) = *((UINT8*)((UINTN)Address) + 1); + *((UINT16*)Value) = *((UINT16*) ((UINTN)Address)); break; case AccWidthUint32: *((UINT32*)Value) = *((UINT32*) ((UINTN)Address)); @@ -69,9 +67,7 @@ *((UINT8*) ((UINTN)Address)) = *((UINT8*)Value); break; case AccWidthUint16: - //*((UINT16*) ((UINTN)Address)) = *((UINT16*)Value); //gcc break strict-aliasing rules - *((UINT8*)((UINTN)Address)) = *((UINT8*)Value); - *((UINT8*)((UINTN)Address) + 1) = *((UINT8*)Value + 1); + *((UINT16*) ((UINTN)Address)) = *((UINT16*)Value); break; case AccWidthUint32: *((UINT32*) ((UINTN)Address)) = *((UINT32*)Value); -- To view, visit https://review.coreboot.org/c/coreboot/+/34783 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia7e3a1eff88b855a05b33c7dafba16ed23784e43 Gerrit-Change-Number: 34783 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

4 6

Change in coreboot[master]: mainboard,autoport: Do not use others when enabling all ports
by Jacob Garber (Code Review) 20 Aug '19

20 Aug '19

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34918 ) Change subject: mainboard,autoport: Do not use others when enabling all ports ...................................................................... mainboard,autoport: Do not use others when enabling all ports Port_List is an array of 8 elements, and GCC 9 is warning that there are no 'others' when all 8 elements are explicitly initialized, which is causing the build to fail. Remove the 'others => Disabled' clause to silence this. Change-Id: Id082e7a76641438f3fb4c4d976dbd254a7053473 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> --- M src/mainboard/apple/macbookair4_2/gma-mainboard.ads M src/mainboard/google/butterfly/gma-mainboard.ads M src/mainboard/google/parrot/gma-mainboard.ads M src/mainboard/google/stout/gma-mainboard.ads M src/mainboard/hp/8460p/gma-mainboard.ads M src/mainboard/hp/8470p/gma-mainboard.ads M src/mainboard/hp/folio_9470m/gma-mainboard.ads M src/mainboard/hp/revolve_810_g1/gma-mainboard.ads M src/mainboard/lenovo/l520/gma-mainboard.ads M src/mainboard/lenovo/s230u/gma-mainboard.ads M src/mainboard/lenovo/t420/gma-mainboard.ads M src/mainboard/lenovo/t420s/gma-mainboard.ads M src/mainboard/lenovo/t430/gma-mainboard.ads M src/mainboard/lenovo/t430s/gma-mainboard.ads M src/mainboard/lenovo/t520/gma-mainboard.ads M src/mainboard/lenovo/t530/gma-mainboard.ads M src/mainboard/lenovo/x1_carbon_gen1/gma-mainboard.ads M src/mainboard/lenovo/x220/gma-mainboard.ads M src/mainboard/lenovo/x230/gma-mainboard.ads M src/mainboard/packardbell/ms2290/gma-mainboard.ads M src/mainboard/samsung/lumpy/gma-mainboard.ads M util/autoport/main.go 22 files changed, 22 insertions(+), 44 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/34918/1 diff --git a/src/mainboard/apple/macbookair4_2/gma-mainboard.ads b/src/mainboard/apple/macbookair4_2/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/apple/macbookair4_2/gma-mainboard.ads +++ b/src/mainboard/apple/macbookair4_2/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/google/butterfly/gma-mainboard.ads b/src/mainboard/google/butterfly/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/google/butterfly/gma-mainboard.ads +++ b/src/mainboard/google/butterfly/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/google/parrot/gma-mainboard.ads b/src/mainboard/google/parrot/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/google/parrot/gma-mainboard.ads +++ b/src/mainboard/google/parrot/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/google/stout/gma-mainboard.ads b/src/mainboard/google/stout/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/google/stout/gma-mainboard.ads +++ b/src/mainboard/google/stout/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/hp/8460p/gma-mainboard.ads b/src/mainboard/hp/8460p/gma-mainboard.ads index da495f6..01ae99a 100644 --- a/src/mainboard/hp/8460p/gma-mainboard.ads +++ b/src/mainboard/hp/8460p/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/hp/8470p/gma-mainboard.ads b/src/mainboard/hp/8470p/gma-mainboard.ads index da495f6..01ae99a 100644 --- a/src/mainboard/hp/8470p/gma-mainboard.ads +++ b/src/mainboard/hp/8470p/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/hp/folio_9470m/gma-mainboard.ads b/src/mainboard/hp/folio_9470m/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/hp/folio_9470m/gma-mainboard.ads +++ b/src/mainboard/hp/folio_9470m/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/hp/revolve_810_g1/gma-mainboard.ads b/src/mainboard/hp/revolve_810_g1/gma-mainboard.ads index da495f6..01ae99a 100644 --- a/src/mainboard/hp/revolve_810_g1/gma-mainboard.ads +++ b/src/mainboard/hp/revolve_810_g1/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/l520/gma-mainboard.ads b/src/mainboard/lenovo/l520/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/lenovo/l520/gma-mainboard.ads +++ b/src/mainboard/lenovo/l520/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/s230u/gma-mainboard.ads b/src/mainboard/lenovo/s230u/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/lenovo/s230u/gma-mainboard.ads +++ b/src/mainboard/lenovo/s230u/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/t420/gma-mainboard.ads b/src/mainboard/lenovo/t420/gma-mainboard.ads index e9471cb..a19bf33 100644 --- a/src/mainboard/lenovo/t420/gma-mainboard.ads +++ b/src/mainboard/lenovo/t420/gma-mainboard.ads @@ -27,7 +27,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/t420s/gma-mainboard.ads b/src/mainboard/lenovo/t420s/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/lenovo/t420s/gma-mainboard.ads +++ b/src/mainboard/lenovo/t420s/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/t430/gma-mainboard.ads b/src/mainboard/lenovo/t430/gma-mainboard.ads index e9471cb..a19bf33 100644 --- a/src/mainboard/lenovo/t430/gma-mainboard.ads +++ b/src/mainboard/lenovo/t430/gma-mainboard.ads @@ -27,7 +27,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/t430s/gma-mainboard.ads b/src/mainboard/lenovo/t430s/gma-mainboard.ads index d635d88..8cf5d2f 100644 --- a/src/mainboard/lenovo/t430s/gma-mainboard.ads +++ b/src/mainboard/lenovo/t430s/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/t520/gma-mainboard.ads b/src/mainboard/lenovo/t520/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/lenovo/t520/gma-mainboard.ads +++ b/src/mainboard/lenovo/t520/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/t530/gma-mainboard.ads b/src/mainboard/lenovo/t530/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/lenovo/t530/gma-mainboard.ads +++ b/src/mainboard/lenovo/t530/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/x1_carbon_gen1/gma-mainboard.ads b/src/mainboard/lenovo/x1_carbon_gen1/gma-mainboard.ads index 99d0a28..6dca56b 100644 --- a/src/mainboard/lenovo/x1_carbon_gen1/gma-mainboard.ads +++ b/src/mainboard/lenovo/x1_carbon_gen1/gma-mainboard.ads @@ -25,7 +25,6 @@ HDMI1, HDMI2, HDMI3, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/x220/gma-mainboard.ads b/src/mainboard/lenovo/x220/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/lenovo/x220/gma-mainboard.ads +++ b/src/mainboard/lenovo/x220/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/lenovo/x230/gma-mainboard.ads b/src/mainboard/lenovo/x230/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/lenovo/x230/gma-mainboard.ads +++ b/src/mainboard/lenovo/x230/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/packardbell/ms2290/gma-mainboard.ads b/src/mainboard/packardbell/ms2290/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/packardbell/ms2290/gma-mainboard.ads +++ b/src/mainboard/packardbell/ms2290/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/src/mainboard/samsung/lumpy/gma-mainboard.ads b/src/mainboard/samsung/lumpy/gma-mainboard.ads index d4a5d7d..e45320f 100644 --- a/src/mainboard/samsung/lumpy/gma-mainboard.ads +++ b/src/mainboard/samsung/lumpy/gma-mainboard.ads @@ -28,7 +28,6 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; diff --git a/util/autoport/main.go b/util/autoport/main.go index ab49a14..03523a2 100644 --- a/util/autoport/main.go +++ b/util/autoport/main.go @@ -925,8 +925,7 @@ HDMI2, HDMI3, Analog, - Internal, - others => Disabled); + Internal); end GMA.Mainboard; `) -- To view, visit https://review.coreboot.org/c/coreboot/+/34918 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id082e7a76641438f3fb4c4d976dbd254a7053473 Gerrit-Change-Number: 34918 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

3 5

Change in coreboot[master]: nb/amd/pi,sr5650: Remove unnecessary memory allocation
by Jacob Garber (Code Review) 20 Aug '19

20 Aug '19

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34387 ) Change subject: nb/amd/pi,sr5650: Remove unnecessary memory allocation ...................................................................... nb/amd/pi,sr5650: Remove unnecessary memory allocation add_ivrs_device_entries() is a recursive function, and each recursive call is passed a pointer to a root_level variable declared outside the function. In an attempt to make the function self-contained, the initial call is made with the root_level pointer set to NULL, and then the function attempts to detect this and allocate a root_level variable for the rest of the calls. This makes memory management very tricky - for example, the pi code incorrectly attempts to free the root_level variable at the end of *each* recursive call, which only avoids being a double-free because free() in coreboot is currently a no-op. Let's dispense with this memory weirdness and declare root_level as a local variable outside the first function call instead. Change-Id: Ifd63ee368fb89345b9b42ccb86cebcca64f32ac8 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Found-by: Coverity CID 1362811 --- M src/northbridge/amd/pi/00730F01/northbridge.c M src/southbridge/amd/sr5650/sr5650.c 2 files changed, 4 insertions(+), 19 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34387/1 diff --git a/src/northbridge/amd/pi/00730F01/northbridge.c b/src/northbridge/amd/pi/00730F01/northbridge.c index 377d91e..ba17c61 100644 --- a/src/northbridge/amd/pi/00730F01/northbridge.c +++ b/src/northbridge/amd/pi/00730F01/northbridge.c @@ -491,11 +491,6 @@ unsigned int header_type; unsigned int is_pcie; - if (!root_level) { - root_level = malloc(sizeof(int8_t)); - *root_level = -1; - } - if (dev->path.type == DEVICE_PATH_PCI) { if ((dev->bus->secondary == 0x0) && @@ -536,8 +531,6 @@ sibling->sibling) add_ivrs_device_entries(dev, sibling, depth + 1, depth, root_level, current, length); - - free(root_level); } unsigned long acpi_fill_ivrs_ioapic(acpi_ivrs_t *ivrs, unsigned long current) @@ -643,7 +636,8 @@ current += 8; /* Describe PCI devices */ - add_ivrs_device_entries(NULL, all_devices, 0, -1, NULL, &current, + int8_t root_level = -1; + add_ivrs_device_entries(NULL, all_devices, 0, -1, &root_level, &current, &ivrs->ivhd.length); /* Describe IOAPICs */ diff --git a/src/southbridge/amd/sr5650/sr5650.c b/src/southbridge/amd/sr5650/sr5650.c index 90ca564..8131e77 100644 --- a/src/southbridge/amd/sr5650/sr5650.c +++ b/src/southbridge/amd/sr5650/sr5650.c @@ -725,13 +725,6 @@ struct device *sibling; struct bus *link; - if (!root_level) { - root_level = malloc(sizeof(int8_t)); - if (root_level == NULL) - die("Error: Could not allocate a byte!\n"); - *root_level = -1; - } - if ((dev->path.type == DEVICE_PATH_PCI) && (dev->bus->secondary == 0x0) && (dev->path.pci.devfn == 0x0)) *root_level = depth; @@ -798,9 +791,6 @@ sibling = sibling->sibling) add_ivrs_device_entries(dev, sibling, depth + 1, depth, root_level, current, length); - - if (depth == 0) - free(root_level); } unsigned long acpi_fill_mcfg(unsigned long current) @@ -879,7 +869,8 @@ current += 8; /* Describe PCI devices */ - add_ivrs_device_entries(NULL, all_devices, 0, -1, NULL, &current, + int8_t root_level = -1; + add_ivrs_device_entries(NULL, all_devices, 0, -1, &root_level, &current, &ivrs->ivhd.length); /* Describe IOAPICs */ -- To view, visit https://review.coreboot.org/c/coreboot/+/34387 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifd63ee368fb89345b9b42ccb86cebcca64f32ac8 Gerrit-Change-Number: 34387 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

2 4

Change in coreboot[master]: util/nvidia/cbootimage: Update to upstream master
by Jacob Garber (Code Review) 20 Aug '19

20 Aug '19

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34409 ) Change subject: util/nvidia/cbootimage: Update to upstream master ...................................................................... util/nvidia/cbootimage: Update to upstream master This brings in 4 new commits from the upstream repository. 65a6d94 Free image buffer on read error 9de64c7 Fix various abort(), crashes, and memory errors 7c9db58 Bump to version 1.8 3b3c3cc Use C99 uintXX_t instead of implementation-specific u_intXX_t types Change-Id: If949309a7481537de6529c205fe745d5509906a9 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> --- M util/nvidia/cbootimage 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/34409/1 diff --git a/util/nvidia/cbootimage b/util/nvidia/cbootimage index 64045f9..65a6d94 160000 --- a/util/nvidia/cbootimage +++ b/util/nvidia/cbootimage @@ -1 +1 @@ -Subproject commit 64045f993c2cd8989838aeaad3d22107d96d5596 +Subproject commit 65a6d94dd5f442578551e0a81ecbe5235e673fd4 -- To view, visit https://review.coreboot.org/c/coreboot/+/34409 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If949309a7481537de6529c205fe745d5509906a9 Gerrit-Change-Number: 34409 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

2 3

Change in coreboot[master]: src: Remove variable length arrays
by Patrick Georgi (Code Review) 20 Aug '19

20 Aug '19

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33821 ) Change subject: src: Remove variable length arrays ...................................................................... src: Remove variable length arrays Variable length arrays were a feature added in C99 that allows the length of an array to be determined at runtime. Eg. int sum(size_t n) { int arr[n]; ... } This adds a small amount of runtime overhead, but is also very dangerous, since it allows use of an unlimited amount of stack memory, potentially leading to stack overflow. This is only worsened in coreboot, which often has very little stack space to begin with. Citing concerns like this, all instances of VLA's were recently removed from the Linux kernel. In the immortal words of Linus Torvalds [0], AND USING VLA'S IS ACTIVELY STUPID! It generates much more code, and much _slower_ code (and more fragile code), than just using a fixed key size would have done. [...] Anyway, some of these are definitely easy to just fix, and using VLA's is actively bad not just for security worries, but simply because VLA's are a really horribly bad idea in general in the kernel. This patch follows suit and zaps all VLA's in coreboot. Some of the existing VLA's are accidental ones, and all but one can be replaced with small fixed-size buffers. The single tricky exception is in the SPI controller interface, which will require a rewrite of old drivers to remove [1]. [0] https://lkml.org/lkml/2018/3/7/621 [1] https://ticket.coreboot.org/issues/217 Change-Id: I7d9d1ddadbf1cee5f695165bbe3f0effb7bd32b9 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Reviewed-on: https://review.coreboot.org/c/coreboot/+/33821 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Patrick Georgi <pgeorgi(a)google.com> --- M src/arch/x86/smbios.c M src/drivers/spi/spi_flash.c M src/northbridge/amd/amdmct/mct_ddr3/mctdqs_d.c M src/northbridge/amd/amdmct/mct_ddr3/mhwlc_d.c M src/soc/intel/baytrail/spi.c M src/soc/intel/braswell/spi.c M src/soc/intel/broadwell/spi.c M src/soc/intel/fsp_baytrail/spi.c M src/soc/intel/skylake/acpi.c M src/southbridge/intel/common/spi.c M src/southbridge/intel/fsp_rangeley/spi.c M src/vendorcode/google/chromeos/sar.c 12 files changed, 87 insertions(+), 65 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 346e874..f95516e 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -275,20 +275,18 @@ static void smbios_fill_dimm_part_number(const char *part_number, struct smbios_type17 *t) { - const size_t trimmed_buffer_size = DIMM_INFO_PART_NUMBER_SIZE; - int invalid; size_t i, len; - char trimmed_part_number[trimmed_buffer_size]; + char trimmed_part_number[DIMM_INFO_PART_NUMBER_SIZE]; - strncpy(trimmed_part_number, part_number, trimmed_buffer_size); - trimmed_part_number[trimmed_buffer_size - 1] = '\0'; + strncpy(trimmed_part_number, part_number, sizeof(trimmed_part_number)); + trimmed_part_number[sizeof(trimmed_part_number) - 1] = '\0'; /* * SPD mandates that unused characters be represented with a ' '. * We don't want to publish the whitespace in the SMBIOS tables. */ - trim_trailing_whitespace(trimmed_part_number, trimmed_buffer_size); + trim_trailing_whitespace(trimmed_part_number, sizeof(trimmed_part_number)); len = strlen(trimmed_part_number); @@ -304,8 +302,7 @@ /* Null String in Part Number will have "None" instead. */ t->part_number = smbios_add_string(t->eos, "None"); } else if (invalid) { - char string_buffer[trimmed_buffer_size + - 10 /* strlen("Invalid ()") */]; + char string_buffer[sizeof(trimmed_part_number) + 10]; snprintf(string_buffer, sizeof(string_buffer), "Invalid (%s)", trimmed_part_number); diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index cfa500e..e2485ab 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -100,6 +100,7 @@ #pragma GCC diagnostic push #if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic ignored "-Wstack-usage=" +#pragma GCC diagnostic ignored "-Wvla" #endif int spi_flash_cmd_write(const struct spi_slave *spi, const u8 *cmd, size_t cmd_len, const void *data, size_t data_len) diff --git a/src/northbridge/amd/amdmct/mct_ddr3/mctdqs_d.c b/src/northbridge/amd/amdmct/mct_ddr3/mctdqs_d.c index 3f4f1bf..d34b2dc 100644 --- a/src/northbridge/amd/amdmct/mct_ddr3/mctdqs_d.c +++ b/src/northbridge/amd/amdmct/mct_ddr3/mctdqs_d.c @@ -1252,11 +1252,15 @@ stop_dram_dqs_training_pattern_fam15(pMCTstat, pDCTstat, dct, Receiver); } +#define LANE_DIFF 1 + /* DQS Position Training * Algorithm detailed in the Fam15h BKDG Rev. 3.14 section 2.10.5.8.4 */ static uint8_t TrainDQSRdWrPos_D_Fam15(struct MCTStatStruc *pMCTstat, - struct DCTStatStruc *pDCTstat, uint8_t dct, uint8_t receiver_start, uint8_t receiver_end, uint8_t lane_start, uint8_t lane_end) + struct DCTStatStruc *pDCTstat, + uint8_t dct, uint8_t receiver_start, + uint8_t receiver_end, uint8_t lane_start) { uint8_t dimm; uint8_t lane; @@ -1276,7 +1280,8 @@ uint16_t current_read_dqs_delay[MAX_BYTE_LANES]; uint16_t current_write_dqs_delay[MAX_BYTE_LANES]; uint8_t passing_dqs_delay_found[MAX_BYTE_LANES]; - uint8_t dqs_results_array[2][(lane_end - lane_start)][32][48]; /* [rank][lane][write step][read step + 16] */ + /* [rank][lane][write step][read step + 16] */ + uint8_t dqs_results_array[2][LANE_DIFF][32][48]; uint8_t last_pos = 0; uint8_t cur_count = 0; @@ -1286,6 +1291,8 @@ uint32_t index_reg = 0x98; uint32_t dev = pDCTstat->dev_dct; + uint8_t lane_end = lane_start + LANE_DIFF; + uint8_t lane_count; lane_count = get_available_lane_count(pMCTstat, pDCTstat); @@ -1734,7 +1741,10 @@ Calc_SetMaxRdLatency_D_Fam15(pMCTstat, pDCTstat, dct, 0); /* 2.10.5.8.3 (4 B) */ - dqs_results_array[current_phy_phase_delay[lane]] = TrainDQSRdWrPos_D_Fam15(pMCTstat, pDCTstat, dct, Receiver, Receiver + 2, lane, lane + 1); + dqs_results_array[current_phy_phase_delay[lane]] = + TrainDQSRdWrPos_D_Fam15(pMCTstat, pDCTstat, dct, + Receiver, Receiver + 2, + lane); if (dqs_results_array[current_phy_phase_delay[lane]]) lane_success_count++; @@ -1790,7 +1800,9 @@ /* Update hardware registers with final values */ write_dqs_receiver_enable_control_registers(current_phy_phase_delay, dev, dct, dimm, index_reg); - TrainDQSRdWrPos_D_Fam15(pMCTstat, pDCTstat, dct, Receiver, Receiver + 2, lane, lane + 1); + TrainDQSRdWrPos_D_Fam15(pMCTstat, pDCTstat, dct, + Receiver, Receiver + 2, + lane); break; } prev = dqs_results_array[current_phy_phase_delay[lane]]; diff --git a/src/northbridge/amd/amdmct/mct_ddr3/mhwlc_d.c b/src/northbridge/amd/amdmct/mct_ddr3/mhwlc_d.c index 76f72ae..353aa7a 100644 --- a/src/northbridge/amd/amdmct/mct_ddr3/mhwlc_d.c +++ b/src/northbridge/amd/amdmct/mct_ddr3/mhwlc_d.c @@ -15,6 +15,7 @@ */ #include <stdint.h> +#include <assert.h> #include <console/console.h> #include <northbridge/amd/amdfam10/amdfam10.h> @@ -31,6 +32,8 @@ void setWLByteDelay(struct DCTStatStruc *pDCTstat, uint8_t dct, u8 ByteLane, u8 dimm, u8 targetAddr, uint8_t pass, uint8_t lane_count); void getWLByteDelay(struct DCTStatStruc *pDCTstat, uint8_t dct, u8 ByteLane, u8 dimm, uint8_t pass, uint8_t nibble, uint8_t lane_count); +#define MAX_LANE_COUNT 9 + /*----------------------------------------------------------------------------- * uint8_t AgesaHwWlPhase1(SPDStruct *SPDData,MCTStruct *MCTData, DCTStruct *DCTData, * u8 Dimm, u8 Pass) @@ -185,8 +188,10 @@ lane_count = get_available_lane_count(pMCTstat, pDCTstat); + assert(lane_count <= MAX_LANE_COUNT); + if (is_fam15h()) { - int32_t gross_diff[lane_count]; + int32_t gross_diff[MAX_LANE_COUNT]; int32_t cgd = pDCTData->WLCriticalGrossDelayPrevPass; uint8_t index = (uint8_t)(lane_count * dimm); @@ -274,9 +279,11 @@ lane_count = get_available_lane_count(pMCTstat, pDCTstat); + assert(lane_count <= MAX_LANE_COUNT); + if (is_fam15h()) { uint32_t dword; - int32_t gross_diff[lane_count]; + int32_t gross_diff[MAX_LANE_COUNT]; int32_t cgd = pDCTData->WLCriticalGrossDelayPrevPass; uint8_t index = (uint8_t)(lane_count * dimm); @@ -1005,6 +1012,8 @@ lane_count = get_available_lane_count(pMCTstat, pDCTstat); + assert(lane_count <= MAX_LANE_COUNT); + if (is_fam15h()) { /* MemClkFreq: 0x4: 333MHz; 0x6: 400MHz; 0xa: 533MHz; 0xe: 667MHz; 0x12: 800MHz; 0x16: 933MHz */ MemClkFreq = get_Bits(pDCTData, dct, pDCTData->NodeId, @@ -1168,8 +1177,8 @@ /* From BKDG, Write Leveling Seed Value. */ if (is_fam15h()) { uint32_t RegisterDelay; - int32_t SeedTotal[lane_count]; - int32_t SeedTotalPreScaling[lane_count]; + int32_t SeedTotal[MAX_LANE_COUNT]; + int32_t SeedTotalPreScaling[MAX_LANE_COUNT]; uint32_t WrDqDqsEarly; uint8_t AddrCmdPrelaunch = 0; /* TODO: Fetch the correct value from RC2[0] */ diff --git a/src/soc/intel/baytrail/spi.c b/src/soc/intel/baytrail/spi.c index d5b962f..26b717c 100644 --- a/src/soc/intel/baytrail/spi.c +++ b/src/soc/intel/baytrail/spi.c @@ -248,6 +248,8 @@ return (void *)sbase; } +#define MENU_BYTES member_size(struct ich9_spi_regs, opmenu) + void spi_init(void) { ich9_spi_regs *ich9_spi = spi_regs(); @@ -332,7 +334,7 @@ static int spi_setup_opcode(spi_transaction *trans) { uint16_t optypes; - uint8_t opmenu[cntlr.menubytes]; + uint8_t opmenu[MENU_BYTES]; trans->opcode = trans->out[0]; spi_use_out(trans, 1); @@ -353,13 +355,12 @@ return 0; read_reg(cntlr.opmenu, opmenu, sizeof(opmenu)); - for (opcode_index = 0; opcode_index < cntlr.menubytes; - opcode_index++) { + for (opcode_index = 0; opcode_index < ARRAY_SIZE(opmenu); opcode_index++) { if (opmenu[opcode_index] == trans->opcode) break; } - if (opcode_index == cntlr.menubytes) { + if (opcode_index == ARRAY_SIZE(opmenu)) { printk(BIOS_DEBUG, "ICH SPI: Opcode %x not found\n", trans->opcode); return -1; diff --git a/src/soc/intel/braswell/spi.c b/src/soc/intel/braswell/spi.c index b968283..00ec48f 100644 --- a/src/soc/intel/braswell/spi.c +++ b/src/soc/intel/braswell/spi.c @@ -221,6 +221,8 @@ return (void *)sbase; } +#define MENU_BYTES member_size(struct ich9_spi_regs, opmenu) + void spi_init(void) { ich9_spi_regs *ich9_spi; @@ -310,7 +312,7 @@ static int spi_setup_opcode(spi_transaction *trans) { uint16_t optypes; - uint8_t opmenu[cntlr.menubytes]; + uint8_t opmenu[MENU_BYTES]; trans->opcode = trans->out[0]; spi_use_out(trans, 1); @@ -332,13 +334,12 @@ return 0; read_reg(cntlr.opmenu, opmenu, sizeof(opmenu)); - for (opcode_index = 0; opcode_index < cntlr.menubytes; - opcode_index++) { + for (opcode_index = 0; opcode_index < ARRAY_SIZE(opmenu); opcode_index++) { if (opmenu[opcode_index] == trans->opcode) break; } - if (opcode_index == cntlr.menubytes) { + if (opcode_index == ARRAY_SIZE(opmenu)) { printk(BIOS_DEBUG, "ICH SPI: Opcode %x not found\n", trans->opcode); return -1; diff --git a/src/soc/intel/broadwell/spi.c b/src/soc/intel/broadwell/spi.c index 01d2830..ac893ea 100644 --- a/src/soc/intel/broadwell/spi.c +++ b/src/soc/intel/broadwell/spi.c @@ -231,6 +231,8 @@ writel_(ichspi_bbar, cntlr.bbar); } +#define MENU_BYTES member_size(struct ich9_spi_regs, opmenu) + void spi_init(void) { uint8_t *rcrb; /* Root Complex Register Block */ @@ -332,7 +334,7 @@ static int spi_setup_opcode(spi_transaction *trans) { uint16_t optypes; - uint8_t opmenu[cntlr.menubytes]; + uint8_t opmenu[MENU_BYTES]; trans->opcode = trans->out[0]; spi_use_out(trans, 1); @@ -354,13 +356,12 @@ return 0; read_reg(cntlr.opmenu, opmenu, sizeof(opmenu)); - for (opcode_index = 0; opcode_index < cntlr.menubytes; - opcode_index++) { + for (opcode_index = 0; opcode_index < ARRAY_SIZE(opmenu); opcode_index++) { if (opmenu[opcode_index] == trans->opcode) break; } - if (opcode_index == cntlr.menubytes) { + if (opcode_index == ARRAY_SIZE(opmenu)) { printk(BIOS_DEBUG, "ICH SPI: Opcode %x not found\n", trans->opcode); return -1; diff --git a/src/soc/intel/fsp_baytrail/spi.c b/src/soc/intel/fsp_baytrail/spi.c index 9375d19..0b52ea96 100644 --- a/src/soc/intel/fsp_baytrail/spi.c +++ b/src/soc/intel/fsp_baytrail/spi.c @@ -237,6 +237,8 @@ return (void *)sbase; } +#define MENU_BYTES member_size(struct ich9_spi_regs, opmenu) + void spi_init(void) { ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); @@ -314,7 +316,7 @@ { ich_spi_controller *cntlr = car_get_var_ptr(&g_cntlr); uint16_t optypes; - uint8_t opmenu[cntlr->menubytes]; + uint8_t opmenu[MENU_BYTES]; trans->opcode = trans->out[0]; spi_use_out(trans, 1); @@ -335,13 +337,12 @@ return 0; read_reg(cntlr->opmenu, opmenu, sizeof(opmenu)); - for (opcode_index = 0; opcode_index < cntlr->menubytes; - opcode_index++) { + for (opcode_index = 0; opcode_index < ARRAY_SIZE(opmenu); opcode_index++) { if (opmenu[opcode_index] == trans->opcode) break; } - if (opcode_index == cntlr->menubytes) { + if (opcode_index == ARRAY_SIZE(opmenu)) { printk(BIOS_DEBUG, "ICH SPI: Opcode %x not found\n", trans->opcode); return -1; diff --git a/src/soc/intel/skylake/acpi.c b/src/soc/intel/skylake/acpi.c index 869ca7f..aa51cbe 100644 --- a/src/soc/intel/skylake/acpi.c +++ b/src/soc/intel/skylake/acpi.c @@ -350,25 +350,26 @@ fadt->x_gpe1_blk.addrh = 0x0; } -static void generate_c_state_entries(int s0ix_enable, int max_cstate) +static void write_c_state_entries(acpi_cstate_t *map, const int *set, size_t max_c_state) { - - acpi_cstate_t map[max_cstate]; - int *set; - int i; - - if (s0ix_enable) - set = cstate_set_s0ix; - else - set = cstate_set_non_s0ix; - - for (i = 0; i < max_cstate; i++) { + for (size_t i = 0; i < max_c_state; i++) { memcpy(&map[i], &cstate_map[set[i]], sizeof(acpi_cstate_t)); map[i].ctype = i + 1; } /* Generate C-state tables */ - acpigen_write_CST_package(map, ARRAY_SIZE(map)); + acpigen_write_CST_package(map, max_c_state); +} + +static void generate_c_state_entries(int s0ix_enable) +{ + if (s0ix_enable) { + acpi_cstate_t map[ARRAY_SIZE(cstate_set_s0ix)]; + write_c_state_entries(map, cstate_set_s0ix, ARRAY_SIZE(map)); + } else { + acpi_cstate_t map[ARRAY_SIZE(cstate_set_non_s0ix)]; + write_c_state_entries(map, cstate_set_non_s0ix, ARRAY_SIZE(map)); + } } static int calculate_power(int tdp, int p1_ratio, int ratio) @@ -506,12 +507,6 @@ int numcpus = totalcores/cores_per_package; config_t *config = config_of_path(SA_DEVFN_ROOT); int is_s0ix_enable = config->s0ix_enable; - int max_c_state; - - if (is_s0ix_enable) - max_c_state = ARRAY_SIZE(cstate_set_s0ix); - else - max_c_state = ARRAY_SIZE(cstate_set_non_s0ix); printk(BIOS_DEBUG, "Found %d CPU(s) with %d core(s) each.\n", numcpus, cores_per_package); @@ -534,8 +529,7 @@ cpu_id*cores_per_package+core_id, pcontrol_blk, plen); /* Generate C-state tables */ - generate_c_state_entries(is_s0ix_enable, - max_c_state); + generate_c_state_entries(is_s0ix_enable); if (config->eist_enable) { /* Generate P-state tables */ diff --git a/src/southbridge/intel/common/spi.c b/src/southbridge/intel/common/spi.c index 0095dd9..6569351 100644 --- a/src/southbridge/intel/common/spi.c +++ b/src/southbridge/intel/common/spi.c @@ -264,6 +264,12 @@ writel_(ichspi_bbar, cntlr->bbar); } +#if CONFIG(SOUTHBRIDGE_INTEL_I82801GX) +#define MENU_BYTES member_size(struct ich7_spi_regs, opmenu) +#else +#define MENU_BYTES member_size(struct ich9_spi_regs, opmenu) +#endif + void spi_init(void) { struct ich_spi_controller *cntlr = &g_cntlr; @@ -410,7 +416,7 @@ { struct ich_spi_controller *cntlr = &g_cntlr; uint16_t optypes; - uint8_t opmenu[cntlr->menubytes]; + uint8_t opmenu[MENU_BYTES]; trans->opcode = trans->out[0]; spi_use_out(trans, 1); @@ -432,13 +438,12 @@ return 0; read_reg(cntlr->opmenu, opmenu, sizeof(opmenu)); - for (opcode_index = 0; opcode_index < cntlr->menubytes; - opcode_index++) { + for (opcode_index = 0; opcode_index < ARRAY_SIZE(opmenu); opcode_index++) { if (opmenu[opcode_index] == trans->opcode) break; } - if (opcode_index == cntlr->menubytes) { + if (opcode_index == ARRAY_SIZE(opmenu)) { printk(BIOS_DEBUG, "ICH SPI: Opcode %x not found\n", trans->opcode); return -1; diff --git a/src/southbridge/intel/fsp_rangeley/spi.c b/src/southbridge/intel/fsp_rangeley/spi.c index afd89a7..d2f2a0b 100644 --- a/src/southbridge/intel/fsp_rangeley/spi.c +++ b/src/southbridge/intel/fsp_rangeley/spi.c @@ -310,6 +310,8 @@ return 0; } +#define MENU_BYTES member_size(struct ich10_spi_regs, opmenu) + void spi_init(void) { int ich_version = 0; @@ -444,7 +446,7 @@ static int spi_setup_opcode(spi_transaction *trans) { uint16_t optypes; - uint8_t opmenu[cntlr.menubytes]; + uint8_t opmenu[MENU_BYTES]; trans->opcode = trans->out[0]; spi_use_out(trans, 1); @@ -465,13 +467,12 @@ return 0; read_reg(cntlr.opmenu, opmenu, sizeof(opmenu)); - for (opcode_index = 0; opcode_index < cntlr.menubytes; - opcode_index++) { + for (opcode_index = 0; opcode_index < ARRAY_SIZE(opmenu); opcode_index++) { if (opmenu[opcode_index] == trans->opcode) break; } - if (opcode_index == cntlr.menubytes) { + if (opcode_index == ARRAY_SIZE(opmenu)) { printk(BIOS_DEBUG, "ICH SPI: Opcode %x not found\n", trans->opcode); return -1; diff --git a/src/vendorcode/google/chromeos/sar.c b/src/vendorcode/google/chromeos/sar.c index bbcb211..01de60c 100644 --- a/src/vendorcode/google/chromeos/sar.c +++ b/src/vendorcode/google/chromeos/sar.c @@ -61,11 +61,10 @@ const char *wifi_sar_limit_key = CROS_VPD_WIFI_SAR_NAME; /* vpd_gets() reads in one less than size characters from the VPD * with a terminating null byte ('\0') stored as the last character into - * the buffer, thus the increasing by 1 for buffer_size. */ - const size_t buffer_size = (sizeof(struct wifi_sar_limits) / - sizeof(uint8_t)) * 2 + 1; - char wifi_sar_limit_str[buffer_size]; + * the buffer, thus the increasing by 1 for the buffer size. */ + char wifi_sar_limit_str[2 * sizeof(struct wifi_sar_limits) + 1]; uint8_t bin_buffer[sizeof(struct wifi_sar_limits)]; + const size_t buffer_size = ARRAY_SIZE(wifi_sar_limit_str); size_t sar_cbfs_len, sar_expected_len, bin_buff_adjusted_size; /* keep it backward compatible. Some older platform are shipping -- To view, visit https://review.coreboot.org/c/coreboot/+/33821 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7d9d1ddadbf1cee5f695165bbe3f0effb7bd32b9 Gerrit-Change-Number: 33821 Gerrit-PatchSet: 15 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: merged

1 0

Change in coreboot[master]: src: Remove variable length arrays
by Patrick Georgi (Code Review) 20 Aug '19

20 Aug '19

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33821 ) Change subject: src: Remove variable length arrays ...................................................................... Patch Set 14: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/33821 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7d9d1ddadbf1cee5f695165bbe3f0effb7bd32b9 Gerrit-Change-Number: 33821 Gerrit-PatchSet: 14 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 20 Aug 2019 15:27:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in em100[master]: em100: Fix malloc() failure checks
by Jacob Garber (Code Review) 20 Aug '19

20 Aug '19

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/34916 ) Change subject: em100: Fix malloc() failure checks ...................................................................... em100: Fix malloc() failure checks The checks should be readback == NULL, since those are the buffers that we just allocated. The check for data == NULL was already done earlier. We also need to free() the data buffer if these allocations fail. Change-Id: Ibc798673a3ce6796bf036bd962da248d12dee47c Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> --- M em100.c 1 file changed, 4 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/em100 refs/changes/16/34916/1 diff --git a/em100.c b/em100.c index 64a4ab2..f861ffe 100644 --- a/em100.c +++ b/em100.c @@ -827,8 +827,9 @@ if (spi_start_address) { readback = malloc(maxlen); - if (data == NULL) { + if (readback == NULL) { printf("FATAL: couldn't allocate memory(size: %x)\n", maxlen); + free(data); return 1; } done = read_sdram(&em100, readback, 0, maxlen); @@ -841,8 +842,9 @@ if (verify) { readback = malloc(length); - if (data == NULL) { + if (readback == NULL) { printf("FATAL: couldn't allocate memory\n"); + free(data); return 1; } done = read_sdram(&em100, readback, spi_start_address, length); -- To view, visit https://review.coreboot.org/c/em100/+/34916 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: em100 Gerrit-Branch: master Gerrit-Change-Id: Ibc798673a3ce6796bf036bd962da248d12dee47c Gerrit-Change-Number: 34916 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

2 2

Change in coreboot[master]: util/cbfstool: Remove unused assignment
by Jacob Garber (Code Review) 20 Aug '19

20 Aug '19

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34644 ) Change subject: util/cbfstool: Remove unused assignment ...................................................................... util/cbfstool: Remove unused assignment This variable is overwritten on one branch of the next if statement, and the other branch returns, so this assignment does nothing. Change-Id: I63737929d47c882bbcf637182bc8bf73c19daa9f Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Found-by: scan-build 8.0.0 --- M util/cbfstool/ifittool.c 1 file changed, 0 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/34644/1 diff --git a/util/cbfstool/ifittool.c b/util/cbfstool/ifittool.c index 3b16c3f..dce37c8 100644 --- a/util/cbfstool/ifittool.c +++ b/util/cbfstool/ifittool.c @@ -336,7 +336,6 @@ case ADD_REGI_OP: { struct buffer region; - addr = 0; if (partitioned_file_read_region(&region, image_file, name)) { addr = -convert_to_from_top_aligned(&region, 0); -- To view, visit https://review.coreboot.org/c/coreboot/+/34644 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I63737929d47c882bbcf637182bc8bf73c19daa9f Gerrit-Change-Number: 34644 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

2 2

Change in em100[master]: em100: Remove unused assignment
by Jacob Garber (Code Review) 20 Aug '19

20 Aug '19

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/34917 ) Change subject: em100: Remove unused assignment ...................................................................... em100: Remove unused assignment The 'done' variable is only used along the verification code path, not this one. Change-Id: Ibcdde9548d5ecd55ebecf7d824cf679a28e2322c Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Found-by: Coverity CID 205421 --- M em100.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/em100 refs/changes/17/34917/1 diff --git a/em100.c b/em100.c index 64a4ab2..d2199c5 100644 --- a/em100.c +++ b/em100.c @@ -784,7 +784,6 @@ if (filename) { int maxlen = desiredchip ? chip->size : 0x4000000; /* largest size - 64MB */ void *data = malloc(maxlen); - int done; void *readback = NULL; if (data == NULL) { @@ -831,7 +830,7 @@ printf("FATAL: couldn't allocate memory(size: %x)\n", maxlen); return 1; } - done = read_sdram(&em100, readback, 0, maxlen); + read_sdram(&em100, readback, 0, maxlen); memcpy((unsigned char*)readback + spi_start_address, data, length); write_sdram(&em100, (unsigned char*)readback, 0x00000000, maxlen); free(readback); @@ -840,6 +839,7 @@ } if (verify) { + int done; readback = malloc(length); if (data == NULL) { printf("FATAL: couldn't allocate memory\n"); -- To view, visit https://review.coreboot.org/c/em100/+/34917 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: em100 Gerrit-Branch: master Gerrit-Change-Id: Ibcdde9548d5ecd55ebecf7d824cf679a28e2322c Gerrit-Change-Number: 34917 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

4 6

Change in coreboot[master]: mb/amd/serengeti_cheetah_fam10: Add null pointer check
by Jacob Garber (Code Review) 20 Aug '19

20 Aug '19

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34353 ) Change subject: mb/amd/serengeti_cheetah_fam10: Add null pointer check ...................................................................... mb/amd/serengeti_cheetah_fam10: Add null pointer check Print an error message and die if the PCI device cannot be found. Change-Id: I10c58502658ebf12d1a8fe826ee7d47a618fd1c8 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Found-by: Coverity CID 1403000 --- M src/mainboard/amd/serengeti_cheetah_fam10/get_bus_conf.c 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/34353/1 diff --git a/src/mainboard/amd/serengeti_cheetah_fam10/get_bus_conf.c b/src/mainboard/amd/serengeti_cheetah_fam10/get_bus_conf.c index e1f2409..9e73ddf 100644 --- a/src/mainboard/amd/serengeti_cheetah_fam10/get_bus_conf.c +++ b/src/mainboard/amd/serengeti_cheetah_fam10/get_bus_conf.c @@ -37,6 +37,10 @@ dev = dev_find_slot(busn, PCI_DEVFN(devn,0)); + if (dev == NULL) + die("ERROR - could not find PCI %02x:%02x.0, using defaults\n", + busn, PCI_DEVFN(devn, 0)); + switch (dev->device) { case 0x7458: /* 8132 */ id = 1; -- To view, visit https://review.coreboot.org/c/coreboot/+/34353 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I10c58502658ebf12d1a8fe826ee7d47a618fd1c8 Gerrit-Change-Number: 34353 Gerrit-PatchSet: 1 Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

2 5

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit August 2019