[PATCH v3 0/4] usb: typec: ucsi: allow retry to find role switch

[PATCH v3 0/4] usb: typec: ucsi: allow retry to find role switch

[Linyu Yuan]

when one role switch module built as module, it may load later then
ucsi, this change series allow retry from ucsi.

v2: improve ucsi_connector_clean() to cover all condition,
    and add one new change to avoid allocate one unused connector.

v3: fix comment from ucsi maintainer.

Linyu Yuan (4):
  usb: typec: ucsi: set con->port to NULL when register port fail
  usb: typec: ucsi: add a common function ucsi_unregister_connectors()
  usb: typec: ucsi: do not allocate one extra unused connector
  usb: typec: ucsi: retry find role swithch when module load late

 drivers/usb/typec/ucsi/ucsi.c | 82 ++++++++++++++++++++++++-------------------
 drivers/usb/typec/ucsi/ucsi.h |  2 +-
 2 files changed, 47 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] usb: typec: ucsi: set con->port to NULL when register port fail

[Linyu Yuan]

As con->port will be used in error path of ucsi_init(),
it should be NULL or valid.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: no change

 drivers/usb/typec/ucsi/ucsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index f0c2fa1..77ac0b7 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1100,6 +1100,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	con->port = typec_register_port(ucsi->dev, cap);
 	if (IS_ERR(con->port)) {
 		ret = PTR_ERR(con->port);
+		con->port = NULL;
 		goto out;
 	}
 
-- 
2.7.4

Re: [PATCH v3 1/4] usb: typec: ucsi: set con->port to NULL when register port fail

[Heikki Krogerus]

On Wed, Apr 13, 2022 at 05:58:08PM +0800, Linyu Yuan wrote:
> As con->port will be used in error path of ucsi_init(),
> it should be NULL or valid.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: no change
> v3: no change
> 
>  drivers/usb/typec/ucsi/ucsi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index f0c2fa1..77ac0b7 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1100,6 +1100,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  	con->port = typec_register_port(ucsi->dev, cap);
>  	if (IS_ERR(con->port)) {
>  		ret = PTR_ERR(con->port);
> +		con->port = NULL;
>  		goto out;
>  	}

Please merge this into the next patch.

thanks,

-- 
heikki

RE: [PATCH v3 1/4] usb: typec: ucsi: set con->port to NULL when register port fail

[Linyu Yuan (QUIC)]

> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Wednesday, April 13, 2022 7:32 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> Subject: Re: [PATCH v3 1/4] usb: typec: ucsi: set con->port to NULL when
> register port fail
> 
> On Wed, Apr 13, 2022 at 05:58:08PM +0800, Linyu Yuan wrote:
> > As con->port will be used in error path of ucsi_init(),
> > it should be NULL or valid.
> >
> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > ---
> > v2: no change
> > v3: no change
> >
> >  drivers/usb/typec/ucsi/ucsi.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index f0c2fa1..77ac0b7 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -1100,6 +1100,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> index)
> >  	con->port = typec_register_port(ucsi->dev, cap);
> >  	if (IS_ERR(con->port)) {
> >  		ret = PTR_ERR(con->port);
> > +		con->port = NULL;
> >  		goto out;
> >  	}
> 
> Please merge this into the next patch.
Sure, will do after all discussion are done.
> 
> thanks,
> 
> --
> heikki

[PATCH v3 2/4] usb: typec: ucsi: add a common function ucsi_unregister_connectors()

[Linyu Yuan]

In error path of ucsi_init(), it will unregister all valid ucsi connector,
and samiliar operation also happen in ucsi_unregister(),
add a common function for two places.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: improve ucsi_connector_clean(), check total number of connector.
v3: rename to ucsi_unregister_connectors(), suggest by maintainer

 drivers/usb/typec/ucsi/ucsi.c | 51 ++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 77ac0b7..af9a2a1 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1187,6 +1187,32 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	return ret;
 }
 
