[usb-bus] Opt out of USB callbacks part 1 Handles the simple case where all requests complete successfully. Remove old batch_cb API. Also modifies testing so we can specify when we want to set or expect a callback. Currently those fields are equal in the tests, but will not necessarily be when we start testing with set_error=true. Part 2 will track queued / silently completed requests and handle callbacks for the error cases. ZX-932 #comment TEST= plug in FX3 and run: fx shell usb-fwloader && runtests -t usb-test Change-Id: I596c18b3e0edac25638214977e3668b4efccbe38
diff --git a/system/banjo/ddk-protocol-usb-request/usb-request.banjo b/system/banjo/ddk-protocol-usb-request/usb-request.banjo index 50843a8..35b584b 100644 --- a/system/banjo/ddk-protocol-usb-request/usb-request.banjo +++ b/system/banjo/ddk-protocol-usb-request/usb-request.banjo
@@ -58,13 +58,18 @@ usize alloc_size; - /// For requests queued on endpoints which have batching enabled via - /// usb_configure_batch_callback(). - /// Set by the requester if a callback is required on this request's completion. + /// Set by the requester if the callback should be skipped on successful completion. /// This is useful for isochronous requests, where the requester does not care about /// most callbacks. - /// The requester should ensure the last request has this set to true. - bool require_batch_cb; + /// The requester is in charge of keeping track of the order of queued requests and + /// requeuing silently completed requests. + /// + /// If the requester receives a success callback, they may assume requests queued + /// prior to it at the same endpoint have silently completed. + /// If the requester receives an error callback, they will receive an additional + /// callback when the request queued prior to the erroneous request completes, if any. + /// This is as errors are reported as soon as possible rather than preserving queue order. + bool cb_on_error_only; }; [Layout="ddk-callback"]
diff --git a/system/banjo/ddk-protocol-usb/usb.banjo b/system/banjo/ddk-protocol-usb/usb.banjo index 477ee3c..3bfc279 100644 --- a/system/banjo/ddk-protocol-usb/usb.banjo +++ b/system/banjo/ddk-protocol-usb/usb.banjo
@@ -8,11 +8,6 @@ using zircon.hw.usb; using zx; -[Layout="ddk-callback"] -interface UsbBatchRequestComplete { - Callback(vector<ddk.protocol.usb.request.UsbRequest> reqs) -> (); -}; - [Layout = "ddk-protocol"] interface Usb { // Initiates a control transfer with the device in the OUT direction. @@ -27,14 +22,6 @@ RequestQueue(ddk.protocol.usb.request.UsbRequest? usb_request, ddk.protocol.usb.request.UsbRequestComplete? complete_cb) -> (); - /// Configures an endpoint to batch multiple requests to a single callback. - /// Requests will receive a callback if they have set require_batch_cb to true, or an error occurs. - /// ep_address: the endpoint which requests will be queued on. - /// complete_cb: callback for the batch of completed requests. - /// cookie: user data passed to the |complete_cb|. - ConfigureBatchCallback(uint8 ep_address, UsbBatchRequestComplete complete_cb) - -> (zx.status status); - /// Returns the speed of the device. GetSpeed() -> (zircon.hw.usb.UsbSpeed s);
diff --git a/system/dev/usb/usb-bus/usb-device.cpp b/system/dev/usb/usb-bus/usb-device.cpp index 9f7d422..ef53a3e 100644 --- a/system/dev/usb/usb-bus/usb-device.cpp +++ b/system/dev/usb/usb-bus/usb-device.cpp
@@ -106,6 +106,10 @@ void UsbDevice::RequestComplete(usb_request_t* req) { fbl::AutoLock lock(&callback_lock_); + if (req->cb_on_error_only && req->response.status == ZX_OK) { + return; + } + // move original request to completed_reqs list so it can be completed on the callback_thread UsbRequestInternal* req_int = USB_REQ_TO_DEV_INTERNAL(req, parent_req_size_); list_add_tail(&completed_reqs_, &req_int->node); @@ -295,12 +299,6 @@ hci_.RequestQueue(req, &complete); } -zx_status_t UsbDevice::UsbConfigureBatchCallback(uint8_t ep_address, - const usb_batch_request_complete_t* complete_cb) { - // TODO(jocelyndang): implement this. - return ZX_ERR_NOT_SUPPORTED; -} - usb_speed_t UsbDevice::UsbGetSpeed() { return speed_; }
diff --git a/system/dev/usb/usb-bus/usb-device.h b/system/dev/usb/usb-bus/usb-device.h index 1b1abda..a4083dc 100644 --- a/system/dev/usb/usb-bus/usb-device.h +++ b/system/dev/usb/usb-bus/usb-device.h
@@ -50,8 +50,6 @@ int64_t timeout, void* out_read_buffer, size_t read_size, size_t* out_read_actual); void UsbRequestQueue(usb_request_t* usb_request, const usb_request_complete_t* complete_cb); - zx_status_t UsbConfigureBatchCallback(uint8_t ep_address, - const usb_batch_request_complete_t* complete_cb); usb_speed_t UsbGetSpeed(); zx_status_t UsbSetInterface(uint8_t interface_number, uint8_t alt_setting); uint8_t UsbGetConfiguration();
diff --git a/system/dev/usb/usb-composite/usb-interface.c b/system/dev/usb/usb-composite/usb-interface.c index 5ec0b1b..f215e0b 100644 --- a/system/dev/usb/usb-composite/usb-interface.c +++ b/system/dev/usb/usb-composite/usb-interface.c
@@ -144,12 +144,6 @@ usb_request_queue(&intf->comp->usb, usb_request, complete_cb); } -static zx_status_t usb_interface_configure_batch_callback(void* ctx, uint8_t ep_address, - const usb_batch_request_complete_t* - complete_cb) { - usb_interface_t* intf = ctx; - return usb_configure_batch_callback(&intf->comp->usb, ep_address, complete_cb); -} static usb_speed_t usb_interface_get_speed(void* ctx) { usb_interface_t* intf = ctx; @@ -354,7 +348,6 @@ .control_out = usb_interface_control_out, .control_in = usb_interface_control_in, .request_queue = usb_interface_request_queue, - .configure_batch_callback = usb_interface_configure_batch_callback, .get_speed = usb_interface_get_speed, .set_interface = usb_interface_set_interface, .get_configuration = usb_interface_get_configuration,
diff --git a/system/dev/usb/usb-test/usb-tester/usb-tester.cpp b/system/dev/usb/usb-test/usb-tester/usb-tester.cpp index eb7bcee..ae20a89 100644 --- a/system/dev/usb/usb-test/usb-tester/usb-tester.cpp +++ b/system/dev/usb/usb-test/usb-tester/usb-tester.cpp
@@ -38,17 +38,19 @@ namespace usb { -std::optional<TestRequest> TestRequest::Create(size_t len, uint8_t ep_address, size_t req_size) { +std::optional<TestRequest> TestRequest::Create(size_t len, uint8_t ep_address, size_t req_size, + bool set_cb, bool expect_cb) { usb_request_t* usb_req; zx_status_t status = usb_request_alloc(&usb_req, len, ep_address, req_size); if (status != ZX_OK) { return std::nullopt; } - return TestRequest(usb_req); + return TestRequest(usb_req, set_cb, expect_cb); } std::optional<TestRequest> TestRequest::Create(const fuchsia_hardware_usb_tester_SgList& sg_list, - uint8_t ep_address, size_t req_size) { + uint8_t ep_address, size_t req_size, bool set_cb, + bool expect_cb) { size_t buffer_size = 0; // We need to allocate a usb request buffer that covers all the scatter gather entries. for (uint64_t i = 0; i < sg_list.len; ++i) { @@ -72,10 +74,14 @@ usb_request_release(usb_req); return std::nullopt; } - return TestRequest(usb_req); + return TestRequest(usb_req, set_cb, expect_cb); } -TestRequest::TestRequest(usb_request_t* usb_req) : usb_req_(usb_req) { +TestRequest::TestRequest(usb_request_t* usb_req, bool set_cb, bool expect_cb) + : usb_req_(usb_req), + expect_cb_(expect_cb), + got_cb_(false) { + usb_req_->cb_on_error_only = !set_cb; } TestRequest::~TestRequest() { @@ -86,7 +92,10 @@ void TestRequest::RequestCompleteCallback(void* ctx, usb_request_t* request) { ZX_DEBUG_ASSERT(ctx != nullptr); - sync_completion_signal(reinterpret_cast<sync_completion_t*>(ctx)); + auto test_req = reinterpret_cast<TestRequest*>(ctx); + test_req->got_cb_ = true; + zxlogf(TRACE, "%p: complete callback\n", request); + sync_completion_signal(&test_req->completion_); } zx_status_t TestRequest::WaitComplete(usb_protocol_t* usb) { @@ -153,19 +162,33 @@ return ZX_OK; } -zx_status_t UsbTester::AllocTestReqs(size_t num_reqs, size_t len, uint8_t ep_addr, - fbl::Vector<TestRequest>* out_test_reqs, size_t req_size) { +zx_status_t UsbTester::AllocIsochTestReqs(size_t num_reqs, size_t len, uint8_t ep_addr, + fbl::Vector<TestRequest>* out_test_reqs, size_t req_size, + const fuchsia_hardware_usb_tester_PacketOptions* opts, + size_t opts_len) { fbl::AllocChecker ac; out_test_reqs->reserve(num_reqs, &ac); if (!ac.check()) { return ZX_ERR_NO_MEMORY; } + + fuchsia_hardware_usb_tester_PacketOptions default_opts = + { .set_cb = true, .set_error = false, .expect_cb = true }; + for (size_t i = 0; i < num_reqs; ++i) { - auto test_req = TestRequest::Create(len, ep_addr, req_size); + auto& req_opts = i < opts_len ? opts[i] : default_opts; + auto test_req = TestRequest::Create(len, ep_addr, req_size, + req_opts.set_cb, req_opts.expect_cb); if (!test_req.has_value()) { return ZX_ERR_NO_MEMORY; } + if (req_opts.set_error) { + // Zero length isoch requests will fail. + test_req->Get()->header.length = 0; + } + zxlogf(SPEW, "%lu (%p): set callback=%d, set_error=%d expect_cb=%d\n", + i, test_req->Get(), req_opts.set_cb, req_opts.set_error, req_opts.expect_cb); out_test_reqs->push_back(std::move(test_req.value())); } return ZX_OK; @@ -173,7 +196,9 @@ void UsbTester::WaitTestReqs(const fbl::Vector<TestRequest>& test_reqs) { for (auto& test_req : test_reqs) { - test_req.WaitComplete(&usb_); + if (test_req.expect_cb()) { + test_req.WaitComplete(&usb_); + } } } @@ -342,6 +367,34 @@ return ZX_OK; } +zx_status_t UsbTester::VerifyCallbacks(const fbl::Vector<TestRequest>& reqs) { + size_t num_cbs = 0; + size_t i = 0; + for (auto& req : reqs) { + if (req.Get()->response.status == ZX_OK) { + if (req.expect_cb() != req.got_cb()) { + zxlogf(ERROR, "%lu (%p): %s\n", i, req.Get(), + req.expect_cb() ? "missing callback" : "got unexpected callback"); + return ZX_ERR_IO; + } + } else { + // Requests with errors should always get callbacks. Sometimes isochronous + // requests may fail unexpectedly. + if (!req.got_cb()) { + zxlogf(ERROR, "%lu (%p): missing callback for erroneous request\n", i, req.Get()); + return ZX_ERR_IO; + } + } + if (req.got_cb()) { + num_cbs++; + } + i++; + } + zxlogf(TRACE, "got %lu/%lu callbacks\n", num_cbs, i); + return ZX_OK; +} + + zx_status_t UsbTester::IsochLoopback(const fuchsia_hardware_usb_tester_IsochTestParams* params, fuchsia_hardware_usb_tester_IsochResult* result) { IsochLoopbackIntf* intf = &isoch_loopback_intf_; @@ -370,12 +423,13 @@ fbl::Vector<TestRequest> out_reqs; // We will likely get a few empty IN requests, as there is a delay between the start of an // OUT transfer and it being received. Allocate a few more IN requests to account for this. - status = AllocTestReqs(num_reqs + kIsochAdditionalInReqs, packet_size, intf->in_addr, - &in_reqs, parent_req_size_); + status = AllocIsochTestReqs(num_reqs + kIsochAdditionalInReqs, packet_size, intf->in_addr, + &in_reqs, parent_req_size_, nullptr, 0); if (status != ZX_OK) { goto done; } - status = AllocTestReqs(num_reqs, packet_size, intf->out_addr, &out_reqs, parent_req_size_); + status = AllocIsochTestReqs(num_reqs, packet_size, intf->out_addr, &out_reqs, parent_req_size_, + params->packet_opts, params->packet_opts_len); if (status != ZX_OK) { goto done; } @@ -403,6 +457,10 @@ if (status != ZX_OK) { goto done; } + status = VerifyCallbacks(out_reqs); + if (status != ZX_OK) { + goto done; + } result->num_passed = num_passed; result->num_packets = num_reqs; zxlogf(TRACE, "%lu / %lu passed\n", num_passed, num_reqs);
diff --git a/system/dev/usb/usb-test/usb-tester/usb-tester.h b/system/dev/usb/usb-test/usb-tester/usb-tester.h index 239716d..b180929 100644 --- a/system/dev/usb/usb-test/usb-tester/usb-tester.h +++ b/system/dev/usb/usb-test/usb-tester/usb-tester.h
@@ -30,17 +30,21 @@ public: // Creates a request for transferring |len| bytes at the given |ep_address|. static std::optional<TestRequest> Create(size_t len, uint8_t ep_address, - size_t parent_req_size); + size_t parent_req_size, bool set_cb = true, + bool expect_cb = true); // Creates a request for transferring data using the given scatter gather list. static std::optional<TestRequest> Create(const fuchsia_hardware_usb_tester_SgList& sg_list, - uint8_t ep_address, size_t parent_req_size); + uint8_t ep_address, size_t parent_req_size, + bool set_cb = true, bool expect_cb = true); ~TestRequest(); void MoveHelper(TestRequest& other) { if (usb_req_) { usb_request_release(usb_req_); } usb_req_ = other.usb_req_; other.usb_req_ = nullptr; + expect_cb_ = other.expect_cb_; + got_cb_ = other.got_cb_; } TestRequest(TestRequest&& other) : usb_req_(nullptr) { MoveHelper(other); } @@ -63,15 +67,22 @@ // Returns the underlying usb request. usb_request_t* Get() const { return usb_req_; } usb_request_complete_t* GetCompleteCb() { return &req_complete_; } + + bool expect_cb() const { return expect_cb_; } + bool got_cb() const { return got_cb_; } + private: - explicit TestRequest(usb_request_t* usb_req); + explicit TestRequest(usb_request_t* usb_req, bool set_cb, bool expect_cb); static void RequestCompleteCallback(void* ctx, usb_request_t* request); usb_request_complete_t req_complete_ = { .callback = RequestCompleteCallback, - .ctx = &completion_, + .ctx = this, }; usb_request_t* usb_req_; sync_completion_t completion_; + + bool expect_cb_; + bool got_cb_; }; class UsbTester; @@ -122,8 +133,10 @@ zx_status_t Bind(); // Allocates the test requests and adds them to the out_test_reqs list. - zx_status_t AllocTestReqs(size_t num_reqs, size_t len, uint8_t ep_addr, - fbl::Vector<TestRequest>* out_test_reqs, size_t parent_req_size); + zx_status_t AllocIsochTestReqs(size_t num_reqs, size_t len, uint8_t ep_addr, + fbl::Vector<TestRequest>* out_test_reqs, size_t parent_req_size, + const fuchsia_hardware_usb_tester_PacketOptions* packet_opts, + size_t packet_opts_len); // Waits for the completion of each request contained in the test_reqs list in sequential // order. // The caller should check each request for its completion status. @@ -141,6 +154,8 @@ zx_status_t VerifyLoopback(const fbl::Vector<TestRequest>& out_reqs, const fbl::Vector<TestRequest>& in_reqs, size_t* out_num_passed); + // Returns ZX_OK if callbacks were received only when expected. + zx_status_t VerifyCallbacks(const fbl::Vector<TestRequest>& reqs); usb_protocol_t usb_;
diff --git a/system/fidl/fuchsia-hardware-usb-tester/usb-tester.fidl b/system/fidl/fuchsia-hardware-usb-tester/usb-tester.fidl index a0ac654..a9db117 100644 --- a/system/fidl/fuchsia-hardware-usb-tester/usb-tester.fidl +++ b/system/fidl/fuchsia-hardware-usb-tester/usb-tester.fidl
@@ -7,6 +7,7 @@ using zx; const uint32 MAX_SG_SEGMENTS = 256; +const uint64 MAX_PACKETS = 256; enum DataPatternType : uint8 { CONSTANT = 1; @@ -20,6 +21,15 @@ uint64 len; }; +struct PacketOptions { + /// Whether to request a callback for the transfer. + bool set_cb; + /// Whether we want the transfer to fail with an error. + bool set_error; + /// Whether to expect a callback for the transfer. + bool expect_cb; +}; + struct IsochTestParams { /// The type of data to transfer. DataPatternType data_pattern; @@ -27,6 +37,14 @@ uint64 num_packets; /// Number of bytes in each packet. uint16 packet_size; + + /// Optional array of additional options for the OUT packets. + // TODO(jocelyndang): A vector would break the current requirement for a simple C binding. + array<PacketOptions>:MAX_PACKETS packet_opts; + /// Number of entries in |packet_opts|. This can be less than |num_packets|, + /// in which case defaults will be chosen for the remaining packets. + /// Any entries provided after |num_packets| will be ignored. + uint64 packet_opts_len; }; struct SgEntry {
diff --git a/system/utest/usb/usb-test.c b/system/utest/usb/usb-test.c index 1c7ec71..5163771 100644 --- a/system/utest/usb/usb-test.c +++ b/system/utest/usb/usb-test.c
@@ -197,11 +197,49 @@ END_TEST; } +static bool usb_callbacks_opt_out_test(void) { + BEGIN_TEST; + + zx_handle_t dev_svc; + if (open_test_device(&dev_svc) != ZX_OK) { + unittest_printf_critical(" [SKIPPING]"); + return true; + } + ASSERT_NE(dev_svc, ZX_HANDLE_INVALID, "Invalid device service handle"); + + fuchsia_hardware_usb_tester_IsochTestParams params = { + .data_pattern = fuchsia_hardware_usb_tester_DataPatternType_CONSTANT, + .num_packets = 64, + .packet_size = 1024, + .packet_opts_len = params.num_packets + }; + size_t reqs_per_callback = 10; + for (size_t i = 0; i < params.num_packets; ++i) { + // Set a callback on every 10 requests, and also on the last request. + bool set_cb = ((i + 1) % reqs_per_callback == 0) || + (i == params.num_packets - 1); + params.packet_opts[i].set_cb = set_cb; + params.packet_opts[i].expect_cb = set_cb; + } + + zx_status_t status; + fuchsia_hardware_usb_tester_IsochResult result = {}; + ASSERT_EQ(fuchsia_hardware_usb_tester_DeviceIsochLoopback(dev_svc, ¶ms, &status, &result), + ZX_OK, "failed to call DeviceIsochLoopback"); + ASSERT_EQ(status, ZX_OK, ""); + ASSERT_TRUE(usb_isoch_verify_result(&result), "callbacks test failed: 10 reqs per callback"); + + close(dev_svc); + + END_TEST; +} + BEGIN_TEST_CASE(usb_tests) RUN_TEST(usb_root_hubs_test) RUN_TEST(usb_bulk_loopback_test) RUN_TEST(usb_bulk_scatter_gather_test) RUN_TEST(usb_isoch_loopback_test) +RUN_TEST(usb_callbacks_opt_out_test) END_TEST_CASE(usb_tests) int main(int argc, char** argv) {