Re: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
From: Matthijs Kooijman [mailto:matth...@stdin.nl] 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
Hey Paul, 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. Possibly, yeah. In any case, I'm about to send a new patch series which has the easy fixes up front and then tries to make things a bit better separated with less invasive changes. Just see if you think any of those are ok, if not we'll drop them for now and I'll continue with the next batch of cleanup patches I have :-) Gr. Matthijs -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
Hey Paul, On Tue, Apr 16, 2013 at 07:39:22PM +, Paul Zimmerman wrote: From: Matthijs Kooijman [mailto:matth...@stdin.nl] 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
From: Matthijs Kooijman [mailto:matth...@stdin.nl] Sent: Wednesday, April 17, 2013 10:06 AM On Tue, Apr 16, 2013 at 07:39:22PM +, Paul Zimmerman wrote: 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? Ah yes, good point. I guess code like this which changes the driver state should still be executed. What should _not_ be executed is any code that touches host-mode registers, because the host-mode registers disappear once the core has switched itself to device mode. 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? 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. 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. 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. So should not be necessary to explicitly disable interrupts. -- Paul -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
From: Matthijs Kooijman [mailto:matth...@stdin.nl] Sent: Wednesday, April 17, 2013 10:06 AM On Tue, Apr 16, 2013 at 07:39:22PM +, Paul Zimmerman wrote: ... 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. By the way, when I wrote this, I was thinking that the host interrupts should be cleared and then disabled. But, as I mentioned in another email, the host-mode registers actually disappear when the core switches to device mode. So it is impossible to clear HChInt or PrtInt. Only DisconnInt can be cleared, the rest should just be disabled in GINTMSK. -- Paul -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
Hi Paul, On Mon, Apr 15, 2013 at 10:22:12PM +, Paul Zimmerman wrote: Resending as plain text. From: Matthijs Kooijman [mailto:matth...@stdin.nl] Sent: Monday, April 15, 2013 7:14 AM For host mode, this interrupt is already handled by the hcd interrupt handler. The common interrupt handler additionally did a noop handling (it only cleared the flag and nothing else) when in device mode. Since the driver currently supports only host mode, this shouldn't result in any behaviour change in the driver. When device mode is implemented later on, this interrupt should be properly handled by the device interupt handler, if needed. This change allows to make a clean cut between common interrupts and host interrupts, since there are no longer any interrupts handled by both. Hi Matthijs, I'd rather keep this code as-is. The reason is, even though the driver is currently host-only, the core that it is operating may not be. In that case, when the USB cable is unplugged, the core will switch to device mode. In that case the interrupt handler for host mode will exit without clearing the interrupt. Ok, so if I understand this correctly this interrupt is really not a common interrupt, but it is a host interrupt as well as a device interrupt? And the code in dwc2_handle_common_intr would be moved to the device-mode interrupt handler once it gets added? If so, I could replace this patch with a comment stating that. However, the fact that an interrupt can be both a host and a device interrupt, might complicate things. I'll think a bit about this (also considering your other comments that I haven't answered yet) and see what this means for the separation I have been trying to achieve :-) Gr. Matthijs -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
From: Matthijs Kooijman [mailto:matth...@stdin.nl] Sent: Tuesday, April 16, 2013 9:53 AM On Mon, Apr 15, 2013 at 10:22:12PM +, Paul Zimmerman wrote: From: Matthijs Kooijman [mailto:matth...@stdin.nl] Sent: Monday, April 15, 2013 7:14 AM For host mode, this interrupt is already handled by the hcd interrupt handler. The common interrupt handler additionally did a noop handling (it only cleared the flag and nothing else) when in device mode. Since the driver currently supports only host mode, this shouldn't result in any behaviour change in the driver. When device mode is implemented later on, this interrupt should be properly handled by the device interupt handler, if needed. This change allows to make a clean cut between common interrupts and host interrupts, since there are no longer any interrupts handled by both. Hi Matthijs, I'd rather keep this code as-is. The reason is, even though the driver is currently host-only, the core that it is operating may not be. In that case, when the USB cable is unplugged, the core will switch to device mode. In that case the interrupt handler for host mode will exit without clearing the interrupt. Ok, so if I understand this correctly this interrupt is really not a common interrupt, but it is a host interrupt as well as a device interrupt? 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. So there is an inherent race condition due to the way the hardware works. And the code in dwc2_handle_common_intr would be moved to the device-mode interrupt handler once it gets added? If so, I could replace this patch with a comment stating that. No, PRTINT has no meaning in device mode. However, the fact that an interrupt can be both a host and a device interrupt, might complicate things. I'll think a bit about this (also considering your other comments that I haven't answered yet) and see what this means for the separation I have been trying to achieve :-) -- Paul -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
Hi Paul, 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? Gr. Matthijs -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
From: Matthijs Kooijman [mailto:matth...@stdin.nl] 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. According to the databook, the host-only interrupts are DisconnInt, PTxFEmp, HChInt, and PrtInt. 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. Want to take a crack at that? -- Paul -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/10] staging: dwc2: do not handle PRTINT in dwc2_handle_common_intr
Resending as plain text. From: Matthijs Kooijman [mailto:matth...@stdin.nl] Sent: Monday, April 15, 2013 7:14 AM For host mode, this interrupt is already handled by the hcd interrupt handler. The common interrupt handler additionally did a noop handling (it only cleared the flag and nothing else) when in device mode. Since the driver currently supports only host mode, this shouldn't result in any behaviour change in the driver. When device mode is implemented later on, this interrupt should be properly handled by the device interupt handler, if needed. This change allows to make a clean cut between common interrupts and host interrupts, since there are no longer any interrupts handled by both. Hi Matthijs, I'd rather keep this code as-is. The reason is, even though the driver is currently host-only, the core that it is operating may not be. In that case, when the USB cable is unplugged, the core will switch to device mode. In that case the interrupt handler for host mode will exit without clearing the interrupt. So NAK on this patch, unless you have another reason why it is required. -- Paul -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