+static void ucsi_unregister_connectors(struct ucsi *ucsi)
+{
+	struct ucsi_connector *con;
+	int i;
+
+	if (!ucsi->connector)
+		return;
+
+	for (i = 0; i < ucsi->cap.num_connectors; i++) {
+		con = &ucsi->connector[i];
+		if (!con->port)
+			break;
+
+		cancel_work_sync(&con->work);
+		ucsi_unregister_partner(con);
+		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
+		ucsi_unregister_port_psy(con);
+		if (con->wq)
+			destroy_workqueue(con->wq);
+		typec_unregister_port(con->port);
+	}
+
+	kfree(ucsi->connector);
+	ucsi->connector = NULL;
+}
+
 /**
  * ucsi_init - Initialize UCSI interface
  * @ucsi: UCSI to be initialized
@@ -1195,7 +1221,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
  */
 static int ucsi_init(struct ucsi *ucsi)
 {
-	struct ucsi_connector *con;
 	u64 command;
 	int ret;
 	int i;
@@ -1250,15 +1275,7 @@ static int ucsi_init(struct ucsi *ucsi)
 	return 0;
 
 err_unregister:
-	for (con = ucsi->connector; con->port; con++) {
-		ucsi_unregister_partner(con);
-		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
-		ucsi_unregister_port_psy(con);
-		if (con->wq)
-			destroy_workqueue(con->wq);
-		typec_unregister_port(con->port);
-		con->port = NULL;
-	}
+	ucsi_unregister_connectors(ucsi);
 
 err_reset:
 	memset(&ucsi->cap, 0, sizeof(ucsi->cap));
@@ -1364,7 +1381,6 @@ EXPORT_SYMBOL_GPL(ucsi_register);
 void ucsi_unregister(struct ucsi *ucsi)
 {
 	u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
-	int i;
 
 	/* Make sure that we are not in the middle of driver initialization */
 	cancel_work_sync(&ucsi->work);
@@ -1372,18 +1388,7 @@ void ucsi_unregister(struct ucsi *ucsi)
 	/* Disable notifications */
 	ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
 
-	for (i = 0; i < ucsi->cap.num_connectors; i++) {
-		cancel_work_sync(&ucsi->connector[i].work);
-		ucsi_unregister_partner(&ucsi->connector[i]);
-		ucsi_unregister_altmodes(&ucsi->connector[i],
-					 UCSI_RECIPIENT_CON);
-		ucsi_unregister_port_psy(&ucsi->connector[i]);
-		if (ucsi->connector[i].wq)
-			destroy_workqueue(ucsi->connector[i].wq);
-		typec_unregister_port(ucsi->connector[i].port);
-	}
-
-	kfree(ucsi->connector);
+	ucsi_unregister_connectors(ucsi);
 }
 EXPORT_SYMBOL_GPL(ucsi_unregister);
 
-- 
2.7.4

Re: [PATCH v3 2/4] usb: typec: ucsi: add a common function ucsi_unregister_connectors()

[Heikki Krogerus]

On Wed, Apr 13, 2022 at 05:58:09PM +0800, Linyu Yuan wrote:
> In error path of ucsi_init(), it will unregister all valid ucsi connector,
> and samiliar operation also happen in ucsi_unregister(),

Sorry but I have to confirm this: with "samiliar" you mean "the same",
right?

> add a common function for two places.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: improve ucsi_connector_clean(), check total number of connector.
> v3: rename to ucsi_unregister_connectors(), suggest by maintainer
> 
>  drivers/usb/typec/ucsi/ucsi.c | 51 ++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 77ac0b7..af9a2a1 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1187,6 +1187,32 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  	return ret;
>  }
>  
> +static void ucsi_unregister_connectors(struct ucsi *ucsi)
> +{
> +	struct ucsi_connector *con;
> +	int i;
> +
> +	if (!ucsi->connector)
> +		return;

Can that actually ever happen?

> +	for (i = 0; i < ucsi->cap.num_connectors; i++) {
> +		con = &ucsi->connector[i];
> +		if (!con->port)
> +			break;
> +
> +		cancel_work_sync(&con->work);
> +		ucsi_unregister_partner(con);
> +		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
> +		ucsi_unregister_port_psy(con);
> +		if (con->wq)
> +			destroy_workqueue(con->wq);
> +		typec_unregister_port(con->port);
> +	}
> +
> +	kfree(ucsi->connector);
> +	ucsi->connector = NULL;
> +}

Another way of doing this would be to just remove a single connector
in the function, and leave the loops to the callers.

static void ucsi_unregister_connector(struct ucsi_connector *con)
{
        cancel_work_sync(&con->work);
        ucsi_unregister_partner(con);
        ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
        ucsi_unregister_port_psy(con);
        if (con->wq)
                destroy_workqueue(con->wq);
        typec_unregister_port(con->port);
}

I wonder would it actually be a bit more clearer to do it like that...
I'll leave the decision to you.

>  /**
>   * ucsi_init - Initialize UCSI interface
>   * @ucsi: UCSI to be initialized
> @@ -1195,7 +1221,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>   */
>  static int ucsi_init(struct ucsi *ucsi)
>  {
> -	struct ucsi_connector *con;
>  	u64 command;
>  	int ret;
>  	int i;
> @@ -1250,15 +1275,7 @@ static int ucsi_init(struct ucsi *ucsi)
>  	return 0;
>  
>  err_unregister:
> -	for (con = ucsi->connector; con->port; con++) {
> -		ucsi_unregister_partner(con);
> -		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
> -		ucsi_unregister_port_psy(con);
> -		if (con->wq)
> -			destroy_workqueue(con->wq);
> -		typec_unregister_port(con->port);
> -		con->port = NULL;
> -	}
> +	ucsi_unregister_connectors(ucsi);
>  
>  err_reset:
>  	memset(&ucsi->cap, 0, sizeof(ucsi->cap));
> @@ -1364,7 +1381,6 @@ EXPORT_SYMBOL_GPL(ucsi_register);
>  void ucsi_unregister(struct ucsi *ucsi)
>  {
>  	u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
> -	int i;
>  
>  	/* Make sure that we are not in the middle of driver initialization */
>  	cancel_work_sync(&ucsi->work);
> @@ -1372,18 +1388,7 @@ void ucsi_unregister(struct ucsi *ucsi)
>  	/* Disable notifications */
>  	ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
>  
> -	for (i = 0; i < ucsi->cap.num_connectors; i++) {
> -		cancel_work_sync(&ucsi->connector[i].work);
> -		ucsi_unregister_partner(&ucsi->connector[i]);
> -		ucsi_unregister_altmodes(&ucsi->connector[i],
> -					 UCSI_RECIPIENT_CON);
> -		ucsi_unregister_port_psy(&ucsi->connector[i]);
> -		if (ucsi->connector[i].wq)
> -			destroy_workqueue(ucsi->connector[i].wq);
> -		typec_unregister_port(ucsi->connector[i].port);
> -	}
> -
> -	kfree(ucsi->connector);
> +	ucsi_unregister_connectors(ucsi);
>  }
>  EXPORT_SYMBOL_GPL(ucsi_unregister);

thanks,

-- 
heikki

RE: [PATCH v3 2/4] usb: typec: ucsi: add a common function ucsi_unregister_connectors()

[Linyu Yuan (QUIC)]

> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Wednesday, April 13, 2022 8:09 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> Subject: Re: [PATCH v3 2/4] usb: typec: ucsi: add a common function
> ucsi_unregister_connectors()
> 
> On Wed, Apr 13, 2022 at 05:58:09PM +0800, Linyu Yuan wrote:
> > In error path of ucsi_init(), it will unregister all valid ucsi connector,
> > and samiliar operation also happen in ucsi_unregister(),
> 
> Sorry but I have to confirm this: with "samiliar" you mean "the same",
> right?

Only one small difference for original code which is no cancel_work_sync() of each connector in ucsi _init(),
But in ucsi_register_port(), we get role switch after connector work initialized,
So I think it is safe to call cancel_work_sync() to connector work if role switch return -EPROBE_DEFER.

> 
> > add a common function for two places.
> >
> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > ---
> > v2: improve ucsi_connector_clean(), check total number of connector.
> > v3: rename to ucsi_unregister_connectors(), suggest by maintainer
> >
> >  drivers/usb/typec/ucsi/ucsi.c | 51 ++++++++++++++++++++++++-----------
> --------
> >  1 file changed, 28 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index 77ac0b7..af9a2a1 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -1187,6 +1187,32 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> index)
> >  	return ret;
> >  }
> >
> > +static void ucsi_unregister_connectors(struct ucsi *ucsi)
> > +{
> > +	struct ucsi_connector *con;
> > +	int i;
> > +
> > +	if (!ucsi->connector)
> > +		return;
> 
> Can that actually ever happen?

Consider a case, ucsi_init() failed, we will call ucsi_unregister_connectors() to free all connectors,
After that the UCSI implementation like ucsi_acpi call ucsi_unregister() again,
It should not unregister connectors again.

> 
> > +	for (i = 0; i < ucsi->cap.num_connectors; i++) {
> > +		con = &ucsi->connector[i];
> > +		if (!con->port)
> > +			break;
> > +
> > +		cancel_work_sync(&con->work);
> > +		ucsi_unregister_partner(con);
> > +		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
> > +		ucsi_unregister_port_psy(con);
> > +		if (con->wq)
> > +			destroy_workqueue(con->wq);
> > +		typec_unregister_port(con->port);
> > +	}
> > +
> > +	kfree(ucsi->connector);
> > +	ucsi->connector = NULL;
> > +}
> 
> Another way of doing this would be to just remove a single connector
> in the function, and leave the loops to the callers.
> 
> static void ucsi_unregister_connector(struct ucsi_connector *con)
> {
>         cancel_work_sync(&con->work);
>         ucsi_unregister_partner(con);
>         ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
>         ucsi_unregister_port_psy(con);
>         if (con->wq)
>                 destroy_workqueue(con->wq);
>         typec_unregister_port(con->port);
> }
> 
> I wonder would it actually be a bit more clearer to do it like that...
> I'll leave the decision to you.

