Change in flashrom[master]: dediprog: add serial argument

List overview All Threads
Download

newer

older

Change in flashrom[master]:...

Change in...

David Hendricks (Code Review)

1 Dec 2019 1 Dec '19

10:19 p.m.

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

dediprog: add serial argument

A quick hack to be able to select dediprogs by USB serial argument by just adding a @serial_number parameter to dediprog_open() and using it in preference to @id if available (since it is more specific).

Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Signed-off-by: Inaky Perez-Gonzalez inaky.perez-gonzalez@intel.com --- M dediprog.c M flashrom.8.tmpl 2 files changed, 23 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/37406/1

diff --git a/dediprog.c b/dediprog.c index 175e099..480cf67 100644 --- a/dediprog.c +++ b/dediprog.c @@ -1006,15 +1006,19 @@ /* * Open a dediprog_handle with the USB device at the given index. * @index index of the USB device + * @serial_number serial number of the USB device (id is ignored then) * @return 0 for success, -1 for error, -2 for busy device */ -static int dediprog_open(int index) +static int dediprog_open(int index, char *serial_number) { const uint16_t vid = devs_dediprog[0].vendor_id; const uint16_t pid = devs_dediprog[0].device_id; int ret;

- dediprog_handle = usb_dev_get_by_vid_pid_number(usb_ctx, vid, pid, (unsigned int) index); + if (serial_number) + dediprog_handle = usb_dev_get_by_vid_pid_serial(usb_ctx, vid, pid, serial_number); + else + dediprog_handle = usb_dev_get_by_vid_pid_number(usb_ctx, vid, pid, (unsigned int) index); if (!dediprog_handle) { msg_perr("Could not find a Dediprog programmer on USB.\n"); libusb_exit(usb_ctx); @@ -1057,7 +1061,8 @@

int dediprog_init(void) { - char *voltage, *id_str, *device, *spispeed, *target_str; + char *voltage, *id_str, *device, *spispeed, *target_str, + *serial_number; int spispeed_idx = 1; int millivolt = 3500; int id = -1; /* -1 defaults to enumeration order */ @@ -1091,6 +1096,7 @@ msg_pinfo("Setting voltage to %i mV\n", millivolt); }

+ serial_number = extract_programmer_param("serial"); id_str = extract_programmer_param("id"); if (id_str) { char prefix0, prefix1; @@ -1183,9 +1189,14 @@ return 1; }

- if (id != -1) { + if (serial_number) { + if (dediprog_open(0, serial_number)) { + return 1; + } + found_id = dediprog_read_id(); + } else if (id != -1) { for (i = 0; ; i++) { - ret = dediprog_open(i); + ret = dediprog_open(i, NULL); if (ret == -1) { /* no dev */ libusb_exit(usb_ctx); @@ -1218,7 +1229,7 @@ break; } } else { - if (dediprog_open(usedevice)) { + if (dediprog_open(usedevice, NULL)) { return 1; } found_id = dediprog_read_id(); @@ -1276,6 +1287,5 @@

if (register_spi_master(&spi_master_dediprog) || dediprog_set_leds(LED_NONE)) return 1; - return 0; } diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index 27f3846..5af1a38 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -928,6 +928,12 @@ .BR 0V ", " 1.8V ", " 2.5V ", " 3.5V or the equivalent in mV. .sp +You can use the +.B serial +parameter to explicitly specify which dediprog device should be used +based on their USB serial number:: +.sp +.B " flashrom -p dediprog:serial=1230A12" An optional .B device parameter specifies which of multiple connected Dediprog devices should be used.

Attachments:

attachment.htm (text/html — 7.1 KB)

Show replies by date

David Hendricks (Code Review)

1 Dec 1 Dec

10:24 p.m.

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

Patch Set 2: Code-Review+1

From https://github.com/flashrom/flashrom/pull/110.

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Sun, 01 Dec 2019 22:24:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Nico Huber (Code Review)

10 Dec 10 Dec

5:13 p.m.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

Patch Set 2:

(6 comments)

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c File dediprog.c:

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1008 PS2, Line 1008: * @index index of the USB device : * @serial_number serial number of the USB device (id is ignored then) : * @return 0 for success, -1 for error, -2 for busy device Please re-align.

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1065 PS2, Line 1065: *serial_number; Line break seems unnecessary.

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1099 PS2, Line 1099: serial_number = extract_programmer_param("serial"); Leaking `serial_number` and missing check if conflicting options (id, device) are given.

One way would be to do the check in advance:

serial_number = ... id_str = ... device = ... if (serial_number && id_str || serial_number && device || id_str && device) { msg_perr("Error: Only one of 'serial', 'id' or 'device' can be used.\n"); // bail out? }

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1192 PS2, Line 1192: if (serial_number) { : if (dediprog_open(0, serial_number)) { : return 1; : } : found_id = dediprog_read_id(); Can be handled more elegantly in the else path below.

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1232 PS2, Line 1232: NULL Could just pass `serial_number` here. Either one of the variables would be 0.

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1236 PS2, Line 1236: } From here, we won't need `serial_number` anymore.

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 10 Dec 2019 17:13:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Ryan O'Leary (Code Review)

14 Dec 14 Dec

6:53 a.m.

Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

Patch Set 2:

Just a warning, I had two dediprog SF100s with the same USB serial number which was what led me to the more convoluted id approach. I'm not sure how widespread this issue is.

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Ryan O'Leary ryanoleary@google.com Gerrit-Comment-Date: Sat, 14 Dec 2019 06:53:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nico Huber (Code Review)

4:22 p.m.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

Patch Set 2:

...

Just a warning, I had two dediprog SF100s with the same USB serial number which was what led me to the more convoluted id approach. I'm not sure how widespread this issue is.

Hmmm, you are right. Now that you mentioned it, I remember that they pick random numbers out of a pool or something like that.

So the question is, how useful is this option?

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Ryan O'Leary ryanoleary@google.com Gerrit-Comment-Date: Sat, 14 Dec 2019 16:22:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Inaky Perez-Gonzalez (Code Review)

19 Jan 19 Jan

9:40 p.m.

Inaky Perez-Gonzalez has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

Patch Set 2:

...

Patch Set 2:

...
Just a warning, I had two dediprog SF100s with the same USB serial number which was what led me to the more convoluted id approach. I'm not sure how widespread this issue is.

Hmmm, you are right. Now that you mentioned it, I remember that they pick random numbers out of a pool or something like that.

So the question is, how useful is this option?

Hi All

Thanks for the feedback -- I had not signed in to gerrit, so I didn't realize I had replies. I'll address it ASAP.

For quick replies, yes, the problem is still there if there are two identical serials. That's evil on the HW side (for starters).

In our usage model we have more than 1 dediprogs connected to the same machine. Since we have a pool of machines, we can always move one of the offending dediprogs to another host to avoid the serial# conflict. While this is not a solution for everyone, at least it is a solution--we can't fix the serial# conflict.

For someone that keeps having this problem, zeroing on other parameters might help (eg: USB tree path); that one is impractical for us, but might be for others.

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Inaky Perez-Gonzalez Gerrit-CC: Ryan O'Leary ryanoleary@google.com Gerrit-Comment-Date: Sun, 19 Jan 2020 21:40:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Inaky Perez-Gonzalez (Code Review)

20 Jan 20 Jan

9:17 p.m.

Inaky Perez-Gonzalez has uploaded a new patch set (#3) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

dediprog: add serial argument

Changes since v1:

- fix leak in serial_number

- allow specifying both id and serial_number to solve the case of multiple devices matching the same serial nuber.

- simplify the selection flow per feedback in comments

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/37406/3

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Inaky Perez-Gonzalez Gerrit-CC: Ryan O'Leary ryanoleary@google.com Gerrit-MessageType: newpatchset

Inaky Perez-Gonzalez (Code Review)

9:20 p.m.

Inaky Perez-Gonzalez has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

Patch Set 3:

(6 comments)

Updated hte patch set; I believe all feedback is addressed and by allowing id and serial_number together we can address the case when two different dongles have the same USB serial number. If they also have the same ID number, not much that can be done unless we'd want to add filtering by USB path, which would be a different task.

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c File dediprog.c:

...

Please re-align.

Ack

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1065 PS2, Line 1065: *serial_number;

...

Line break seems unnecessary.

Gets a couple chars over 80 column limit; I'll paste it up.

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1099 PS2, Line 1099: serial_number = extract_programmer_param("serial");

...

Leaking `serial_number` and missing check if conflicting options (id, device) […]

I do agree on the leakage but...does it matter? Meaning if this is a one shot execution tool and once done all resources are freed by the OS, it's done--or am I wrong and there is a library usage of this I am not aware of where resource allocation would accumulate into a leakage?

Re id vs serial, this could be used to work around when identical USB serials are in the system (comment somewhere else in the thread), so this check could be an AND (id == SOMETHING and serial_number == SOMETHINGELSE). I'll rework a wee bit.

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1192 PS2, Line 1192: if (serial_number) { : if (dediprog_open(0, serial_number)) { : return 1; : } : found_id = dediprog_read_id();

...

Can be handled more elegantly in the else path below.

oh, collapse both--took me three times to figure out what you meant--very good point, thx.

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1232 PS2, Line 1232: NULL

...

Could just pass `serial_number` here. Either one of the variables would be 0.

Done

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1236 PS2, Line 1236: }

...

From here, we won't need `serial_number` anymore.

Done

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Inaky Perez-Gonzalez Gerrit-CC: Ryan O'Leary ryanoleary@google.com Gerrit-Comment-Date: Mon, 20 Jan 2020 21:20:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Nico Huber (Code Review)

22 Jan 22 Jan

3:08 p.m.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c File dediprog.c:

https://review.coreboot.org/c/flashrom/+/37406/2/dediprog.c@1099 PS2, Line 1099: serial_number = extract_programmer_param("serial");

...

I do agree on the leakage but...does it matter? Meaning if this is a one shot execution tool and once done all resources are freed by the OS, it's done--or am I wrong and there is a library usage of this I am not aware of where resource allocation would accumulate into a leakage?

There is a library version of flashrom.

...

Re id vs serial, this could be used to work around when identical USB serials are in the system (comment somewhere else in the thread), so this check could be an AND (id == SOMETHING and serial_number == SOMETHINGELSE). I'll rework a wee bit.

I thought the IDs are unique, aren't they?

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Inaky Perez-Gonzalez Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Ryan O'Leary ryanoleary@google.com Gerrit-Comment-Date: Wed, 22 Jan 2020 15:08:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Inaky Perez-Gonzalez Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Inaky Perez-Gonzalez (Code Review)

18 Feb 18 Feb

11:21 p.m.

Inaky Perez-Gonzalez has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

Patch Set 3:

K, thanks for the clarification--in any case, based also on your input, the leakage should be not.

Re the IDs, that's what I thought too, but there were reports from other users showing how for them it was not the case.

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Inaky Perez-Gonzalez Gerrit-CC: Inaky Perez-Gonzalez inakypg@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Ryan O'Leary ryanoleary@google.com Gerrit-Comment-Date: Tue, 18 Feb 2020 23:21:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

22 Mar 22 Mar

10:29 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37406 )

Change subject: dediprog: add serial argument ......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/flashrom/+/37406/3/dediprog.c File dediprog.c:

https://review.coreboot.org/c/flashrom/+/37406/3/dediprog.c@1062 PS3, Line 1062: /* Check if the current device in device_handle has a USB serial This should be:

/* * Check if...

https://review.coreboot.org/c/flashrom/+/37406/3/dediprog.c@1238 PS3, Line 1238: if (serial_number : && !dediprog_is_usb_serial_number(serial_number)) : { Why isn't this a single line?

-- To view, visit https://review.coreboot.org/c/flashrom/+/37406 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9cdfbce6cf941c16bf7b7364aa4166b91369e661 Gerrit-Change-Number: 37406 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: David Hendricks david.hendricks@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Inaky Perez-Gonzalez Gerrit-CC: Inaky Perez-Gonzalez inakypg@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Ryan O'Leary ryanoleary@google.com Gerrit-Comment-Date: Sun, 22 Mar 2020 22:29:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1736

days inactive

1848

days old

flashrom-gerrit@flashrom.org

10 comments

5 participants

tags (0)

participants (5)

Angel Pons (Code Review)
David Hendricks (Code Review)
Inaky Perez-Gonzalez (Code Review)
Nico Huber (Code Review)
Ryan O'Leary (Code Review)