August 2018 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Add initial support for Dediprog SF200.
by Angel Pons (Code Review) Aug. 31, 2018

Aug. 31, 2018

Angel Pons has posted comments on this change. ( https://review.coreboot.org/28272 ) Change subject: Add initial support for Dediprog SF200. ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/#/c/28272/1/dediprog.c File dediprog.c: https://review.coreboot.org/#/c/28272/1/dediprog.c@1146 PS1, Line 1146: /* SF100 only has 1 endpoint for in/out, SF600 uses two separate endpoints instead. */ Should this comment be updated? -- To view, visit https://review.coreboot.org/28272 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I025d1533e249f6a75b6d9015a18a6abf350456b6 Gerrit-Change-Number: 28272 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 31 Aug 2018 09:58:09 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: helpers: Add reverse_byte() and reverse_bytes()
by Nico Huber (Code Review) Aug. 30, 2018

Aug. 30, 2018

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/28086 ) Change subject: helpers: Add reverse_byte() and reverse_bytes() ...................................................................... helpers: Add reverse_byte() and reverse_bytes() Change-Id: I9d2e1e2856c835d22eed3b3a34bc0379773dd831 Signed-off-by: Marc Schink <flashrom-dev(a)marcschink.de> Reviewed-on: https://review.coreboot.org/28086 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M ch341a_spi.c M flash.h M helpers.c 3 files changed, 21 insertions(+), 11 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/ch341a_spi.c b/ch341a_spi.c index 9ebda0d..83d5ccf 100644 --- a/ch341a_spi.c +++ b/ch341a_spi.c @@ -274,15 +274,6 @@ return ret; } -/* ch341 requires LSB first, swap the bit order before send and after receive */ -static uint8_t swap_byte(uint8_t x) -{ - x = ((x >> 1) & 0x55) | ((x << 1) & 0xaa); - x = ((x >> 2) & 0x33) | ((x << 2) & 0xcc); - x = ((x >> 4) & 0x0f) | ((x << 4) & 0xf0); - return x; -} - /* The assumed map between UIO command bits, pins on CH341A chip and pins on SPI chip: * UIO CH341A SPI CH341A SPI name * 0 D0/15 CS/1 (CS0) @@ -372,7 +363,7 @@ *ptr++ = CH341A_CMD_SPI_STREAM; unsigned int i; for (i = 0; i < write_now; ++i) - *ptr++ = swap_byte(*writearr++); + *ptr++ = reverse_byte(*writearr++); if (read_now) { memset(ptr, 0xFF, read_now); read_left -= read_now; @@ -387,7 +378,7 @@ unsigned int i; for (i = 0; i < readcnt; i++) { - *readarr++ = swap_byte(rbuf[writecnt + i]); + *readarr++ = reverse_byte(rbuf[writecnt + i]); } return 0; diff --git a/flash.h b/flash.h index fca3775..01ff5cd 100644 --- a/flash.h +++ b/flash.h @@ -295,6 +295,8 @@ int min(int a, int b); char *strcat_realloc(char *dest, const char *src); void tolower_string(char *str); +uint8_t reverse_byte(uint8_t x); +void reverse_bytes(uint8_t *dst, const uint8_t *src, size_t length); #ifdef __MINGW32__ char* strtok_r(char *str, const char *delim, char **nextp); #endif diff --git a/helpers.c b/helpers.c index 2fcda65..a714908 100644 --- a/helpers.c +++ b/helpers.c @@ -66,6 +66,23 @@ *str = (char)tolower((unsigned char)*str); } +uint8_t reverse_byte(uint8_t x) +{ + x = ((x >> 1) & 0x55) | ((x << 1) & 0xaa); + x = ((x >> 2) & 0x33) | ((x << 2) & 0xcc); + x = ((x >> 4) & 0x0f) | ((x << 4) & 0xf0); + + return x; +} + +void reverse_bytes(uint8_t *dst, const uint8_t *src, size_t length) +{ + size_t i; + + for (i = 0; i < length; i++) + dst[i] = reverse_byte(src[i]); +} + /* FIXME: Find a better solution for MinGW. Maybe wrap strtok_s (C11) if it becomes available */ #ifdef __MINGW32__ char* strtok_r(char *str, const char *delim, char **nextp) -- To view, visit https://review.coreboot.org/28086 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9d2e1e2856c835d22eed3b3a34bc0379773dd831 Gerrit-Change-Number: 28086 Gerrit-PatchSet: 3 Gerrit-Owner: Marc Schink <flashrom-dev(a)marcschink.de> 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>