I will keep it,
There will be no duplicate loop entry in two function, a little lazy.

> 
> >  /**
> >   * ucsi_init - Initialize UCSI interface
> >   * @ucsi: UCSI to be initialized
> > @@ -1195,7 +1221,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> index)
> >   */
> >  static int ucsi_init(struct ucsi *ucsi)
> >  {
> > -	struct ucsi_connector *con;
> >  	u64 command;
> >  	int ret;
> >  	int i;
> > @@ -1250,15 +1275,7 @@ static int ucsi_init(struct ucsi *ucsi)
> >  	return 0;
> >
> >  err_unregister:
> > -	for (con = ucsi->connector; con->port; con++) {
> > -		ucsi_unregister_partner(con);
> > -		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
> > -		ucsi_unregister_port_psy(con);
> > -		if (con->wq)
> > -			destroy_workqueue(con->wq);
> > -		typec_unregister_port(con->port);
> > -		con->port = NULL;
> > -	}
> > +	ucsi_unregister_connectors(ucsi);
> >
> >  err_reset:
> >  	memset(&ucsi->cap, 0, sizeof(ucsi->cap));
> > @@ -1364,7 +1381,6 @@ EXPORT_SYMBOL_GPL(ucsi_register);
> >  void ucsi_unregister(struct ucsi *ucsi)
> >  {
> >  	u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
> > -	int i;
> >
> >  	/* Make sure that we are not in the middle of driver initialization */
> >  	cancel_work_sync(&ucsi->work);
> > @@ -1372,18 +1388,7 @@ void ucsi_unregister(struct ucsi *ucsi)
> >  	/* Disable notifications */
> >  	ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
> >
> > -	for (i = 0; i < ucsi->cap.num_connectors; i++) {
> > -		cancel_work_sync(&ucsi->connector[i].work);
> > -		ucsi_unregister_partner(&ucsi->connector[i]);
> > -		ucsi_unregister_altmodes(&ucsi->connector[i],
> > -					 UCSI_RECIPIENT_CON);
> > -		ucsi_unregister_port_psy(&ucsi->connector[i]);
> > -		if (ucsi->connector[i].wq)
> > -			destroy_workqueue(ucsi->connector[i].wq);
> > -		typec_unregister_port(ucsi->connector[i].port);
> > -	}
> > -
> > -	kfree(ucsi->connector);
> > +	ucsi_unregister_connectors(ucsi);
> >  }
> >  EXPORT_SYMBOL_GPL(ucsi_unregister);
> 
> thanks,
> 
> --
> heikki

Re: [PATCH v3 2/4] usb: typec: ucsi: add a common function ucsi_unregister_connectors()

[Heikki Krogerus]

On Wed, Apr 13, 2022 at 01:27:52PM +0000, Linyu Yuan (QUIC) wrote:
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Wednesday, April 13, 2022 8:09 PM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> > usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> > Subject: Re: [PATCH v3 2/4] usb: typec: ucsi: add a common function
> > ucsi_unregister_connectors()
> > 
> > On Wed, Apr 13, 2022 at 05:58:09PM +0800, Linyu Yuan wrote:
> > > In error path of ucsi_init(), it will unregister all valid ucsi connector,
> > > and samiliar operation also happen in ucsi_unregister(),
> > 
> > Sorry but I have to confirm this: with "samiliar" you mean "the same",
> > right?
> 
> Only one small difference for original code which is no cancel_work_sync() of each connector in ucsi _init(),
> But in ucsi_register_port(), we get role switch after connector work initialized,
> So I think it is safe to call cancel_work_sync() to connector work if role switch return -EPROBE_DEFER.
> 
> > 
> > > add a common function for two places.
> > >
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > ---
> > > v2: improve ucsi_connector_clean(), check total number of connector.
> > > v3: rename to ucsi_unregister_connectors(), suggest by maintainer
> > >
> > >  drivers/usb/typec/ucsi/ucsi.c | 51 ++++++++++++++++++++++++-----------
> > --------
> > >  1 file changed, 28 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > index 77ac0b7..af9a2a1 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > @@ -1187,6 +1187,32 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> > index)
> > >  	return ret;
> > >  }
> > >
> > > +static void ucsi_unregister_connectors(struct ucsi *ucsi)
> > > +{
> > > +	struct ucsi_connector *con;
> > > +	int i;
> > > +
> > > +	if (!ucsi->connector)
> > > +		return;
> > 
> > Can that actually ever happen?
> 
> Consider a case, ucsi_init() failed, we will call ucsi_unregister_connectors() to free all connectors,
> After that the UCSI implementation like ucsi_acpi call ucsi_unregister() again,
> It should not unregister connectors again.

I'm sorry but I don't understand your answer. I'm trying to ask what
are you trying to say with the word "samiliar"?

I do not believe there is a word "samiliar" in English language.

thanks,

-- 
heikki

RE: [PATCH v3 2/4] usb: typec: ucsi: add a common function ucsi_unregister_connectors()

[Linyu Yuan (QUIC)]

> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Thursday, April 21, 2022 4:00 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> Subject: Re: [PATCH v3 2/4] usb: typec: ucsi: add a common function
> ucsi_unregister_connectors()
> 
> On Wed, Apr 13, 2022 at 01:27:52PM +0000, Linyu Yuan (QUIC) wrote:
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Sent: Wednesday, April 13, 2022 8:09 PM
> > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> > > usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> > > Subject: Re: [PATCH v3 2/4] usb: typec: ucsi: add a common function
> > > ucsi_unregister_connectors()
> > >
> > > On Wed, Apr 13, 2022 at 05:58:09PM +0800, Linyu Yuan wrote:
> > > > In error path of ucsi_init(), it will unregister all valid ucsi connector,
> > > > and samiliar operation also happen in ucsi_unregister(),
> > >
> > > Sorry but I have to confirm this: with "samiliar" you mean "the same",
> > > right?
> >
> > Only one small difference for original code which is no cancel_work_sync()
> of each connector in ucsi _init(),
> > But in ucsi_register_port(), we get role switch after connector work
> initialized,
> > So I think it is safe to call cancel_work_sync() to connector work if role
> switch return -EPROBE_DEFER.
> >
> > >
> > > > add a common function for two places.
> > > >
> > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > > ---
> > > > v2: improve ucsi_connector_clean(), check total number of connector.
> > > > v3: rename to ucsi_unregister_connectors(), suggest by maintainer
> > > >
> > > >  drivers/usb/typec/ucsi/ucsi.c | 51 ++++++++++++++++++++++++------
> -----
> > > --------
> > > >  1 file changed, 28 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c
> b/drivers/usb/typec/ucsi/ucsi.c
> > > > index 77ac0b7..af9a2a1 100644
> > > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > > @@ -1187,6 +1187,32 @@ static int ucsi_register_port(struct ucsi *ucsi,
> int
> > > index)
> > > >  	return ret;
> > > >  }
> > > >
> > > > +static void ucsi_unregister_connectors(struct ucsi *ucsi)
> > > > +{
> > > > +	struct ucsi_connector *con;
> > > > +	int i;
> > > > +
> > > > +	if (!ucsi->connector)
> > > > +		return;
> > >
> > > Can that actually ever happen?
> >
> > Consider a case, ucsi_init() failed, we will call ucsi_unregister_connectors()
> to free all connectors,
> > After that the UCSI implementation like ucsi_acpi call ucsi_unregister()
> again,
> > It should not unregister connectors again.
> 
> I'm sorry but I don't understand your answer. I'm trying to ask what
> are you trying to say with the word "samiliar"?
No, here I answer your question  "Can that actually ever happen"
Of check
if(!ucsi->connector)
     return


> 
> I do not believe there is a word "samiliar" in English language.


Sorry, it is similar.
I will fix the wrong word samiliar  of commit description,

But what I want to say is the original two place is indeed similar as below.
ucsi_init()
err_unregister:
	for (con = ucsi->connector; con->port; con++) {
		ucsi_unregister_partner(con);
		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
		ucsi_unregister_port_psy(con);
		if (con->wq)
			destroy_workqueue(con->wq);
		typec_unregister_port(con->port);
		con->port = NULL;
	}

One difference is cancel_work_sync().

ucsi_unregister():
	for (i = 0; i < ucsi->cap.num_connectors; i++) {
		cancel_work_sync(&ucsi->connector[i].work);
		ucsi_unregister_partner(&ucsi->connector[i]);
		ucsi_unregister_altmodes(&ucsi->connector[i],
					 UCSI_RECIPIENT_CON);
		ucsi_unregister_port_psy(&ucsi->connector[i]);
		if (ucsi->connector[i].wq)
			destroy_workqueue(ucsi->connector[i].wq);
		typec_unregister_port(ucsi->connector[i].port);
	}

> 
> thanks,
> 
> --
> heikki

[PATCH v3 3/4] usb: typec: ucsi: do not allocate one extra unused connector

[Linyu Yuan]

ucsi_connector_clean() will use actual connector number in the loop,
there is no need to allocate one extral as the ending.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: new change
v3: no change

 drivers/usb/typec/ucsi/ucsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index af9a2a1..ce9192e 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1251,7 +1251,7 @@ static int ucsi_init(struct ucsi *ucsi)
 	}
 
 	/* Allocate the connectors. Released in ucsi_unregister() */
