On Fri, Apr 22, 2022 at 11:10:20AM +0800, Linyu Yuan wrote: > As con->port will be used in error path of ucsi_init(), > it should be NULL or valid. > > In error path of ucsi_init(), it will unregister all valid ucsi connectors, > and similar operation also happen in ucsi_unregister(), > 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 > v4: merge patch#1 in V3, fix a typo samiliar -> similar in commit description > v5: no change > > drivers/usb/typec/ucsi/ucsi.c | 52 ++++++++++++++++++++++++------------------- > 1 file changed, 29 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index f0c2fa1..af9a2a1 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; I'm not sure you need to add that line. See below. > goto out; > } > > @@ -1186,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; if (IS_ERR_OR_NULL(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 > @@ -1194,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; > @@ -1249,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)); > @@ -1363,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); > @@ -1371,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