usb: chipidea: move extcon initial state read

usb: chipidea: move extcon initial state read

[Jonathan]

Any extcon events between the initial state read and ci_extcon_register are
"lost". This patch doesn't fix the issue entirely but reduces the chance of
the controller entering a bad state.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
  drivers/usb/chipidea/core.c | 35 +++++++++++++++++++----------------
  1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 33ae87f..a1fb2fb 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -704,25 +704,9 @@ static int ci_get_platdata(struct device *dev,
  	cable->nb.notifier_call = ci_cable_notifier;
  	cable->edev = ext_vbus;

-	if (!IS_ERR(ext_vbus)) {
-		ret = extcon_get_state(cable->edev, EXTCON_USB);
-		if (ret)
-			cable->connected = true;
-		else
-			cable->connected = false;
-	}
-
  	cable = &platdata->id_extcon;
  	cable->nb.notifier_call = ci_cable_notifier;
  	cable->edev = ext_id;
-
-	if (!IS_ERR(ext_id)) {
-		ret = extcon_get_state(cable->edev, EXTCON_USB_HOST);
-		if (ret)
-			cable->connected = true;
-		else
-			cable->connected = false;
-	}
  	return 0;
  }

@@ -892,6 +876,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
  {
  	struct device	*dev = &pdev->dev;
  	struct ci_hdrc	*ci;
+	struct ci_hdrc_cable *cable;
  	struct resource	*res;
  	void __iomem	*base;
  	int		ret;
@@ -1009,6 +994,24 @@ static int ci_hdrc_probe(struct platform_device *pdev)
  		}
  	}

+	cable = &ci->platdata->vbus_extcon;
+	if (!IS_ERR(cable->edev)) {
+		ret = extcon_get_state(cable->edev, EXTCON_USB);
+		if (ret)
+			cable->connected = true;
+		else
+			cable->connected = false;
+	}
+
+	cable = &ci->platdata->id_extcon;
+	if (!IS_ERR(cable->edev)) {
+		ret = extcon_get_state(cable->edev, EXTCON_USB_HOST);
+		if (ret)
+			cable->connected = true;
+		else
+			cable->connected = false;
+	}
+
  	if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
  		if (ci->is_otg) {
  			ci->role = ci_otg_role(ci);

usb: chipidea: move extcon initial state read

[Greg Kroah-Hartman]

On Thu, Mar 15, 2018 at 11:41:23AM -0400, Jonathan wrote:
> Any extcon events between the initial state read and ci_extcon_register are
> "lost". This patch doesn't fix the issue entirely but reduces the chance of
> the controller entering a bad state.
> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

This doesn't match your "From:" line in your email :(
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: chipidea: move extcon initial state read

[Peter Chen]

> 
> Any extcon events between the initial state read and ci_extcon_register are "lost".
> This patch doesn't fix the issue entirely but reduces the chance of the controller
> entering a bad state.

It seems you have already known the rare cases the lost for extcon exents,
why not fix it at this patch?

Peter
> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>   drivers/usb/chipidea/core.c | 35 +++++++++++++++++++----------------
>   1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
> 33ae87f..a1fb2fb 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -704,25 +704,9 @@ static int ci_get_platdata(struct device *dev,
>   	cable->nb.notifier_call = ci_cable_notifier;
>   	cable->edev = ext_vbus;
> 
> -	if (!IS_ERR(ext_vbus)) {
> -		ret = extcon_get_state(cable->edev, EXTCON_USB);
> -		if (ret)
> -			cable->connected = true;
> -		else
> -			cable->connected = false;
> -	}
> -
>   	cable = &platdata->id_extcon;
>   	cable->nb.notifier_call = ci_cable_notifier;
>   	cable->edev = ext_id;
> -
> -	if (!IS_ERR(ext_id)) {
> -		ret = extcon_get_state(cable->edev, EXTCON_USB_HOST);
> -		if (ret)
> -			cable->connected = true;
> -		else
> -			cable->connected = false;
> -	}
>   	return 0;
>   }
> 
> @@ -892,6 +876,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>   {
>   	struct device	*dev = &pdev->dev;
>   	struct ci_hdrc	*ci;
> +	struct ci_hdrc_cable *cable;
>   	struct resource	*res;
>   	void __iomem	*base;
>   	int		ret;
> @@ -1009,6 +994,24 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>   		}
>   	}
> 
> +	cable = &ci->platdata->vbus_extcon;
> +	if (!IS_ERR(cable->edev)) {
> +		ret = extcon_get_state(cable->edev, EXTCON_USB);
> +		if (ret)
> +			cable->connected = true;
> +		else
> +			cable->connected = false;
> +	}
> +
> +	cable = &ci->platdata->id_extcon;
> +	if (!IS_ERR(cable->edev)) {
> +		ret = extcon_get_state(cable->edev, EXTCON_USB_HOST);
> +		if (ret)
> +			cable->connected = true;
> +		else
> +			cable->connected = false;
> +	}
> +
>   	if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
>   		if (ci->is_otg) {
>   			ci->role = ci_otg_role(ci);
> --
> 2.9.3
> 
> On 02/23/2018 10:06 PM, Peter Chen wrote:
> >
> >> Subject: Bugged initial state with Chipidea driver
> >>
> >> Hi,
> >>
> >> In the Chipidea driver the extcon state is read early in the
> >> initialization and notifications for extcon events are only enabled
> >> at the end of the initialization. If the extcon state changes in
> >> between the driver will miss the change in extcon state and be a bad state.
> >
> > Yes, it is the limitation, we need to move ci_extcon_register and
> > update extcon state in chipidea driver before we read ID or VBUS value
> > (through extcon)
> >
> >> Here is what happens in my case:
> >> 1. EXTCON_USB_HOST=1, EXTCON_USB=1
> >
> > Why the two events are both 1 at same time?
> >
> >> 2. USB driver reads initial extcon state 3. EXTCON_USB_HOST=0 4. USB
> >> driver choses initial role 5. USB driver enables extcon notifiers
> >> then the USB driver is stuck in host mode until the EXTCON_USB_HOST
> >> value cycles again, when it should be in gadget mode. This happens
> >> every other boot because my EXTCON_USB_HOST value is unreliable
> >> during init.
> >>
> >> I have implemented a temporary solution for myself but if you propose
> >> a proper solution I can implement it and submit a patch.
> >>
> >
> > Feel free to submit your patch.
> >
> >> Additionally, I guess this could technically happen without extcon,
> >> but it is much less likely.
> >>
> >
> > For non-extcon solution, the register (otgsc) reflect real-time vbus
> > and id value, so it should be without problem you meet.
> >
> > Peter
> >