-	ucsi->connector = kcalloc(ucsi->cap.num_connectors + 1,
+	ucsi->connector = kcalloc(ucsi->cap.num_connectors,
 				  sizeof(*ucsi->connector), GFP_KERNEL);
 	if (!ucsi->connector) {
 		ret = -ENOMEM;
-- 
2.7.4

[PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when module load late

[Linyu Yuan]

When role switch enabled, return -EAGAIN if fail to find it due to
module load ordering issue, then restart ucsi init work to find
it again every 100ms.

It also means change ucsi init work to delayed_work.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: keep original con->num in debug log
v3: change return value from -EAGAIN to PTR_ERR()

 drivers/usb/typec/ucsi/ucsi.c | 28 ++++++++++++++++------------
 drivers/usb/typec/ucsi/ucsi.h |  2 +-
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ce9192e..63c25dd 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1053,6 +1053,14 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	con->num = index + 1;
 	con->ucsi = ucsi;
 
+	cap->fwnode = ucsi_find_fwnode(con);
+	con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
+	if (IS_ERR(con->usb_role_sw)) {
+		dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
+			con->num);
+		return PTR_ERR(con->usb_role_sw);
+	}
+
 	/* Delay other interactions with the con until registration is complete */
 	mutex_lock(&con->lock);
 
@@ -1088,7 +1096,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
 		*accessory = TYPEC_ACCESSORY_DEBUG;
 
-	cap->fwnode = ucsi_find_fwnode(con);
 	cap->driver_data = con;
 	cap->ops = &ucsi_ops;
 
@@ -1147,13 +1154,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 		ucsi_port_psy_changed(con);
 	}
 
-	con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
-	if (IS_ERR(con->usb_role_sw)) {
-		dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
-			con->num);
-		con->usb_role_sw = NULL;
-	}
-
 	/* Only notify USB controller if partner supports USB data */
 	if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB))
 		u_role = USB_ROLE_NONE;
