Hey Paul, > > 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? > > Yes, these are relevant in both modes. > > I wonder if it would make sense to have a stub device-mode interrupt > handler for now, which would be responsible for clearing these. Once > device mode is implemented, it would be replaced with a real device- > mode interrupt handler. If we make sure that these interrupts are only enabled when the device is in host-mode, then this shouldn't be needed, right? (As long as we don't have code for device-mode, no interrupts will be enabled for device mode either. > > 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). > > It turns out that dwc2_handle_conn_id_status_change_intr() already > disables all interrupts. It calls dwc2_core_init(), which calls > dwc2_core_reset(), which does a soft reset of the core. According > to the databook, that resets most of the core's state, and clears > most of the registers, including GINTMSK. This is true, but the step from dwc2_handle_conn_id_status_change_intr to dwc2_conn_id_status_change (which then calls dwc2_core_init) happens through a work queue, not as a direct call. I think this is the reason there is the reason DISCONN needs to be handled by the common handler right now, or outside of the "is in host mode" check in the hcd handler as we discussed before. If the DISCONN interrupt was handled in the hcd handler, and only if the device is in host mode, the following can happen: - The CONIDSTSCHNG interrupt fires, the hardware starts changing to device mode. - The DISCONN interrupt fires, potentially at the same time or later as the CONIDSTSCHNG interrupt. - By the time the host irq handler is run, the hardware is in device mode, so the hcd irq handler doesn't do anything (in particular, the DISCONN interrupt is not cleared). - In the time between the interrupt and the scheduling of the queued work, the DISCONN interrupt triggers again, and keeps triggering until the queued work is finally scheduled and resets the core. I would propose to clear the host (and later device) interrupts directly in the dwc2_handle_conn_id_status_change_intr function, so the following happens: - The CONIDSTSCHNG interrupt fires, the hardware starts changing to device mode. - dwc2_handle_conn_id_status_change_intr disables all host interrupts, so they don't get handled anymore and don't get triggered. This should remove the race condition, while allowing the DISCONN interrupt to be handled by the host interrupt handler where it would make the most sense. Furthermore, I think this will also close another race condition: If the hardware switches to device mode after the host interupt handler checks for host mode, but before the end of the handler, it could potentially access non-existent registers. With this change, the host mode interrupt handler (effectively) becomes a noop (since it only handles enabled interrupts) as soon as the hardware starts changing to device mode, closeing this gap. Possibly some similar analysis needs to be applied to other places where the hardware or driver can initiate device <-> host mode switches, but perhaps this is only here as long as device mode and the otg protocols are not implemented? 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
