July 2019 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Fix -Wunused-parameter issues
by Jacob Garber (Code Review) July 30, 2019

July 30, 2019

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30410 ) Change subject: Fix -Wunused-parameter issues ...................................................................... Patch Set 5: Code-Review+1 I'll let this sit for a little longer in case someone else wants to chime in, then +2. -- To view, visit https://review.coreboot.org/c/flashrom/+/30410 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If831398c91e9f49bd5216a03ed4caaa2a7ab02bd Gerrit-Change-Number: 30410 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 30 Jul 2019 18:09:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Fix -Wunused-parameter issues
by Nico Huber (Code Review) July 30, 2019

July 30, 2019

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30410 ) Change subject: Fix -Wunused-parameter issues ...................................................................... Patch Set 5: > > Are there any of these parameters that could be deleted, or were they all needed? > > I removed unused parameters from static functions. Didn't check the APIs, though. Much of the noise seems to be due to the usage of global variables, btw. Can't say how often I wrote `(void)flash;`, where `flash` is a reference to the context that was unused and something global instead. Sooner or later, those should be cleaned up anyway. If we take libflashrom serious. -- To view, visit https://review.coreboot.org/c/flashrom/+/30410 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If831398c91e9f49bd5216a03ed4caaa2a7ab02bd Gerrit-Change-Number: 30410 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 30 Jul 2019 17:32:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Fix -Wunused-parameter issues
by Nico Huber (Code Review) July 30, 2019

July 30, 2019

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30410 ) Change subject: Fix -Wunused-parameter issues ...................................................................... Patch Set 5: > Are there any of these parameters that could be deleted, or were they all needed? I removed unused parameters from static functions. Didn't check the APIs, though. -- To view, visit https://review.coreboot.org/c/flashrom/+/30410 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If831398c91e9f49bd5216a03ed4caaa2a7ab02bd Gerrit-Change-Number: 30410 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 30 Jul 2019 17:25:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Fix -Wunused-parameter issues
by Jacob Garber (Code Review) July 30, 2019

July 30, 2019

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30410 ) Change subject: Fix -Wunused-parameter issues ...................................................................... Patch Set 5: Are there any of these parameters that could be deleted, or were they all needed? -- To view, visit https://review.coreboot.org/c/flashrom/+/30410 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If831398c91e9f49bd5216a03ed4caaa2a7ab02bd Gerrit-Change-Number: 30410 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 30 Jul 2019 17:17:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Fix -Wsign-compare trouble
by Jacob Garber (Code Review) July 30, 2019