@@ -1286,12 +1286,16 @@ static int ucsi_init(struct ucsi *ucsi)
 
 static void ucsi_init_work(struct work_struct *work)
 {
-	struct ucsi *ucsi = container_of(work, struct ucsi, work);
+	struct ucsi *ucsi = container_of(work, struct ucsi, work.work);
 	int ret;
 
 	ret = ucsi_init(ucsi);
 	if (ret)
 		dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
+
+
+	if (ret == -EPROBE_DEFER)
+		queue_delayed_work(system_long_wq, &ucsi->work, HZ/10);
 }
 
 /**
@@ -1331,7 +1335,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
 	if (!ucsi)
 		return ERR_PTR(-ENOMEM);
 
-	INIT_WORK(&ucsi->work, ucsi_init_work);
+	INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
 	mutex_init(&ucsi->ppm_lock);
 	ucsi->dev = dev;
 	ucsi->ops = ops;
@@ -1366,7 +1370,7 @@ int ucsi_register(struct ucsi *ucsi)
 	if (!ucsi->version)
 		return -ENODEV;
 
-	queue_work(system_long_wq, &ucsi->work);
+	queue_delayed_work(system_long_wq, &ucsi->work, 0);
 
 	return 0;
 }
@@ -1383,7 +1387,7 @@ void ucsi_unregister(struct ucsi *ucsi)
 	u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
 
 	/* Make sure that we are not in the middle of driver initialization */
