Re: [PATCH v2] platform/chrome: cros_ec_typec: Stop altmode work during partner removal

[Andrei Kuchynski]

On Fri, Oct 2, 2026 at 8:01 PM Myeonghun Pak <mhun512@xxxxxxxxx> wrote:
>
> DisplayPort and Thunderbolt port altmodes queue work to deliver VDM
> responses. Partner removal and port teardown do not drain this work,
> allowing it to race with partner driver removal or access freed port
> altmode data.
>
> Disable and drain port altmode work in the common partner cleanup path.
> Partner drivers can still queue responses during removal, so keep the
> work disabled until they are gone. Clear pending response and DP status
> state, then re-enable it for the next connection. Cancel the work before
> unregistering port altmodes during final teardown.
>
> Allocate list nodes before registering altmodes so allocation failures
> also use the common cleanup path.
>
> This issue was identified during our ongoing static-analysis research
> while reviewing kernel code.
>
> Fixes: dbb3fc0ffa95 ("platform/chrome: cros_ec_typec: Displayport support")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> ---
> Changes in v2:
> - Quiesce port work in cros_typec_unregister_altmodes() before removing
>   partner altmodes, covering disconnects and discovery cleanup.
> - Re-enable work after resetting pending response and DP status state so
>   port altmodes remain usable after reconnecting a partner.
> - Cancel work in cros_typec_unregister_port_altmodes() during final port
>   teardown, following the review suggestion to use cancel_work_sync().
> - Allocate list bookkeeping before registering an altmode so allocation
>   failures use the common guarded cleanup path.
> - Drop the v1 Reviewed-by tag due to the reworked cleanup paths.
>
> Previous version:
> https://lore.kernel.org/all/20260917204209.97699-1-mhun512@xxxxxxxxx/
>
> Validation: apply checks, whitespace checks and static source review.
> No build or runtime testing was performed for this revision.
>
> Remaining review point: an in-flight port active sysfs callback may still
> queue work after the final cancel_work_sync(), before the port altmode
> is unregistered. Please confirm whether that ordering needs additional
> synchronization.
>
>  drivers/platform/chrome/cros_ec_typec.c      | 27 ++++++++----
>  drivers/platform/chrome/cros_typec_altmode.c | 46 ++++++++++++++++++++
>  drivers/platform/chrome/cros_typec_altmode.h |  9 ++++
>  3 files changed, 73 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 50a68819ceb7bbfbd888ca919d678d4c75237c26..2d6cbb7f51445fde4b4e8339081e921a9a143a92 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -282,11 +282,19 @@ static void cros_typec_unregister_altmodes(struct cros_typec_data *typec, int po
>         struct list_head *head;
>
>         head = is_partner ? &port->partner_mode_list : &port->plug_mode_list;
> +       /* Partner drivers can queue port work until their removal completes. */
> +       if (is_partner)
> +               cros_typec_altmodes_set_enabled(port, false);
> +
>         list_for_each_entry_safe(node, tmp, head, list) {
>                 list_del(&node->list);
>                 typec_unregister_altmode(node->amode);
>                 devm_kfree(typec->dev, node);
>         }
> +
> +       /* Port altmodes are reused when a partner reconnects. */
> +       if (is_partner)
> +               cros_typec_altmodes_set_enabled(port, true);

Have you considered fetching the port's altmode using
typec_altmode_get_partner()?

        const struct typec_altmode *pdev =
typec_altmode_get_partner(node->amode);
        if (pdev)
                cros_typec_enable_altmode(pdev, true/false);

This allows implement cros_typec_enable_altmode() eliminating
unnecessary checks and the additional loop:

        struct cros_typec_dp_data *dp_data = typec_altmode_get_drvdata(alt);
        if (enable)
               enable_work(&dp_data->adata.work);
        else
               disable_work_sync(&dp_data->adata.work);

>  }
>
>  /*
> @@ -366,8 +374,10 @@ static void cros_typec_unregister_port_altmodes(struct cros_typec_port *port)
>  {
>         int i;
>
> -       for (i = 0; i < CROS_EC_ALTMODE_MAX; i++)
> +       for (i = 0; i < CROS_EC_ALTMODE_MAX; i++) {
> +               cros_typec_altmode_cancel(port->port_altmode[i]);

That won't be necessary, as the work queue is already canceled in
cros_typec_unregister_altmodes().

>                 typec_unregister_altmode(port->port_altmode[i]);
> +       }
>  }
>
>  static void cros_unregister_ports(struct cros_typec_data *typec)
> @@ -901,24 +911,23 @@ static int cros_typec_register_altmodes(struct cros_typec_data *typec, int port_
>                         desc.mode = j + 1;
>                         desc.vdo = sop_disc->svids[i].mode_vdo[j];
>
> +                       node = devm_kzalloc(typec->dev, sizeof(*node), GFP_KERNEL);
> +                       if (!node) {
> +                               ret = -ENOMEM;
> +                               goto err_cleanup;
> +                       }
> +
>                         if (is_partner)
>                                 amode = typec_partner_register_altmode(port->partner, &desc);
>                         else
>                                 amode = typec_plug_register_altmode(port->plug, &desc);
>
>                         if (IS_ERR(amode)) {
> +                               devm_kfree(typec->dev, node);
>                                 ret = PTR_ERR(amode);
>                                 goto err_cleanup;
>                         }
>
> -                       /* If no memory is available we should unregister and exit. */
> -                       node = devm_kzalloc(typec->dev, sizeof(*node), GFP_KERNEL);
> -                       if (!node) {
> -                               ret = -ENOMEM;
> -                               typec_unregister_altmode(amode);
> -                               goto err_cleanup;
> -                       }
> -

Do we really need this?

Thanks,
Andrei