July 30, 2019

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30409 ) Change subject: Fix -Wsign-compare trouble ...................................................................... Patch Set 5: Code-Review+2 (5 comments) https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c File cbtable.c: https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@43 PS5, Line 43: image + size - 0x10 > Yes, it does. So what is the problem? […] Yes, sorry, there is no problem, I was just double-checking that that was how it was parsed (since the other way can cause problems). https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@51 PS5, Line 51: image + size - 0x80 > Ditto Done https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@69 PS5, Line 69: image + size - mb_part_offset > Ditto Done https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@70 PS5, Line 70: image + size - mb_vendor_offset > Ditto Done https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c File fmap.c: https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c@99 PS5, Line 99: (len - sizeof(struct fmap)) > Sure, but not part of this patch (the problem is in the subtraction not the […] Gah! C is crazy. We only use off_t since this function needs to return negative error values (mumble mumble grumble), so we could just use size_t for `offset`, add an if check before the loop, and then do a cast to off_t at the very end when we return. Either that, or leave it as-is for now and change it in a follow-up. (Also, now that I think about it the signed version of size_t is ssize_t, not off_t, so perhaps a follow-up is better.) -- To view, visit https://review.coreboot.org/c/flashrom/+/30409 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I08895543ffb7a48058bcf91ef6500ca113f2d305 Gerrit-Change-Number: 30409 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 30 Jul 2019 17:16:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Jacob Garber <jgarber1(a)ualberta.ca> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Fix -Wsign-compare trouble
by Nico Huber (Code Review) July 30, 2019

July 30, 2019

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30409 ) Change subject: Fix -Wsign-compare trouble ...................................................................... Patch Set 5: (2 comments) https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c File cbtable.c: https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@43 PS5, Line 43: image + size - 0x10 > I think this parses as (image + size) - 0x10, right? Yes, it does. So what is the problem? image + (size - 0x10) might produce an integer overflow (so it would matter that we changed the type to unsigned). But (image + size) - 0x10 does not (unless `size` isn't the real size of `image`, or `size < 0x10`, but these potential issues existed before). So it seems regression free to me. https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c File fmap.c: https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c@99 PS5, Line 99: (len - sizeof(struct fmap)) > If len < sizeof(struct fmap), we could just do an early return -2, so there's no problem of overflow […] Sure, but not part of this patch (the problem is in the subtraction not the comparison). I tried to reason further about how to make the subtraction safe without an explicit `if` (i.e. by ensuring that we'd have a negative number on the right side). C rules can be very scary here: Even with offset <= (off_t)len - sizeof(struct fmap) We'd risk to compare unsigned numbers, AFAICT: First, the - would be evaluated, if both types (signed `off_t` and unsigned `size_t` of the sizeof()) have the same rank, `len` would be converted back to `size_t`, overflow and we'd compare `0 <=` to a large number. With the assumption that the rank of `off_t` is greater-equal to the rank of `size_t`, this would work: offset <= (off_t)((off_t)len - sizeof(struct fmap)) The inner cast to handle `off_t` having the higher rank and the outer for the case that they have equal ranks. Obviously, an explicit `if` would be better ;) Oh and in case of equal ranks and overflow, the cast to `off_t` is implemen- tation defined ^^ fun. -- To view, visit https://review.coreboot.org/c/flashrom/+/30409 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I08895543ffb7a48058bcf91ef6500ca113f2d305 Gerrit-Change-Number: 30409 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 30 Jul 2019 11:42:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jacob Garber <jgarber1(a)ualberta.ca> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for National Instruments USB-845x devices
by Miklós Márton (Code Review) July 29, 2019

July 29, 2019

Miklós Márton has uploaded a new patch set (#21) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Add support for National Instruments USB-845x devices Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Signed-off-by: Miklós Márton <martonmiklosqdev(a)gmail.com> --- M Makefile M flashrom.8.tmpl M flashrom.c A ni845x_spi.c M programmer.h 5 files changed, 646 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/25683/21 -- To view, visit https://review.coreboot.org/c/flashrom/+/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 21 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan T <stefan.tauner(a)gmx.at> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Fix -Wsign-compare trouble
by Jacob Garber (Code Review) July 29, 2019

July 29, 2019

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30409 ) Change subject: Fix -Wsign-compare trouble ...................................................................... Patch Set 5: (5 comments) https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c File cbtable.c: https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@43 PS5, Line 43: image + size - 0x10 I think this parses as (image + size) - 0x10, right? https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@51 PS5, Line 51: image + size - 0x80 Ditto https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@69 PS5, Line 69: image + size - mb_part_offset Ditto https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@70 PS5, Line 70: image + size - mb_vendor_offset Ditto https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c File fmap.c: https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c@99 PS5, Line 99: (len - sizeof(struct fmap)) > Not sure anymore why I placed the parentheses. It looks like it would be […] If len < sizeof(struct fmap), we could just do an early return -2, so there's no problem of overflow then. -- To view, visit https://review.coreboot.org/c/flashrom/+/30409 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I08895543ffb7a48058bcf91ef6500ca113f2d305 Gerrit-Change-Number: 30409 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 29 Jul 2019 19:55:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for National Instruments USB-845x devices
by Miklós Márton (Code Review) July 29, 2019

July 29, 2019

Miklós Márton has uploaded a new patch set (#20) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Add support for National Instruments USB-845x devices Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Signed-off-by: Miklós Márton <martonmiklosqdev(a)gmail.com> --- M Makefile M flashrom.8.tmpl M flashrom.c A ni845x_spi.c M programmer.h 5 files changed, 649 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/25683/20 -- To view, visit https://review.coreboot.org/c/flashrom/+/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 20 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan T <stefan.tauner(a)gmx.at> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Fix -Wsign-compare trouble
by Nico Huber (Code Review) July 27, 2019

July 27, 2019

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30409 ) Change subject: Fix -Wsign-compare trouble ...................................................................... Patch Set 5: (1 comment) https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c File fmap.c: https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c@99 PS5, Line 99: (len - sizeof(struct fmap)) Not sure anymore why I placed the parentheses. It looks like it would be safer without? i.e. do the calculation as signed, so the overflow wouldn't be a problem? -- To view, visit https://review.coreboot.org/c/flashrom/+/30409 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I08895543ffb7a48058bcf91ef6500ca113f2d305 Gerrit-Change-Number: 30409 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Sat, 27 Jul 2019 23:25:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0