diff mbox series

usb: chipidea: core: complement wakeup support for usb role switch

Message ID 20220812105719.143556-1-xu.yang_2@nxp.com
State New
Headers show
Series usb: chipidea: core: complement wakeup support for usb role switch | expand

Commit Message

Xu Yang Aug. 12, 2022, 10:57 a.m. UTC 
In current design, ci_usb_role_switch_set() will call pm_runtime_get_sync()
firstly, then handle role switch events. But pm_runtime_get_sync() may fail
to resume controller sometimes. This may happen when doing system suspend
and enabled typec wakeup source as below:
  1. starting system suspend, controller is suspended by runtime pm before.
  2. somehow controller get resumed by runtime pm.
  3. ci_suspend() is called. runtime_status = RPM_ACTIVE now.
  4. ci_usb_role_switch_set() is called due to role switch events.
  5. pm_runtime_get_sync() return 1.

This is because pm.runtime_status is still RPM_ACTIVE after ci_suspend().
Then the driver execute wakeup operations in ci_irq(). So there is a need
to call ci_irq() again like extcon do.

Fixes: 876d4e1e8298 ("usb: chipidea: core: add wakeup support for extcon")
Cc: <stable@vger.kernel.org>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/chipidea/core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Peter Chen Aug. 18, 2022, 10:43 p.m. UTC | #1 
On 22-08-12 18:57:19, Xu Yang wrote:
> In current design, ci_usb_role_switch_set() will call pm_runtime_get_sync()
> firstly, then handle role switch events. But pm_runtime_get_sync() may fail
> to resume controller sometimes. This may happen when doing system suspend
> and enabled typec wakeup source as below:
>   1. starting system suspend, controller is suspended by runtime pm before.
>   2. somehow controller get resumed by runtime pm.
>   3. ci_suspend() is called. runtime_status = RPM_ACTIVE now.
>   4. ci_usb_role_switch_set() is called due to role switch events.
>   5. pm_runtime_get_sync() return 1.
> 
> This is because pm.runtime_status is still RPM_ACTIVE after ci_suspend().
> Then the driver execute wakeup operations in ci_irq(). So there is a need
> to call ci_irq() again like extcon do.

You mean role switch happens after ci_lpm is 1, you need to execute
ci_irq twice? I have not run chipidea platform these years, could @Jun
Li help review this patch first?

> 
> Fixes: 876d4e1e8298 ("usb: chipidea: core: add wakeup support for extcon")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  drivers/usb/chipidea/core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 6330fa911792..886b68e45826 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -1305,12 +1305,14 @@ static void ci_extcon_wakeup_int(struct ci_hdrc *ci)
>  	cable_id = &ci->platdata->id_extcon;
>  	cable_vbus = &ci->platdata->vbus_extcon;
>  
> -	if (!IS_ERR(cable_id->edev) && ci->is_otg &&
> -		(otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS))
> +	if ((!IS_ERR(cable_id->edev) || !IS_ERR(ci->role_switch))
> +		&& ci->is_otg && (otgsc & OTGSC_IDIE)
> +		&& (otgsc & OTGSC_IDIS))
>  		ci_irq(ci);
>  
> -	if (!IS_ERR(cable_vbus->edev) && ci->is_otg &&
> -		(otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS))
> +	if ((!IS_ERR(cable_vbus->edev) || !IS_ERR(ci->role_switch))
> +		&& ci->is_otg && (otgsc & OTGSC_BSVIE)
> +		&& (otgsc & OTGSC_BSVIS))
>  		ci_irq(ci);
>  }

With your change, ci_irq needs to execute every time at role-switch case
every time, please confirm it is expected
Jun Li Aug. 20, 2022, 2:05 p.m. UTC | #2 
Hi,

> -----Original Message-----
> From: Peter Chen <peter.chen@kernel.org>
> Sent: Friday, August 19, 2022 6:43 AM
> To: Xu Yang <xu.yang_2@nxp.com>
> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com>
> Subject: Re: [PATCH] usb: chipidea: core: complement wakeup support for usb
> role switch
> 
> On 22-08-12 18:57:19, Xu Yang wrote:
> > In current design, ci_usb_role_switch_set() will call
> > pm_runtime_get_sync() firstly, then handle role switch events. But
> > pm_runtime_get_sync() may fail to resume controller sometimes. This
> > may happen when doing system suspend and enabled typec wakeup source as
> below:
> >   1. starting system suspend, controller is suspended by runtime pm before.
> >   2. somehow controller get resumed by runtime pm.
> >   3. ci_suspend() is called. runtime_status = RPM_ACTIVE now.
> >   4. ci_usb_role_switch_set() is called due to role switch events.
> >   5. pm_runtime_get_sync() return 1.
> >
> > This is because pm.runtime_status is still RPM_ACTIVE after ci_suspend().
> > Then the driver execute wakeup operations in ci_irq(). So there is a
> > need to call ci_irq() again like extcon do.
> 
> You mean role switch happens after ci_lpm is 1, you need to execute ci_irq
> twice? I have not run chipidea platform these years, could @Jun Li help review
> this patch first?

I do see role switch class cannot simply reuse the extcon solution in this case,
I had a talk with Xu and I think he will send out v2.

Thanks
Li Jun
> 
> >
> > Fixes: 876d4e1e8298 ("usb: chipidea: core: add wakeup support for
> > extcon")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/chipidea/core.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index 6330fa911792..886b68e45826 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -1305,12 +1305,14 @@ static void ci_extcon_wakeup_int(struct ci_hdrc
> *ci)
> >  	cable_id = &ci->platdata->id_extcon;
> >  	cable_vbus = &ci->platdata->vbus_extcon;
> >
> > -	if (!IS_ERR(cable_id->edev) && ci->is_otg &&
> > -		(otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS))
> > +	if ((!IS_ERR(cable_id->edev) || !IS_ERR(ci->role_switch))
> > +		&& ci->is_otg && (otgsc & OTGSC_IDIE)
> > +		&& (otgsc & OTGSC_IDIS))
> >  		ci_irq(ci);
> >
> > -	if (!IS_ERR(cable_vbus->edev) && ci->is_otg &&
> > -		(otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS))
> > +	if ((!IS_ERR(cable_vbus->edev) || !IS_ERR(ci->role_switch))
> > +		&& ci->is_otg && (otgsc & OTGSC_BSVIE)
> > +		&& (otgsc & OTGSC_BSVIS))
> >  		ci_irq(ci);
> >  }
> 
> With your change, ci_irq needs to execute every time at role-switch case
> every time, please confirm it is expected
> 
> --
> 
> Thanks,
> Peter Chen
diff mbox series

Patch

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 6330fa911792..886b68e45826 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -1305,12 +1305,14 @@  static void ci_extcon_wakeup_int(struct ci_hdrc *ci)
 	cable_id = &ci->platdata->id_extcon;
 	cable_vbus = &ci->platdata->vbus_extcon;
 
-	if (!IS_ERR(cable_id->edev) && ci->is_otg &&
-		(otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS))
+	if ((!IS_ERR(cable_id->edev) || !IS_ERR(ci->role_switch))
+		&& ci->is_otg && (otgsc & OTGSC_IDIE)
+		&& (otgsc & OTGSC_IDIS))
 		ci_irq(ci);
 
-	if (!IS_ERR(cable_vbus->edev) && ci->is_otg &&
-		(otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS))
+	if ((!IS_ERR(cable_vbus->edev) || !IS_ERR(ci->role_switch))
+		&& ci->is_otg && (otgsc & OTGSC_BSVIE)
+		&& (otgsc & OTGSC_BSVIS))
 		ci_irq(ci);
 }