host: fix enumerate racing #3080
Conversation
- if a previous enumeration failed _ctrl_xfer status could stuck, it needs to be cleared before next attempt. - after _dev0.enumerating is reset in hcd_event_handler(), if an attach event arrived before _ctrl_xfer clean up in remove event, a racing condition will happen. Signed-off-by: HiFiPhile <[email protected]>
Signed-off-by: HiFiPhile <[email protected]>
Signed-off-by: HiFiPhile <[email protected]>
src/host/usbh.c
Outdated
| if ((event.rhport == _dev0.rhport) && (event.connection.hub_addr == _dev0.hub_addr) && | ||
| (event.connection.hub_port == _dev0.hub_port)) { | ||
| _dev0.enumerating = 0; | ||
| hcd_device_close(_dev0.rhport, _dev0.hub_addr); |
There was a problem hiding this comment.
thank yo, we should include _dev0.enumerating == 1 since other value has obsolete (for new device) after enumerated successfully. The isr blindly set the flag, but we need to check since we do more than that here. Also hcd_device_close(_dev0.rhport, 0) instead of using hub_addr
I have rebased and fix this, also move this snippet into process_removing_device() since there is a TODO for dev0 there.
There was a problem hiding this comment.
I haven't tested, is hcd_device_close(_dev0.rhport, 0) works with device under a hub ?
I need to read more how hub is handled ;)
There was a problem hiding this comment.
It should, the 2nd argument is the device addres that is removed. Otherwise, it will try to remove the hub that attached the rhport.
There was a problem hiding this comment.
I think there is a loophole with early return. If set address is done _dev0.enumerating is still true but _usbh_devices also needs to be cleaned.
There was a problem hiding this comment.
ah you are right, putting enumerating in dev0 is confusing. Let me revise and fix this.
replace dev0.enumerating by enumerating_daddr for better clean up on unplugging while enumerating move controller_id & enumerating_daddr into _usbh_data struct
There was a problem hiding this comment.
made an improvement from just a flag enumerating to actual enumerating_daddr to make it easier to clean up when device unplugged while enumerating not completed yet. @HiFiPhile please let me know if this make sense to you.
After this merged, I will make PR similar to the #3075 for ignoring attach/detach on the same rhport/hub/port while debouncing to usbh to make it generic for all host controller. This seems to work rathe well.
Signed-off-by: HiFiPhile <[email protected]>
|
It works pretty well in my side, tried some quick action and no assert happened :) |
Describe the PR
Move
_dev0.enumeratinginto event queueMove decouncing delay before USB reset.
Chirp K for HS detection happens within 7ms of the start of reset, to make speed detection reliable the debouncing delay need to be placed before reset.
Windows 10 does this: