> From: Matthijs Kooijman [mailto:matthijs@xxxxxxxx] > Sent: Thursday, April 25, 2013 2:22 AM > > Hey Paul, Hi Matthijs, What say we just defer this change for now? Once we go to add the device-mode handling, we will probably find out that further changes will be required anyway. For example, handling everything in the GINTSTS_CONIDSTSCHNG handler probably won't work when OTG is enabled, because then you can get host <-> device switching without the cable being unplugged. -- 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
