Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/32213 )
Change subject: pickit2_spi: update to libusb1 and drop libusb0 dependency ......................................................................
Patch Set 7:
(10 comments)
Looks good but I fear some things have to change. At least the `transferred` pointer to libusb_interrupt_transfer() and the missing libusb_exit().
If it's not too much trouble, it would be nice to retest, but only after the review is done. Doesn't have to cover a whole flash, a layout that covers two or more erase blocks with `--noverify-all` could speed things up. `--fmap` is also a nice test for nearly abitratry reads (if the FMAP is hard to find, i.e. not at a naturally aligned location).
Two things were noticed during review that should be fixed across all libusb programmers in the future:
* We shouldn't use the default context (passing NULL to libusb_init() etc.). It could be one libusb context per driver instance (flashrom_programmer_context), or maybe better a single one in a yet to be invented `flashrom_context` that is shared among driver instances. * We could always use libusb_strerror() in the code and implement our own (that simply returns the return code number as string, for instance) for older libusb versions, based on a version check.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c File pickit2_spi.c:
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@54 PS7, Line 54: #endif Not used here in this file? Then drop it.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@104 PS7, Line 104: (unsigned char *) This cast shouldn't be needed any more. And many more below...
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@104 PS7, Line 104: NULL NULL is only allowed since libusb 1.0.21. Sorry.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@366 PS7, Line 366: libusb_strerror It was only introduced 6 years ago, I guess that is why it's not used by any other driver. As you dropped most of these calls, let's not use it for now but keep it in mind.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@373 PS7, Line 373: libusb_close(pickit2_handle); libusb_exit(NULL);
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@428 PS7, Line 428: } Not a bug, could be a library user opening the device a second time. Just drop, please.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@430 PS7, Line 430: NULL NB. This will have to change in the future, but for all libusb drivers.
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@445 PS7, Line 445: msg_perr("Could not open device PICkit2!\n"); libusb_exit(NULL);
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@451 PS7, Line 451: libusb_close(pickit2_handle); libusb_exit(NULL);
https://review.coreboot.org/c/flashrom/+/32213/7/pickit2_spi.c@456 PS7, Line 456: libusb_close(pickit2_handle); libusb_exit(NULL);