From f42ba772c8050da266b69504efff8e16f2fda8c2 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Tue, 29 Sep 2020 10:16:14 +0100 Subject: [usb] Reset control endpoints immediately after failure The current error handling mechanism defers the endpoint reset until the next use of the endpoint, on the basis that errors are detected during completions and completion handling should not recursively call usb_poll(). In the case of usb_control(), we are already at the level that calls usb_poll() and can therefore safely perform the endpoint reset immediately. This has no impact on functionality, but does make debugging traces easier to read since the reset will appear immediately after the causative error. Signed-off-by: Michael Brown --- src/drivers/bus/usb.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src/drivers/bus/usb.c') diff --git a/src/drivers/bus/usb.c b/src/drivers/bus/usb.c index a7b687528..74f11ecb8 100644 --- a/src/drivers/bus/usb.c +++ b/src/drivers/bus/usb.c @@ -818,6 +818,7 @@ int usb_control ( struct usb_device *usb, unsigned int request, "failed: %s\n", usb->name, request, value, index, strerror ( rc ) ); free_iob ( cmplt ); + usb_endpoint_reset ( ep ); return rc; } -- cgit v1.2.3-55-g7522 From 8eb19a178a5d6ca89ca2b8200dae0f7cec15ae2c Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Tue, 29 Sep 2020 14:39:54 +0100 Subject: [usb] Show debug message on device removal Signed-off-by: Michael Brown --- src/drivers/bus/usb.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'src/drivers/bus/usb.c') diff --git a/src/drivers/bus/usb.c b/src/drivers/bus/usb.c index 74f11ecb8..fca47fd36 100644 --- a/src/drivers/bus/usb.c +++ b/src/drivers/bus/usb.c @@ -1637,6 +1637,11 @@ static void unregister_usb ( struct usb_device *usb ) { struct io_buffer *iobuf; struct io_buffer *tmp; + DBGC ( usb, "USB %s addr %d %04x:%04x class %d:%d:%d removed\n", + usb->name, usb->address, le16_to_cpu ( usb->device.vendor ), + le16_to_cpu ( usb->device.product ), usb->device.class.class, + usb->device.class.subclass, usb->device.class.protocol ); + /* Sanity checks */ assert ( port->usb == usb ); -- cgit v1.2.3-55-g7522 From 7c6fdf57eadb382fc86719daf79c7afa78ace530 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Thu, 1 Oct 2020 18:26:37 +0100 Subject: [usb] Avoid integer underflow on malformed string descriptors Signed-off-by: Michael Brown --- src/drivers/bus/usb.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) (limited to 'src/drivers/bus/usb.c') diff --git a/src/drivers/bus/usb.c b/src/drivers/bus/usb.c index fca47fd36..14eabb6b4 100644 --- a/src/drivers/bus/usb.c +++ b/src/drivers/bus/usb.c @@ -913,9 +913,15 @@ int usb_get_string_descriptor ( struct usb_device *usb, unsigned int index, sizeof ( *desc ) ) ) != 0 ) goto err_get_descriptor; - /* Copy to buffer */ + /* Calculate string length */ + if ( desc->header.len < sizeof ( desc->header ) ) { + rc = -EINVAL; + goto err_len; + } actual = ( ( desc->header.len - sizeof ( desc->header ) ) / sizeof ( desc->character[0] ) ); + + /* Copy to buffer */ for ( i = 0 ; ( ( i < actual ) && ( i < max ) ) ; i++ ) buf[i] = le16_to_cpu ( desc->character[i] ); if ( len ) @@ -926,6 +932,7 @@ int usb_get_string_descriptor ( struct usb_device *usb, unsigned int index, return actual; + err_len: err_get_descriptor: free ( desc ); err_alloc: -- cgit v1.2.3-55-g7522 From ebf01660810c4ff923754323049d49d37321fd49 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Mon, 12 Oct 2020 15:21:25 +0100 Subject: [usb] Allow device halt to be cleared independently of host controller Closing and reopening a USB endpoint will clear any halt status recorded by the host controller, but may leave the endpoint halted at the device. This will cause the first packet submitted to the reopened endpoint to be lost, before the automatic stall recovery mechanism detects the halt and resets the endpoint. This is relatively harmless for USB network or HID devices, since the wire protocols will recover gracefully from dropped packets. Some protocols (e.g. for USB mass storage devices) assume zero packet loss and so would be adversely affected. Fix by allowing any device endpoint halt status to be cleared on a freshly opened endpoint. Signed-off-by: Michael Brown --- src/drivers/bus/usb.c | 45 +++++++++++++++++++++++++++++++-------------- src/include/ipxe/usb.h | 1 + 2 files changed, 32 insertions(+), 14 deletions(-) (limited to 'src/drivers/bus/usb.c') diff --git a/src/drivers/bus/usb.c b/src/drivers/bus/usb.c index 14eabb6b4..88df9c64a 100644 --- a/src/drivers/bus/usb.c +++ b/src/drivers/bus/usb.c @@ -362,6 +362,35 @@ static int usb_endpoint_clear_tt ( struct usb_endpoint *ep ) { return 0; } +/** + * Clear endpoint halt (if applicable) + * + * @v ep USB endpoint + * @ret rc Return status code + */ +int usb_endpoint_clear_halt ( struct usb_endpoint *ep ) { + struct usb_device *usb = ep->usb; + unsigned int type; + int rc; + + /* Clear transaction translator, if applicable */ + if ( ( rc = usb_endpoint_clear_tt ( ep ) ) != 0 ) + return rc; + + /* Clear endpoint halt (if applicable) */ + type = ( ep->attributes & USB_ENDPOINT_ATTR_TYPE_MASK ); + if ( ( type != USB_ENDPOINT_ATTR_CONTROL ) && + ( ( rc = usb_clear_feature ( usb, USB_RECIP_ENDPOINT, + USB_ENDPOINT_HALT, + ep->address ) ) != 0 ) ) { + DBGC ( usb, "USB %s %s could not clear endpoint halt: %s\n", + usb->name, usb_endpoint_name ( ep ), strerror ( rc ) ); + return rc; + } + + return 0; +} + /** * Close USB endpoint * @@ -399,27 +428,15 @@ void usb_endpoint_close ( struct usb_endpoint *ep ) { */ static int usb_endpoint_reset ( struct usb_endpoint *ep ) { struct usb_device *usb = ep->usb; - unsigned int type; int rc; /* Sanity check */ assert ( ! list_empty ( &ep->halted ) ); - /* Clear transaction translator, if applicable */ - if ( ( rc = usb_endpoint_clear_tt ( ep ) ) != 0 ) + /* Clear device halt, if applicable */ + if ( ( rc = usb_endpoint_clear_halt ( ep ) ) != 0 ) return rc; - /* Clear endpoint halt, if applicable */ - type = ( ep->attributes & USB_ENDPOINT_ATTR_TYPE_MASK ); - if ( ( type != USB_ENDPOINT_ATTR_CONTROL ) && - ( ( rc = usb_clear_feature ( usb, USB_RECIP_ENDPOINT, - USB_ENDPOINT_HALT, - ep->address ) ) != 0 ) ) { - DBGC ( usb, "USB %s %s could not clear endpoint halt: %s\n", - usb->name, usb_endpoint_name ( ep ), strerror ( rc ) ); - return rc; - } - /* Reset endpoint */ if ( ( rc = ep->host->reset ( ep ) ) != 0 ) { DBGC ( usb, "USB %s %s could not reset: %s\n", diff --git a/src/include/ipxe/usb.h b/src/include/ipxe/usb.h index 68289d26d..a05ac613c 100644 --- a/src/include/ipxe/usb.h +++ b/src/include/ipxe/usb.h @@ -580,6 +580,7 @@ usb_endpoint_described ( struct usb_endpoint *ep, struct usb_interface_descriptor *interface, unsigned int type, unsigned int index ); extern int usb_endpoint_open ( struct usb_endpoint *ep ); +extern int usb_endpoint_clear_halt ( struct usb_endpoint *ep ); extern void usb_endpoint_close ( struct usb_endpoint *ep ); extern int usb_message ( struct usb_endpoint *ep, unsigned int request, unsigned int value, unsigned int index, -- cgit v1.2.3-55-g7522 From e30c26d01c7493a7e02e77fc80c490fc85612d5a Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Mon, 12 Oct 2020 15:28:26 +0100 Subject: [usb] Allow endpoints to be refilled to a specified upper limit For USB mass storage devices, we do not want to submit more bulk IN packets than are required for the inbound data, since this will waste memory. Allow an upper limit to be specified on each refill attempt. The endpoint will be refilled to the lower of this limit or the limit specified by usb_refill_init(). Signed-off-by: Michael Brown --- src/drivers/bus/usb.c | 19 ++++++++++++++++--- src/include/ipxe/usb.h | 1 + 2 files changed, 17 insertions(+), 3 deletions(-) (limited to 'src/drivers/bus/usb.c') diff --git a/src/drivers/bus/usb.c b/src/drivers/bus/usb.c index 88df9c64a..70a86c913 100644 --- a/src/drivers/bus/usb.c +++ b/src/drivers/bus/usb.c @@ -651,12 +651,13 @@ int usb_prefill ( struct usb_endpoint *ep ) { } /** - * Refill endpoint + * Refill endpoint up to specified limit * * @v ep USB endpoint + * @v max Fill limit * @ret rc Return status code */ -int usb_refill ( struct usb_endpoint *ep ) { +int usb_refill_limit ( struct usb_endpoint *ep, unsigned int max ) { struct io_buffer *iobuf; size_t reserve = ep->reserve; size_t len = ( ep->len ? ep->len : ep->mtu ); @@ -667,7 +668,9 @@ int usb_refill ( struct usb_endpoint *ep ) { assert ( ep->max > 0 ); /* Refill endpoint */ - while ( ep->fill < ep->max ) { + if ( max > ep->max ) + max = ep->max; + while ( ep->fill < max ) { /* Get or allocate buffer */ if ( list_empty ( &ep->recycled ) ) { @@ -698,6 +701,16 @@ int usb_refill ( struct usb_endpoint *ep ) { return 0; } +/** + * Refill endpoint + * + * @v ep USB endpoint + * @ret rc Return status code + */ +int usb_refill ( struct usb_endpoint *ep ) { + return usb_refill_limit ( ep, ep->max ); +} + /** * Discard endpoint recycled buffer list * diff --git a/src/include/ipxe/usb.h b/src/include/ipxe/usb.h index a05ac613c..70d6daf33 100644 --- a/src/include/ipxe/usb.h +++ b/src/include/ipxe/usb.h @@ -621,6 +621,7 @@ usb_recycle ( struct usb_endpoint *ep, struct io_buffer *iobuf ) { } extern int usb_prefill ( struct usb_endpoint *ep ); +extern int usb_refill_limit ( struct usb_endpoint *ep, unsigned int max ); extern int usb_refill ( struct usb_endpoint *ep ); extern void usb_flush ( struct usb_endpoint *ep ); -- cgit v1.2.3-55-g7522 From 16873703ddea2d64fac1809967ef5198b8764baa Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 30 Oct 2020 13:51:30 +0000 Subject: [efi] Avoid dragging in USB subsystem via efi_usb_path() Commit 87e39a9c9 ("[efi] Split efi_usb_path() out to a separate function") unintentionally introduced an undefined symbol reference from efi_path.o to usb_depth(), causing the USB subsystem to become a dependency of all EFI builds. Fix by converting usb_depth() to a static inline function. Reported-by: Pico Mitchell Signed-off-by: Michael Brown --- src/drivers/bus/usb.c | 17 ----------------- src/include/ipxe/usb.h | 18 +++++++++++++++++- 2 files changed, 17 insertions(+), 18 deletions(-) (limited to 'src/drivers/bus/usb.c') diff --git a/src/drivers/bus/usb.c b/src/drivers/bus/usb.c index 70a86c913..428ae26c1 100644 --- a/src/drivers/bus/usb.c +++ b/src/drivers/bus/usb.c @@ -2277,23 +2277,6 @@ unsigned int usb_route_string ( struct usb_device *usb ) { return route; } -/** - * Get USB depth - * - * @v usb USB device - * @ret depth Hub depth - */ -unsigned int usb_depth ( struct usb_device *usb ) { - struct usb_device *parent; - unsigned int depth; - - /* Navigate up to root hub, constructing depth as we go */ - for ( depth = 0 ; ( parent = usb->port->hub->usb ) ; usb = parent ) - depth++; - - return depth; -} - /** * Get USB root hub port * diff --git a/src/include/ipxe/usb.h b/src/include/ipxe/usb.h index f41f4c355..911247ede 100644 --- a/src/include/ipxe/usb.h +++ b/src/include/ipxe/usb.h @@ -1239,6 +1239,23 @@ usb_set_interface ( struct usb_device *usb, unsigned int interface, NULL, 0 ); } +/** + * Get USB depth + * + * @v usb USB device + * @ret depth Hub depth + */ +static inline unsigned int usb_depth ( struct usb_device *usb ) { + struct usb_device *parent; + unsigned int depth; + + /* Navigate up to root hub, constructing depth as we go */ + for ( depth = 0 ; ( parent = usb->port->hub->usb ) ; usb = parent ) + depth++; + + return depth; +} + extern struct list_head usb_buses; extern struct usb_interface_descriptor * @@ -1274,7 +1291,6 @@ extern struct usb_bus * find_usb_bus_by_location ( unsigned int bus_type, extern int usb_alloc_address ( struct usb_bus *bus ); extern void usb_free_address ( struct usb_bus *bus, unsigned int address ); extern unsigned int usb_route_string ( struct usb_device *usb ); -extern unsigned int usb_depth ( struct usb_device *usb ); extern struct usb_port * usb_root_hub_port ( struct usb_device *usb ); extern struct usb_port * usb_transaction_translator ( struct usb_device *usb ); -- cgit v1.2.3-55-g7522