> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Sent: Thursday, April 21, 2022 4:00 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-
> usb@xxxxxxxxxxxxxxx; Jack Pham (QUIC) <quic_jackp@xxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> > > Sent: Wednesday, April 13, 2022 8:09 PM
> > > To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-
> > > usb@xxxxxxxxxxxxxxx; Jack Pham (QUIC) <quic_jackp@xxxxxxxxxxx>
> > > 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@xxxxxxxxxxx>
> > > > ---
> > > > 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
