From 118cbef40d87140983685c3838a2a4d9cde1e3f9 Mon Sep 17 00:00:00 2001 From: Artem Egorkine Date: Wed, 23 Oct 2024 01:40:13 +0300 Subject: [PATCH] USB: fix device open & endpoint config code * Detach kernel driver from all interfaces it is attached to. In case of PocketPOD, the kernel driver is attached to both iface 0 and 1 and it all needs to be detached, otherwise setting config will fail! Learned the hard way; * If endpoint alt setting is 0, don't set alt setting 0, otherwise this messed the endpoints up and you won't be able to read/write! Learned the hard way; * Upon error and when closing the device, reattach the kernel back. For some reason, this fails with "resource busy", but the device seems to work fine afterwards. Will investigate; * Fix `check!` macro to correctly match the LIBUSB_SUCCESS constant! LEARNED THE HARD WAY!!! ;( * Fix the callback to correctly write to log and stop on errors; --- usb/src/dev_handler.rs | 111 +++++++++++++++++++++++++++++++++++------ usb/src/endpoint.rs | 25 +--------- usb/src/usb.rs | 23 +++++++-- 3 files changed, 117 insertions(+), 42 deletions(-) diff --git a/usb/src/dev_handler.rs b/usb/src/dev_handler.rs index 877c101..b39ea2b 100644 --- a/usb/src/dev_handler.rs +++ b/usb/src/dev_handler.rs @@ -9,7 +9,7 @@ use rusb::{Direction, UsbContext}; use tokio::sync::mpsc; use pod_core::midi_io::{MidiIn, MidiOut}; use crate::devices::UsbDevice; -use crate::endpoint::{configure_endpoint, Endpoint, find_endpoint}; +use crate::endpoint::{Endpoint, find_endpoint}; use crate::line6::line6_read_serial; use crate::usb::{DeviceHandle, SubmittedTransfer, Transfer, TransferCommand}; use crate::util::usb_address_string; @@ -22,12 +22,20 @@ pub struct Device { inner: Weak } +struct DevOpenState { + /// Active configuration when opening the device + config: u8, + /// Kernel driver attach status per interface + attach: Vec<(u8, bool)> +} + pub struct DeviceInner { name: String, handle: Arc, write_ep: Endpoint, closed: Arc, - read: SubmittedTransfer + read: SubmittedTransfer, + kernel_state: DevOpenState, } pub struct DeviceInput { @@ -91,7 +99,7 @@ impl Device { self.read_ep.clone(), self.write_ep.clone(), tx - )); + )?); self.inner = Arc::downgrade(&inner); let input = DeviceInput { @@ -110,17 +118,28 @@ impl Device { impl DeviceInner { fn new(name: String, handle: Arc, read_ep: Endpoint, write_ep: Endpoint, - tx: mpsc::UnboundedSender>) -> Self { + tx: mpsc::UnboundedSender>) -> Result { - //handle.detach_kernel_driver(read_ep.iface).ok(); - handle.detach_kernel_driver(read_ep.iface).ok(); - configure_endpoint(&handle, &read_ep).map_err(|e| { - error!("Failed to configure end-points: {}", e); + let kernel_state = Self::detach_kernel_driver(&handle).map_err(|e| { + anyhow!("Failed to detach kernel driver when opening device: {e}") + })?; + + handle.reset().map_err(|e| { + error!("Failed to reset USB device: {}", e); }).ok(); - configure_endpoint(&handle, &read_ep).map_err(|e| { - error!("Failed to configure end-points: {}", e); + + handle.set_active_configuration(read_ep.config).map_err(|e| { + error!("Set active config error: {}", e); }).ok(); + Self::claim_interfaces(&handle, &kernel_state); + + if read_ep.setting != 0 { + handle.set_alternate_setting(read_ep.iface, read_ep.setting).map_err(|e| { + error!("Set alt setting error: {}", e); + }).ok(); + } + let closed = Arc::new(AtomicBool::new(false)); const LEN: usize = 1024; @@ -184,15 +203,20 @@ impl DeviceInner { TransferCommand::Resubmit }); - let read = read_transfer.submit().ok().unwrap(); + let read = read_transfer.submit() + .map_err(|e| { + Self::close_inner(&handle, &kernel_state); + anyhow!("Failed to set up read thread: {e}") + })?; - DeviceInner { + Ok(DeviceInner { name, handle, closed, write_ep, - read - } + read, + kernel_state + }) } fn send(&self, bytes: &[u8]) -> Result<()> { @@ -218,6 +242,65 @@ impl DeviceInner { fn close(&mut self) { self.closed.store(true, Ordering::Relaxed); self.read.cancel().ok(); + + Self::close_inner(&self.handle, &self.kernel_state); + } + + fn close_inner(handle: &DeviceHandle, state: &DevOpenState) { + Self::release_interfaces(handle, state); + Self::attach_kernel_driver(handle, state); + } + + fn detach_kernel_driver(handle: &DeviceHandle) -> Result { + let dev = handle.device(); + let config = handle.active_configuration()?; + let desc = dev.active_config_descriptor()?; + let attach = desc.interfaces() + .map(|iface| { + let num = iface.number(); + let kernel_driver_attached = handle.kernel_driver_active(num) + .ok().unwrap_or(false); + + debug!("Kernel driver detach (iface={}): attached={}", num, kernel_driver_attached); + if kernel_driver_attached { + handle.detach_kernel_driver(num).map_err(|e| { + error!("Failed to detach kernel driver (iface={}): {}", num, e); + }).ok(); + } + + (num, kernel_driver_attached) + }) + .collect::>(); + Ok(DevOpenState { config, attach }) + } + + fn attach_kernel_driver(handle: &DeviceHandle, state: &DevOpenState) { + for (num, kernel_driver_attached) in state.attach.iter() { + debug!("Kernel driver attach (iface={}): attached={}", num, kernel_driver_attached); + if *kernel_driver_attached { + handle.attach_kernel_driver(*num).map_err(|e| { + error!("Failed to attach kernel driver (iface={}): {}", num, e); + }).ok(); + } + } + } + + fn claim_interfaces(handle: &DeviceHandle, state: &DevOpenState) { + for (num, _) in state.attach.iter() { + debug!("Claiming interface (iface={})", num); + handle.claim_interface(*num).map_err(|e| { + error!("Failed to claim interface (iface{}): {}", num, e); + }).ok(); + } + } + + fn release_interfaces(handle: &DeviceHandle, state: &DevOpenState) { + for (num, _) in state.attach.iter() { + debug!("Releasing interface (iface={})", num); + handle.release_interface(*num).map_err(|e| { + error!("Failed to release interface (iface{}): {}", num, e); + }).ok(); + } } fn find_message(read_ptr: &mut [u8]) -> &[u8] { diff --git a/usb/src/endpoint.rs b/usb/src/endpoint.rs index 5b70793..4217bff 100644 --- a/usb/src/endpoint.rs +++ b/usb/src/endpoint.rs @@ -1,9 +1,7 @@ // from rusb example code -use anyhow::*; use core::result::Result::Ok; -use log::error; -use rusb::{Device, DeviceDescriptor, DeviceHandle, Direction, TransferType, UsbContext}; +use rusb::{Device, DeviceDescriptor, Direction, TransferType, UsbContext}; #[derive(Debug, Clone)] pub struct Endpoint { @@ -14,27 +12,6 @@ pub struct Endpoint { pub transfer_type: TransferType } -pub fn configure_endpoint( - handle: &DeviceHandle, - endpoint: &Endpoint, -) -> Result<()> { - handle.claim_interface(endpoint.iface).map_err(|e| { - error!("Claim interface 1 error: {}", e); - }).ok(); - handle.set_active_configuration(endpoint.config).map_err(|e| { - error!("Set active config error: {}", e); - }).ok(); - handle.claim_interface(endpoint.iface).map_err(|e| { - error!("Claim interface 2 error: {}", e); - }).ok(); - handle.set_alternate_setting(endpoint.iface, endpoint.setting).map_err(|e| { - error!("Set alt setting error: {}", e); - }).ok(); - Ok(()) -} - - - pub fn find_endpoint( device: &Device, device_desc: &DeviceDescriptor, diff --git a/usb/src/usb.rs b/usb/src/usb.rs index cfd8319..2e6b8e3 100644 --- a/usb/src/usb.rs +++ b/usb/src/usb.rs @@ -48,6 +48,7 @@ impl Usb { /// Vendor/Product ID. pub fn open(&self, vid: u16, pid: u16) -> Result { info!("Opening {:04X}:{:04X}", vid, pid); + // For now skip the full reset... /* let hdl = libusb::reset( self.ctx.open_device_with_vid_pid(vid, pid) @@ -237,8 +238,18 @@ impl Transfer cb(None); TransferCommand::Drop } - (TransferStatus::Error(_), _) | (TransferStatus::Cancel, _) => { - // Transfer submit failed or cancelled without callback + (TransferStatus::Error(e), _) => { + // Transfer submit failed + let dir = match inner.endpoint & LIBUSB_ENDPOINT_DIR_MASK { + LIBUSB_ENDPOINT_OUT => "write", + LIBUSB_ENDPOINT_IN => "read", + _ => unreachable!(), + }; + error!("Failed to submit {} transfer: {}", dir, e); + TransferCommand::Drop + } + (TransferStatus::Cancel, _) => { + // Transfer cancelled without callback TransferCommand::Drop } (TransferStatus::Ok, Some(cb)) => { @@ -323,8 +334,12 @@ pub mod libusb { ($x:expr) => { // SAFETY: C API call match unsafe { $x } { - LIBUSB_SUCCESS => Ok(()), - e => Err($crate::usb::libusb::from_libusb(e)), + rusb::constants::LIBUSB_SUCCESS => { + Ok(()) + }, + e => { + Err($crate::usb::libusb::from_libusb(e)) + }, } }; }