Index: uspace/drv/bus/usb/usbhub/port.c =================================================================== --- uspace/drv/bus/usb/usbhub/port.c (revision 8ad2b0a99f1326c29aad2873ef275fc1d343f087) +++ uspace/drv/bus/usb/usbhub/port.c (revision 94f8c363ee71e2fb5993b37d877a1cb451f25950) @@ -58,37 +58,13 @@ assert(port); memset(port, 0, sizeof(*port)); - fibril_mutex_initialize(&port->guard); - fibril_condvar_initialize(&port->state_cv); port->hub = hub; port->port_number = port_number; -} - -/** - * Utility method to change current port state and notify waiters. - */ -static void port_change_state(usb_hub_port_t *port, port_state_t state) -{ - assert(fibril_mutex_is_locked(&port->guard)); -#define S(STATE) [PORT_##STATE] = #STATE - static const char *state_names [] = { - S(DISABLED), S(CONNECTED), S(ERROR), S(IN_RESET), S(ENABLED), - }; -#undef S - port_log(debug, port, "%s ->%s", state_names[port->state], state_names[state]); - port->state = state; - fibril_condvar_broadcast(&port->state_cv); -} - -/** - * Utility method to wait for a particular state. - * - * @warning Some states might not be reached because of an external error - * condition (PORT_ENABLED). - */ -static void port_wait_state(usb_hub_port_t *port, port_state_t state) -{ - assert(fibril_mutex_is_locked(&port->guard)); - while (port->state != state) - fibril_condvar_wait(&port->state_cv, &port->guard); + usb_port_init(&port->base); +} + +static inline usb_hub_port_t *get_hub_port(usb_port_t *port) +{ + assert(port); + return (usb_hub_port_t *) port; } @@ -96,223 +72,80 @@ * Inform the HC that the device on port is gone. */ -static int usb_hub_port_device_gone(usb_hub_port_t *port) -{ - assert(port); - assert(fibril_mutex_is_locked(&port->guard)); - assert(port->state == PORT_ENABLED); +static void remove_device(usb_port_t *port_base) +{ + usb_hub_port_t *port = get_hub_port(port_base); async_exch_t *exch = usb_device_bus_exchange_begin(port->hub->usb_device); - if (!exch) + if (!exch) { + port_log(error, port, "Cannot remove the device, failed creating exchange."); + return; + } + + const int err = usbhc_device_remove(exch, port->port_number); + if (err) + port_log(error, port, "Failed to remove device: %s", str_error(err)); + + usb_device_bus_exchange_end(exch); +} + +/** + * Routine for adding a new device. + * + * Separate fibril is needed because the operation blocks on waiting for + * requesting default address and resetting port, and we must not block the + * control pipe. + */ +static int enumerate_device(usb_port_t *port_base) +{ + int err; + usb_hub_port_t *port = get_hub_port(port_base); + + port_log(debug, port, "Setting up new device."); + + async_exch_t *exch = usb_device_bus_exchange_begin(port->hub->usb_device); + if (!exch) { + port_log(error, port, "Failed to create exchange."); return ENOMEM; - const int rc = usbhc_device_remove(exch, port->port_number); - usb_device_bus_exchange_end(exch); - return rc; -} - -/** - * Teardown a device on port, no matter which state it was in. - */ -static void port_make_disabled(usb_hub_port_t *port) -{ - assert(fibril_mutex_is_locked(&port->guard)); - - port_log(debug, port, "Making device offline."); - - switch (port->state) { - case PORT_ENABLED: - port_log(debug, port, "Enabled ->"); - if (usb_hub_port_device_gone(port)) - port_log(error, port, "Failed to remove the device node from HC. Continuing anyway."); - port_change_state(port, PORT_DISABLED); - break; - - case PORT_CONNECTED: - port_log(debug, port, "Connected ->"); - /* fallthrough */ - case PORT_IN_RESET: - port_log(debug, port, "In reset ->"); - port_change_state(port, PORT_ERROR); - /* fallthrough */ - case PORT_ERROR: - port_log(debug, port, "Error ->"); - port_wait_state(port, PORT_DISABLED); - /* fallthrough */ - case PORT_DISABLED: - port_log(debug, port, "Disabled."); - break; - } - - assert(port->state == PORT_DISABLED); -} - -/** - * Finalize a port. Make sure no fibril is managing its state, - * and that the HC is aware the device is no longer there. - */ -void usb_hub_port_fini(usb_hub_port_t *port) -{ - assert(port); - - fibril_mutex_lock(&port->guard); - switch (port->state) { - case PORT_ENABLED: - /* - * We shall inform the HC that the device is gone. - * However, we can't wait for it, because if the device is hub, - * it would have to use the same IPC handling fibril as we do. - * But we cannot even defer it to another fibril, because then - * the HC would assume our driver didn't cleanup properly, and - * will remove those devices by himself. - * - * So the solutions seems to behave like a bad driver and leave - * the work for HC. - */ - port_change_state(port, PORT_DISABLED); - break; - - case PORT_CONNECTED: - case PORT_IN_RESET: - port_change_state(port, PORT_ERROR); - /* fallthrough */ - case PORT_ERROR: - port_wait_state(port, PORT_DISABLED); - /* fallthrough */ - case PORT_DISABLED: - break; - } - port_log(debug, port, "Finalized."); - fibril_mutex_unlock(&port->guard); -} - -static int port_reset_sync(usb_hub_port_t *port) -{ - assert(fibril_mutex_is_locked(&port->guard)); - assert(port->state == PORT_CONNECTED); - - port_change_state(port, PORT_IN_RESET); - port_log(debug2, port, "Issuing reset."); + } + + /* Reserve default address */ + err = usb_hub_reserve_default_address(port->hub, exch, &port->base); + if (err != EOK) { + port_log(error, port, "Failed to reserve default address: %s", str_error(err)); + goto out_exch; + } + + /* Reservation of default address could have blocked */ + if (port->base.state != PORT_CONNECTING) + goto out_address; + + port_log(debug, port, "Got default address. Resetting port."); int rc = usb_hub_set_port_feature(port->hub, port->port_number, USB_HUB_FEATURE_PORT_RESET); if (rc != EOK) { port_log(warning, port, "Port reset request failed: %s", str_error(rc)); - return rc; - } - - fibril_condvar_wait_timeout(&port->state_cv, &port->guard, 2000000); - return port->state == PORT_ENABLED ? EOK : ESTALL; -} - -/** - * Routine for adding a new device. - * - * Separate fibril is needed because the operation blocks on waiting for - * requesting default address and resetting port, and we must not block the - * control pipe. - */ -static void setup_device(usb_hub_port_t *port) -{ - int err; - - fibril_mutex_lock(&port->guard); - - if (port->state == PORT_ERROR) { - /* - * The device was removed faster than this fibril acquired the - * mutex. - */ - port_change_state(port, PORT_DISABLED); - goto out; - } - - if (port->state != PORT_CONNECTED) { - /* - * Another fibril already took care of the device. - * This may happen for example when the connection is unstable - * and a sequence of connect, disconnect and connect come - * faster the first fibril manages to request the default - * address. - */ - goto out; - } - - port_log(debug, port, "Setting up new device."); - - async_exch_t *exch = usb_device_bus_exchange_begin(port->hub->usb_device); - if (!exch) { - port_log(error, port, "Failed to create exchange."); - goto out; - } - - /* Reserve default address */ - err = usb_hub_reserve_default_address(port->hub, exch, &port->guard); - if (err != EOK) { - port_log(error, port, "Failed to reserve default address: %s", str_error(err)); - port_change_state(port, PORT_DISABLED); - goto out_exch; - } - - /* Reservation of default address could have blocked */ - if (port->state != PORT_CONNECTED) { - assert(port->state == PORT_ERROR); - port_change_state(port, PORT_DISABLED); - goto out_exch; - } - - port_log(debug, port, "Got default address. Resetting port."); - - if ((err = port_reset_sync(port))) { - port_log(error, port, "Failed to reset port."); - port_change_state(port, PORT_DISABLED); - goto out_address; - } - - assert(port->state == PORT_ENABLED); + goto out_address; + } + + if ((err = usb_port_wait_for_enabled(&port->base))) { + port_log(error, port, "Failed to reset port: %s", str_error(err)); + goto out_address; + } port_log(debug, port, "Port reset, enumerating device."); - if ((err = usbhc_device_enumerate(exch, port->port_number, port->speed))) { + if ((err = usbhc_device_enumerate(exch, port->port_number, port->base.speed))) { port_log(error, port, "Failed to enumerate device: %s", str_error(err)); - port_change_state(port, PORT_DISABLED); - goto out_port; + /* Disable the port */ + usb_hub_clear_port_feature(port->hub, port->port_number, USB_HUB_FEATURE_PORT_ENABLE); + goto out_address; } port_log(debug, port, "Device enumerated"); -out_port: - if (port->state != PORT_ENABLED) - usb_hub_clear_port_feature(port->hub, port->port_number, USB_HUB_FEATURE_C_PORT_ENABLE); out_address: usb_hub_release_default_address(port->hub, exch); out_exch: usb_device_bus_exchange_end(exch); -out: - assert(port->state == PORT_ENABLED || port->state == PORT_DISABLED); - fibril_mutex_unlock(&port->guard); -} - -static int setup_device_worker(void *arg) -{ - setup_device(arg); - return EOK; -} - -/** Start device adding when connection change is detected. - * - * This fires a new fibril to complete the device addition. - * - * @param hub Hub where the change occured. - * @param port Port index (starting at 1). - * @param speed Speed of the device. - * @return Error code. - */ -static int create_setup_device_fibril(usb_hub_port_t *port) -{ - assert(port); - - fid_t fibril = fibril_create(setup_device_worker, port); - if (!fibril) - return ENOMEM; - - fibril_add_ready(fibril); - return EOK; + return err; } @@ -323,13 +156,7 @@ if (connected) { - if (port->state == PORT_ENABLED) - port_log(warning, port, "Connection detected on port that is currently enabled. Resetting."); - - port_make_disabled(port); - port_change_state(port, PORT_CONNECTED); - port->speed = usb_port_speed(status); - create_setup_device_fibril(port); + usb_port_connected(&port->base, &enumerate_device); } else { - port_make_disabled(port); + usb_port_disabled(&port->base, &remove_device); } } @@ -341,5 +168,5 @@ port_log(warning, port, "Port unexpectedly changed to enabled."); } else { - port_make_disabled(port); + usb_port_disabled(&port->base, &remove_device); } } @@ -360,5 +187,5 @@ * back on when the over-current condition is gone */ - port_make_disabled(port); + usb_port_disabled(&port->base, &remove_device); if (!overcurrent) { @@ -373,9 +200,8 @@ const bool enabled = !!(status & USB_HUB_PORT_STATUS_ENABLED); - /* Check if someone is waiting for the result */ - if (port->state != PORT_IN_RESET) - return; - - port_change_state(port, enabled ? PORT_ENABLED : PORT_ERROR); + if (enabled) + usb_port_enabled(&port->base, usb_port_speed(status)); + else + usb_port_disabled(&port->base, &remove_device); } @@ -398,8 +224,7 @@ * @param hub hub representation */ -void usb_hub_port_process_interrupt(usb_hub_port_t *port, usb_hub_dev_t *hub) +void usb_hub_port_process_interrupt(usb_hub_port_t *port) { assert(port); - assert(hub); port_log(debug2, port, "Interrupt."); @@ -411,6 +236,4 @@ } - fibril_mutex_lock(&port->guard); - for (uint32_t feature = 0; feature < sizeof(usb_port_status_t) * 8; ++feature) { uint32_t mask = 1 << feature; @@ -429,6 +252,4 @@ } - fibril_mutex_unlock(&port->guard); - port_log(debug2, port, "Port status after handling: %#08" PRIx32, status); } Index: uspace/drv/bus/usb/usbhub/port.h =================================================================== --- uspace/drv/bus/usb/usbhub/port.h (revision 8ad2b0a99f1326c29aad2873ef275fc1d343f087) +++ uspace/drv/bus/usb/usbhub/port.h (revision 94f8c363ee71e2fb5993b37d877a1cb451f25950) @@ -33,5 +33,5 @@ */ /** @file - * Hub port state machine. + * Hub port handling. */ @@ -41,35 +41,20 @@ #include #include +#include typedef struct usb_hub_dev usb_hub_dev_t; -typedef enum { - PORT_DISABLED, /* No device connected. */ - PORT_CONNECTED, /* A device connected, not yet initialized. */ - PORT_IN_RESET, /* An initial port reset in progress. */ - PORT_ENABLED, /* Port reset complete, port enabled. Device announced to the HC. */ - PORT_ERROR, /* An error occured. There is still a fibril that needs to know it. */ -} port_state_t; - /** Information about single port on a hub. */ typedef struct { + usb_port_t base; /* Parenting hub */ usb_hub_dev_t *hub; - /** Guarding all fields */ - fibril_mutex_t guard; - /** Current state of the port */ - port_state_t state; - /** A speed of the device connected (if any). Valid unless state == PORT_DISABLED. */ - usb_speed_t speed; /** Port number as reported in descriptors. */ unsigned int port_number; - /** CV for waiting to port reset completion. */ - fibril_condvar_t state_cv; } usb_hub_port_t; void usb_hub_port_init(usb_hub_port_t *, usb_hub_dev_t *, unsigned int); -void usb_hub_port_fini(usb_hub_port_t *); -void usb_hub_port_process_interrupt(usb_hub_port_t *port, usb_hub_dev_t *hub); +void usb_hub_port_process_interrupt(usb_hub_port_t *port); #endif Index: uspace/drv/bus/usb/usbhub/usbhub.c =================================================================== --- uspace/drv/bus/usb/usbhub/usbhub.c (revision 8ad2b0a99f1326c29aad2873ef275fc1d343f087) +++ uspace/drv/bus/usb/usbhub/usbhub.c (revision 94f8c363ee71e2fb5993b37d877a1cb451f25950) @@ -200,5 +200,5 @@ for (size_t port = 0; port < hub->port_count; ++port) { - usb_hub_port_fini(&hub->ports[port]); + usb_port_fini(&hub->ports[port].base); } free(hub->ports); @@ -291,5 +291,5 @@ const bool change = (change_bitmap[bit / 8] >> (bit % 8)) & 1; if (change) { - usb_hub_port_process_interrupt(&hub->ports[port], hub); + usb_hub_port_process_interrupt(&hub->ports[port]); } } @@ -605,4 +605,6 @@ } +static FIBRIL_CONDVAR_INITIALIZE(global_hub_default_address_cv); + /** * Reserve a default address for a port across all other devices connected to @@ -611,10 +613,10 @@ * is connected with already attached devices. */ -int usb_hub_reserve_default_address(usb_hub_dev_t *hub, async_exch_t *exch, fibril_mutex_t *guard) +int usb_hub_reserve_default_address(usb_hub_dev_t *hub, async_exch_t *exch, usb_port_t *port) { assert(hub); assert(exch); - assert(guard); - assert(fibril_mutex_is_locked(guard)); + assert(port); + assert(fibril_mutex_is_locked(&port->guard)); fibril_mutex_lock(&hub->default_address_guard); @@ -624,12 +626,11 @@ int err; while ((err = usbhc_reserve_default_address(exch)) == EAGAIN) { - fibril_mutex_unlock(guard); - async_usleep(500000); - fibril_mutex_lock(guard); + // We ignore the return value here, as we cannot give up now. + usb_port_condvar_wait_timeout(port, &global_hub_default_address_cv, 500000); } return err; } else { /* Drop the port guard, we're going to wait */ - fibril_mutex_unlock(guard); + fibril_mutex_unlock(&port->guard); /* Wait for a signal */ @@ -638,5 +639,5 @@ /* Remember ABBA, first drop the hub guard */ fibril_mutex_unlock(&hub->default_address_guard); - fibril_mutex_lock(guard); + fibril_mutex_lock(&port->guard); return EOK; } @@ -655,4 +656,6 @@ // from requesting the address before we release ret = usbhc_release_default_address(exch); + // This is optimistic optimization - it may wake one hub from polling sleep + fibril_condvar_signal(&global_hub_default_address_cv); } else { fibril_condvar_signal(&hub->default_address_cv); Index: uspace/drv/bus/usb/usbhub/usbhub.h =================================================================== --- uspace/drv/bus/usb/usbhub/usbhub.h (revision 8ad2b0a99f1326c29aad2873ef275fc1d343f087) +++ uspace/drv/bus/usb/usbhub/usbhub.h (revision 94f8c363ee71e2fb5993b37d877a1cb451f25950) @@ -89,5 +89,5 @@ bool hub_port_changes_callback(usb_device_t *, uint8_t *, size_t, void *); -int usb_hub_reserve_default_address(usb_hub_dev_t *, async_exch_t *, fibril_mutex_t *); +int usb_hub_reserve_default_address(usb_hub_dev_t *, async_exch_t *, usb_port_t *); int usb_hub_release_default_address(usb_hub_dev_t *, async_exch_t *);