Hey Paul, On Tue, Apr 16, 2013 at 07:39:22PM +0000, Paul Zimmerman wrote: > > From: Matthijs Kooijman [mailto:matthijs@xxxxxxxx] > > Sent: Tuesday, April 16, 2013 12:03 PM > > > > > No, the interrupt is strictly a host interrupt. But due to the > > > design of the hardware, there's a race condition: Say the user > > > unplugs the A connector from the host. The disconnect interrupt gets > > > triggered, and the driver starts to process it. But unplugging the > > > A connector also causes the core to switch to device mode, on its > > > own, without any input from the driver. So by the time the CPU > > > gets to the point in dwc2_hcd_intr() where it checks to see what > > > mode the core is in, it could now be in device mode, even though > > > the interrupt was triggered while in host mode. > > > > Ah, right. This sounds like its the same as for the DISCONNINT. Perhaps > > the handling for PRTINT can be the same as you proposed for DISCONNINT: > > handle it in dwc_hcd_intr, but before the check for host mode? > > Yes. Actually, it would probably be best to check the core's mode at > the start of the host interrupt handler, and if it's in device mode, > just clear ALL asserted host-only interrupts and return without doing > anything further. Yeah, that thought crossed my mind at some point as well. However, the handler for DISCONNINT doesn't just clear the interrupt, it also does: /* Change to L3 (OFF) state */ hsotg->lx_state = DWC2_L3; Is this one no longer relevant when the hardware switched to device mode, or should the above be moved into the host interrupt handler as well? > According to the databook, the host-only interrupts are DisconnInt, > PTxFEmp, HChInt, and PrtInt. In the current code / my patches I also have SOF, NPTxFEmp and RxFLvl marked (and handled) as a host interrupt, but I suspect that these three are used for device mode as well? However, since we don't have device handlers for them already, shouldn't they be cleared by dwc2_hcd_intr() as well if it detects device mode? I suspect not clearing them might currently be a race condition, but it's just not so likely that one of the fifo's is empty at the same moment a cable is unplugged. The SOF interrupt is more likely, but that is cleared directly in the GINTSTS_CONIDSTSCHNG handler (at first glance there still seems to be a race condition here, but assuming that GINTSTS_CONIDSTSCHNG is triggered before switching to device mode, SOF will eventually be masked out). Perhaps it makes sense to actually disable host (and later device) interrupts from GINTSTS_CONIDSTSCHNG? The interrupts for host mode are actually enabled in response to this interrupt already (dwc2_handle_conn_id_status_change_intr -> dwc2_conn_id_status_change -> dwc2_hcd_start -> dwc2_hcd_start_func -> dwc2_host_start -> _dwc2_hcd_start -> dwc2_hcd_reinit -> dwc2_core_host_init -> dwc2_enable_host_interrupts). If we go this route, my concerns for interrupts that are used in both device and host mode are also lifted: These "shared" interrupts can then disabled in disable_(host|device)_interrupts, since we can be sure that one is called before the matching enable_(device|host)_interrupts. Having all interrupts disabled while the core and kernel is switching seems like a good idea to me as well. > DisconnInt is cleared via the GINTSTS register, HChInt is cleared via > the HCINTn register, and PrtInt is cleared via the HPRT register. > PTxFEmp can't be cleared without writing data to the FIFO, so I guess > that one should just be disabled in GINTMSK. Before writing the above, I wrote: if we need to mess with GINTMSK for PTXFEMP, wouldn't it make sense to just disable all (host-only) interrupts when a host-only interrupt is detected when the core is in device mode (instead of clearing all the interrupts)? But it seems to me that doing this in GINTSTS_CONIDSTSCHNG is a better idea if we want to do it like this. Gr. Matthijs -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