1 0

Change in flashrom[master]: usbdev: Refactor device discovery code
by Nico Huber (Code Review) Aug. 30, 2018

Aug. 30, 2018

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/27444 ) Change subject: usbdev: Refactor device discovery code ...................................................................... usbdev: Refactor device discovery code Currently there is a lot of code shared between usb_dev_get_by_vid_pid_serial() and usb_dev_get_by_vid_pid_number(). Fix this by pulling out the conditional filtering at the heart of each loop and calling it via a function pointer. I haven't got (two) dediprog programmers to test with but I have tested both by...serial() and by...number() calls using a pair of Developerboxen and a hacked driver. Change-Id: I31ed572501e4314b9455e1b70a5e934ec96408b1 Signed-off-by: Daniel Thompson <daniel.thompson(a)linaro.org> Reviewed-on: https://review.coreboot.org/27444 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M usbdev.c 1 file changed, 70 insertions(+), 58 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/usbdev.c b/usbdev.c index 5c34ba1..d793b65 100644 --- a/usbdev.c +++ b/usbdev.c @@ -15,12 +15,25 @@ * GNU General Public License for more details. */ +#include <stdbool.h> #include <string.h> #include <libusb.h> #include "programmer.h" -struct libusb_device_handle *usb_dev_get_by_vid_pid_serial( - struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, const char *serialno) +/* + * Check whether we should filter the current device. + * + * The main code filters by VID/PID then calls out to the filter function for extra filtering. + * The filter function is called twice for each device. Once with handle == NULL to allow the + * filter to cull devices without opening them and, assuming the first filter does not trigger, + * also with a real handle to allow the filter to query the device further. + * + * Returns true if the device should be skipped. + */ +typedef bool (*filter_func)(struct libusb_device_descriptor *desc, struct libusb_device_handle *handle, void*ctx); + +static struct libusb_device_handle *get_by_vid_pid_filter(struct libusb_context *usb_ctx, + uint16_t vid, uint16_t pid, filter_func filter_fn, void *filter_ctx) { struct libusb_device **list; ssize_t count = libusb_get_device_list(usb_ctx, &list); @@ -48,28 +61,22 @@ desc.idVendor, desc.idProduct, libusb_get_bus_number(dev), libusb_get_device_address(dev)); + /* allow filters to trigger before the device is opened */ + if (filter_fn(&desc, NULL, filter_ctx)) + continue; + res = libusb_open(dev, &handle); if (res != 0) { - msg_perr("Opening the USB device failed (%s)!\n", libusb_error_name(res)); - continue; + msg_perr("Opening the USB device at address %d-%d failed (%s)!\n", + libusb_get_bus_number(dev), libusb_get_device_address(dev), + libusb_error_name(res)); + break; } - if (serialno) { - unsigned char myserial[64]; - res = libusb_get_string_descriptor_ascii(handle, desc.iSerialNumber, myserial, - sizeof(myserial)); - if (res < 0) { - msg_perr("Reading the USB serialno failed (%s)!\n", libusb_error_name(res)); - libusb_close(handle); - continue; - } - msg_pdbg("Serial number is %s\n", myserial); - - /* Reject any serial number that does not commence with serialno */ - if (0 != strncmp(serialno, (char *)myserial, strlen(serialno))) { - libusb_close(handle); - continue; - } + /* filter can also trigger after a device is opened */ + if (filter_fn(&desc, handle, filter_ctx)) { + libusb_close(handle); + continue; } libusb_free_device_list(list, 1); @@ -78,6 +85,48 @@ libusb_free_device_list(list, 1); return NULL; + +} + +static bool filter_by_serial(struct libusb_device_descriptor *desc, struct libusb_device_handle *handle, + void *serialno) +{ + /* Never filter if device is not yet open or when user did not provide a serial number */ + if (!handle || !serialno) + return false; + + unsigned char myserial[64]; + int res = libusb_get_string_descriptor_ascii(handle, desc->iSerialNumber, myserial, sizeof(myserial)); + if (res < 0) { + msg_perr("Reading the USB serialno failed (%s)!\n", libusb_error_name(res)); + return true; + } + msg_pdbg("Serial number is %s\n", myserial); + + /* Filter out any serial number that does not commence with serialno */ + return 0 != strncmp(serialno, (char *)myserial, strlen(serialno)); +} + +struct libusb_device_handle *usb_dev_get_by_vid_pid_serial( + struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, const char *serialno) +{ + return get_by_vid_pid_filter(usb_ctx, vid, pid, filter_by_serial, (void *)serialno); +} + +static bool filter_by_number(struct libusb_device_descriptor *desc, struct libusb_device_handle *handle, + void *ctx) +{ + /* This filter never triggers once it has allowed the device to be opened */ + if (handle != NULL) + return false; + + unsigned int *nump = ctx; + if (*nump) { + (*nump)--; + return true; + } + + return false; } /* @@ -89,42 +138,5 @@ struct libusb_device_handle *usb_dev_get_by_vid_pid_number( struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, unsigned int num) { - struct libusb_device **list; - ssize_t count = libusb_get_device_list(usb_ctx, &list); - if (count < 0) { - msg_perr("Getting the USB device list failed (%s)!\n", libusb_error_name(count)); - return NULL; - } - - struct libusb_device_handle *handle = NULL; - ssize_t i = 0; - for (i = 0; i < count; i++) { - struct libusb_device *dev = list[i]; - struct libusb_device_descriptor desc; - int err = libusb_get_device_descriptor(dev, &desc); - if (err != 0) { - msg_perr("Reading the USB device descriptor failed (%s)!\n", libusb_error_name(err)); - libusb_free_device_list(list, 1); - return NULL; - } - if ((desc.idVendor == vid) && (desc.idProduct == pid)) { - msg_pdbg("Found USB device %04"PRIx16":%04"PRIx16" at address %d-%d.\n", - desc.idVendor, desc.idProduct, - libusb_get_bus_number(dev), libusb_get_device_address(dev)); - if (num == 0) { - err = libusb_open(dev, &handle); - if (err != 0) { - msg_perr("Opening the USB device failed (%s)!\n", - libusb_error_name(err)); - libusb_free_device_list(list, 1); - return NULL; - } - break; - } - num--; - } - } - libusb_free_device_list(list, 1); - - return handle; + return get_by_vid_pid_filter(usb_ctx, vid, pid, filter_by_number, &num); } -- To view, visit https://review.coreboot.org/27444 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I31ed572501e4314b9455e1b70a5e934ec96408b1 Gerrit-Change-Number: 27444 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> 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>

1 0

Change in flashrom[master]: usbdev: Extract libusb1 device discovery into a separate file
by Nico Huber (Code Review) Aug. 30, 2018

Aug. 30, 2018

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/27443 ) Change subject: usbdev: Extract libusb1 device discovery into a separate file ...................................................................... usbdev: Extract libusb1 device discovery into a separate file Currently there is a TODO-like comment in the dediprog driver: "Might be useful for other USB devices as well". Act on this comment by collecting all the device discovery code for libusb1 devices into a separate file. Change-Id: Idfcc79371241c2c1dea97faf5e532aa971546a79 Signed-off-by: Daniel Thompson <daniel.thompson(a)linaro.org> Reviewed-on: https://review.coreboot.org/27443 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M Makefile M dediprog.c M developerbox_spi.c M programmer.h A usbdev.c 5 files changed, 141 insertions(+), 108 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/Makefile b/Makefile index ddfd711..b13cf7e 100644 --- a/Makefile +++ b/Makefile @@ -1023,6 +1023,7 @@ ifneq ($(NEED_LIBUSB1), ) CHECK_LIBUSB1 = yes FEATURE_CFLAGS += -D'NEED_LIBUSB1=1' +PROGRAMMER_OBJS += usbdev.o # FreeBSD and DragonflyBSD use a reimplementation of libusb-1.0 that is simply called libusb ifeq ($(TARGET_OS),$(filter $(TARGET_OS),FreeBSD DragonFlyBSD)) USB1LIBS += -lusb diff --git a/dediprog.c b/dediprog.c index 7fcadfb..1a469a7 100644 --- a/dediprog.c +++ b/dediprog.c @@ -242,50 +242,6 @@ } -/* Might be useful for other USB devices as well. static for now. - * num parameter allows user to specify one device of multiple installed */ -static struct libusb_device_handle *get_device_by_vid_pid_number(uint16_t vid, uint16_t pid, unsigned int num) -{ - struct libusb_device **list; - ssize_t count = libusb_get_device_list(usb_ctx, &list); - if (count < 0) { - msg_perr("Getting the USB device list failed (%s)!\n", libusb_error_name(count)); - return NULL; - } - - struct libusb_device_handle *handle = NULL; - ssize_t i = 0; - for (i = 0; i < count; i++) { - struct libusb_device *dev = list[i]; - struct libusb_device_descriptor desc; - int err = libusb_get_device_descriptor(dev, &desc); - if (err != 0) { - msg_perr("Reading the USB device descriptor failed (%s)!\n", libusb_error_name(err)); - libusb_free_device_list(list, 1); - return NULL; - } - if ((desc.idVendor == vid) && (desc.idProduct == pid)) { - msg_pdbg("Found USB device %04"PRIx16":%04"PRIx16" at address %d-%d.\n", - desc.idVendor, desc.idProduct, - libusb_get_bus_number(dev), libusb_get_device_address(dev)); - if (num == 0) { - err = libusb_open(dev, &handle); - if (err != 0) { - msg_perr("Opening the USB device failed (%s)!\n", - libusb_error_name(err)); - libusb_free_device_list(list, 1); - return NULL; - } - break; - } - num--; - } - } - libusb_free_device_list(list, 1); - - return handle; -} - /* This function sets the GPIOs connected to the LEDs as well as IO1-IO4. */ static int dediprog_set_leds(int leds) { @@ -1102,7 +1058,7 @@ const uint16_t vid = devs_dediprog[0].vendor_id; const uint16_t pid = devs_dediprog[0].device_id; - dediprog_handle = get_device_by_vid_pid_number(vid, pid, (unsigned int) usedevice); + dediprog_handle = usb_dev_get_by_vid_pid_number(usb_ctx, vid, pid, (unsigned int) usedevice); if (!dediprog_handle) { msg_perr("Could not find a Dediprog programmer on USB.\n"); libusb_exit(usb_ctx); diff --git a/developerbox_spi.c b/developerbox_spi.c index 8482dea..4ad966e 100644 --- a/developerbox_spi.c +++ b/developerbox_spi.c @@ -34,7 +34,6 @@ #include "platform.h" #include <stdlib.h> -#include <string.h> #include <libusb.h> #include "programmer.h" #include "spi.h" @@ -130,67 +129,6 @@ .set_sck_set_mosi = cp210x_bitbang_set_sck_set_mosi, }; -static struct libusb_device_handle *get_device_by_vid_pid_serial(uint16_t vid, uint16_t pid, - const char *serialno) -{ - struct libusb_device **list; - ssize_t count = libusb_get_device_list(usb_ctx, &list); - if (count < 0) { - msg_perr("Getting the USB device list failed (%s)!\n", libusb_error_name(count)); - return NULL; - } - - ssize_t i = 0; - for (i = 0; i < count; i++) { - struct libusb_device *dev = list[i]; - struct libusb_device_descriptor desc; - struct libusb_device_handle *handle; - - int res = libusb_get_device_descriptor(dev, &desc); - if (res != 0) { - msg_perr("Reading the USB device descriptor failed (%s)!\n", libusb_error_name(res)); - continue; - } - - if ((desc.idVendor != vid) && (desc.idProduct != pid)) - continue; - - msg_pdbg("Found USB device %04"PRIx16":%04"PRIx16" at address %d-%d.\n", - desc.idVendor, desc.idProduct, - libusb_get_bus_number(dev), libusb_get_device_address(dev)); - - res = libusb_open(dev, &handle); - if (res != 0) { - msg_perr("Opening the USB device failed (%s)!\n", libusb_error_name(res)); - continue; - } - - if (serialno) { - unsigned char myserial[64]; - res = libusb_get_string_descriptor_ascii(handle, desc.iSerialNumber, myserial, - sizeof(myserial)); - if (res < 0) { - msg_perr("Reading the USB serialno failed (%s)!\n", libusb_error_name(res)); - libusb_close(handle); - continue; - } - msg_pdbg("Serial number is %s\n", myserial); - - /* Filter out any serial number that does not commence with serialno */ - if (0 != strncmp(serialno, (char *) myserial, strlen(serialno))) { - libusb_close(handle); - continue; - } - } - - libusb_free_device_list(list, 1); - return handle; - } - - libusb_free_device_list(list, 1); - return NULL; -} - static int developerbox_spi_shutdown(void *data) { libusb_close(cp210x_handle); @@ -210,7 +148,7 @@ char *serialno = extract_programmer_param("serial"); if (serialno) msg_pdbg("Looking for serial number commencing %s\n", serialno); - cp210x_handle = get_device_by_vid_pid_serial( + cp210x_handle = usb_dev_get_by_vid_pid_serial(usb_ctx, devs_developerbox_spi[0].vendor_id, devs_developerbox_spi[0].device_id, serialno); free(serialno); if (!cp210x_handle) { diff --git a/programmer.h b/programmer.h index 300cf5f..311992a 100644 --- a/programmer.h +++ b/programmer.h @@ -841,4 +841,12 @@ flash->mst->spi.features & SPI_MASTER_4BA; } +/* usbdev.c */ +struct libusb_device_handle; +struct libusb_context; +struct libusb_device_handle *usb_dev_get_by_vid_pid_serial( + struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, const char *serialno); +struct libusb_device_handle *usb_dev_get_by_vid_pid_number( + struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, unsigned int num); + #endif /* !__PROGRAMMER_H__ */ diff --git a/usbdev.c b/usbdev.c new file mode 100644 index 0000000..5c34ba1 --- /dev/null +++ b/usbdev.c @@ -0,0 +1,130 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2016 secunet Security Networks AG + * Copyright (C) 2018 Linaro Limited + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <string.h> +#include <libusb.h> +#include "programmer.h" + +struct libusb_device_handle *usb_dev_get_by_vid_pid_serial( + struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, const char *serialno) +{ + struct libusb_device **list; + ssize_t count = libusb_get_device_list(usb_ctx, &list); + if (count < 0) { + msg_perr("Getting the USB device list failed (%s)!\n", libusb_error_name(count)); + return NULL; + } + + ssize_t i = 0; + for (i = 0; i < count; i++) { + struct libusb_device *dev = list[i]; + struct libusb_device_descriptor desc; + struct libusb_device_handle *handle; + + int res = libusb_get_device_descriptor(dev, &desc); + if (res != 0) { + msg_perr("Reading the USB device descriptor failed (%s)!\n", libusb_error_name(res)); + continue; + } + + if ((desc.idVendor != vid) && (desc.idProduct != pid)) + continue; + + msg_pdbg("Found USB device %04"PRIx16":%04"PRIx16" at address %d-%d.\n", + desc.idVendor, desc.idProduct, + libusb_get_bus_number(dev), libusb_get_device_address(dev)); + + res = libusb_open(dev, &handle); + if (res != 0) { + msg_perr("Opening the USB device failed (%s)!\n", libusb_error_name(res)); + continue; + } + + if (serialno) { + unsigned char myserial[64]; + res = libusb_get_string_descriptor_ascii(handle, desc.iSerialNumber, myserial, + sizeof(myserial)); + if (res < 0) { + msg_perr("Reading the USB serialno failed (%s)!\n", libusb_error_name(res)); + libusb_close(handle); + continue; + } + msg_pdbg("Serial number is %s\n", myserial); + + /* Reject any serial number that does not commence with serialno */ + if (0 != strncmp(serialno, (char *)myserial, strlen(serialno))) { + libusb_close(handle); + continue; + } + } + + libusb_free_device_list(list, 1); + return handle; + } + + libusb_free_device_list(list, 1); + return NULL; +} + +/* + * This function allows different devices to be targeted based on enumeration order. Different + * hotplug sequencing (or simply a reboot) may change the enumeration order. This function should + * only be used if a programmers does not provide an alternative way to identify itself uniquely + * (such as a unique serial number). + */ +struct libusb_device_handle *usb_dev_get_by_vid_pid_number( + struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, unsigned int num) +{ + struct libusb_device **list; + ssize_t count = libusb_get_device_list(usb_ctx, &list); + if (count < 0) { + msg_perr("Getting the USB device list failed (%s)!\n", libusb_error_name(count)); + return NULL; + } + + struct libusb_device_handle *handle = NULL; + ssize_t i = 0; + for (i = 0; i < count; i++) { + struct libusb_device *dev = list[i]; + struct libusb_device_descriptor desc; + int err = libusb_get_device_descriptor(dev, &desc); + if (err != 0) { + msg_perr("Reading the USB device descriptor failed (%s)!\n", libusb_error_name(err)); + libusb_free_device_list(list, 1); + return NULL; + } + if ((desc.idVendor == vid) && (desc.idProduct == pid)) { + msg_pdbg("Found USB device %04"PRIx16":%04"PRIx16" at address %d-%d.\n", + desc.idVendor, desc.idProduct, + libusb_get_bus_number(dev), libusb_get_device_address(dev)); + if (num == 0) { + err = libusb_open(dev, &handle); + if (err != 0) { + msg_perr("Opening the USB device failed (%s)!\n", + libusb_error_name(err)); + libusb_free_device_list(list, 1); + return NULL; + } + break; + } + num--; + } + } + libusb_free_device_list(list, 1); + + return handle; +} -- To view, visit https://review.coreboot.org/27443 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Idfcc79371241c2c1dea97faf5e532aa971546a79 Gerrit-Change-Number: 27443 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> 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>