-	cancel_work_sync(&ucsi->work);
+	cancel_delayed_work_sync(&ucsi->work);
 
 	/* Disable notifications */
 	ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 280f1e1..3812017 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -287,7 +287,7 @@ struct ucsi {
 	struct ucsi_capability cap;
 	struct ucsi_connector *connector;
 
-	struct work_struct work;
+	struct delayed_work work;
 
 	/* PPM Communication lock */
 	struct mutex ppm_lock;
-- 
2.7.4

Re: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when module load late

[Heikki Krogerus]

Hi,

You have to make the subject line a bit more clear. Perhaps you could
just say "Wait for the USB role switches".

On Wed, Apr 13, 2022 at 05:58:11PM +0800, Linyu Yuan wrote:
> When role switch enabled, return -EAGAIN if fail to find it due to
> module load ordering issue, then restart ucsi init work to find
> it again every 100ms.

It looks like you did not update that.

> It also means change ucsi init work to delayed_work.

So from where does that 100ms come from?

> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: keep original con->num in debug log
> v3: change return value from -EAGAIN to PTR_ERR()
> 
>  drivers/usb/typec/ucsi/ucsi.c | 28 ++++++++++++++++------------
>  drivers/usb/typec/ucsi/ucsi.h |  2 +-
>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index ce9192e..63c25dd 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1053,6 +1053,14 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  	con->num = index + 1;
>  	con->ucsi = ucsi;
>  
> +	cap->fwnode = ucsi_find_fwnode(con);
> +	con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> +	if (IS_ERR(con->usb_role_sw)) {
> +		dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> +			con->num);
> +		return PTR_ERR(con->usb_role_sw);
> +	}
> +
>  	/* Delay other interactions with the con until registration is complete */
>  	mutex_lock(&con->lock);
>  
> @@ -1088,7 +1096,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
>  		*accessory = TYPEC_ACCESSORY_DEBUG;
>  
> -	cap->fwnode = ucsi_find_fwnode(con);
>  	cap->driver_data = con;
>  	cap->ops = &ucsi_ops;
>  
> @@ -1147,13 +1154,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  		ucsi_port_psy_changed(con);
>  	}
>  
> -	con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> -	if (IS_ERR(con->usb_role_sw)) {
> -		dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> -			con->num);
> -		con->usb_role_sw = NULL;
> -	}
> -
>  	/* Only notify USB controller if partner supports USB data */
>  	if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB))
>  		u_role = USB_ROLE_NONE;
> @@ -1286,12 +1286,16 @@ static int ucsi_init(struct ucsi *ucsi)
>  
>  static void ucsi_init_work(struct work_struct *work)
>  {
> -	struct ucsi *ucsi = container_of(work, struct ucsi, work);
> +	struct ucsi *ucsi = container_of(work, struct ucsi, work.work);
>  	int ret;
>  
>  	ret = ucsi_init(ucsi);
>  	if (ret)
>  		dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
> +
> +

Extra line.

> +	if (ret == -EPROBE_DEFER)
> +		queue_delayed_work(system_long_wq, &ucsi->work, HZ/10);

You have to stop queueing that eventually. You need a counter.

I'm still not sure why do you want to queue this same work again and
again? Why not just call ucsi_init() in a loop?

        int my_counter = 1000;

        while (ucsi_init(ucsi) == -EPROBE_DEFER && my_counter--)
                msleep(10);

>  }
>  
>  /**
> @@ -1331,7 +1335,7 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
>  	if (!ucsi)
>  		return ERR_PTR(-ENOMEM);
>  
> -	INIT_WORK(&ucsi->work, ucsi_init_work);
> +	INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
>  	mutex_init(&ucsi->ppm_lock);
>  	ucsi->dev = dev;
>  	ucsi->ops = ops;
> @@ -1366,7 +1370,7 @@ int ucsi_register(struct ucsi *ucsi)
>  	if (!ucsi->version)
>  		return -ENODEV;
>  
> -	queue_work(system_long_wq, &ucsi->work);
> +	queue_delayed_work(system_long_wq, &ucsi->work, 0);
>  
>  	return 0;
>  }
> @@ -1383,7 +1387,7 @@ void ucsi_unregister(struct ucsi *ucsi)
>  	u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
>  
>  	/* Make sure that we are not in the middle of driver initialization */
> -	cancel_work_sync(&ucsi->work);
> +	cancel_delayed_work_sync(&ucsi->work);
>  
>  	/* Disable notifications */
>  	ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 280f1e1..3812017 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -287,7 +287,7 @@ struct ucsi {
>  	struct ucsi_capability cap;
>  	struct ucsi_connector *connector;
>  
> -	struct work_struct work;
> +	struct delayed_work work;
>  
>  	/* PPM Communication lock */
>  	struct mutex ppm_lock;
> -- 
> 2.7.4

thanks,

-- 
heikki

RE: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when module load late

[Linyu Yuan (QUIC)]

> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Wednesday, April 13, 2022 8:40 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> Subject: Re: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when
> module load late
> 
> Hi,
> 
> You have to make the subject line a bit more clear. Perhaps you could
> just say "Wait for the USB role switches".

Sound great, will change once all discussion in this thread done.

> 
> On Wed, Apr 13, 2022 at 05:58:11PM +0800, Linyu Yuan wrote:
> > When role switch enabled, return -EAGAIN if fail to find it due to
> > module load ordering issue, then restart ucsi init work to find
> > it again every 100ms.
> 
> It looks like you did not update that.

Sorry, will update later.

> 
> > It also means change ucsi init work to delayed_work.
> 
> So from where does that 100ms come from?

Yes, no place define it, just an experiment value.
Any suggestion ?

> 
> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > ---
> > v2: keep original con->num in debug log
> > v3: change return value from -EAGAIN to PTR_ERR()
> >
> >  drivers/usb/typec/ucsi/ucsi.c | 28 ++++++++++++++++------------
> >  drivers/usb/typec/ucsi/ucsi.h |  2 +-
> >  2 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index ce9192e..63c25dd 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -1053,6 +1053,14 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> index)
> >  	con->num = index + 1;
> >  	con->ucsi = ucsi;
> >
> > +	cap->fwnode = ucsi_find_fwnode(con);
> > +	con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> > +	if (IS_ERR(con->usb_role_sw)) {
> > +		dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> > +			con->num);
> > +		return PTR_ERR(con->usb_role_sw);
> > +	}
> > +
> >  	/* Delay other interactions with the con until registration is complete
> */
> >  	mutex_lock(&con->lock);
> >
> > @@ -1088,7 +1096,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> index)
> >  	if (con->cap.op_mode &
> UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
> >  		*accessory = TYPEC_ACCESSORY_DEBUG;
> >
> > -	cap->fwnode = ucsi_find_fwnode(con);
> >  	cap->driver_data = con;
> >  	cap->ops = &ucsi_ops;
> >
> > @@ -1147,13 +1154,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> index)
> >  		ucsi_port_psy_changed(con);
> >  	}
> >
> > -	con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> > -	if (IS_ERR(con->usb_role_sw)) {
> > -		dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> > -			con->num);
> > -		con->usb_role_sw = NULL;
> > -	}
> > -
> >  	/* Only notify USB controller if partner supports USB data */
> >  	if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
> UCSI_CONSTAT_PARTNER_FLAG_USB))
> >  		u_role = USB_ROLE_NONE;
> > @@ -1286,12 +1286,16 @@ static int ucsi_init(struct ucsi *ucsi)
> >
> >  static void ucsi_init_work(struct work_struct *work)
> >  {
> > -	struct ucsi *ucsi = container_of(work, struct ucsi, work);
> > +	struct ucsi *ucsi = container_of(work, struct ucsi, work.work);
> >  	int ret;
> >
> >  	ret = ucsi_init(ucsi);
> >  	if (ret)
> >  		dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
> > +
> > +
> 
> Extra line.
> 
> > +	if (ret == -EPROBE_DEFER)
> > +		queue_delayed_work(system_long_wq, &ucsi->work,
> HZ/10);
> 
> You have to stop queueing that eventually. You need a counter.
> 
> I'm still not sure why do you want to queue this same work again and
> again? Why not just call ucsi_init() in a loop?

Will follow your suggestion below.

> 
>         int my_counter = 1000;
So you prefer 10 second in total ?
If so, next version, I will change it to 500, as msleep(20), check next.
> 
>         while (ucsi_init(ucsi) == -EPROBE_DEFER && my_counter--)
>                 msleep(10);
In checkpatch.pl, it suggest msleep no less than 20ms each time.
msleep(20) ?

> 
> >  }

