diff --git a/zircon/system/dev/ethernet/ethernet/ethernet.cpp b/zircon/system/dev/ethernet/ethernet/ethernet.cpp index 4a22d216d33b432fdda643a1ed0ab66fcb06e39e..144ef3a31a4b65bc23270619b4b353bcf93c1c3a 100644 --- a/zircon/system/dev/ethernet/ethernet/ethernet.cpp +++ b/zircon/system/dev/ethernet/ethernet/ethernet.cpp @@ -3,10 +3,13 @@ // found in the LICENSE file. #include "ethernet.h" +#include <type_traits> namespace eth { TransmitInfo* EthDev0::NetbufToTransmitInfo(ethmac_netbuf_t* netbuf) { + // NOTE: Alignment is guaranteed by the static_asserts for alignment and padding of the + // TransmitInfo structure, combined with the value of transmit_buffer_size_. return reinterpret_cast<TransmitInfo*>(reinterpret_cast<uintptr_t>(netbuf) + info_.netbuf_size); } @@ -210,11 +213,16 @@ TransmitInfo* EthDev::GetTransmitInfo() { if (transmit_info == nullptr) { zxlogf(ERROR, "eth [%s]: transmit_info pool empty\n", name_); } + new (transmit_info) TransmitInfo(); + transmit_info->edev = fbl::RefPtr<EthDev>(this); return transmit_info; } // Returns a TX buffer to the pool. void EthDev::PutTransmitInfo(TransmitInfo* transmit_info) { + // Call the destructor on TransmitInfo since we are effectively "freeing" the + // TransmitInfo structure. This needs to be done manually, since it is an inline structure. + transmit_info->~TransmitInfo(); fbl::AutoLock lock(&lock_); list_add_head(&free_transmit_buffers_, &transmit_info->node); } @@ -245,7 +253,7 @@ void EthDev0::Recv(const void* data, size_t len, uint32_t flags) TA_NO_THREAD_SA void EthDev0::CompleteTx(ethmac_netbuf_t* netbuf, zx_status_t status) { TransmitInfo* transmit_info = NetbufToTransmitInfo(netbuf); - EthDev* edev = transmit_info->edev; + auto edev = transmit_info->edev; eth_fifo_entry_t entry = { .offset = static_cast<uint32_t>(reinterpret_cast<const char*>(netbuf->data_buffer) - reinterpret_cast<const char*>(edev->io_buffer_.start())), @@ -544,7 +552,7 @@ zx_status_t EthDev::StartLocked() TA_NO_THREAD_SAFETY_ANALYSIS { if (status == ZX_OK) { state_ |= kStateRunning; edev0_->list_idle_.erase(*this); - edev0_->list_active_.push_back(this); + edev0_->list_active_.push_back(fbl::WrapRefPtr(this)); // Trigger the status signal so the client will query the status at the start. receive_fifo_.signal_peer(0, fuchsia_hardware_ethernet_SIGNAL_STATUS); } else { @@ -560,7 +568,7 @@ zx_status_t EthDev::StopLocked() TA_NO_THREAD_SAFETY_ANALYSIS { if (state_ & kStateRunning) { state_ &= (~kStateRunning); edev0_->list_active_.erase(*this); - edev0_->list_idle_.push_back(this); + edev0_->list_idle_.push_back(fbl::WrapRefPtr(this)); // The next three lines clean up promisc, multicast-promisc, and multicast-filter, in case // this ethdev had any state set. Ignore failures, which may come from drivers not // supporting the feature. (TODO: check failure codes). @@ -802,7 +810,17 @@ void EthDev::KillLocked() { void EthDev::StopAndKill() { fbl::AutoLock lock(&edev0_->ethdev_lock_); StopLocked(); - KillLocked(); + SetPromiscLocked(false); + if (transmit_fifo_.is_valid()) { + // Ask the Transmit thread to exit. + transmit_fifo_.signal(0, kSignalFifoTerminate); + } + if (state_ & kStateTransmitThreadCreated) { + state_ &= (~kStateTransmitThreadCreated); + int ret; + thrd_join(transmit_thread_, &ret); + zxlogf(TRACE, "eth [%s]: kill: tx thread exited\n", name_); + } // Check if it is part of the idle list and remove. // It will not be part of active list as StopLocked() would have moved it to Idle. if (InContainer()) { @@ -811,9 +829,35 @@ void EthDev::StopAndKill() { } void EthDev::DdkRelease() { - // Ensure that the instance is stopped. - ZX_DEBUG_ASSERT(state_ & kStateDead); - delete this; + // Release the device (and wait for completion)! + if (Release()) { + delete this; + } else { + // TODO (ZX-3934): It is not presently safe to block here. + // So we cannot satisfy the assumptions of the DDK. + // If we block here, we will deadlock the entire system + // due to the virtual bus's control channel being controlled via FIDL. + // as well as its need to issue lifecycle events to the main event loop + // in order to remove the bus during shutdown. + // Uncomment the lines below when we can do so safely. + // sync_completion_t completion; + // completion_ = &completion; + // sync_completion_wait(&completion, ZX_TIME_INFINITE); + } +} + +EthDev::~EthDev() { + if (transmit_fifo_.is_valid()) { + // Ask the Transmit thread to exit. + transmit_fifo_.signal(0, kSignalFifoTerminate); + } + if (state_ & kStateTransmitThreadCreated) { + state_ &= (~kStateTransmitThreadCreated); + int ret; + thrd_join(transmit_thread_, &ret); + zxlogf(TRACE, "eth [%s]: kill: tx thread exited\n", name_); + } + // sync_completion_signal(completion_); } zx_status_t EthDev::DdkOpen(zx_device_t** out, uint32_t flags) { @@ -848,8 +892,13 @@ zx_status_t EthDev::DdkClose(uint32_t flags) { zx_status_t EthDev::AddDevice(zx_device_t** out) { zx_status_t status; - transmit_buffer_size_ = ROUNDUP(sizeof(TransmitInfo) + edev0_->info_.netbuf_size, 8); - + transmit_buffer_size_ = + ROUNDUP(sizeof(TransmitInfo) + edev0_->info_.netbuf_size, __STDCPP_DEFAULT_NEW_ALIGNMENT__); + // Ensure that we can meet alignment requirement of TransmitInfo in this allocation, + // and that sufficient padding exists between elements in the struct to guarantee safe + // accesses of this array. + static_assert(std::alignment_of_v<TransmitInfo> <= __STDCPP_DEFAULT_NEW_ALIGNMENT__); + static_assert(std::alignment_of_v<TransmitInfo> <= sizeof(ethmac_netbuf_t)); fbl::AllocChecker ac; fbl::unique_ptr<uint8_t[]> all_transmit_buffers = fbl::unique_ptr<uint8_t[]>(new (&ac) uint8_t[kFifoDepth * transmit_buffer_size_]()); @@ -859,11 +908,9 @@ zx_status_t EthDev::AddDevice(zx_device_t** out) { list_initialize(&free_transmit_buffers_); for (size_t ndx = 0; ndx < kFifoDepth; ndx++) { - ethmac_netbuf_t* netbuf = - (ethmac_netbuf_t*)((uintptr_t)all_transmit_buffers.get() + - (transmit_buffer_size_ * ndx)); + ethmac_netbuf_t* netbuf = (ethmac_netbuf_t*)((uintptr_t)all_transmit_buffers.get() + + (transmit_buffer_size_ * ndx)); TransmitInfo* transmit_info = edev0_->NetbufToTransmitInfo(netbuf); - transmit_info->edev = this; list_add_tail(&free_transmit_buffers_, &transmit_info->node); } @@ -880,11 +927,15 @@ zx_status_t EthDev::AddDevice(zx_device_t** out) { zx_status_t EthDev0::DdkOpen(zx_device_t** out, uint32_t flags) { fbl::AllocChecker ac; - auto edev = fbl::make_unique_checked<EthDev>(&ac, this->zxdev_, this); + auto edev = fbl::MakeRefCountedChecked<EthDev>(&ac, this->zxdev_, this); + // Hold a second reference to the device to prevent a use-after-free + // in the case where DdkRelease is called immediately after AddDevice. + fbl::RefPtr<EthDev> dev_ref_2 = edev; if (!ac.check()) { return ZX_ERR_NO_MEMORY; } - + // Add a reference for the devhost handle. + // This will be removed in DdkRelease. zx_status_t status; if ((status = edev->AddDevice(out)) < 0) { return status; @@ -892,13 +943,9 @@ zx_status_t EthDev0::DdkOpen(zx_device_t** out, uint32_t flags) { { fbl::AutoLock lock(ðdev_lock_); - list_idle_.push_back(edev.get()); + list_idle_.push_back(edev); } - - // On successful Add, Devmgr takes ownership (relinquished on DdkRelease), - // so transfer our ownership to a local var, and let it go out of scope. - auto __UNUSED temp_ref = edev.release(); - + __UNUSED auto dev = edev.leak_ref(); return ZX_OK; } @@ -1011,7 +1058,15 @@ static zx_driver_ops_t eth_driver_ops = { .init = nullptr, .bind = ð::EthDev0::EthBind, .create = nullptr, - .release = nullptr}; + .release = [](void* ctx) { + // We don't support unloading. Assert if this ever + // happens. In order to properly support unloading, + // we need a way to inform the DDK when all of our + // resources have been freed, so it can safely + // unload the driver. This mechanism does not currently + // exist. + ZX_ASSERT(false); + }}; // clang-format off ZIRCON_DRIVER_BEGIN(ethernet, eth_driver_ops, "zircon", "0.1", 1) diff --git a/zircon/system/dev/ethernet/ethernet/ethernet.h b/zircon/system/dev/ethernet/ethernet/ethernet.h index 8d503e14ed5f1b95faa87c07e9cd1d852f7000ef..2f94800b20fe7441c641866d77749be6a569ee5a 100644 --- a/zircon/system/dev/ethernet/ethernet/ethernet.h +++ b/zircon/system/dev/ethernet/ethernet/ethernet.h @@ -30,6 +30,9 @@ #include <zircon/thread_annotations.h> #include <zircon/types.h> +#include <fbl/ref_counted.h> +#include <fbl/ref_ptr.h> +#include <lib/sync/completion.h> #include <limits.h> #include <stdio.h> #include <stdlib.h> @@ -42,7 +45,7 @@ class EthDev0; class EthDev; struct TransmitInfo { - EthDev* edev; + fbl::RefPtr<EthDev> edev; uint64_t fifo_cookie; list_node_t node; }; @@ -91,15 +94,16 @@ private: fbl::Mutex ethdev_lock_; // Active and idle instances (EthDev). - fbl::DoublyLinkedList<EthDev*> list_active_ __TA_GUARDED(ethdev_lock_); - fbl::DoublyLinkedList<EthDev*> list_idle_ __TA_GUARDED(ethdev_lock_); + fbl::DoublyLinkedList<fbl::RefPtr<EthDev>> list_active_ __TA_GUARDED(ethdev_lock_); + fbl::DoublyLinkedList<fbl::RefPtr<EthDev>> list_idle_ __TA_GUARDED(ethdev_lock_); }; using EthDevType = ddk::Device<EthDev, ddk::Openable, ddk::Closable, ddk::Messageable>; class EthDev : public EthDevType, public ddk::EmptyProtocol<ZX_PROTOCOL_ETHERNET>, - public fbl::DoublyLinkedListable<EthDev*> { + public fbl::DoublyLinkedListable<fbl::RefPtr<EthDev>>, + public fbl::RefCounted<EthDev> { public: EthDev(zx_device_t* parent, EthDev0* edev0) @@ -140,8 +144,8 @@ public: __TA_REQUIRES(edev0_->ethdev_lock_); zx_status_t MsgConfigMulticastTestFilterLocked(fidl_txn_t* txn) __TA_REQUIRES(edev0_->ethdev_lock_); - zx_status_t MsgDumpRegistersLocked(fidl_txn_t* txn) - __TA_REQUIRES(edev0_->ethdev_lock_); + zx_status_t MsgDumpRegistersLocked(fidl_txn_t* txn) __TA_REQUIRES(edev0_->ethdev_lock_); + ~EthDev(); private: friend class EthDev0; @@ -253,6 +257,7 @@ private: uint32_t fail_receive_write_ = 0; uint32_t ethmac_request_count_ = 0; uint32_t ethmac_response_count_ = 0; + // sync_completion_t* completion_ = nullptr; }; } // namespace eth diff --git a/zircon/system/dev/ethernet/usb-cdc-ecm/usb-cdc-ecm.c b/zircon/system/dev/ethernet/usb-cdc-ecm/usb-cdc-ecm.c index 1b58f34fd6fba16914e0f56034efb05da8d8b1c4..013115bb006315e6c10a3faf2ca0f5aef5ee88da 100644 --- a/zircon/system/dev/ethernet/usb-cdc-ecm/usb-cdc-ecm.c +++ b/zircon/system/dev/ethernet/usb-cdc-ecm/usb-cdc-ecm.c @@ -44,7 +44,8 @@ typedef struct { zx_device_t* zxdev; zx_device_t* usb_device; usb_protocol_t usb; - + // Ethmac lock -- must be acquired after tx_mutex + // when both locks are held. mtx_t ethmac_mutex; ethmac_ifc_protocol_t ethmac_ifc; @@ -64,6 +65,8 @@ typedef struct { thrd_t int_thread; // Send context + // TX lock -- Must be acquired before ethmac_mutex + // when both locks are held. mtx_t tx_mutex; ecm_endpoint_t tx_endpoint; list_node_t tx_txn_bufs; // list of usb_request_t @@ -180,9 +183,11 @@ static zx_status_t ecm_ethmac_query(void* ctx, uint32_t options, ethmac_info_t* static void ecm_ethmac_stop(void* cookie) { zxlogf(TRACE, "%s: %s called\n", module_name, __FUNCTION__); ecm_ctx_t* ctx = cookie; + mtx_lock(&ctx->tx_mutex); mtx_lock(&ctx->ethmac_mutex); ctx->ethmac_ifc.ops = NULL; mtx_unlock(&ctx->ethmac_mutex); + mtx_unlock(&ctx->tx_mutex); } static zx_status_t ecm_ethmac_start(void* ctx_cookie, const ethmac_ifc_protocol_t* ifc) { @@ -205,9 +210,13 @@ static zx_status_t ecm_ethmac_start(void* ctx_cookie, const ethmac_ifc_protocol_ static zx_status_t queue_request(ecm_ctx_t* ctx, const uint8_t* data, size_t length, usb_request_t* req) { req->header.length = length; + if (!ctx->ethmac_ifc.ops) { + return ZX_ERR_BAD_STATE; + } ssize_t bytes_copied = usb_request_copy_to(req, data, length, 0); if (bytes_copied < 0) { - zxlogf(ERROR, "%s: failed to copy data into send txn (error %zd)\n", module_name, bytes_copied); + zxlogf(ERROR, "%s: failed to copy data into send txn (error %zd)\n", module_name, + bytes_copied); return ZX_ERR_IO; } usb_request_complete_t complete = { diff --git a/zircon/system/dev/ethernet/usb-cdc-function/cdc-eth-function.cpp b/zircon/system/dev/ethernet/usb-cdc-function/cdc-eth-function.cpp index 1a8cc72d3a61bfbe648e43ba451f93c4f0339afb..fabe869ad892b1728b4459b5069fb7a9738c18a7 100644 --- a/zircon/system/dev/ethernet/usb-cdc-function/cdc-eth-function.cpp +++ b/zircon/system/dev/ethernet/usb-cdc-function/cdc-eth-function.cpp @@ -3,12 +3,16 @@ // found in the LICENSE file. #include <assert.h> +#include <atomic> +#include <fbl/auto_lock.h> #include <inttypes.h> +#include <memory> #include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <threads.h> +#include <zircon/errors.h> #include <ddk/binding.h> #include <ddk/debug.h> @@ -32,37 +36,43 @@ namespace usb_cdc_function { #define BULK_RX_COUNT 16 #define INTR_COUNT 8 -#define BULK_MAX_PACKET 512 // FIXME(voydanoff) USB 3.0 support -#define INTR_MAX_PACKET sizeof(usb_cdc_speed_change_notification_t) +#define BULK_MAX_PACKET 512 // FIXME(voydanoff) USB 3.0 support +#define INTR_MAX_PACKET sizeof(usb_cdc_speed_change_notification_t) typedef struct { - zx_device_t* zxdev; - usb_function_protocol_t function; + zx_device_t* zxdev = nullptr; + usb_function_protocol_t function = {}; - list_node_t bulk_out_reqs; // list of usb_request_t - list_node_t bulk_in_reqs; // list of usb_request_t - list_node_t intr_reqs; // list of usb_request_t - list_node_t tx_pending_infos; // list of ethmac_netbuf_t - bool unbound; // set to true when device is going away. Guarded by tx_mutex + list_node_t bulk_out_reqs = {}; // list of usb_request_t + list_node_t bulk_in_reqs = {}; // list of usb_request_t + list_node_t intr_reqs = {}; // list of usb_request_t + list_node_t tx_pending_infos = {}; // list of ethmac_netbuf_t + bool unbound = false; // set to true when device is going away. Guarded by tx_mutex // Device attributes - uint8_t mac_addr[ETH_MAC_SIZE]; - - mtx_t ethmac_mutex; - ethmac_ifc_protocol_t ethmac_ifc; - bool online; - usb_speed_t speed; - - mtx_t tx_mutex; - mtx_t rx_mutex; - mtx_t intr_mutex; - - uint8_t bulk_out_addr; - uint8_t bulk_in_addr; - uint8_t intr_addr; - uint16_t bulk_max_packet; - - size_t parent_req_size; + uint8_t mac_addr[ETH_MAC_SIZE] = {}; + // Ethmac lock -- must be acquired after tx_mutex + // when both locks are held. + mtx_t ethmac_mutex = {}; + ethmac_ifc_protocol_t ethmac_ifc = {}; + bool online = false; + usb_speed_t speed = 0; + // TX lock -- Must be acquired before ethmac_mutex + // when both locks are held. + mtx_t tx_mutex = {}; + mtx_t rx_mutex = {}; + mtx_t intr_mutex = {}; + + uint8_t bulk_out_addr = 0; + uint8_t bulk_in_addr = 0; + uint8_t intr_addr = 0; + uint16_t bulk_max_packet = 0; + + size_t parent_req_size = 0; + mtx_t pending_request_lock = {}; + cnd_t pending_requests_completed = {}; + std::atomic_int32_t pending_request_count; + size_t usb_request_offset = 0; } usb_cdc_t; typedef struct txn_info { @@ -165,6 +175,41 @@ typedef struct txn_info { static void cdc_tx_complete(void* ctx, usb_request_t* req); +static void usb_request_callback(void* ctx, usb_request_t* req) { + usb_cdc_t* cdc = static_cast<usb_cdc_t*>(ctx); + // Invoke the real completion if not shutting down. + if (!cdc->unbound) { + usb_request_complete_t completion; + memcpy(&completion, reinterpret_cast<unsigned char*>(req) + cdc->usb_request_offset, + sizeof(completion)); + completion.callback(completion.ctx, req); + } + int value = --cdc->pending_request_count; + if (value == 0) { + mtx_lock(&cdc->pending_request_lock); + cnd_signal(&cdc->pending_requests_completed); + mtx_unlock(&cdc->pending_request_lock); + } +} + +static void usb_request_queue(void* ctx, usb_function_protocol_t* function, usb_request_t* req, + const usb_request_complete_t* completion) { + usb_cdc_t* cdc = static_cast<usb_cdc_t*>(ctx); + mtx_lock(&cdc->pending_request_lock); + if (cdc->unbound) { + mtx_unlock(&cdc->pending_request_lock); + return; + } + cdc->pending_request_count++; + mtx_unlock(&cdc->pending_request_lock); + usb_request_complete_t internal_completion; + internal_completion.callback = usb_request_callback; + internal_completion.ctx = ctx; + memcpy(reinterpret_cast<unsigned char*>(req) + cdc->usb_request_offset, completion, + sizeof(*completion)); + usb_function_request_queue(function, req, &internal_completion); +} + static zx_status_t cdc_generate_mac_address(usb_cdc_t* cdc) { zx_cprng_draw(cdc->mac_addr, sizeof(cdc->mac_addr)); @@ -204,17 +249,20 @@ static zx_status_t cdc_ethmac_query(void* ctx, uint32_t options, ethmac_info_t* static void cdc_ethmac_stop(void* cookie) { zxlogf(TRACE, "%s:\n", __func__); auto* cdc = static_cast<usb_cdc_t*>(cookie); - + mtx_lock(&cdc->tx_mutex); mtx_lock(&cdc->ethmac_mutex); cdc->ethmac_ifc.ops = NULL; mtx_unlock(&cdc->ethmac_mutex); + mtx_unlock(&cdc->tx_mutex); } static zx_status_t cdc_ethmac_start(void* ctx_cookie, const ethmac_ifc_protocol_t* ifc) { zxlogf(TRACE, "%s:\n", __func__); auto* cdc = static_cast<usb_cdc_t*>(ctx_cookie); zx_status_t status = ZX_OK; - + if (cdc->unbound) { + return ZX_ERR_BAD_STATE; + } mtx_lock(&cdc->ethmac_mutex); if (cdc->ethmac_ifc.ops) { status = ZX_ERR_ALREADY_BOUND; @@ -228,6 +276,9 @@ static zx_status_t cdc_ethmac_start(void* ctx_cookie, const ethmac_ifc_protocol_ } static zx_status_t cdc_send_locked(usb_cdc_t* cdc, ethmac_netbuf_t* netbuf) { + if (!cdc->ethmac_ifc.ops) { + return ZX_ERR_BAD_STATE; + } const auto* byte_data = static_cast<const uint8_t*>(netbuf->data_buffer); size_t length = netbuf->data_size; @@ -254,7 +305,7 @@ static zx_status_t cdc_send_locked(usb_cdc_t* cdc, ethmac_netbuf_t* netbuf) { .callback = cdc_tx_complete, .ctx = cdc, }; - usb_function_request_queue(&cdc->function, tx_req, &complete); + usb_request_queue(cdc, &cdc->function, tx_req, &complete); return ZX_OK; } @@ -264,7 +315,7 @@ static zx_status_t cdc_ethmac_queue_tx(void* cookie, uint32_t options, ethmac_ne size_t length = netbuf->data_size; zx_status_t status; - if (!cdc->online || length > ETH_MTU || length == 0) { + if (!cdc->online || length > ETH_MTU || length == 0 || cdc->unbound) { return ZX_ERR_INVALID_ARGS; } @@ -362,7 +413,7 @@ static void cdc_send_notifications(usb_cdc_t* cdc) { .callback = cdc_intr_complete, .ctx = cdc, }; - usb_function_request_queue(&cdc->function, req, &complete); + usb_request_queue(cdc, &cdc->function, req, &complete); mtx_lock(&cdc->intr_mutex); req = usb_req_list_remove_head(&cdc->intr_reqs, cdc->parent_req_size); @@ -375,7 +426,7 @@ static void cdc_send_notifications(usb_cdc_t* cdc) { usb_request_copy_to(req, &speed_notification, sizeof(speed_notification), 0); req->header.length = sizeof(speed_notification); - usb_function_request_queue(&cdc->function, req, &complete); + usb_request_queue(cdc, &cdc->function, req, &complete); } static void cdc_rx_complete(void* ctx, usb_request_t* req) { @@ -409,14 +460,16 @@ static void cdc_rx_complete(void* ctx, usb_request_t* req) { .callback = cdc_rx_complete, .ctx = cdc, }; - usb_function_request_queue(&cdc->function, req, &complete); + usb_request_queue(cdc, &cdc->function, req, &complete); } static void cdc_tx_complete(void* ctx, usb_request_t* req) { auto* cdc = static_cast<usb_cdc_t*>(ctx); zxlogf(LTRACE, "%s %d %ld\n", __func__, req->response.status, req->response.actual); - + if (cdc->unbound) { + return; + } mtx_lock(&cdc->tx_mutex); zx_status_t status = usb_req_list_add_tail(&cdc->bulk_in_reqs, req, cdc->parent_req_size); ZX_DEBUG_ASSERT(status == ZX_OK); @@ -536,7 +589,7 @@ static zx_status_t cdc_set_interface(void* ctx, uint8_t interface, uint8_t alt_s .callback = cdc_rx_complete, .ctx = cdc, }; - usb_function_request_queue(&cdc->function, req, &complete); + usb_request_queue(cdc, &cdc->function, req, &complete); } mtx_unlock(&cdc->rx_mutex); } @@ -565,18 +618,26 @@ usb_function_interface_protocol_ops_t device_ops = { static void usb_cdc_unbind(void* ctx) { zxlogf(TRACE, "%s\n", __func__); auto* cdc = static_cast<usb_cdc_t*>(ctx); - - mtx_lock(&cdc->tx_mutex); - cdc->unbound = true; - if (cdc->ethmac_ifc.ops) { - txn_info_t* txn; - while ((txn = list_remove_head_type(&cdc->tx_pending_infos, txn_info_t, node)) != - NULL) { - ethmac_ifc_complete_tx(&cdc->ethmac_ifc, &txn->netbuf, ZX_ERR_PEER_CLOSED); + { + fbl::AutoLock l(&cdc->tx_mutex); + cdc->unbound = true; + } + { + fbl::AutoLock l(&cdc->pending_request_lock); + while (cdc->pending_request_count) { + cnd_wait(&cdc->pending_requests_completed, &cdc->pending_request_lock); + } + } + { + fbl::AutoLock l(&cdc->tx_mutex); + if (cdc->ethmac_ifc.ops) { + txn_info_t* txn; + while ((txn = list_remove_head_type(&cdc->tx_pending_infos, txn_info_t, node)) != + NULL) { + ethmac_ifc_complete_tx(&cdc->ethmac_ifc, &txn->netbuf, ZX_ERR_PEER_CLOSED); + } } } - mtx_unlock(&cdc->tx_mutex); - device_remove(cdc->zxdev); } @@ -598,10 +659,10 @@ static void usb_cdc_release(void* ctx) { mtx_destroy(&cdc->tx_mutex); mtx_destroy(&cdc->rx_mutex); mtx_destroy(&cdc->intr_mutex); - free(cdc); + delete cdc; } -static zx_protocol_device_t usb_cdc_proto = [](){ +static zx_protocol_device_t usb_cdc_proto = []() { zx_protocol_device_t dev = {}; dev.version = DEVICE_OPS_VERSION; dev.unbind = usb_cdc_unbind; @@ -611,20 +672,18 @@ static zx_protocol_device_t usb_cdc_proto = [](){ zx_status_t usb_cdc_bind(void* ctx, zx_device_t* parent) { zxlogf(INFO, "%s\n", __func__); - device_add_args_t args = {}; - auto* cdc = static_cast<usb_cdc_t*>(calloc(1, sizeof(usb_cdc_t))); + auto cdc = std::make_unique<usb_cdc_t>(); if (!cdc) { return ZX_ERR_NO_MEMORY; } zx_status_t status = device_get_protocol(parent, ZX_PROTOCOL_USB_FUNCTION, &cdc->function); if (status != ZX_OK) { - free(cdc); return status; } - + cnd_init(&cdc->pending_requests_completed); list_initialize(&cdc->bulk_out_reqs); list_initialize(&cdc->bulk_in_reqs); list_initialize(&cdc->intr_reqs); @@ -636,8 +695,9 @@ zx_status_t usb_cdc_bind(void* ctx, zx_device_t* parent) { cdc->bulk_max_packet = BULK_MAX_PACKET; // FIXME(voydanoff) USB 3.0 support cdc->parent_req_size = usb_function_get_request_size(&cdc->function); - uint64_t req_size = cdc->parent_req_size + sizeof(usb_req_internal_t); - + uint64_t req_size = + cdc->parent_req_size + sizeof(usb_req_internal_t) + sizeof(usb_request_complete_t); + cdc->usb_request_offset = cdc->parent_req_size + sizeof(usb_req_internal_t); status = usb_function_alloc_interface(&cdc->function, &descriptors.comm_intf.bInterfaceNumber); if (status != ZX_OK) { zxlogf(ERROR, "%s: usb_function_alloc_interface failed\n", __func__); @@ -672,7 +732,7 @@ zx_status_t usb_cdc_bind(void* ctx, zx_device_t* parent) { descriptors.bulk_in_ep.bEndpointAddress = cdc->bulk_in_addr; descriptors.intr_ep.bEndpointAddress = cdc->intr_addr; - status = cdc_generate_mac_address(cdc); + status = cdc_generate_mac_address(cdc.get()); if (status != ZX_OK) { goto fail; } @@ -715,7 +775,7 @@ zx_status_t usb_cdc_bind(void* ctx, zx_device_t* parent) { args.version = DEVICE_ADD_ARGS_VERSION; args.name = "cdc-eth-function"; - args.ctx = cdc; + args.ctx = cdc.get(); args.ops = &usb_cdc_proto; args.proto_id = ZX_PROTOCOL_ETHMAC; args.proto_ops = ðmac_ops; @@ -725,13 +785,15 @@ zx_status_t usb_cdc_bind(void* ctx, zx_device_t* parent) { zxlogf(ERROR, "%s: add_device failed %d\n", __func__, status); goto fail; } - - usb_function_set_interface(&cdc->function, cdc, &device_ops); - + usb_function_set_interface(&cdc->function, cdc.get(), &device_ops); + { + // The DDK now owns this reference. + __UNUSED auto released = cdc.release(); + } return ZX_OK; fail: - usb_cdc_release(cdc); + usb_cdc_release(cdc.get()); return status; } diff --git a/zircon/system/dev/usb/usb-peripheral/usb-peripheral.cpp b/zircon/system/dev/usb/usb-peripheral/usb-peripheral.cpp index 761ee511d3e0586975d292a8d15d24a231dba05a..da0b1dc55383755e7d7364e8999f586c59101a8d 100644 --- a/zircon/system/dev/usb/usb-peripheral/usb-peripheral.cpp +++ b/zircon/system/dev/usb/usb-peripheral/usb-peripheral.cpp @@ -61,6 +61,10 @@ void UsbPeripheral::RequestComplete(usb_request_t* req) { void UsbPeripheral::UsbPeripheralRequestQueue(usb_request_t* usb_request, const usb_request_complete_t* complete_cb) { + if (shutting_down_) { + usb_request_complete(usb_request, ZX_ERR_IO_NOT_PRESENT, 0, complete_cb); + return; + } fbl::AutoLock l(&pending_requests_lock_); usb::UnownedRequest<void> request(usb_request, *complete_cb, dci_.GetRequestSize()); __UNUSED usb_request_complete_t completion; @@ -461,6 +465,7 @@ zx_status_t UsbPeripheral::BindFunctions() { zx_status_t UsbPeripheral::ClearFunctions() { fbl::AutoLock lock(&lock_); + shutting_down_ = true; for (size_t i = 0; i < 256; i++) { dci_.CancelAll(static_cast<uint8_t>(i)); } @@ -470,7 +475,7 @@ zx_status_t UsbPeripheral::ClearFunctions() { function->DdkRemove(); } } - this->shutting_down_ = false; + shutting_down_ = false; functions_.reset(); config_desc_.reset(); functions_bound_ = false; diff --git a/zircon/system/dev/usb/usb-peripheral/usb-peripheral.h b/zircon/system/dev/usb/usb-peripheral/usb-peripheral.h index 35d6a2e5567d30b07519d48c56c3fa6f36a22c67..0058a41a2f86a0d7666e5b1beb3a08302ca1d889 100644 --- a/zircon/system/dev/usb/usb-peripheral/usb-peripheral.h +++ b/zircon/system/dev/usb/usb-peripheral/usb-peripheral.h @@ -191,7 +191,7 @@ private: // True if we are connected to a host, bool connected_ __TA_GUARDED(lock_) = false; // True if we are shutting down/clearing functions - bool shutting_down_ __TA_GUARDED(lock_) = false; + bool shutting_down_ = false; // Current configuration number selected via USB_REQ_SET_CONFIGURATION // (will be 0 or 1 since we currently do not support multiple configurations). uint8_t configuration_;