1 0

Change in flashrom[master]: Add support to get layout from fmap (e.g. coreboot rom)
by Nico Huber (Code Review) Aug. 29, 2018

Aug. 29, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23203 ) Change subject: Add support to get layout from fmap (e.g. coreboot rom) ...................................................................... Patch Set 22: (16 comments) Ok, found some more flaws. Most are minor and easy to fix. But the bsearch has some major issues that I couldn't ignore, sorry ;) https://review.coreboot.org/#/c/23203/22//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/23203/22//COMMIT_MSG@14 PS22, Line 14: to This `to` seems spurious. https://review.coreboot.org/#/c/23203/22//COMMIT_MSG@22 PS22, Line 22: mostly copied from Rather `based on` now? https://review.coreboot.org/#/c/23203/22/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/23203/22/cli_classic.c@642 PS22, Line 642: && !fmapfile I stared at this for a moment, now I'm sure the `!fmapfile` is redundant. https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl File flashrom.8.tmpl: https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@52 PS22, Line 52: [\fB\-i\fR <image>]] [\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR]] This line is unnecessarily long now. Please break before the `-n`. Would still look good and fit an 80 chars view if inden- tation is reduced by 3 spaces (the current 15 spaces indent is odd anyway). https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@202 PS22, Line 202: partioning par*ti*tioning https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@217 PS22, Line 217: partioning partitioning https://review.coreboot.org/#/c/23203/22/fmap.h File fmap.h: https://review.coreboot.org/#/c/23203/22/fmap.h@3 PS22, Line 3: -present I know it has been discussed already. But this is incredibly annoying. While everybody seems to know that this `-present` is not legally binding and nonsense, coming from a huge com- pany like Facebook (that should know better) feels quite of- fensive. Do they really want to be the Disney of open source? I won't block it. Just saying... you can't complain about this to your superiors often enough. Just every day "Every minute a(nother) copy of this is online, it's lowering Face- book's reputation". https://review.coreboot.org/#/c/23203/22/fmap.h@45 PS22, Line 45: #define FMAP_VER_MINOR 1 /* this header's FMAP minor version */ Didn't check earlier versions, but now this is unused? shouldn't we check the minor version too? https://review.coreboot.org/#/c/23203/22/fmap.h@67 PS22, Line 67: int is_valid_fmap(const struct fmap *fmap); nit, could be static now https://review.coreboot.org/#/c/23203/22/fmap.c File fmap.c: https://review.coreboot.org/#/c/23203/22/fmap.c@1 PS22, Line 1: nit, missing initial line break https://review.coreboot.org/#/c/23203/22/fmap.c@203 PS22, Line 203: return 1; leaking `fmap` https://review.coreboot.org/#/c/23203/22/fmap.c@260 PS22, Line 260: fmap_len = fmap_size(fmap); : struct fmap *tmp = fmap; : fmap = realloc(fmap, fmap_len); : if (!fmap) { : msg_gerr("Failed to realloc.\n"); : free(tmp); : goto _free_ret; : } : : if (flashctx->chip->read(flashctx, (uint8_t *)fmap + sizeof(*fmap), : offset + sizeof(*fmap), fmap_len - sizeof(*fmap))) { : msg_cerr("Cannot read %zu bytes at offset %06zx\n", : fmap_len - sizeof(*fmap), offset + sizeof(*fmap)); : /* Treat read failure to be fatal since this : * should be a valid, usable fmap. */ : goto _free_ret; : } : : *fmap_out = malloc(fmap_len); : if (*fmap_out == NULL) { : msg_gerr("Out of memory.\n"); : goto _free_ret; : } : : memcpy(*fmap_out, fmap, fmap_len); This path is flawed a little. At least it's missing an unconditional `break` after the memcpy(). But this also shows that everything after setting `fmap_found = 1` could happen outside the loops. So optio- nally we could refactor as follows: for (stride = ... ) { ... for (offset = ... ) { ... if (is_valid_fmap(fmap)) { msg_gdbg(... fmap_found = 1; break; } msg_gerr(... ret = 2; } if (fmap_found) break; } if (!fmap_found) goto _free_ret; /* code from above follows, moved out of the loops */ Um, with that said, there seems to be no valid path to hit the realloc() a second time. Every failure after the realloc() is fatal, and if we found and read a valid fmap, we should stop sear- ching, right? https://review.coreboot.org/#/c/23203/22/fmap.c@292 PS22, Line 292: if (fmap_found) This would still be set after a read failure, for instance. So it's probably safest to place the `ret = 0` right after the memcpy() to `*fmap_out`. https://review.coreboot.org/#/c/23203/22/fmap.c@321 PS22, Line 321: a fmap How do you pronounce it? Is it `*a* fmap` or `*an* eff-map`? https://review.coreboot.org/#/c/23203/22/libflashrom.c File libflashrom.c: https://review.coreboot.org/#/c/23203/22/libflashrom.c@433 PS22, Line 433: { Ugly, but as we were too lazy for proper deserialization again, we should guard here with __FLASHROM_LITTLE_ENDIAN__ like it's done in flasrom_layout_read_from_ifd(). https://review.coreboot.org/#/c/23203/22/libflashrom.c@468 PS22, Line 468: { Same guard here. -- To view, visit https://review.coreboot.org/23203 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12 Gerrit-Change-Number: 23203 Gerrit-PatchSet: 22 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(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: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Comment-Date: Wed, 29 Aug 2018 17:52:56 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add support to get layout from fmap (e.g. coreboot rom)
by Nico Huber (Code Review) Aug. 28, 2018