Great idea, it will be less code change.

> >
> >  /**
> > @@ -1331,7 +1335,7 @@ struct ucsi *ucsi_create(struct device *dev, const
> struct ucsi_operations *ops)
> >  	if (!ucsi)
> >  		return ERR_PTR(-ENOMEM);
> >
> > -	INIT_WORK(&ucsi->work, ucsi_init_work);
> > +	INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
> >  	mutex_init(&ucsi->ppm_lock);
> >  	ucsi->dev = dev;
> >  	ucsi->ops = ops;
> > @@ -1366,7 +1370,7 @@ int ucsi_register(struct ucsi *ucsi)
> >  	if (!ucsi->version)
> >  		return -ENODEV;
> >
> > -	queue_work(system_long_wq, &ucsi->work);
> > +	queue_delayed_work(system_long_wq, &ucsi->work, 0);
> >
> >  	return 0;
> >  }
> > @@ -1383,7 +1387,7 @@ void ucsi_unregister(struct ucsi *ucsi)
> >  	u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
> >
> >  	/* Make sure that we are not in the middle of driver initialization */
> > -	cancel_work_sync(&ucsi->work);
> > +	cancel_delayed_work_sync(&ucsi->work);
> >
> >  	/* Disable notifications */
> >  	ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
> > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > index 280f1e1..3812017 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.h
> > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > @@ -287,7 +287,7 @@ struct ucsi {
> >  	struct ucsi_capability cap;
> >  	struct ucsi_connector *connector;
> >
> > -	struct work_struct work;
> > +	struct delayed_work work;
> >
> >  	/* PPM Communication lock */
> >  	struct mutex ppm_lock;
> > --
> > 2.7.4
> 
> thanks,
> 
> --
> heikki

RE: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when module load late

[Linyu Yuan (QUIC)]

> From: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Sent: Wednesday, April 13, 2022 9:43 PM
> To: Heikki Krogerus <heikki.krogerus@linux.intel.com>; Linyu Yuan (QUIC)
> <quic_linyyuan@quicinc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> Subject: RE: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when
> module load late
> 
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Wednesday, April 13, 2022 8:40 PM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> > usb@vger.kernel.org; Jack Pham (QUIC) <quic_jackp@quicinc.com>
> > Subject: Re: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when
> > module load late
> >
> > Hi,
> >
> > You have to make the subject line a bit more clear. Perhaps you could
> > just say "Wait for the USB role switches".
> 
> Sound great, will change once all discussion in this thread done.
> 
> >
> > On Wed, Apr 13, 2022 at 05:58:11PM +0800, Linyu Yuan wrote:
> > > When role switch enabled, return -EAGAIN if fail to find it due to
> > > module load ordering issue, then restart ucsi init work to find
> > > it again every 100ms.
> >
> > It looks like you did not update that.
> 
> Sorry, will update later.
> 
> >
> > > It also means change ucsi init work to delayed_work.
> >
> > So from where does that 100ms come from?
> 
> Yes, no place define it, just an experiment value.
> Any suggestion ?
> 
> >
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > ---
> > > v2: keep original con->num in debug log
> > > v3: change return value from -EAGAIN to PTR_ERR()
> > >
> > >  drivers/usb/typec/ucsi/ucsi.c | 28 ++++++++++++++++------------
> > >  drivers/usb/typec/ucsi/ucsi.h |  2 +-
> > >  2 files changed, 17 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > index ce9192e..63c25dd 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > @@ -1053,6 +1053,14 @@ static int ucsi_register_port(struct ucsi *ucsi,
> int
> > index)
> > >  	con->num = index + 1;
> > >  	con->ucsi = ucsi;
> > >
> > > +	cap->fwnode = ucsi_find_fwnode(con);
> > > +	con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> > > +	if (IS_ERR(con->usb_role_sw)) {
> > > +		dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> > > +			con->num);
> > > +		return PTR_ERR(con->usb_role_sw);
> > > +	}
> > > +
> > >  	/* Delay other interactions with the con until registration is complete
> > */
> > >  	mutex_lock(&con->lock);
> > >
> > > @@ -1088,7 +1096,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int
> > index)
> > >  	if (con->cap.op_mode &
> > UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY)
> > >  		*accessory = TYPEC_ACCESSORY_DEBUG;
> > >
> > > -	cap->fwnode = ucsi_find_fwnode(con);
> > >  	cap->driver_data = con;
> > >  	cap->ops = &ucsi_ops;
> > >
> > > @@ -1147,13 +1154,6 @@ static int ucsi_register_port(struct ucsi *ucsi,
> int
> > index)
> > >  		ucsi_port_psy_changed(con);
> > >  	}
> > >
> > > -	con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
> > > -	if (IS_ERR(con->usb_role_sw)) {
> > > -		dev_err(ucsi->dev, "con%d: failed to get usb role switch\n",
> > > -			con->num);
> > > -		con->usb_role_sw = NULL;
> > > -	}
> > > -
> > >  	/* Only notify USB controller if partner supports USB data */
> > >  	if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) &
> > UCSI_CONSTAT_PARTNER_FLAG_USB))
> > >  		u_role = USB_ROLE_NONE;
> > > @@ -1286,12 +1286,16 @@ static int ucsi_init(struct ucsi *ucsi)
> > >
> > >  static void ucsi_init_work(struct work_struct *work)
> > >  {
> > > -	struct ucsi *ucsi = container_of(work, struct ucsi, work);
> > > +	struct ucsi *ucsi = container_of(work, struct ucsi, work.work);
> > >  	int ret;
> > >
> > >  	ret = ucsi_init(ucsi);
> > >  	if (ret)
> > >  		dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
> > > +
> > > +
> >
> > Extra line.
> >
> > > +	if (ret == -EPROBE_DEFER)
> > > +		queue_delayed_work(system_long_wq, &ucsi->work,
> > HZ/10);
> >
> > You have to stop queueing that eventually. You need a counter.
> >
> > I'm still not sure why do you want to queue this same work again and
> > again? Why not just call ucsi_init() in a loop?
> 
> Will follow your suggestion below.
> 
> >
> >         int my_counter = 1000;
> So you prefer 10 second in total ?
> If so, next version, I will change it to 500, as msleep(20), check next.
> >
> >         while (ucsi_init(ucsi) == -EPROBE_DEFER && my_counter--)
> >                 msleep(10);
> In checkpatch.pl, it suggest msleep no less than 20ms each time.
> msleep(20) ?
> 
> >
> > >  }
> 
> Great idea, it will be less code change.


