If you've got your interrupt signals bouncing a bit as you insert your
USB device, you might end up in a state when the device is connected but
the driver doesn't know it.
Specifically, the observed order is:
1. hardware sees connect
2. hardware sees disconnect
3. hardware sees connect
4. dwc2_port_intr() - clears connect interrupt
5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
Now you'll be stuck with the cable plugged in and no further interrupts
coming in but the driver will think we're disconnected.
We'll fix this by checking for the missing connect interrupt and
re-connecting after the disconnect is posted. We don't skip the
disconnect because if there is a transitory disconnect we really want to
de-enumerate and re-enumerate.
Signed-off-by: Douglas Anderson <[email protected]>
---
Changes in v2:
- Don't reconnect when called from _dwc2_hcd_stop() (John Youn)
drivers/usb/dwc2/core.h | 6 ++++--
drivers/usb/dwc2/core_intr.c | 4 ++--
drivers/usb/dwc2/hcd.c | 40 ++++++++++++++++++++++++++++++++++++++--
drivers/usb/dwc2/hcd_intr.c | 6 +-----
4 files changed, 45 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index a66d3cb62b65..daa1c342cdac 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1154,12 +1154,14 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg,
#if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg);
-extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg);
+extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
+extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
#else
static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
{ return 0; }
-static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {}
+static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
+static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 27daa42788b1..61601d16e233 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n",
hsotg->op_state);
spin_unlock(&hsotg->lock);
- dwc2_hcd_disconnect(hsotg);
+ dwc2_hcd_disconnect(hsotg, false);
spin_lock(&hsotg->lock);
hsotg->op_state = OTG_STATE_A_PERIPHERAL;
} else {
@@ -401,7 +401,7 @@ static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
dwc2_op_state_str(hsotg));
if (hsotg->op_state == OTG_STATE_A_HOST)
- dwc2_hcd_disconnect(hsotg);
+ dwc2_hcd_disconnect(hsotg, false);
dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
}
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index e79baf73c234..e208eac7ff57 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -268,15 +268,33 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
}
/**
+ * dwc2_hcd_connect() - Handles connect of the HCD
+ *
+ * @hsotg: Pointer to struct dwc2_hsotg
+ *
+ * Must be called with interrupt disabled and spinlock held
+ */
+void dwc2_hcd_connect(struct dwc2_hsotg *hsotg)
+{
+ if (hsotg->lx_state != DWC2_L0)
+ usb_hcd_resume_root_hub(hsotg->priv);
+
+ hsotg->flags.b.port_connect_status_change = 1;
+ hsotg->flags.b.port_connect_status = 1;
+}
+
+/**
* dwc2_hcd_disconnect() - Handles disconnect of the HCD
*
* @hsotg: Pointer to struct dwc2_hsotg
+ * @force: If true, we won't try to reconnect even if we see device connected.
*
* Must be called with interrupt disabled and spinlock held
*/
-void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
+void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force)
{
u32 intr;
+ u32 hprt0;
/* Set status flags for the hub driver */
hsotg->flags.b.port_connect_status_change = 1;
@@ -315,6 +333,24 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
dwc2_hcd_cleanup_channels(hsotg);
dwc2_host_disconnect(hsotg);
+
+ /*
+ * Add an extra check here to see if we're actually connected but
+ * we don't have a detection interrupt pending. This can happen if:
+ * 1. hardware sees connect
+ * 2. hardware sees disconnect
+ * 3. hardware sees connect
+ * 4. dwc2_port_intr() - clears connect interrupt
+ * 5. dwc2_handle_common_intr() - calls here
+ *
+ * Without the extra check here we will end calling disconnect
+ * and won't get any future interrupts to handle the connect.
+ */
+ if (!force) {
+ hprt0 = dwc2_readl(hsotg->regs + HPRT0);
+ if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS))
+ dwc2_hcd_connect(hsotg);
+ }
}
/**
@@ -2365,7 +2401,7 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
spin_lock_irqsave(&hsotg->lock, flags);
/* Ensure hcd is disconnected */
- dwc2_hcd_disconnect(hsotg);
+ dwc2_hcd_disconnect(hsotg, true);
dwc2_hcd_stop(hsotg);
hsotg->lx_state = DWC2_L3;
hcd->state = HC_STATE_HALT;
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index bda0b21b850f..03504ac2fecc 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -350,11 +350,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
dev_vdbg(hsotg->dev,
"--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
hprt0);
- if (hsotg->lx_state != DWC2_L0)
- usb_hcd_resume_root_hub(hsotg->priv);
-
- hsotg->flags.b.port_connect_status_change = 1;
- hsotg->flags.b.port_connect_status = 1;
+ dwc2_hcd_connect(hsotg);
hprt0_modify |= HPRT0_CONNDET;
/*
--
2.6.0.rc2.230.g3dd15c0
In general it is wise to clear interrupts before processing them. If
you don't do that, you can get:
1. Interrupt happens
2. You look at system state and process interrupt
3. A new interrupt happens
4. You clear interrupt without processing it.
This patch was actually a first attempt to fix missing device insertions
as described in (usb: dwc2: host: Fix missing device insertions) and it
did solve some of the signal bouncing problems but not all of
them (which is why I submitted the other patch). Specifically, this
patch itself would sometimes change:
1. hardware sees connect
2. hardware sees disconnect
3. hardware sees connect
4. dwc2_port_intr() - clears connect interrupt
5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
...to:
1. hardware sees connect
2. hardware sees disconnect
3. dwc2_port_intr() - clears connect interrupt
4. hardware sees connect
5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
...but with different timing then sometimes we'd still miss cable
insertions.
In any case, though this patch doesn't fix any (known) problems, it
still seems wise as a general policy to clear interrupt before handling
them.
Signed-off-by: Douglas Anderson <[email protected]>
---
Changes in v2: None
drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
drivers/usb/dwc2/hcd_intr.c | 16 ++++++-------
2 files changed, 35 insertions(+), 36 deletions(-)
diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 61601d16e233..2a166b7eec41 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
*/
static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
{
- u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
+ u32 hprt0;
+ /* Clear interrupt */
+ dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
+
+ hprt0 = dwc2_readl(hsotg->regs + HPRT0);
if (hprt0 & HPRT0_ENACHG) {
hprt0 &= ~HPRT0_ENA;
dwc2_writel(hprt0, hsotg->regs + HPRT0);
}
-
- /* Clear interrupt */
- dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
}
/**
@@ -98,11 +99,11 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
*/
static void dwc2_handle_mode_mismatch_intr(struct dwc2_hsotg *hsotg)
{
- dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
- dwc2_is_host_mode(hsotg) ? "Host" : "Device");
-
/* Clear interrupt */
dwc2_writel(GINTSTS_MODEMIS, hsotg->regs + GINTSTS);
+
+ dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
+ dwc2_is_host_mode(hsotg) ? "Host" : "Device");
}
/**
@@ -276,9 +277,13 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
*/
static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
{
- u32 gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+ u32 gintmsk;
+
+ /* Clear interrupt */
+ dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
/* Need to disable SOF interrupt immediately */
+ gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
gintmsk &= ~GINTSTS_SOF;
dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
@@ -295,9 +300,6 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
queue_work(hsotg->wq_otg, &hsotg->wf_otg);
spin_lock(&hsotg->lock);
}
-
- /* Clear interrupt */
- dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
}
/**
@@ -315,12 +317,12 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
{
int ret;
- dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
- hsotg->lx_state);
-
/* Clear interrupt */
dwc2_writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
+ dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
+ hsotg->lx_state);
+
if (dwc2_is_device_mode(hsotg)) {
if (hsotg->lx_state == DWC2_L2) {
ret = dwc2_exit_hibernation(hsotg, true);
@@ -347,6 +349,10 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
{
int ret;
+
+ /* Clear interrupt */
+ dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
+
dev_dbg(hsotg->dev, "++Resume or Remote Wakeup Detected Interrupt++\n");
dev_dbg(hsotg->dev, "%s lxstate = %d\n", __func__, hsotg->lx_state);
@@ -368,10 +374,9 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
/* Change to L0 state */
hsotg->lx_state = DWC2_L0;
} else {
- if (hsotg->core_params->hibernation) {
- dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
+ if (hsotg->core_params->hibernation)
return;
- }
+
if (hsotg->lx_state != DWC2_L1) {
u32 pcgcctl = dwc2_readl(hsotg->regs + PCGCTL);
@@ -385,9 +390,6 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
hsotg->lx_state = DWC2_L0;
}
}
-
- /* Clear interrupt */
- dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
}
/*
@@ -396,14 +398,14 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
*/
static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
{
+ dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
+
dev_dbg(hsotg->dev, "++Disconnect Detected Interrupt++ (%s) %s\n",
dwc2_is_host_mode(hsotg) ? "Host" : "Device",
dwc2_op_state_str(hsotg));
if (hsotg->op_state == OTG_STATE_A_HOST)
dwc2_hcd_disconnect(hsotg, false);
-
- dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
}
/*
@@ -419,6 +421,9 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
u32 dsts;
int ret;
+ /* Clear interrupt */
+ dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
+
dev_dbg(hsotg->dev, "USB SUSPEND\n");
if (dwc2_is_device_mode(hsotg)) {
@@ -437,7 +442,7 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
if (!dwc2_is_device_connected(hsotg)) {
dev_dbg(hsotg->dev,
"ignore suspend request before enumeration\n");
- goto clear_int;
+ return;
}
ret = dwc2_enter_hibernation(hsotg);
@@ -476,10 +481,6 @@ skip_power_saving:
hsotg->op_state = OTG_STATE_A_HOST;
}
}
-
-clear_int:
- /* Clear interrupt */
- dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
}
#define GINTMSK_COMMON (GINTSTS_WKUPINT | GINTSTS_SESSREQINT | \
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 03504ac2fecc..4c7f709b8182 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -122,6 +122,9 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
struct dwc2_qh *qh;
enum dwc2_transaction_type tr_type;
+ /* Clear interrupt */
+ dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
+
#ifdef DEBUG_SOF
dev_vdbg(hsotg->dev, "--Start of Frame Interrupt--\n");
#endif
@@ -146,9 +149,6 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
tr_type = dwc2_hcd_select_transactions(hsotg);
if (tr_type != DWC2_TRANSACTION_NONE)
dwc2_hcd_queue_transactions(hsotg, tr_type);
-
- /* Clear interrupt */
- dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
}
/*
@@ -347,11 +347,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
* Set flag and clear if detected
*/
if (hprt0 & HPRT0_CONNDET) {
+ writel(hprt0_modify | HPRT0_CONNDET, hsotg->regs + HPRT0);
+
dev_vdbg(hsotg->dev,
"--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
hprt0);
dwc2_hcd_connect(hsotg);
- hprt0_modify |= HPRT0_CONNDET;
/*
* The Hub driver asserts a reset when it sees port connect
@@ -364,10 +365,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
* Clear if detected - Set internal flag if disabled
*/
if (hprt0 & HPRT0_ENACHG) {
+ writel(hprt0_modify | HPRT0_ENACHG, hsotg->regs + HPRT0);
dev_vdbg(hsotg->dev,
" --Port Interrupt HPRT0=0x%08x Port Enable Changed (now %d)--\n",
hprt0, !!(hprt0 & HPRT0_ENA));
- hprt0_modify |= HPRT0_ENACHG;
if (hprt0 & HPRT0_ENA)
dwc2_hprt0_enable(hsotg, hprt0, &hprt0_modify);
else
@@ -376,15 +377,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
/* Overcurrent Change Interrupt */
if (hprt0 & HPRT0_OVRCURRCHG) {
+ writel(hprt0_modify | HPRT0_OVRCURRCHG, hsotg->regs + HPRT0);
dev_vdbg(hsotg->dev,
" --Port Interrupt HPRT0=0x%08x Port Overcurrent Changed--\n",
hprt0);
hsotg->flags.b.port_over_current_change = 1;
- hprt0_modify |= HPRT0_OVRCURRCHG;
}
-
- /* Clear Port Interrupts */
- dwc2_writel(hprt0_modify, hsotg->regs + HPRT0);
}
/*
--
2.6.0.rc2.230.g3dd15c0
On 11/3/2015 12:31 PM, Douglas Anderson wrote:
> If you've got your interrupt signals bouncing a bit as you insert your
> USB device, you might end up in a state when the device is connected but
> the driver doesn't know it.
>
> Specifically, the observed order is:
> 1. hardware sees connect
> 2. hardware sees disconnect
> 3. hardware sees connect
> 4. dwc2_port_intr() - clears connect interrupt
> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> Now you'll be stuck with the cable plugged in and no further interrupts
> coming in but the driver will think we're disconnected.
>
> We'll fix this by checking for the missing connect interrupt and
> re-connecting after the disconnect is posted. We don't skip the
> disconnect because if there is a transitory disconnect we really want to
> de-enumerate and re-enumerate.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> Changes in v2:
> - Don't reconnect when called from _dwc2_hcd_stop() (John Youn)
>
> drivers/usb/dwc2/core.h | 6 ++++--
> drivers/usb/dwc2/core_intr.c | 4 ++--
> drivers/usb/dwc2/hcd.c | 40 ++++++++++++++++++++++++++++++++++++++--
> drivers/usb/dwc2/hcd_intr.c | 6 +-----
> 4 files changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index a66d3cb62b65..daa1c342cdac 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -1154,12 +1154,14 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg,
>
> #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
> extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg);
> -extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg);
> +extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
> +extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
> extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
> #else
> static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
> { return 0; }
> -static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {}
> +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
> +static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
> static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
> static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 27daa42788b1..61601d16e233 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
> dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n",
> hsotg->op_state);
> spin_unlock(&hsotg->lock);
> - dwc2_hcd_disconnect(hsotg);
> + dwc2_hcd_disconnect(hsotg, false);
> spin_lock(&hsotg->lock);
> hsotg->op_state = OTG_STATE_A_PERIPHERAL;
> } else {
> @@ -401,7 +401,7 @@ static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
> dwc2_op_state_str(hsotg));
>
> if (hsotg->op_state == OTG_STATE_A_HOST)
> - dwc2_hcd_disconnect(hsotg);
> + dwc2_hcd_disconnect(hsotg, false);
>
> dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
> }
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index e79baf73c234..e208eac7ff57 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -268,15 +268,33 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
> }
>
> /**
> + * dwc2_hcd_connect() - Handles connect of the HCD
> + *
> + * @hsotg: Pointer to struct dwc2_hsotg
> + *
> + * Must be called with interrupt disabled and spinlock held
> + */
> +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg)
> +{
> + if (hsotg->lx_state != DWC2_L0)
> + usb_hcd_resume_root_hub(hsotg->priv);
> +
> + hsotg->flags.b.port_connect_status_change = 1;
> + hsotg->flags.b.port_connect_status = 1;
> +}
> +
> +/**
> * dwc2_hcd_disconnect() - Handles disconnect of the HCD
> *
> * @hsotg: Pointer to struct dwc2_hsotg
> + * @force: If true, we won't try to reconnect even if we see device connected.
> *
> * Must be called with interrupt disabled and spinlock held
> */
> -void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
> +void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force)
> {
> u32 intr;
> + u32 hprt0;
>
> /* Set status flags for the hub driver */
> hsotg->flags.b.port_connect_status_change = 1;
> @@ -315,6 +333,24 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
> dwc2_hcd_cleanup_channels(hsotg);
>
> dwc2_host_disconnect(hsotg);
> +
> + /*
> + * Add an extra check here to see if we're actually connected but
> + * we don't have a detection interrupt pending. This can happen if:
> + * 1. hardware sees connect
> + * 2. hardware sees disconnect
> + * 3. hardware sees connect
> + * 4. dwc2_port_intr() - clears connect interrupt
> + * 5. dwc2_handle_common_intr() - calls here
> + *
> + * Without the extra check here we will end calling disconnect
> + * and won't get any future interrupts to handle the connect.
> + */
> + if (!force) {
> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> + if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS))
> + dwc2_hcd_connect(hsotg);
> + }
> }
>
> /**
> @@ -2365,7 +2401,7 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
>
> spin_lock_irqsave(&hsotg->lock, flags);
> /* Ensure hcd is disconnected */
> - dwc2_hcd_disconnect(hsotg);
> + dwc2_hcd_disconnect(hsotg, true);
> dwc2_hcd_stop(hsotg);
> hsotg->lx_state = DWC2_L3;
> hcd->state = HC_STATE_HALT;
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index bda0b21b850f..03504ac2fecc 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -350,11 +350,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
> dev_vdbg(hsotg->dev,
> "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
> hprt0);
> - if (hsotg->lx_state != DWC2_L0)
> - usb_hcd_resume_root_hub(hsotg->priv);
> -
> - hsotg->flags.b.port_connect_status_change = 1;
> - hsotg->flags.b.port_connect_status = 1;
> + dwc2_hcd_connect(hsotg);
> hprt0_modify |= HPRT0_CONNDET;
>
> /*
>
Acked-by: John Youn <[email protected]>
Tested-by: John Youn <[email protected]>
Tested on Synopsys HAPS platform IP v3.20a.
Regards,
John
On 11/3/2015 12:31 PM, Douglas Anderson wrote:
> In general it is wise to clear interrupts before processing them. If
> you don't do that, you can get:
> 1. Interrupt happens
> 2. You look at system state and process interrupt
> 3. A new interrupt happens
> 4. You clear interrupt without processing it.
>
> This patch was actually a first attempt to fix missing device insertions
> as described in (usb: dwc2: host: Fix missing device insertions) and it
> did solve some of the signal bouncing problems but not all of
> them (which is why I submitted the other patch). Specifically, this
> patch itself would sometimes change:
> 1. hardware sees connect
> 2. hardware sees disconnect
> 3. hardware sees connect
> 4. dwc2_port_intr() - clears connect interrupt
> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> ...to:
> 1. hardware sees connect
> 2. hardware sees disconnect
> 3. dwc2_port_intr() - clears connect interrupt
> 4. hardware sees connect
> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> ...but with different timing then sometimes we'd still miss cable
> insertions.
>
> In any case, though this patch doesn't fix any (known) problems, it
> still seems wise as a general policy to clear interrupt before handling
> them.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> Changes in v2: None
>
> drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
> drivers/usb/dwc2/hcd_intr.c | 16 ++++++-------
> 2 files changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 61601d16e233..2a166b7eec41 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
> */
> static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
> {
> - u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> + u32 hprt0;
>
> + /* Clear interrupt */
> + dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
> +
> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> if (hprt0 & HPRT0_ENACHG) {
> hprt0 &= ~HPRT0_ENA;
> dwc2_writel(hprt0, hsotg->regs + HPRT0);
> }
> -
> - /* Clear interrupt */
> - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
> }
>
> /**
> @@ -98,11 +99,11 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
> */
> static void dwc2_handle_mode_mismatch_intr(struct dwc2_hsotg *hsotg)
> {
> - dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
> - dwc2_is_host_mode(hsotg) ? "Host" : "Device");
> -
> /* Clear interrupt */
> dwc2_writel(GINTSTS_MODEMIS, hsotg->regs + GINTSTS);
> +
> + dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
> + dwc2_is_host_mode(hsotg) ? "Host" : "Device");
> }
>
> /**
> @@ -276,9 +277,13 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
> */
> static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
> {
> - u32 gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
> + u32 gintmsk;
> +
> + /* Clear interrupt */
> + dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
>
> /* Need to disable SOF interrupt immediately */
> + gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
> gintmsk &= ~GINTSTS_SOF;
> dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
>
> @@ -295,9 +300,6 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
> queue_work(hsotg->wq_otg, &hsotg->wf_otg);
> spin_lock(&hsotg->lock);
> }
> -
> - /* Clear interrupt */
> - dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
> }
>
> /**
> @@ -315,12 +317,12 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
> {
> int ret;
>
> - dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
> - hsotg->lx_state);
> -
> /* Clear interrupt */
> dwc2_writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
>
> + dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
> + hsotg->lx_state);
> +
> if (dwc2_is_device_mode(hsotg)) {
> if (hsotg->lx_state == DWC2_L2) {
> ret = dwc2_exit_hibernation(hsotg, true);
> @@ -347,6 +349,10 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> {
> int ret;
> +
> + /* Clear interrupt */
> + dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> +
> dev_dbg(hsotg->dev, "++Resume or Remote Wakeup Detected Interrupt++\n");
> dev_dbg(hsotg->dev, "%s lxstate = %d\n", __func__, hsotg->lx_state);
>
> @@ -368,10 +374,9 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> /* Change to L0 state */
> hsotg->lx_state = DWC2_L0;
> } else {
> - if (hsotg->core_params->hibernation) {
> - dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> + if (hsotg->core_params->hibernation)
> return;
> - }
> +
> if (hsotg->lx_state != DWC2_L1) {
> u32 pcgcctl = dwc2_readl(hsotg->regs + PCGCTL);
>
> @@ -385,9 +390,6 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> hsotg->lx_state = DWC2_L0;
> }
> }
> -
> - /* Clear interrupt */
> - dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> }
>
> /*
> @@ -396,14 +398,14 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> */
> static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
> {
> + dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
> +
> dev_dbg(hsotg->dev, "++Disconnect Detected Interrupt++ (%s) %s\n",
> dwc2_is_host_mode(hsotg) ? "Host" : "Device",
> dwc2_op_state_str(hsotg));
>
> if (hsotg->op_state == OTG_STATE_A_HOST)
> dwc2_hcd_disconnect(hsotg, false);
> -
> - dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
> }
>
> /*
> @@ -419,6 +421,9 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
> u32 dsts;
> int ret;
>
> + /* Clear interrupt */
> + dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
> +
> dev_dbg(hsotg->dev, "USB SUSPEND\n");
>
> if (dwc2_is_device_mode(hsotg)) {
> @@ -437,7 +442,7 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
> if (!dwc2_is_device_connected(hsotg)) {
> dev_dbg(hsotg->dev,
> "ignore suspend request before enumeration\n");
> - goto clear_int;
> + return;
> }
>
> ret = dwc2_enter_hibernation(hsotg);
> @@ -476,10 +481,6 @@ skip_power_saving:
> hsotg->op_state = OTG_STATE_A_HOST;
> }
> }
> -
> -clear_int:
> - /* Clear interrupt */
> - dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
> }
>
> #define GINTMSK_COMMON (GINTSTS_WKUPINT | GINTSTS_SESSREQINT | \
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index 03504ac2fecc..4c7f709b8182 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -122,6 +122,9 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
> struct dwc2_qh *qh;
> enum dwc2_transaction_type tr_type;
>
> + /* Clear interrupt */
> + dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
> +
> #ifdef DEBUG_SOF
> dev_vdbg(hsotg->dev, "--Start of Frame Interrupt--\n");
> #endif
> @@ -146,9 +149,6 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
> tr_type = dwc2_hcd_select_transactions(hsotg);
> if (tr_type != DWC2_TRANSACTION_NONE)
> dwc2_hcd_queue_transactions(hsotg, tr_type);
> -
> - /* Clear interrupt */
> - dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
> }
>
> /*
> @@ -347,11 +347,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
> * Set flag and clear if detected
> */
> if (hprt0 & HPRT0_CONNDET) {
> + writel(hprt0_modify | HPRT0_CONNDET, hsotg->regs + HPRT0);
> +
> dev_vdbg(hsotg->dev,
> "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
> hprt0);
> dwc2_hcd_connect(hsotg);
> - hprt0_modify |= HPRT0_CONNDET;
>
> /*
> * The Hub driver asserts a reset when it sees port connect
> @@ -364,10 +365,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
> * Clear if detected - Set internal flag if disabled
> */
> if (hprt0 & HPRT0_ENACHG) {
> + writel(hprt0_modify | HPRT0_ENACHG, hsotg->regs + HPRT0);
> dev_vdbg(hsotg->dev,
> " --Port Interrupt HPRT0=0x%08x Port Enable Changed (now %d)--\n",
> hprt0, !!(hprt0 & HPRT0_ENA));
> - hprt0_modify |= HPRT0_ENACHG;
> if (hprt0 & HPRT0_ENA)
> dwc2_hprt0_enable(hsotg, hprt0, &hprt0_modify);
> else
> @@ -376,15 +377,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>
> /* Overcurrent Change Interrupt */
> if (hprt0 & HPRT0_OVRCURRCHG) {
> + writel(hprt0_modify | HPRT0_OVRCURRCHG, hsotg->regs + HPRT0);
> dev_vdbg(hsotg->dev,
> " --Port Interrupt HPRT0=0x%08x Port Overcurrent Changed--\n",
> hprt0);
> hsotg->flags.b.port_over_current_change = 1;
> - hprt0_modify |= HPRT0_OVRCURRCHG;
> }
> -
> - /* Clear Port Interrupts */
> - dwc2_writel(hprt0_modify, hsotg->regs + HPRT0);
> }
>
> /*
>
Acked-by: John Youn <[email protected]>
Tested-by: John Youn <[email protected]>
Regards,
John
Hi,
Douglas Anderson <[email protected]> writes:
> In general it is wise to clear interrupts before processing them. If
> you don't do that, you can get:
> 1. Interrupt happens
> 2. You look at system state and process interrupt
> 3. A new interrupt happens
> 4. You clear interrupt without processing it.
>
> This patch was actually a first attempt to fix missing device insertions
> as described in (usb: dwc2: host: Fix missing device insertions) and it
> did solve some of the signal bouncing problems but not all of
> them (which is why I submitted the other patch). Specifically, this
> patch itself would sometimes change:
> 1. hardware sees connect
> 2. hardware sees disconnect
> 3. hardware sees connect
> 4. dwc2_port_intr() - clears connect interrupt
> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> ...to:
> 1. hardware sees connect
> 2. hardware sees disconnect
> 3. dwc2_port_intr() - clears connect interrupt
> 4. hardware sees connect
> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> ...but with different timing then sometimes we'd still miss cable
> insertions.
>
> In any case, though this patch doesn't fix any (known) problems, it
> still seems wise as a general policy to clear interrupt before handling
> them.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> Changes in v2: None
>
> drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
> drivers/usb/dwc2/hcd_intr.c | 16 ++++++-------
> 2 files changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 61601d16e233..2a166b7eec41 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
> */
> static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
> {
> - u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> + u32 hprt0;
>
> + /* Clear interrupt */
> + dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
> +
> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> if (hprt0 & HPRT0_ENACHG) {
> hprt0 &= ~HPRT0_ENA;
> dwc2_writel(hprt0, hsotg->regs + HPRT0);
> }
> -
> - /* Clear interrupt */
> - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
isn't this a regression ? You're first clearing the interrupts and only
then reading to check what's pending, however, what's pending has just
been cleared. Seems like this should be:
hprt0 = dwc2_readl(HPRT0);
dwc2_writeal(PRTINT, GINTSTS);
Also, these two patches look very large for the -rc. I'll move them to
v4.5 merge window unless you can convince me that they are, indeed, the
smallest change possible to fix the problem you're facing and that they
are, indeed, regressions.
--
balbi
Felipe,
On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi <[email protected]> wrote:
>
> Hi,
>
> Douglas Anderson <[email protected]> writes:
>> In general it is wise to clear interrupts before processing them. If
>> you don't do that, you can get:
>> 1. Interrupt happens
>> 2. You look at system state and process interrupt
>> 3. A new interrupt happens
>> 4. You clear interrupt without processing it.
>>
>> This patch was actually a first attempt to fix missing device insertions
>> as described in (usb: dwc2: host: Fix missing device insertions) and it
>> did solve some of the signal bouncing problems but not all of
>> them (which is why I submitted the other patch). Specifically, this
>> patch itself would sometimes change:
>> 1. hardware sees connect
>> 2. hardware sees disconnect
>> 3. hardware sees connect
>> 4. dwc2_port_intr() - clears connect interrupt
>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>
>> ...to:
>> 1. hardware sees connect
>> 2. hardware sees disconnect
>> 3. dwc2_port_intr() - clears connect interrupt
>> 4. hardware sees connect
>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>
>> ...but with different timing then sometimes we'd still miss cable
>> insertions.
>>
>> In any case, though this patch doesn't fix any (known) problems, it
>> still seems wise as a general policy to clear interrupt before handling
>> them.
>>
>> Signed-off-by: Douglas Anderson <[email protected]>
>> ---
>> Changes in v2: None
>>
>> drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
>> drivers/usb/dwc2/hcd_intr.c | 16 ++++++-------
>> 2 files changed, 35 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index 61601d16e233..2a166b7eec41 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
>> */
>> static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>> {
>> - u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>> + u32 hprt0;
>>
>> + /* Clear interrupt */
>> + dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>> +
>> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>> if (hprt0 & HPRT0_ENACHG) {
>> hprt0 &= ~HPRT0_ENA;
>> dwc2_writel(hprt0, hsotg->regs + HPRT0);
>> }
>> -
>> - /* Clear interrupt */
>> - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>
> isn't this a regression ? You're first clearing the interrupts and only
> then reading to check what's pending, however, what's pending has just
> been cleared. Seems like this should be:
>
> hprt0 = dwc2_readl(HPRT0);
> dwc2_writeal(PRTINT, GINTSTS);
Actually, we could probably remove the setting of GINTSTS_PRTINT
completely. The docs I have say that the GINTSTS_PRTINT is read only
and that:
> The core sets this bit to indicate a change in port status of one of the
> DWC_otg core ports in Host mode. The application must read the
> Host Port Control and Status (HPRT) register to determine the exact
> event that caused this interrupt. The application must clear the
> appropriate status bit in the Host Port Control and Status register to
> clear this bit.
...so writing PRTINT is probably useless, but John can confirm.
...if it wasn't useless to clear PRTINT it would depend on how things
were implemented. One (purely speculative) case where my patch would
be more correct:
1. Interrupt source 1 in HPRT fires. PRTINT is latched to 1 and
interrupt handler is called.
2. Read HPRT
3. Interrupt source 2 in HPRT fires. PRTINT is already 1, so no new
interrupt handler called.
4. Clear PRTINT
5. Handle interrupt source 1
...now we'll never get an interrupt for interrupt source 2.
In any case, I think this particular change is a no-op, but other
changes in this patch (probably) aren't.
> Also, these two patches look very large for the -rc. I'll move them to
> v4.5 merge window unless you can convince me that they are, indeed, the
> smallest change possible to fix the problem you're facing and that they
> are, indeed, regressions.
Patch #2 should definitely _not_ go into the RC. It is a relatively
intrusive patch, fixes no known issues, and has only had light
testing. I haven't even landed this change locally in ChromeOS since
it's a bit scary to me I haven't found any good use for it. I do keep
testing with it to see if it fixes any of my issues, though. ;)
I'd like to see patch #1 should go into the RC since it fixes problems
that are really easy to reproduce. IMHO It's also neither big nor
scary. FWIW it's already landed in the ChromeOS tree as of Oct 20th
(almost a month). While it hasn't hit stable channel yet, it's been
on beta channel and nobody has discovered any issues with it yet.
Thanks! :)
-Doug
Hi,
(replying here for the context of why I think this is NOT the smallest
patch possible for the -rc)
Douglas Anderson <[email protected]> writes:
> If you've got your interrupt signals bouncing a bit as you insert your
> USB device, you might end up in a state when the device is connected but
> the driver doesn't know it.
>
> Specifically, the observed order is:
> 1. hardware sees connect
> 2. hardware sees disconnect
> 3. hardware sees connect
> 4. dwc2_port_intr() - clears connect interrupt
> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> Now you'll be stuck with the cable plugged in and no further interrupts
> coming in but the driver will think we're disconnected.
>
> We'll fix this by checking for the missing connect interrupt and
> re-connecting after the disconnect is posted. We don't skip the
> disconnect because if there is a transitory disconnect we really want to
> de-enumerate and re-enumerate.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> Changes in v2:
> - Don't reconnect when called from _dwc2_hcd_stop() (John Youn)
>
> drivers/usb/dwc2/core.h | 6 ++++--
> drivers/usb/dwc2/core_intr.c | 4 ++--
> drivers/usb/dwc2/hcd.c | 40 ++++++++++++++++++++++++++++++++++++++--
> drivers/usb/dwc2/hcd_intr.c | 6 +-----
> 4 files changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index a66d3cb62b65..daa1c342cdac 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -1154,12 +1154,14 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg,
>
> #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
> extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg);
> -extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg);
> +extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
> +extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
this 'force' parameter is new and looks unnecessary for the -rc.
> extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
> #else
> static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
> { return 0; }
> -static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {}
> +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
> +static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
> static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
> static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 27daa42788b1..61601d16e233 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
> dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n",
> hsotg->op_state);
> spin_unlock(&hsotg->lock);
> - dwc2_hcd_disconnect(hsotg);
> + dwc2_hcd_disconnect(hsotg, false);
because force is unnecessary (it seems to be just an optimization to
me), this change is unnecessary.
> spin_lock(&hsotg->lock);
> hsotg->op_state = OTG_STATE_A_PERIPHERAL;
> } else {
> @@ -401,7 +401,7 @@ static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
> dwc2_op_state_str(hsotg));
>
> if (hsotg->op_state == OTG_STATE_A_HOST)
> - dwc2_hcd_disconnect(hsotg);
> + dwc2_hcd_disconnect(hsotg, false);
and so is this.
> dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
> }
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index e79baf73c234..e208eac7ff57 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -268,15 +268,33 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
> }
>
> /**
> + * dwc2_hcd_connect() - Handles connect of the HCD
> + *
> + * @hsotg: Pointer to struct dwc2_hsotg
> + *
> + * Must be called with interrupt disabled and spinlock held
> + */
> +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg)
> +{
> + if (hsotg->lx_state != DWC2_L0)
> + usb_hcd_resume_root_hub(hsotg->priv);
> +
> + hsotg->flags.b.port_connect_status_change = 1;
> + hsotg->flags.b.port_connect_status = 1;
> +}
you make no mention of why this is needed. This is basically a refactor,
not a fix.
> +/**
> * dwc2_hcd_disconnect() - Handles disconnect of the HCD
> *
> * @hsotg: Pointer to struct dwc2_hsotg
> + * @force: If true, we won't try to reconnect even if we see device connected.
> *
> * Must be called with interrupt disabled and spinlock held
> */
> -void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
> +void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force)
as mentioned before, unnecessary 'force' parameter.
> @@ -315,6 +333,24 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
> dwc2_hcd_cleanup_channels(hsotg);
>
> dwc2_host_disconnect(hsotg);
> +
> + /*
> + * Add an extra check here to see if we're actually connected but
> + * we don't have a detection interrupt pending. This can happen if:
> + * 1. hardware sees connect
> + * 2. hardware sees disconnect
> + * 3. hardware sees connect
> + * 4. dwc2_port_intr() - clears connect interrupt
> + * 5. dwc2_handle_common_intr() - calls here
> + *
> + * Without the extra check here we will end calling disconnect
> + * and won't get any future interrupts to handle the connect.
> + */
> + if (!force) {
> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> + if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS))
> + dwc2_hcd_connect(hsotg);
what's inside this 'force' branch is what you really need to fix the
problem, right ? It seems like for the -rc cycle, all you need is:
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 571c21727ff9..967d1e686efe 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
*/
void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
{
+ u32 hprt0;
u32 intr;
/* Set status flags for the hub driver */
@@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
dwc2_hcd_cleanup_channels(hsotg);
dwc2_host_disconnect(hsotg);
+
+ hprt0 = dwc2_readl(hsotg->regs + HPRT0);
+ if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) {
+ if (hsotg->lx_state != DWC2_L0)
+ usb_hcd_resume_roothub(hsotg->priv);
+
+ hsotg->flags.b.port_connect_status_change = 1;
+ hsotg->flags.b.port_connect_status = 1;
+ }
}
/**
This has some code duplication and it'll *always* check for CONNDET |
CONNSTS, but that's okay for the -rc. Follow-up patches (for v4.5 merge
window) can refactor the code duplication and introduce 'force'
parameter.
> @@ -2365,7 +2401,7 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
>
> spin_lock_irqsave(&hsotg->lock, flags);
> /* Ensure hcd is disconnected */
> - dwc2_hcd_disconnect(hsotg);
> + dwc2_hcd_disconnect(hsotg, true);
unnecessary change.
> dwc2_hcd_stop(hsotg);
> hsotg->lx_state = DWC2_L3;
> hcd->state = HC_STATE_HALT;
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index bda0b21b850f..03504ac2fecc 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -350,11 +350,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
> dev_vdbg(hsotg->dev,
> "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
> hprt0);
> - if (hsotg->lx_state != DWC2_L0)
> - usb_hcd_resume_root_hub(hsotg->priv);
> -
> - hsotg->flags.b.port_connect_status_change = 1;
> - hsotg->flags.b.port_connect_status = 1;
> + dwc2_hcd_connect(hsotg);
unnecessary change.
Do you agree or do you think 'force' is really necessary for this fix ?
--
balbi
Felipe,
On Mon, Nov 16, 2015 at 9:44 AM, Felipe Balbi <[email protected]> wrote:
>> extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
>> #else
>> static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
>> { return 0; }
>> -static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {}
>> +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
>> +static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
>> static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>> static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index 27daa42788b1..61601d16e233 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
>> dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n",
>> hsotg->op_state);
>> spin_unlock(&hsotg->lock);
>> - dwc2_hcd_disconnect(hsotg);
>> + dwc2_hcd_disconnect(hsotg, false);
>
> because force is unnecessary (it seems to be just an optimization to
> me), this change is unnecessary.
I added "force" in v2 of the patch in response to John's feedback to
v1. He pointed out that when you unload the module when you have a
device connected that my v1 patch would not properly disconnect the
device (or, rather, it would disconnect it and then reconnect it).
That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force of "true".
>> /**
>> + * dwc2_hcd_connect() - Handles connect of the HCD
>> + *
>> + * @hsotg: Pointer to struct dwc2_hsotg
>> + *
>> + * Must be called with interrupt disabled and spinlock held
>> + */
>> +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg)
>> +{
>> + if (hsotg->lx_state != DWC2_L0)
>> + usb_hcd_resume_root_hub(hsotg->priv);
>> +
>> + hsotg->flags.b.port_connect_status_change = 1;
>> + hsotg->flags.b.port_connect_status = 1;
>> +}
>
> you make no mention of why this is needed. This is basically a refactor,
> not a fix.
This new function is called from two places now, isn't it?
Basically this is a little bit of code that used to be directly in
dwc2_port_intr(). I still want it there, but I also want to call the
same bit of code after a disconnect if I detect that the device has
been inserted again.
>> @@ -315,6 +333,24 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
>> dwc2_hcd_cleanup_channels(hsotg);
>>
>> dwc2_host_disconnect(hsotg);
>> +
>> + /*
>> + * Add an extra check here to see if we're actually connected but
>> + * we don't have a detection interrupt pending. This can happen if:
>> + * 1. hardware sees connect
>> + * 2. hardware sees disconnect
>> + * 3. hardware sees connect
>> + * 4. dwc2_port_intr() - clears connect interrupt
>> + * 5. dwc2_handle_common_intr() - calls here
>> + *
>> + * Without the extra check here we will end calling disconnect
>> + * and won't get any future interrupts to handle the connect.
>> + */
>> + if (!force) {
>> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>> + if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS))
>> + dwc2_hcd_connect(hsotg);
>
> what's inside this 'force' branch is what you really need to fix the
> problem, right ? It seems like for the -rc cycle, all you need is:
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 571c21727ff9..967d1e686efe 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
> */
> void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
> {
> + u32 hprt0;
> u32 intr;
>
> /* Set status flags for the hub driver */
> @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
> dwc2_hcd_cleanup_channels(hsotg);
>
> dwc2_host_disconnect(hsotg);
> +
> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) {
> + if (hsotg->lx_state != DWC2_L0)
> + usb_hcd_resume_roothub(hsotg->priv);
> +
> + hsotg->flags.b.port_connect_status_change = 1;
> + hsotg->flags.b.port_connect_status = 1;
> + }
I'd really rather not add the duplication unless you insist. To me it
makes it clearer to include the (small) refactor in the same patch.
If the refactor makes this change too big for an RC, then it's OK with
me to just skip this for the RC. It's not fixing a regression or
anything. I have no requirements to have this land in 4.4. It fixes
a bug and I thought that the fix was pretty small and safe (despite
having a diffstat that's bigger than the bare minimum). I will leave
it to your judgement.
>> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
>> index bda0b21b850f..03504ac2fecc 100644
>> --- a/drivers/usb/dwc2/hcd_intr.c
>> +++ b/drivers/usb/dwc2/hcd_intr.c
>> @@ -350,11 +350,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>> dev_vdbg(hsotg->dev,
>> "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
>> hprt0);
>> - if (hsotg->lx_state != DWC2_L0)
>> - usb_hcd_resume_root_hub(hsotg->priv);
>> -
>> - hsotg->flags.b.port_connect_status_change = 1;
>> - hsotg->flags.b.port_connect_status = 1;
>> + dwc2_hcd_connect(hsotg);
>
> unnecessary change.
>
> Do you agree or do you think 'force' is really necessary for this fix ?
As per above, John thought it was a good idea to really make the
disconnect happen upon module unload. If you disagree then we could
probably just totally remove the "dwc2_hcd_disconnect(hsotg);" from
the _dwc2_hcd_stop() function.
-Doug
Hi,
Doug Anderson <[email protected]> writes:
> Felipe,
>
> On Mon, Nov 16, 2015 at 9:44 AM, Felipe Balbi <[email protected]> wrote:
>>> extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
>>> #else
>>> static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
>>> { return 0; }
>>> -static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {}
>>> +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
>>> +static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
>>> static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>>> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>>> static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>> index 27daa42788b1..61601d16e233 100644
>>> --- a/drivers/usb/dwc2/core_intr.c
>>> +++ b/drivers/usb/dwc2/core_intr.c
>>> @@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
>>> dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n",
>>> hsotg->op_state);
>>> spin_unlock(&hsotg->lock);
>>> - dwc2_hcd_disconnect(hsotg);
>>> + dwc2_hcd_disconnect(hsotg, false);
>>
>> because force is unnecessary (it seems to be just an optimization to
>> me), this change is unnecessary.
>
> I added "force" in v2 of the patch in response to John's feedback to
> v1. He pointed out that when you unload the module when you have a
> device connected that my v1 patch would not properly disconnect the
> device (or, rather, it would disconnect it and then reconnect it).
>
> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force
> of "true".
There's no mention of this in commit log. It would be great to add a:
"while at that, also make sure that we don't try to detect a device on
module unload because of foo bar baz as pointed out by John Youn".
Or something along these lines.
>>> /**
>>> + * dwc2_hcd_connect() - Handles connect of the HCD
>>> + *
>>> + * @hsotg: Pointer to struct dwc2_hsotg
>>> + *
>>> + * Must be called with interrupt disabled and spinlock held
>>> + */
>>> +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg)
>>> +{
>>> + if (hsotg->lx_state != DWC2_L0)
>>> + usb_hcd_resume_root_hub(hsotg->priv);
>>> +
>>> + hsotg->flags.b.port_connect_status_change = 1;
>>> + hsotg->flags.b.port_connect_status = 1;
>>> +}
>>
>> you make no mention of why this is needed. This is basically a refactor,
>> not a fix.
>
> This new function is called from two places now, isn't it?
>
> Basically this is a little bit of code that used to be directly in
> dwc2_port_intr(). I still want it there, but I also want to call the
> same bit of code after a disconnect if I detect that the device has
> been inserted again.
I got that :-) But it's not mentioned in commit and it's apparently
unnecessary for fixing the bug :-) Another "we're also adding a new
hsotg_disconnect() function by means of refactoring to avoid code
duplication" would've been enough.
>>> @@ -315,6 +333,24 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
>>> dwc2_hcd_cleanup_channels(hsotg);
>>>
>>> dwc2_host_disconnect(hsotg);
>>> +
>>> + /*
>>> + * Add an extra check here to see if we're actually connected but
>>> + * we don't have a detection interrupt pending. This can happen if:
>>> + * 1. hardware sees connect
>>> + * 2. hardware sees disconnect
>>> + * 3. hardware sees connect
>>> + * 4. dwc2_port_intr() - clears connect interrupt
>>> + * 5. dwc2_handle_common_intr() - calls here
>>> + *
>>> + * Without the extra check here we will end calling disconnect
>>> + * and won't get any future interrupts to handle the connect.
>>> + */
>>> + if (!force) {
>>> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>> + if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS))
>>> + dwc2_hcd_connect(hsotg);
>>
>> what's inside this 'force' branch is what you really need to fix the
>> problem, right ? It seems like for the -rc cycle, all you need is:
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 571c21727ff9..967d1e686efe 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
>> */
>> void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
>> {
>> + u32 hprt0;
>> u32 intr;
>>
>> /* Set status flags for the hub driver */
>> @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
>> dwc2_hcd_cleanup_channels(hsotg);
>>
>> dwc2_host_disconnect(hsotg);
>> +
>> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>> + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) {
>> + if (hsotg->lx_state != DWC2_L0)
>> + usb_hcd_resume_roothub(hsotg->priv);
>> +
>> + hsotg->flags.b.port_connect_status_change = 1;
>> + hsotg->flags.b.port_connect_status = 1;
>> + }
>
> I'd really rather not add the duplication unless you insist. To me it
> makes it clearer to include the (small) refactor in the same patch.
>
> If the refactor makes this change too big for an RC, then it's OK with
> me to just skip this for the RC. It's not fixing a regression or
> anything. I have no requirements to have this land in 4.4. It fixes
> a bug and I thought that the fix was pretty small and safe (despite
> having a diffstat that's bigger than the bare minimum). I will leave
> it to your judgement.
let's at least modify commit log to make all these extra changes clear
that they are needed because of reason (a) or (b) or whatever. If you
just send a patch doing much more than it apparently should without no
mention as to why it was done this way, I can't know for sure those
changes are needed; next thing you know, Greg refuses to apply my pull
request because the change is too large :-)
We don't want that to happen :-)
>>> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
>>> index bda0b21b850f..03504ac2fecc 100644
>>> --- a/drivers/usb/dwc2/hcd_intr.c
>>> +++ b/drivers/usb/dwc2/hcd_intr.c
>>> @@ -350,11 +350,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>>> dev_vdbg(hsotg->dev,
>>> "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
>>> hprt0);
>>> - if (hsotg->lx_state != DWC2_L0)
>>> - usb_hcd_resume_root_hub(hsotg->priv);
>>> -
>>> - hsotg->flags.b.port_connect_status_change = 1;
>>> - hsotg->flags.b.port_connect_status = 1;
>>> + dwc2_hcd_connect(hsotg);
>>
>> unnecessary change.
>>
>> Do you agree or do you think 'force' is really necessary for this fix ?
>
> As per above, John thought it was a good idea to really make the
> disconnect happen upon module unload. If you disagree then we could
> probably just totally remove the "dwc2_hcd_disconnect(hsotg);" from
> the _dwc2_hcd_stop() function.
see above.
thanks
--
balbi
Felipe,
On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <[email protected]> wrote:
>> I added "force" in v2 of the patch in response to John's feedback to
>> v1. He pointed out that when you unload the module when you have a
>> device connected that my v1 patch would not properly disconnect the
>> device (or, rather, it would disconnect it and then reconnect it).
>>
>> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force
>> of "true".
>
> There's no mention of this in commit log. It would be great to add a:
>
> "while at that, also make sure that we don't try to detect a device on
> module unload because of foo bar baz as pointed out by John Youn".
>
> Or something along these lines.
...well, except that it's not new behavior. In other words:
* Without my patch at all: we don't try to detect a device on module unload.
* With my v1 patch: we (incorrectly) did try to detect a device on
module unload.
* With my v2 patch: we're back to not trying to detect a device on
module unload.
In other words: my v2 patch (correctly) doesn't change the behavior on
module unload. That's why I didn't mention it in the commit message.
It's in the "v2" changelog though.
I'll try to come up with something for the commit message though. See
below for new proposed commit message.
>>> you make no mention of why this is needed. This is basically a refactor,
>>> not a fix.
>>
>> This new function is called from two places now, isn't it?
>>
>> Basically this is a little bit of code that used to be directly in
>> dwc2_port_intr(). I still want it there, but I also want to call the
>> same bit of code after a disconnect if I detect that the device has
>> been inserted again.
>
> I got that :-) But it's not mentioned in commit and it's apparently
> unnecessary for fixing the bug :-) Another "we're also adding a new
> hsotg_disconnect() function by means of refactoring to avoid code
> duplication" would've been enough.
OK, sure.
>> I'd really rather not add the duplication unless you insist. To me it
>> makes it clearer to include the (small) refactor in the same patch.
>>
>> If the refactor makes this change too big for an RC, then it's OK with
>> me to just skip this for the RC. It's not fixing a regression or
>> anything. I have no requirements to have this land in 4.4. It fixes
>> a bug and I thought that the fix was pretty small and safe (despite
>> having a diffstat that's bigger than the bare minimum). I will leave
>> it to your judgement.
>
> let's at least modify commit log to make all these extra changes clear
> that they are needed because of reason (a) or (b) or whatever. If you
> just send a patch doing much more than it apparently should without no
> mention as to why it was done this way, I can't know for sure those
> changes are needed; next thing you know, Greg refuses to apply my pull
> request because the change is too large :-)
>
> We don't want that to happen :-)
Totally understand. It's your butt on the line for the pull request
to Greg, so definitely want to make sure you're comfortable with
anything you pass on. As always I definitely appreciate your reviews
and your time.
How about if we just add a "Notes" to the end of the patch
description. I can repost a patch or you can feel free to change the
description as per below (just let me know). ...so in total:
---
usb: dwc2: host: Fix missing device insertions
If you've got your interrupt signals bouncing a bit as you insert your
USB device, you might end up in a state when the device is connected but
the driver doesn't know it.
Specifically, the observed order is:
1. hardware sees connect
2. hardware sees disconnect
3. hardware sees connect
4. dwc2_port_intr() - clears connect interrupt
5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
Now you'll be stuck with the cable plugged in and no further interrupts
coming in but the driver will think we're disconnected.
We'll fix this by checking for the missing connect interrupt and
re-connecting after the disconnect is posted. We don't skip the
disconnect because if there is a transitory disconnect we really want to
de-enumerate and re-enumerate.
Notes:
1. As part of this change we add a "force" parameter to
dwc2_hcd_disconnect() so that when we're unloading the module we
avoid the new behavior. The need for this was pointed out by John
Youn.
2. The bit of code needed at the end of dwc2_hcd_disconnect() is
exactly the same bit of code from dwc2_port_intr(). To avoid
duplication, we refactor that code out into a new function
dwc2_hcd_connect().
-Doug
hi,
Doug Anderson <[email protected]> writes:
> Felipe,
>
> On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <[email protected]> wrote:
>>> I added "force" in v2 of the patch in response to John's feedback to
>>> v1. He pointed out that when you unload the module when you have a
>>> device connected that my v1 patch would not properly disconnect the
>>> device (or, rather, it would disconnect it and then reconnect it).
>>>
>>> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force
>>> of "true".
>>
>> There's no mention of this in commit log. It would be great to add a:
>>
>> "while at that, also make sure that we don't try to detect a device on
>> module unload because of foo bar baz as pointed out by John Youn".
>>
>> Or something along these lines.
>
> ...well, except that it's not new behavior. In other words:
>
> * Without my patch at all: we don't try to detect a device on module unload.
>
> * With my v1 patch: we (incorrectly) did try to detect a device on
> module unload.
>
> * With my v2 patch: we're back to not trying to detect a device on
> module unload.
>
> In other words: my v2 patch (correctly) doesn't change the behavior on
> module unload. That's why I didn't mention it in the commit message.
> It's in the "v2" changelog though.
>
>
> I'll try to come up with something for the commit message though. See
> below for new proposed commit message.
>
>
>>>> you make no mention of why this is needed. This is basically a refactor,
>>>> not a fix.
>>>
>>> This new function is called from two places now, isn't it?
>>>
>>> Basically this is a little bit of code that used to be directly in
>>> dwc2_port_intr(). I still want it there, but I also want to call the
>>> same bit of code after a disconnect if I detect that the device has
>>> been inserted again.
>>
>> I got that :-) But it's not mentioned in commit and it's apparently
>> unnecessary for fixing the bug :-) Another "we're also adding a new
>> hsotg_disconnect() function by means of refactoring to avoid code
>> duplication" would've been enough.
>
> OK, sure.
>
>
>>> I'd really rather not add the duplication unless you insist. To me it
>>> makes it clearer to include the (small) refactor in the same patch.
>>>
>>> If the refactor makes this change too big for an RC, then it's OK with
>>> me to just skip this for the RC. It's not fixing a regression or
>>> anything. I have no requirements to have this land in 4.4. It fixes
>>> a bug and I thought that the fix was pretty small and safe (despite
>>> having a diffstat that's bigger than the bare minimum). I will leave
>>> it to your judgement.
>>
>> let's at least modify commit log to make all these extra changes clear
>> that they are needed because of reason (a) or (b) or whatever. If you
>> just send a patch doing much more than it apparently should without no
>> mention as to why it was done this way, I can't know for sure those
>> changes are needed; next thing you know, Greg refuses to apply my pull
>> request because the change is too large :-)
>>
>> We don't want that to happen :-)
>
> Totally understand. It's your butt on the line for the pull request
> to Greg, so definitely want to make sure you're comfortable with
> anything you pass on. As always I definitely appreciate your reviews
> and your time.
>
>
> How about if we just add a "Notes" to the end of the patch
> description. I can repost a patch or you can feel free to change the
> description as per below (just let me know). ...so in total:
>
> ---
>
> usb: dwc2: host: Fix missing device insertions
>
> If you've got your interrupt signals bouncing a bit as you insert your
> USB device, you might end up in a state when the device is connected but
> the driver doesn't know it.
>
> Specifically, the observed order is:
> 1. hardware sees connect
> 2. hardware sees disconnect
> 3. hardware sees connect
> 4. dwc2_port_intr() - clears connect interrupt
> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> Now you'll be stuck with the cable plugged in and no further interrupts
> coming in but the driver will think we're disconnected.
>
> We'll fix this by checking for the missing connect interrupt and
> re-connecting after the disconnect is posted. We don't skip the
> disconnect because if there is a transitory disconnect we really want to
> de-enumerate and re-enumerate.
>
> Notes:
>
> 1. As part of this change we add a "force" parameter to
> dwc2_hcd_disconnect() so that when we're unloading the module we
> avoid the new behavior. The need for this was pointed out by John
> Youn.
> 2. The bit of code needed at the end of dwc2_hcd_disconnect() is
> exactly the same bit of code from dwc2_port_intr(). To avoid
> duplication, we refactor that code out into a new function
> dwc2_hcd_connect().
this should be enough, thanks for being so responsive
--
balbi
On Mon, 16 Nov 2015, Doug Anderson wrote:
> ---
>
> usb: dwc2: host: Fix missing device insertions
>
> If you've got your interrupt signals bouncing a bit as you insert your
> USB device, you might end up in a state when the device is connected but
> the driver doesn't know it.
>
> Specifically, the observed order is:
> 1. hardware sees connect
> 2. hardware sees disconnect
> 3. hardware sees connect
> 4. dwc2_port_intr() - clears connect interrupt
> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> Now you'll be stuck with the cable plugged in and no further interrupts
> coming in but the driver will think we're disconnected.
>
> We'll fix this by checking for the missing connect interrupt and
> re-connecting after the disconnect is posted. We don't skip the
> disconnect because if there is a transitory disconnect we really want to
> de-enumerate and re-enumerate.
Why do you need to do anything special here? Normally a driver's
interrupt handler should query the hardware status after clearing the
interrupt source. That way no transitions ever get missed.
In your example, at step 5 the dwc2 driver would check the port status
and see that it currently is connected. Therefore the driver would
pass a "connect status changed" event to the USB core and set the port
status to "connected". No extra checking is needed, and transitory
connects or disconnects get handled correctly.
Alan Stern
Alan,
On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <[email protected]> wrote:
> On Mon, 16 Nov 2015, Doug Anderson wrote:
>
>> ---
>>
>> usb: dwc2: host: Fix missing device insertions
>>
>> If you've got your interrupt signals bouncing a bit as you insert your
>> USB device, you might end up in a state when the device is connected but
>> the driver doesn't know it.
>>
>> Specifically, the observed order is:
>> 1. hardware sees connect
>> 2. hardware sees disconnect
>> 3. hardware sees connect
>> 4. dwc2_port_intr() - clears connect interrupt
>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>
>> Now you'll be stuck with the cable plugged in and no further interrupts
>> coming in but the driver will think we're disconnected.
>>
>> We'll fix this by checking for the missing connect interrupt and
>> re-connecting after the disconnect is posted. We don't skip the
>> disconnect because if there is a transitory disconnect we really want to
>> de-enumerate and re-enumerate.
>
> Why do you need to do anything special here? Normally a driver's
> interrupt handler should query the hardware status after clearing the
> interrupt source. That way no transitions ever get missed.
>
> In your example, at step 5 the dwc2 driver would check the port status
> and see that it currently is connected. Therefore the driver would
> pass a "connect status changed" event to the USB core and set the port
> status to "connected". No extra checking is needed, and transitory
> connects or disconnects get handled correctly.
Things are pretty ugly at the moment because the dwc2 interrupt
handler is split in two. There's dwc2_handle_hcd_intr() and
dwc2_handle_common_intr(). Both are registered for the same "shared"
IRQ. If I had to guess, I'd say that this is probably because someone
wanted to assign the ".irq" field in the "struct hc_driver" for the
host controller but that they also needed the other interrupt handler
to handle things shared between host and gadget mode.
In any case, one of these two interrupt routines handles connect and
the other disconnect. That's pretty ugly but means that you can't
just rely on standard techniques for keeping things in sync.
It would probably be a pretty reasonable idea to try to clean that up
more, but that would be a very major and intrusive change.
-Doug
Another point to note, which I think is what prevents the flow Alan
suggested from working, is this little snippet in DWC2's hub_control
GetPortStatus callback:
if (!hsotg->flags.b.port_connect_status) {
/*
* The port is disconnected, which means the core is
* either in device mode or it soon will be.
Just
* return 0's for the remainder of the port status
* since the port register can't be read if the core
* is in device mode.
*/
*(__le32 *)buf = cpu_to_le32(port_status);
break;
}
This is before it actually checks the register state of the port. So
it relies on dwc2_hcd_connect() and dwc2_hcd_disconnect() to be called
in the right order to give the correct answer for
USB_PORT_STAT_CONNECTION. This could maybe be improved but I don't
know what kind of weird interactions with device mode operation made
that snippet necessary in the first place.
On Mon, 16 Nov 2015, Doug Anderson wrote:
> Alan,
>
> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <[email protected]> wrote:
> > On Mon, 16 Nov 2015, Doug Anderson wrote:
> >
> >> ---
> >>
> >> usb: dwc2: host: Fix missing device insertions
> >>
> >> If you've got your interrupt signals bouncing a bit as you insert your
> >> USB device, you might end up in a state when the device is connected but
> >> the driver doesn't know it.
> >>
> >> Specifically, the observed order is:
> >> 1. hardware sees connect
> >> 2. hardware sees disconnect
> >> 3. hardware sees connect
> >> 4. dwc2_port_intr() - clears connect interrupt
> >> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
> >>
> >> Now you'll be stuck with the cable plugged in and no further interrupts
> >> coming in but the driver will think we're disconnected.
> >>
> >> We'll fix this by checking for the missing connect interrupt and
> >> re-connecting after the disconnect is posted. We don't skip the
> >> disconnect because if there is a transitory disconnect we really want to
> >> de-enumerate and re-enumerate.
> >
> > Why do you need to do anything special here? Normally a driver's
> > interrupt handler should query the hardware status after clearing the
> > interrupt source. That way no transitions ever get missed.
> >
> > In your example, at step 5 the dwc2 driver would check the port status
> > and see that it currently is connected. Therefore the driver would
> > pass a "connect status changed" event to the USB core and set the port
> > status to "connected". No extra checking is needed, and transitory
> > connects or disconnects get handled correctly.
>
> Things are pretty ugly at the moment because the dwc2 interrupt
> handler is split in two. There's dwc2_handle_hcd_intr() and
> dwc2_handle_common_intr(). Both are registered for the same "shared"
> IRQ. If I had to guess, I'd say that this is probably because someone
> wanted to assign the ".irq" field in the "struct hc_driver" for the
> host controller but that they also needed the other interrupt handler
> to handle things shared between host and gadget mode.
>
> In any case, one of these two interrupt routines handles connect and
> the other disconnect. That's pretty ugly but means that you can't
> just rely on standard techniques for keeping things in sync.
Okay, that is rather weird. Still, I don't see why it should matter.
Fundamentally there's no difference between a "connect" interrupt and a
"disconnect" interrupt. They should both do exactly the same things:
clear the interrupt source, post a "connection change" event, and set
the driver's connect status based on the hardware's current state.
The second and third parts can be handled by a shared subroutine.
If you think of these things as "connect changed" interrupts rather
than as "connect" and "disconnect" interrupts, it makes a lot of sense.
> It would probably be a pretty reasonable idea to try to clean that up
> more, but that would be a very major and intrusive change.
Maybe so -- I'll take your word for it since I'm not at all familiar
with the dwc2 code.
Alan Stern
On Mon, 16 Nov 2015, Julius Werner wrote:
> Another point to note, which I think is what prevents the flow Alan
> suggested from working, is this little snippet in DWC2's hub_control
> GetPortStatus callback:
>
> if (!hsotg->flags.b.port_connect_status) {
> /*
> * The port is disconnected, which means the core is
> * either in device mode or it soon will be.
> Just
> * return 0's for the remainder of the port status
> * since the port register can't be read if the core
> * is in device mode.
> */
> *(__le32 *)buf = cpu_to_le32(port_status);
> break;
> }
>
> This is before it actually checks the register state of the port. So
> it relies on dwc2_hcd_connect() and dwc2_hcd_disconnect() to be called
> in the right order to give the correct answer for
> USB_PORT_STAT_CONNECTION. This could maybe be improved but I don't
> know what kind of weird interactions with device mode operation made
> that snippet necessary in the first place.
Why does this test hsotg->flags.b.port_connect_status? What it really
needs to know is whether the core is in device mode. It should test
for that instead. Then it could accurately report the true connection
state whenever the core is in host mode, regardless of the order in
which dwc2_hcd_connect() and dwc2_hcd_disconnect() get called. No?
My guess would be that using the wrong test was simply a misguided
attempt at optimization.
Alan Stern
Alan,
On Mon, Nov 16, 2015 at 12:31 PM, Alan Stern <[email protected]> wrote:
> On Mon, 16 Nov 2015, Doug Anderson wrote:
>
>> Alan,
>>
>> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <[email protected]> wrote:
>> > On Mon, 16 Nov 2015, Doug Anderson wrote:
>> >
>> >> ---
>> >>
>> >> usb: dwc2: host: Fix missing device insertions
>> >>
>> >> If you've got your interrupt signals bouncing a bit as you insert your
>> >> USB device, you might end up in a state when the device is connected but
>> >> the driver doesn't know it.
>> >>
>> >> Specifically, the observed order is:
>> >> 1. hardware sees connect
>> >> 2. hardware sees disconnect
>> >> 3. hardware sees connect
>> >> 4. dwc2_port_intr() - clears connect interrupt
>> >> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>> >>
>> >> Now you'll be stuck with the cable plugged in and no further interrupts
>> >> coming in but the driver will think we're disconnected.
>> >>
>> >> We'll fix this by checking for the missing connect interrupt and
>> >> re-connecting after the disconnect is posted. We don't skip the
>> >> disconnect because if there is a transitory disconnect we really want to
>> >> de-enumerate and re-enumerate.
>> >
>> > Why do you need to do anything special here? Normally a driver's
>> > interrupt handler should query the hardware status after clearing the
>> > interrupt source. That way no transitions ever get missed.
>> >
>> > In your example, at step 5 the dwc2 driver would check the port status
>> > and see that it currently is connected. Therefore the driver would
>> > pass a "connect status changed" event to the USB core and set the port
>> > status to "connected". No extra checking is needed, and transitory
>> > connects or disconnects get handled correctly.
>>
>> Things are pretty ugly at the moment because the dwc2 interrupt
>> handler is split in two. There's dwc2_handle_hcd_intr() and
>> dwc2_handle_common_intr(). Both are registered for the same "shared"
>> IRQ. If I had to guess, I'd say that this is probably because someone
>> wanted to assign the ".irq" field in the "struct hc_driver" for the
>> host controller but that they also needed the other interrupt handler
>> to handle things shared between host and gadget mode.
>>
>> In any case, one of these two interrupt routines handles connect and
>> the other disconnect. That's pretty ugly but means that you can't
>> just rely on standard techniques for keeping things in sync.
>
> Okay, that is rather weird. Still, I don't see why it should matter.
>
> Fundamentally there's no difference between a "connect" interrupt and a
> "disconnect" interrupt. They should both do exactly the same things:
> clear the interrupt source, post a "connection change" event, and set
> the driver's connect status based on the hardware's current state.
> The second and third parts can be handled by a shared subroutine.
Ah, sorry I misunderstood. OK, fair enough. So you're saying that
the problem is that dwc2_hcd_disconnect() has a line that looks like
this:
hsotg->flags.b.port_connect_status = 0;
...and the dwc2_port_intr() has a line that looks like this:
hsotg->flags.b.port_connect_status = 1;
...and both should just query the status.
Do you think we should to block landing this patch on cleaning up how
dwc2 handles port_connect_status? I'm not sure what side effects
changing port_connect_status will have, so I'll need to test and dig a
bit...
I'm currently working on trying to fix the microframe scheduler and
was planning to post the next series of patches there pretty soon.
I'm also planning to keep digging to figure out how to overall
increase compatibility with devices (and compatibility with many
devices plugged in).
If it were up to me, I'd prefer to land this patch in either 4.4 or
4.5 since it does seem to work. ...then put seeing what we can do to
cleanup all of the port_connect_status on the todo list.
Julius points out in his response that there are comments saying that
HPRT0 can't be read in device mode. John: since you're setup to test
device mode, can you check if my patch (which now adds a read of
HPRT0) will cause problems? Maybe holding this off and keeping it out
of the RC is a good idea after all...
-Doug
On 11/16/2015 3:14 PM, Doug Anderson wrote:
> Alan,
>
> On Mon, Nov 16, 2015 at 12:31 PM, Alan Stern <[email protected]> wrote:
>> On Mon, 16 Nov 2015, Doug Anderson wrote:
>>
>>> Alan,
>>>
>>> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <[email protected]> wrote:
>>>> On Mon, 16 Nov 2015, Doug Anderson wrote:
>>>>
>>>>> ---
>>>>>
>>>>> usb: dwc2: host: Fix missing device insertions
>>>>>
>>>>> If you've got your interrupt signals bouncing a bit as you insert your
>>>>> USB device, you might end up in a state when the device is connected but
>>>>> the driver doesn't know it.
>>>>>
>>>>> Specifically, the observed order is:
>>>>> 1. hardware sees connect
>>>>> 2. hardware sees disconnect
>>>>> 3. hardware sees connect
>>>>> 4. dwc2_port_intr() - clears connect interrupt
>>>>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>>>
>>>>> Now you'll be stuck with the cable plugged in and no further interrupts
>>>>> coming in but the driver will think we're disconnected.
>>>>>
>>>>> We'll fix this by checking for the missing connect interrupt and
>>>>> re-connecting after the disconnect is posted. We don't skip the
>>>>> disconnect because if there is a transitory disconnect we really want to
>>>>> de-enumerate and re-enumerate.
>>>>
>>>> Why do you need to do anything special here? Normally a driver's
>>>> interrupt handler should query the hardware status after clearing the
>>>> interrupt source. That way no transitions ever get missed.
>>>>
>>>> In your example, at step 5 the dwc2 driver would check the port status
>>>> and see that it currently is connected. Therefore the driver would
>>>> pass a "connect status changed" event to the USB core and set the port
>>>> status to "connected". No extra checking is needed, and transitory
>>>> connects or disconnects get handled correctly.
>>>
>>> Things are pretty ugly at the moment because the dwc2 interrupt
>>> handler is split in two. There's dwc2_handle_hcd_intr() and
>>> dwc2_handle_common_intr(). Both are registered for the same "shared"
>>> IRQ. If I had to guess, I'd say that this is probably because someone
>>> wanted to assign the ".irq" field in the "struct hc_driver" for the
>>> host controller but that they also needed the other interrupt handler
>>> to handle things shared between host and gadget mode.
>>>
>>> In any case, one of these two interrupt routines handles connect and
>>> the other disconnect. That's pretty ugly but means that you can't
>>> just rely on standard techniques for keeping things in sync.
>>
>> Okay, that is rather weird. Still, I don't see why it should matter.
>>
>> Fundamentally there's no difference between a "connect" interrupt and a
>> "disconnect" interrupt. They should both do exactly the same things:
>> clear the interrupt source, post a "connection change" event, and set
>> the driver's connect status based on the hardware's current state.
>> The second and third parts can be handled by a shared subroutine.
>
> Ah, sorry I misunderstood. OK, fair enough. So you're saying that
> the problem is that dwc2_hcd_disconnect() has a line that looks like
> this:
>
> hsotg->flags.b.port_connect_status = 0;
>
> ...and the dwc2_port_intr() has a line that looks like this:
>
> hsotg->flags.b.port_connect_status = 1;
>
> ...and both should just query the status.
>
>
> Do you think we should to block landing this patch on cleaning up how
> dwc2 handles port_connect_status? I'm not sure what side effects
> changing port_connect_status will have, so I'll need to test and dig a
> bit...
>
> I'm currently working on trying to fix the microframe scheduler and
> was planning to post the next series of patches there pretty soon.
> I'm also planning to keep digging to figure out how to overall
> increase compatibility with devices (and compatibility with many
> devices plugged in).
>
> If it were up to me, I'd prefer to land this patch in either 4.4 or
> 4.5 since it does seem to work. ...then put seeing what we can do to
> cleanup all of the port_connect_status on the todo list.
That sound good to me. Though I'd prefer to see this series
queued for 4.5 for more testing.
>
> Julius points out in his response that there are comments saying that
> HPRT0 can't be read in device mode. John: since you're setup to test
> device mode, can you check if my patch (which now adds a read of
> HPRT0) will cause problems? Maybe holding this off and keeping it out
> of the RC is a good idea after all...
>
Yup it's only available in host mode. The same with all the
host-mode registers. You will get a ModeMis interrupt if you
try to access them in device mode.
I gave my test-by on the v2 patches earlier, no problems here.
John
John,
On Mon, Nov 16, 2015 at 5:53 PM, John Youn <[email protected]> wrote:
> Yup it's only available in host mode. The same with all the
> host-mode registers. You will get a ModeMis interrupt if you
> try to access them in device mode.
>
> I gave my test-by on the v2 patches earlier, no problems here.
Yup, I appreciate it! I just wanted to confirm that you tested this
particular case in gadget mode. ;)
On Mon, 16 Nov 2015, Doug Anderson wrote:
> > Fundamentally there's no difference between a "connect" interrupt and a
> > "disconnect" interrupt. They should both do exactly the same things:
> > clear the interrupt source, post a "connection change" event, and set
> > the driver's connect status based on the hardware's current state.
> > The second and third parts can be handled by a shared subroutine.
>
> Ah, sorry I misunderstood. OK, fair enough. So you're saying that
> the problem is that dwc2_hcd_disconnect() has a line that looks like
> this:
>
> hsotg->flags.b.port_connect_status = 0;
>
> ...and the dwc2_port_intr() has a line that looks like this:
>
> hsotg->flags.b.port_connect_status = 1;
>
> ...and both should just query the status.
Well, I don't know how the driver uses flags.b.port_connect_status. In
principle it could do away with that flag completely and always query
the hardware status.
> Do you think we should to block landing this patch on cleaning up how
> dwc2 handles port_connect_status? I'm not sure what side effects
> changing port_connect_status will have, so I'll need to test and dig a
> bit...
>
> I'm currently working on trying to fix the microframe scheduler and
> was planning to post the next series of patches there pretty soon.
> I'm also planning to keep digging to figure out how to overall
> increase compatibility with devices (and compatibility with many
> devices plugged in).
>
> If it were up to me, I'd prefer to land this patch in either 4.4 or
> 4.5 since it does seem to work. ...then put seeing what we can do to
> cleanup all of the port_connect_status on the todo list.
It's up to you guys. All I've been doing here is pointing out that
your proposed approach didn't seem like the best.
Alan Stern
Alan,
On Tue, Nov 17, 2015 at 7:40 AM, Alan Stern <[email protected]> wrote:
> On Mon, 16 Nov 2015, Doug Anderson wrote:
>
>> > Fundamentally there's no difference between a "connect" interrupt and a
>> > "disconnect" interrupt. They should both do exactly the same things:
>> > clear the interrupt source, post a "connection change" event, and set
>> > the driver's connect status based on the hardware's current state.
>> > The second and third parts can be handled by a shared subroutine.
>>
>> Ah, sorry I misunderstood. OK, fair enough. So you're saying that
>> the problem is that dwc2_hcd_disconnect() has a line that looks like
>> this:
>>
>> hsotg->flags.b.port_connect_status = 0;
>>
>> ...and the dwc2_port_intr() has a line that looks like this:
>>
>> hsotg->flags.b.port_connect_status = 1;
>>
>> ...and both should just query the status.
>
> Well, I don't know how the driver uses flags.b.port_connect_status. In
> principle it could do away with that flag completely and always query
> the hardware status.
>
>> Do you think we should to block landing this patch on cleaning up how
>> dwc2 handles port_connect_status? I'm not sure what side effects
>> changing port_connect_status will have, so I'll need to test and dig a
>> bit...
>>
>> I'm currently working on trying to fix the microframe scheduler and
>> was planning to post the next series of patches there pretty soon.
>> I'm also planning to keep digging to figure out how to overall
>> increase compatibility with devices (and compatibility with many
>> devices plugged in).
>>
>> If it were up to me, I'd prefer to land this patch in either 4.4 or
>> 4.5 since it does seem to work. ...then put seeing what we can do to
>> cleanup all of the port_connect_status on the todo list.
>
> It's up to you guys. All I've been doing here is pointing out that
> your proposed approach didn't seem like the best.
Thanks! Just wanted to make sure you know that I'm very very
appreciative of your reviews and suggestions here. Having someone
intimately familiar with how other USB host drivers work that's
willing to point out how dwc2 can do things better will be very
helpful in helping dwc2 grow.
-Doug
On 11/16/2015 9:22 AM, Doug Anderson wrote:
> Felipe,
>
> On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi <[email protected]> wrote:
>>
>> Hi,
>>
>> Douglas Anderson <[email protected]> writes:
>>> In general it is wise to clear interrupts before processing them. If
>>> you don't do that, you can get:
>>> 1. Interrupt happens
>>> 2. You look at system state and process interrupt
>>> 3. A new interrupt happens
>>> 4. You clear interrupt without processing it.
>>>
>>> This patch was actually a first attempt to fix missing device insertions
>>> as described in (usb: dwc2: host: Fix missing device insertions) and it
>>> did solve some of the signal bouncing problems but not all of
>>> them (which is why I submitted the other patch). Specifically, this
>>> patch itself would sometimes change:
>>> 1. hardware sees connect
>>> 2. hardware sees disconnect
>>> 3. hardware sees connect
>>> 4. dwc2_port_intr() - clears connect interrupt
>>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>
>>> ...to:
>>> 1. hardware sees connect
>>> 2. hardware sees disconnect
>>> 3. dwc2_port_intr() - clears connect interrupt
>>> 4. hardware sees connect
>>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>
>>> ...but with different timing then sometimes we'd still miss cable
>>> insertions.
>>>
>>> In any case, though this patch doesn't fix any (known) problems, it
>>> still seems wise as a general policy to clear interrupt before handling
>>> them.
>>>
>>> Signed-off-by: Douglas Anderson <[email protected]>
>>> ---
>>> Changes in v2: None
>>>
>>> drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
>>> drivers/usb/dwc2/hcd_intr.c | 16 ++++++-------
>>> 2 files changed, 35 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>> index 61601d16e233..2a166b7eec41 100644
>>> --- a/drivers/usb/dwc2/core_intr.c
>>> +++ b/drivers/usb/dwc2/core_intr.c
>>> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
>>> */
>>> static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>>> {
>>> - u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>> + u32 hprt0;
>>>
>>> + /* Clear interrupt */
>>> + dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>> +
>>> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>> if (hprt0 & HPRT0_ENACHG) {
>>> hprt0 &= ~HPRT0_ENA;
>>> dwc2_writel(hprt0, hsotg->regs + HPRT0);
>>> }
>>> -
>>> - /* Clear interrupt */
>>> - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>
>> isn't this a regression ? You're first clearing the interrupts and only
>> then reading to check what's pending, however, what's pending has just
>> been cleared. Seems like this should be:
>>
>> hprt0 = dwc2_readl(HPRT0);
>> dwc2_writeal(PRTINT, GINTSTS);
>
> Actually, we could probably remove the setting of GINTSTS_PRTINT
> completely. The docs I have say that the GINTSTS_PRTINT is read only
> and that:
>
>> The core sets this bit to indicate a change in port status of one of the
>> DWC_otg core ports in Host mode. The application must read the
>> Host Port Control and Status (HPRT) register to determine the exact
>> event that caused this interrupt. The application must clear the
>> appropriate status bit in the Host Port Control and Status register to
>> clear this bit.
>
> ...so writing PRTINT is probably useless, but John can confirm.
>
Yup, it seems it can be removed.
John
Hi,
On Wed, Nov 18, 2015 at 5:43 PM, John Youn <[email protected]> wrote:
> On 11/16/2015 9:22 AM, Doug Anderson wrote:
>> Felipe,
>>
>> On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> Douglas Anderson <[email protected]> writes:
>>>> In general it is wise to clear interrupts before processing them. If
>>>> you don't do that, you can get:
>>>> 1. Interrupt happens
>>>> 2. You look at system state and process interrupt
>>>> 3. A new interrupt happens
>>>> 4. You clear interrupt without processing it.
>>>>
>>>> This patch was actually a first attempt to fix missing device insertions
>>>> as described in (usb: dwc2: host: Fix missing device insertions) and it
>>>> did solve some of the signal bouncing problems but not all of
>>>> them (which is why I submitted the other patch). Specifically, this
>>>> patch itself would sometimes change:
>>>> 1. hardware sees connect
>>>> 2. hardware sees disconnect
>>>> 3. hardware sees connect
>>>> 4. dwc2_port_intr() - clears connect interrupt
>>>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>>
>>>> ...to:
>>>> 1. hardware sees connect
>>>> 2. hardware sees disconnect
>>>> 3. dwc2_port_intr() - clears connect interrupt
>>>> 4. hardware sees connect
>>>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>>
>>>> ...but with different timing then sometimes we'd still miss cable
>>>> insertions.
>>>>
>>>> In any case, though this patch doesn't fix any (known) problems, it
>>>> still seems wise as a general policy to clear interrupt before handling
>>>> them.
>>>>
>>>> Signed-off-by: Douglas Anderson <[email protected]>
>>>> ---
>>>> Changes in v2: None
>>>>
>>>> drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
>>>> drivers/usb/dwc2/hcd_intr.c | 16 ++++++-------
>>>> 2 files changed, 35 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>> index 61601d16e233..2a166b7eec41 100644
>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
>>>> */
>>>> static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>>>> {
>>>> - u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>>> + u32 hprt0;
>>>>
>>>> + /* Clear interrupt */
>>>> + dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>>> +
>>>> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>>> if (hprt0 & HPRT0_ENACHG) {
>>>> hprt0 &= ~HPRT0_ENA;
>>>> dwc2_writel(hprt0, hsotg->regs + HPRT0);
>>>> }
>>>> -
>>>> - /* Clear interrupt */
>>>> - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>>
>>> isn't this a regression ? You're first clearing the interrupts and only
>>> then reading to check what's pending, however, what's pending has just
>>> been cleared. Seems like this should be:
>>>
>>> hprt0 = dwc2_readl(HPRT0);
>>> dwc2_writeal(PRTINT, GINTSTS);
>>
>> Actually, we could probably remove the setting of GINTSTS_PRTINT
>> completely. The docs I have say that the GINTSTS_PRTINT is read only
>> and that:
>>
>>> The core sets this bit to indicate a change in port status of one of the
>>> DWC_otg core ports in Host mode. The application must read the
>>> Host Port Control and Status (HPRT) register to determine the exact
>>> event that caused this interrupt. The application must clear the
>>> appropriate status bit in the Host Port Control and Status register to
>>> clear this bit.
>>
>> ...so writing PRTINT is probably useless, but John can confirm.
>>
>
> Yup, it seems it can be removed.
How do you guys want this handled? Should I send up a new version of
this patch? ...or should I send a followon patch that does this
removal?
-Doug
Hi,
Doug Anderson <[email protected]> writes:
>>>> isn't this a regression ? You're first clearing the interrupts and only
>>>> then reading to check what's pending, however, what's pending has just
>>>> been cleared. Seems like this should be:
>>>>
>>>> hprt0 = dwc2_readl(HPRT0);
>>>> dwc2_writeal(PRTINT, GINTSTS);
>>>
>>> Actually, we could probably remove the setting of GINTSTS_PRTINT
>>> completely. The docs I have say that the GINTSTS_PRTINT is read only
>>> and that:
>>>
>>>> The core sets this bit to indicate a change in port status of one of the
>>>> DWC_otg core ports in Host mode. The application must read the
>>>> Host Port Control and Status (HPRT) register to determine the exact
>>>> event that caused this interrupt. The application must clear the
>>>> appropriate status bit in the Host Port Control and Status register to
>>>> clear this bit.
>>>
>>> ...so writing PRTINT is probably useless, but John can confirm.
>>>
>>
>> Yup, it seems it can be removed.
>
> How do you guys want this handled? Should I send up a new version of
> this patch? ...or should I send a followon patch that does this
> removal?
I'll leave the final decision to John, but my opinion is that a new
version of the patch would be preferrable.
--
balbi
On 11/19/2015 10:19 AM, Felipe Balbi wrote:
>
> Hi,
>
> Doug Anderson <[email protected]> writes:
>>>>> isn't this a regression ? You're first clearing the interrupts and only
>>>>> then reading to check what's pending, however, what's pending has just
>>>>> been cleared. Seems like this should be:
>>>>>
>>>>> hprt0 = dwc2_readl(HPRT0);
>>>>> dwc2_writeal(PRTINT, GINTSTS);
>>>>
>>>> Actually, we could probably remove the setting of GINTSTS_PRTINT
>>>> completely. The docs I have say that the GINTSTS_PRTINT is read only
>>>> and that:
>>>>
>>>>> The core sets this bit to indicate a change in port status of one of the
>>>>> DWC_otg core ports in Host mode. The application must read the
>>>>> Host Port Control and Status (HPRT) register to determine the exact
>>>>> event that caused this interrupt. The application must clear the
>>>>> appropriate status bit in the Host Port Control and Status register to
>>>>> clear this bit.
>>>>
>>>> ...so writing PRTINT is probably useless, but John can confirm.
>>>>
>>>
>>> Yup, it seems it can be removed.
>>
>> How do you guys want this handled? Should I send up a new version of
>> this patch? ...or should I send a followon patch that does this
>> removal?
>
> I'll leave the final decision to John, but my opinion is that a new
> version of the patch would be preferrable.
>
Hi Doug,
Could you resend with the change?
Regards,
John