[M] Change in flashrom[master]: tests: add basic lifecycle test for ch341a_spi

[Alexander Goncharov (Code Review)]

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.