Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk.

View Change

7 comments:

• File tests/ch341a_spi.c:

  • Patch Set #5, 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.

  • Patch Set #5, 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:

  • Patch Set #5, Line 75:

    You need 2 tabs here as indentation (see below __wrap_libusb_get_config_descriptor in the same situa […]

    Done

  • Patch Set #5, 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

  • Patch Set #5, 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:

  • Patch Set #5, 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.

  • Patch Set #5, 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 change 67664. To unsubscribe, or for help writing mail filters, visit settings.