On Mon, Jan 09, 2023 at 11:12:18PM -0800, Jack Pham wrote:
> During ucsi_unregister() when destroying a connector's workqueue, there
> may still be pending delayed work items that haven't been scheduled yet.
> Because queue_delayed_work() uses a separate timer to schedule a work
> item, the destroy_workqueue() call is not aware of any pending items.
> Hence when a pending item's timer expires it would then try to queue on
> a dangling workqueue pointer.
>
> Fix this by keeping track of all work items in a list, so that prior to
> destroying the workqueue any pending items can be flushed. Do this by
> calling mod_delayed_work() as that will cause pending items to get
> queued immediately, which then allows the ensuing destroy_workqueue() to
> implicitly drain all currently queued items to completion and free
> themselves.
>
> Fixes: b9aa02ca39a4 ("usb: typec: ucsi: Add polling mechanism for partner tasks like alt mode checking")
> Suggested-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Co-developed-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> Signed-off-by: Jack Pham <quic_jackp@xxxxxxxxxxx>
This looks OK to me.
Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> v3: Adopted Heikki's suggestion to track work items in a list. Updated subject
> and commit description again.
> v2: update commit description and remove cc stable list
>
> drivers/usb/typec/ucsi/ucsi.c | 24 +++++++++++++++++++++---
> drivers/usb/typec/ucsi/ucsi.h | 1 +
> 2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index eabe519013e7..1292241d581a 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -187,6 +187,7 @@ EXPORT_SYMBOL_GPL(ucsi_send_command);
>
> struct ucsi_work {
> struct delayed_work work;
> + struct list_head node;
> unsigned long delay;
> unsigned int count;
> struct ucsi_connector *con;
> @@ -202,6 +203,7 @@ static void ucsi_poll_worker(struct work_struct *work)
> mutex_lock(&con->lock);
>
> if (!con->partner) {
> + list_del(&uwork->node);
> mutex_unlock(&con->lock);
> kfree(uwork);
> return;
> @@ -209,10 +211,12 @@ static void ucsi_poll_worker(struct work_struct *work)
>
> ret = uwork->cb(con);
>
> - if (uwork->count-- && (ret == -EBUSY || ret == -ETIMEDOUT))
> + if (uwork->count-- && (ret == -EBUSY || ret == -ETIMEDOUT)) {
> queue_delayed_work(con->wq, &uwork->work, uwork->delay);
> - else
> + } else {
> + list_del(&uwork->node);
> kfree(uwork);
> + }
>
> mutex_unlock(&con->lock);
> }
> @@ -236,6 +240,7 @@ static int ucsi_partner_task(struct ucsi_connector *con,
> uwork->con = con;
> uwork->cb = cb;
>
> + list_add_tail(&uwork->node, &con->partner_tasks);
> queue_delayed_work(con->wq, &uwork->work, delay);
>
> return 0;
> @@ -1056,6 +1061,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
> INIT_WORK(&con->work, ucsi_handle_connector_change);
> init_completion(&con->complete);
> mutex_init(&con->lock);
> + INIT_LIST_HEAD(&con->partner_tasks);
> con->num = index + 1;
> con->ucsi = ucsi;
>
> @@ -1420,8 +1426,20 @@ void ucsi_unregister(struct ucsi *ucsi)
> ucsi_unregister_altmodes(&ucsi->connector[i],
> UCSI_RECIPIENT_CON);
> ucsi_unregister_port_psy(&ucsi->connector[i]);
> - if (ucsi->connector[i].wq)
> +
> + if (ucsi->connector[i].wq) {
> + struct ucsi_work *uwork;
> +
> + mutex_lock(&ucsi->connector[i].lock);
> + /*
> + * queue delayed items immediately so they can execute
> + * and free themselves before the wq is destroyed
> + */
> + list_for_each_entry(uwork, &ucsi->connector[i].partner_tasks, node)
> + mod_delayed_work(ucsi->connector[i].wq, &uwork->work, 0);
> + mutex_unlock(&ucsi->connector[i].lock);
> destroy_workqueue(ucsi->connector[i].wq);
> + }
> typec_unregister_port(ucsi->connector[i].port);
> }
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index c968474ee547..60ce9fb6e745 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -322,6 +322,7 @@ struct ucsi_connector {
> struct work_struct work;
> struct completion complete;
> struct workqueue_struct *wq;
> + struct list_head partner_tasks;
>
> struct typec_port *port;
> struct typec_partner *partner;
> --
> 2.24.0
thanks,
--
heikki
