September 2022 - flashrom-gerrit

[M] Change in flashrom[master]: writeprotect_ranges.c: add more range functions
by Anastasia Klimchuk (Code Review) 29 Sep '22

29 Sep '22

Attention is currently required from: Nico Huber, Maciej Pijanowski, Angel Pons, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66212 ) Change subject: writeprotect_ranges.c: add more range functions ...................................................................... Patch Set 6: (1 comment) Patchset: PS3: > I'm afraid we will not be able to test all of them. […] Maciej, thank you for response. So you tested on some chips but not all of them, is that correct? In this case, you definitely need to add the info in commit message, with the list of chips that you did test. Something like "tested at the end of a chain with chips A,B,C,...". Also I talked with Nikolai on this topic and tldr: we currently don't have the chips you are adding, and can't help with testing. However if in future we get new devices with those chips we surely will test them. The suggestion of > we might flag somehow the ones we tested and the ones which are purely based on the datasheet is a good one but I think to do this in full we would need to implement: https://ticket.coreboot.org/issues/377 Meanwhile, maybe indicating in commit message will be fine. Angel, Nico, you are here, what are you thoughts on adding some of chips based on datasheets? Is it fine to add info in commit message. -- To view, visit https://review.coreboot.org/c/flashrom/+/66212 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ied7b27be2ee2426af8f473432e2b01a290de2365 Gerrit-Change-Number: 66212 Gerrit-PatchSet: 6 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Thu, 29 Sep 2022 06:15:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: cli_classic.c: Add error messages for invalid --wp-region use
by Nikolai Artemiev (Code Review) 29 Sep '22

29 Sep '22

Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67725 ) Change subject: cli_classic.c: Add error messages for invalid --wp-region use ...................................................................... Patch Set 7: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/67725/comment/d69f8764_03d9755a PS6, Line 9: warning > errors Done -- To view, visit https://review.coreboot.org/c/flashrom/+/67725 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie606ba7f8a423405099679ca62169c395d994b5d Gerrit-Change-Number: 67725 Gerrit-PatchSet: 7 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 29 Sep 2022 06:02:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: cli_classic.c: Add error messages for invalid --wp-region use
by Nikolai Artemiev (Code Review) 29 Sep '22

29 Sep '22

Attention is currently required from: Nikolai Artemiev. Hello Felix Singer, build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67725 to look at the new patch set (#7). Change subject: cli_classic.c: Add error messages for invalid --wp-region use ...................................................................... cli_classic.c: Add error messages for invalid --wp-region use Print errors if --wp-region is used without a layout file or the layout file doesn't contain the region. BUG=b:247055486 TEST=builds Change-Id: Ie606ba7f8a423405099679ca62169c395d994b5d Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M cli_classic.c 1 file changed, 26 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/67725/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/67725 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie606ba7f8a423405099679ca62169c395d994b5d Gerrit-Change-Number: 67725 Gerrit-PatchSet: 7 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[master]: dirtyjtag: Add DirtyJTAG programmer
by Jean THOMAS (Code Review) 28 Sep '22

28 Sep '22

Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Alexander Goncharov. Jean THOMAS has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67878 ) Change subject: dirtyjtag: Add DirtyJTAG programmer ...................................................................... Patch Set 6: (4 comments) Patchset: PS6: Hi Nico, thanks for your feedback. I'm still hesitating about limiting the amount of memcpy in dirtyjtag_djtag1_spi_send_command. It would make the code run faster for sure, but it would also make it harder to read. File dirtyjtag_spi.c: https://review.coreboot.org/c/flashrom/+/67878/comment/ca4aadeb_080c4b08 PS5, Line 18: * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > Please remove this paragraph. The address became outdated in the past so […] Done https://review.coreboot.org/c/flashrom/+/67878/comment/059481c9_1fe31c58 PS5, Line 121: size_t num_xfer = (len + max_xfer_size - 1 ) / max_xfer_size; // ceil(len/max_xfer_size) > 1. Lift the `.max_data_*` limits so this function gets called for bigger > chunks. There should be less overhead as we wouldn't have to send the > TMS/STOP sequence as often. I assume this could speed up long reads. > Some of the memcpy() also seems unnecessary, but the USB transfers > probably limit speed so much that it doesn't matter. I went with that option. > Btw. is it necessary to drain input buffers with the receive > call even if we don't need the data? Unfortunately, yes. We'll probably fix this in DJTAG2 (see https://github.com/jeanthom/DirtyJTAG/issues/77#issuecomment-758623004 for more info), but I still want to keep backwards compatibility with DJTAG1. https://review.coreboot.org/c/flashrom/+/67878/comment/f21365cf_e24c1090 PS5, Line 275: except TRST > What about SRST and TMS? I might read the code wrong, but it looks like […] Yes SRST and TMS are also high during init. I added a little table right after the copyright to better explain how the JTAG interface should be connected to an SPI flash. -- To view, visit https://review.coreboot.org/c/flashrom/+/67878 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic43e9a014ed7d04e429e73b30c9dcfdde1a78913 Gerrit-Change-Number: 67878 Gerrit-PatchSet: 6 Gerrit-Owner: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Wed, 28 Sep 2022 22:16:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: dirtyjtag: Add DirtyJTAG programmer
by Jean THOMAS (Code Review) 28 Sep '22