Sorry, I think there is one concern of your suggestion is that
As the while loop inside the worker, if we can this worker,
It may spent too much time.

Although my original design is crazy(queue worker again inside it),
but it allow cancel worker wait short time.

please let me know what is your suggestion now.
> 
> > >
> > >  /**
> > > @@ -1331,7 +1335,7 @@ struct ucsi *ucsi_create(struct device *dev,
> const
> > struct ucsi_operations *ops)
> > >  	if (!ucsi)
> > >  		return ERR_PTR(-ENOMEM);
> > >
> > > -	INIT_WORK(&ucsi->work, ucsi_init_work);
> > > +	INIT_DELAYED_WORK(&ucsi->work, ucsi_init_work);
> > >  	mutex_init(&ucsi->ppm_lock);
> > >  	ucsi->dev = dev;
> > >  	ucsi->ops = ops;
> > > @@ -1366,7 +1370,7 @@ int ucsi_register(struct ucsi *ucsi)
> > >  	if (!ucsi->version)
> > >  		return -ENODEV;
> > >
> > > -	queue_work(system_long_wq, &ucsi->work);
> > > +	queue_delayed_work(system_long_wq, &ucsi->work, 0);
> > >
> > >  	return 0;
> > >  }
> > > @@ -1383,7 +1387,7 @@ void ucsi_unregister(struct ucsi *ucsi)
> > >  	u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
> > >
> > >  	/* Make sure that we are not in the middle of driver initialization */
> > > -	cancel_work_sync(&ucsi->work);
> > > +	cancel_delayed_work_sync(&ucsi->work);
> > >
> > >  	/* Disable notifications */
> > >  	ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > > index 280f1e1..3812017 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.h
> > > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > > @@ -287,7 +287,7 @@ struct ucsi {
> > >  	struct ucsi_capability cap;
> > >  	struct ucsi_connector *connector;
> > >
> > > -	struct work_struct work;
> > > +	struct delayed_work work;
> > >
> > >  	/* PPM Communication lock */
> > >  	struct mutex ppm_lock;
> > > --
> > > 2.7.4
> >
> > thanks,
> >
> > --
> > heikki

Re: [PATCH v3 4/4] usb: typec: ucsi: retry find role swithch when module load late

[Heikki Krogerus]

Hi,

On Thu, Apr 21, 2022 at 07:40:56AM +0000, Linyu Yuan (QUIC) wrote:
> > > >  static void ucsi_init_work(struct work_struct *work)
> > > >  {
> > > > -	struct ucsi *ucsi = container_of(work, struct ucsi, work);
> > > > +	struct ucsi *ucsi = container_of(work, struct ucsi, work.work);
> > > >  	int ret;
> > > >
> > > >  	ret = ucsi_init(ucsi);
> > > >  	if (ret)
> > > >  		dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
> > > > +
> > > > +
> > >
> > > Extra line.
> > >
> > > > +	if (ret == -EPROBE_DEFER)
> > > > +		queue_delayed_work(system_long_wq, &ucsi->work,
> > > HZ/10);
> > >
> > > You have to stop queueing that eventually. You need a counter.
> > >
> > > I'm still not sure why do you want to queue this same work again and
> > > again? Why not just call ucsi_init() in a loop?
> > 
> > Will follow your suggestion below.
> > 
> > >
> > >         int my_counter = 1000;
> > So you prefer 10 second in total ?
> > If so, next version, I will change it to 500, as msleep(20), check next.
> > >
> > >         while (ucsi_init(ucsi) == -EPROBE_DEFER && my_counter--)
> > >                 msleep(10);
> > In checkpatch.pl, it suggest msleep no less than 20ms each time.
> > msleep(20) ?
> > 
> > >
> > > >  }
> > 
> > Great idea, it will be less code change.
> 
> 
> Sorry, I think there is one concern of your suggestion is that
> As the while loop inside the worker, if we can this worker,
> It may spent too much time.
> 
> Although my original design is crazy(queue worker again inside it),
> but it allow cancel worker wait short time.
> 
> please let me know what is your suggestion now.

OK, that's a good point. So just re-schedule the work like you do.

thanks,

-- 
heikki