Aug. 28, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23203 ) Change subject: Add support to get layout from fmap (e.g. coreboot rom) ...................................................................... Patch Set 22: (5 comments) Looks good, just some minor comments. I have only looked at the diff of different patch sets lately, will try to have a final look over everything soon. https://review.coreboot.org/#/c/23203/20/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/23203/20/cli_classic.c@95 PS20, Line 95: int main(int argc, char *argv[]) > I just remembered that we have a read_buf_from_file() function in > flashrom.c. Well, you could have kept read_file() and called read_buf_from_file() from there. Doesn't matter. > Would be nice to have a file handling library > eventually to make this stuff more obvious. Agreed. https://review.coreboot.org/#/c/23203/22/cli_classic.c File cli_classic.c: https://review.coreboot.org/#/c/23203/22/cli_classic.c@619 PS22, Line 619: out out_shutdown https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl File flashrom.8.tmpl: https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@219 PS22, Line 219: (e.g. a coreboot rom image) in the argument and use it to create a layout. leftover https://review.coreboot.org/#/c/23203/20/fmap.h File fmap.h: https://review.coreboot.org/#/c/23203/20/fmap.h@70 PS20, Line 70: I should have mentioned, that I didn't mark all occurrences ;) https://review.coreboot.org/#/c/23203/22/fmap.c File fmap.c: https://review.coreboot.org/#/c/23203/22/fmap.c@153 PS22, Line 153: if (!flashctx || !flashctx->chip) : return 1; Redundant, probably leftover? -- To view, visit https://review.coreboot.org/23203 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12 Gerrit-Change-Number: 23203 Gerrit-PatchSet: 22 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(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: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Comment-Date: Tue, 28 Aug 2018 13:45:01 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

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