28 Sep '22

Attention is currently required from: Felix Singer, Thomas Heijligen, Jean THOMAS, Alexander Goncharov. Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67878 to look at the new patch set (#6). Change subject: dirtyjtag: Add DirtyJTAG programmer ...................................................................... dirtyjtag: Add DirtyJTAG programmer Add a new programmer driver for the DirtyJTAG project (a USB-JTAG firmware for STM32 MCUs). Successfully tested with DirtyJTAG 1.4 running on an Olimex STM32-H103 development board and a SST25VF020B SPI flash chip. Change-Id: Ic43e9a014ed7d04e429e73b30c9dcfdde1a78913 Signed-off-by: Jean THOMAS <virgule(a)jeanthomas.me> --- M Makefile A dirtyjtag_spi.c M flashrom.8.tmpl M include/programmer.h M meson.build M meson_options.txt M programmer_table.c 7 files changed, 366 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/67878/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/67878 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic43e9a014ed7d04e429e73b30c9dcfdde1a78913 Gerrit-Change-Number: 67878 Gerrit-PatchSet: 6 Gerrit-Owner: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Jean THOMAS <virgule(a)jeanthomas.me> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tree: provide flashrom context into programmer's delay
by Felix Singer (Code Review) 28 Sep '22

28 Sep '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Alexander Goncharov. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66373 ) Change subject: tree: provide flashrom context into programmer's delay ...................................................................... Patch Set 7: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/66373/comment/3d2f5776_3b71c8e2 PS7, Line 19: Ticket: https://ticket.coreboot.org/issues/391 Same here. Not sure how the patch is related to this ticket? -- To view, visit https://review.coreboot.org/c/flashrom/+/66373 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibb0bce26ce2052853ee52158d7ba742967a9e229 Gerrit-Change-Number: 66373 Gerrit-PatchSet: 7 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Wed, 28 Sep 2022 18:45:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree: provide flashrom context into programmer's delay
by Felix Singer (Code Review) 28 Sep '22

28 Sep '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Alexander Goncharov. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66373 ) Change subject: tree: provide flashrom context into programmer's delay ...................................................................... Patch Set 7: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/66373/comment/e1529d0b_c26b29a4 PS4, Line 10: This is an important step to : remove the global state because it'll allow the programmer's data : to be used in delay functions. > Sure! Honestly I don't like that we removed that from the commit message. It worked very well for that commit, but I still consider this as a hack or quick fix because I just wanted to move on. This shouldn't become our standard. Context is *always* useful and important for the reviewers and readers. So, please consider using `commit 0123456789ab` or `CB:123456` in the future as I have just explained in my other comment. Commit Message: https://review.coreboot.org/c/flashrom/+/66373/comment/5fd9fe44_61816dd6 PS7, Line 14: TOPIC=register_master_api > How is this patch related to this topic? Also, if you don't have a huge patch series then a topic doesn't make much sense. To explain the relations between commits, or to reference commits for giving some context, rather mention the commit ids for already merged ones using the format `commit 0123456789ab` (12 chars) or use `CB:123456` (the number from the URL) for patches which are not merged yet. The Gerrit UI makes an URL out of it and you can easily jump to the related patches. This way you can explain your effort in your initial patch for example. -- To view, visit https://review.coreboot.org/c/flashrom/+/66373 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibb0bce26ce2052853ee52158d7ba742967a9e229 Gerrit-Change-Number: 66373 Gerrit-PatchSet: 7 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Wed, 28 Sep 2022 18:37:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree: provide flashrom context into programmer's delay
by Felix Singer (Code Review) 28 Sep '22

28 Sep '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Alexander Goncharov. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66373 ) Change subject: tree: provide flashrom context into programmer's delay ...................................................................... Patch Set 7: (11 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/66373/comment/d3c54984_e2a12db3 PS7, Line 7: tree: provide flashrom context into programmer's delay It's not passed to the programmers anymore https://review.coreboot.org/c/flashrom/+/66373/comment/59ed5ac8_f6f8543f PS7, Line 9: Add the flashrom context to the delay function declaration, which : located in `struct programmer_entry`. Doesn't apply anymore. Also, there is not *that* delay function. So mention the name `programmer_delay`. https://review.coreboot.org/c/flashrom/+/66373/comment/b794405e_f55a9860 PS7, Line 14: TOPIC=register_master_api How is this patch related to this topic? File amd_imc.c: https://review.coreboot.org/c/flashrom/+/66373/comment/097acf57_cab11ed9 PS7, Line 63: /* flashctx is not completely ready during init, so it's not : * needed (NULL). */ Same here File atavia.c: https://review.coreboot.org/c/flashrom/+/66373/comment/9d979fa2_25b0e610 PS7, Line 93: /* flashctx is not needed (NULL) because atavia does : * not use it in its delay function. */ Same here File dediprog.c: https://review.coreboot.org/c/flashrom/+/66373/comment/977c3f41_2e24d75e PS7, Line 319: /* flashctx not needed (NULL) because dediprog does not use it in its : * delay function. */ Same here File flashrom.c: https://review.coreboot.org/c/flashrom/+/66373/comment/3918bf2d_ce7644c6 PS5, Line 220: void programmer_delay(const struct flashctx *flash, unsigned int usecs) > The commit message to this patch is hard to have both ways, it is either very forward looking and me […] My comment is related to an older patch set in which the function was reworked while the commit message just says "pass flashctx to functions", which was wrong. However, the commit message still doesn't fit to the recent patch set. See my other comments. File ichspi.c: https://review.coreboot.org/c/flashrom/+/66373/comment/6e2f4beb_e9d388d1 PS7, Line 878: flashctx is not needed (NULL) because ichspi does not use : * it in its delay function. */ Same here File pony_spi.c: https://review.coreboot.org/c/flashrom/+/66373/comment/8969ae7d_7a5b0211 PS7, Line 247: flashctx not needed (NULL) because pony_spi does not : * use it in its delay function. The parameter should be documented at the function declaration, not in the programmers code. https://review.coreboot.org/c/flashrom/+/66373/comment/23c1da22_109faf12 PS7, Line 248: Also flashctx is not : * completely ready during init anyway. This doesn't add much value. So please remove. File raiden_debug_spi.c: https://review.coreboot.org/c/flashrom/+/66373/comment/ea070601_95c81183 PS7, Line 1015: * flashctx not needed (NULL) because raiden_debug_spi does not : * use it in its delay function. Also flashctx is not : * completely ready during init anyway. */ Same here -- To view, visit https://review.coreboot.org/c/flashrom/+/66373 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibb0bce26ce2052853ee52158d7ba742967a9e229 Gerrit-Change-Number: 66373 Gerrit-PatchSet: 7 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Wed, 28 Sep 2022 17:57:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: add basic lifecycle test for ch341a_spi
by Alexander Goncharov (Code Review) 28 Sep '22

28 Sep '22

Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67664 ) Change subject: tests: add basic lifecycle test for ch341a_spi ...................................................................... Patch Set 6: (7 comments) File tests/ch341a_spi.c: https://review.coreboot.org/c/flashrom/+/67664/comment/1fa82610_973cecd9 PS5, Line 22: transfer->status = LIBUSB_TRANSFER_COMPLETED; : transfer->actual_length = transfer->length; : transfer->callback(transfer); > Are you sure these lines needed? What happens if you remove them, and just return 0? […] *TL;DR* Yeah, it's important step. Without it the code go to an infinite loop. CH341A programmer uses asynchronous (non-blocking) API for USB device I/O. 1. Real `libusb_alloc_transfer` function allocates memory for `struct usbi_transfer` that contains `struct libusb_transfer` as the private data. libusb/io.c:1285: ``` struct libusb_transfer *libusb_alloc_transfer(int iso_packets) { struct usbi_transfer *itransfer; struct libusb_transfer *transfer; ... alloc_size = priv_size + sizeof(struct usbi_transfer) + sizeof(struct libusb_transfer) + ...; ptr = calloc(1, alloc_size); ... itransfer = (struct usbi_transfer *)(ptr + priv_size); itransfer->priv = ptr; transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); return transfer; } ``` libusb/libusb.h:566: `from a _libusb_transfer_, you can get the _usbi_transfer_ by rewinding the appropriate number of bytes.` 2. `libusb_fill_bulk_transfer` populates the required `libusb_transfer` fields. One of the parameters is `libusb_transfer_cb_fn callback`. This parameter in the structure is described as `Callback function. This will be invoked when the transfer completes, fails, or is cancelled.`. flashrom/ch341a_spi.c:496: ``` libusb_fill_bulk_transfer(..., cb_out, ...); for (i = 0; i < USB_IN_TRANSFERS; i++) libusb_fill_bulk_transfer(..., cb_in, ...); ``` When we invoke `transfer->callback()` we actually invoke functions `cb_out` and `cb_in`. 3. `libusb_submit_transfer` returns immediately but can be regarded as firing off the I/O request in the background. libusb/io.c:1490: ``` int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer) { ... r = usbi_backend.submit_transfer(itransfer); ... return r; } ``` `struct usbi_os_backend usbi_backend` is a global interface (variable) for interacting with OS-specific stuff. 4. `libusb_handle_events_timeout` handles any pending events (seems like real interaction with USB). It calls `handle_events` function. libusb/io.c:2198: ``` static int handle_events(struct libusb_context *ctx, struct timeval *tv) { ... r = usbi_backend.handle_events(ctx, reported_events.event_data, reported_events.event_data_count, reported_events.num_ready); ... return r; } ``` When the transfer is complete, it calls `usbi_handle_transfer_completion` libusb/io.c:1683: ``` int usbi_handle_transfer_completion(struct usbi_transfer *itransfer, ...) { ... flags = transfer->flags; transfer->status = status; transfer->actual_length = itransfer->transferred; usbi_dbg(ctx, "transfer %p has callback %p", transfer, transfer->callback); if (transfer->callback) transfer->callback(transfer); ... } ``` So, as you see this lines should be in `handle_events_timeout`, but it doesn't have `struct libusb_transfer` parameter. It might not be 100% correct, but it works! :) One of these days I will try to research this long way on real hardware. https://review.coreboot.org/c/flashrom/+/67664/comment/7618bd2e_bbf1a0e3 PS5, Line 43: mediatek_i2c_spi_basic_lifecycle_test_success > This is a different test probably a copy-paste ;) […] Oops.. copy-paste stuff. Done File tests/libusb_wraps.c: https://review.coreboot.org/c/flashrom/+/67664/comment/e74627a7_919f7120 PS5, Line 75: > You need 2 tabs here as indentation (see below __wrap_libusb_get_config_descriptor in the same situa […] Done https://review.coreboot.org/c/flashrom/+/67664/comment/cbca30de_4d21fe01 PS5, Line 196: calloc(1, sizeof(struct libusb_transfer)); > This is a custom logic (a small one but still), so it should go into custom mocks. […] Done https://review.coreboot.org/c/flashrom/+/67664/comment/344c592f_eaff19e7 PS5, Line 219: free(transfer); > Same here, custom logic, please move to io_mock, and implement in tests/ch341a_spi. […] Done File tests/usb_unittests.h: https://review.coreboot.org/c/flashrom/+/67664/comment/1cc9b685_b4aae621 PS5, Line 28: CONFIG_CH341A_SPI == 1 > You can run tests with this option enabled and disabled. […] Thank you for the tips. I've verified that the current patchset handles SKIP_TEST macro. https://review.coreboot.org/c/flashrom/+/67664/comment/2ccc386b_2ec1ab56 PS5, Line 58: struct libusb_transfer; > If you add typedef like the others, you can avoid repeating `struct` in wraps. Typedefs above are exciting in libusb code. But there's no typedef for `struct libusb_transfer`. I find it good practice to have the same function signatures as libusb. -- To view, visit https://review.coreboot.org/c/flashrom/+/67664 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If28fbe09ad685082152aa3a7e8d5a150169aee9e Gerrit-Change-Number: 67664 Gerrit-PatchSet: 6 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 28 Sep 2022 17:30:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: add basic lifecycle test for ch341a_spi
by Alexander Goncharov (Code Review) 28 Sep '22

28 Sep '22

Attention is currently required from: Felix Singer, Thomas Heijligen, Alexander Goncharov. Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/67664 to look at the new patch set (#6). Change subject: tests: add basic lifecycle test for ch341a_spi ...................................................................... tests: add basic lifecycle test for ch341a_spi Change-Id: If28fbe09ad685082152aa3a7e8d5a150169aee9e Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> --- A tests/ch341a_spi.c M tests/io_mock.h M tests/libusb_wraps.c M tests/libusb_wraps.h M tests/meson.build M tests/tests.c M tests/tests.h M tests/usb_unittests.h 8 files changed, 162 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/67664/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/67664 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If28fbe09ad685082152aa3a7e8d5a150169aee9e Gerrit-Change-Number: 67664 Gerrit-PatchSet: 6 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2022