Aug. 27, 2018

Márton Miklós has posted comments on this change. ( https://review.coreboot.org/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Patch Set 11: (1 comment) Fixed, thanks for reviewing! https://review.coreboot.org/#/c/25683/8/flashrom.8.tmpl File flashrom.8.tmpl: https://review.coreboot.org/#/c/25683/8/flashrom.8.tmpl@1125 PS8, Line 1125: > kilo is always lowercase. Done -- To view, visit https://review.coreboot.org/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 11 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: Elyes HAOUAS <ehaouas(a)noos.fr> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Comment-Date: Mon, 27 Aug 2018 17:18:17 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

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

Aug. 27, 2018

Miklós Márton has uploaded a new patch set (#11) to the change originally created by David Hendricks. ( https://review.coreboot.org/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, 629 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/25683/11 -- To view, visit https://review.coreboot.org/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 11 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: Elyes HAOUAS <ehaouas(a)noos.fr> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan Tauner <stefan.tauner(a)gmx.at>

1 0

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

Aug. 27, 2018

Márton Miklós has posted comments on this change. ( https://review.coreboot.org/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Patch Set 10: (1 comment) https://review.coreboot.org/#/c/25683/10/ni845x_spi.c File ni845x_spi.c: https://review.coreboot.org/#/c/25683/10/ni845x_spi.c@123 PS10, Line 123: %ld > This is not the standard-compliant format specifier for int32. You include inttypes. […] int32 is coming from the NI's header where it is defined as a long: typedef long int32; As far as I know it translates to long int which (according to this matrix: http://www.cplusplus.com/reference/cstdio/printf/) should be okay for the %ld, but let me know if I missed something. I have used the NI's types only where I interact with their API just to be sure. -- To view, visit https://review.coreboot.org/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 10 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: Elyes HAOUAS <ehaouas(a)noos.fr> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Comment-Date: Mon, 27 Aug 2018 16:34:35 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: Add support for National Instruments USB-845x devices
by Stefan Tauner (Code Review) Aug. 27, 2018

Aug. 27, 2018

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/25683 ) Change subject: Add support for National Instruments USB-845x devices ...................................................................... Patch Set 10: (1 comment) https://review.coreboot.org/#/c/25683/10/ni845x_spi.c File ni845x_spi.c: https://review.coreboot.org/#/c/25683/10/ni845x_spi.c@123 PS10, Line 123: %ld This is not the standard-compliant format specifier for int32. You include inttypes.h already, please use it (everywhere...). -- To view, visit https://review.coreboot.org/25683 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549 Gerrit-Change-Number: 25683 Gerrit-PatchSet: 10 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: Elyes HAOUAS <ehaouas(a)noos.fr> Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com> Gerrit-CC: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Comment-Date: Mon, 27 Aug 2018 00:17:26 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0