1. Patchew
  2. linux
  3. [PATCH 00/10] USB Type-C mode selection
  4. View patch

[PATCH 06/10] Revert "usb: typec: displayport: Receive DP Status Update NAK request exit dp altmode"

Andrei Kuchynski posted 10 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH 06/10] Revert "usb: typec: displayport: Receive DP Status Update NAK request exit dp altmode"
Posted by Andrei Kuchynski 3 months, 3 weeks ago 
This reverts commit b4b38ffb38c91afd4dc387608db26f6fc34ed40b.

The commit introduced a deadlock with the cros_ec_typec driver.
The deadlock occurs due to a recursive lock acquisition of
`cros_typec_altmode_work::mutex`.
The call chain is as follows: 
1. cros_typec_altmode_work() acquires the mutex
2. typec_altmode_vdm() -> dp_altmode_vdm() ->
3. typec_altmode_exit() -> cros_typec_altmode_exit()
4. cros_typec_altmode_exit() attempts to acquire the mutex again

This revert is considered safe as no other known driver sends back
DP_CMD_STATUS_UPDATE command with the NAK flag.

Signed-off-by: Andrei Kuchynski <akuchynski@chromium.org>
---
 drivers/usb/typec/altmodes/displayport.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index b09b58d7311d..ac84a6d64c2f 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -393,10 +393,6 @@ static int dp_altmode_vdm(struct typec_altmode *alt,
 		break;
 	case CMDT_RSP_NAK:
 		switch (cmd) {
-		case DP_CMD_STATUS_UPDATE:
-			if (typec_altmode_exit(alt))
-				dev_err(&dp->alt->dev, "Exit Mode Failed!\n");
-			break;
 		case DP_CMD_CONFIGURE:
 			dp->data.conf = 0;
 			ret = dp_altmode_configured(dp);
-- 
2.50.0.rc1.591.g9c95f17f64-goog
Re: [PATCH 06/10] Revert "usb: typec: displayport: Receive DP Status Update NAK request exit dp altmode"
Posted by Heikki Krogerus 3 months, 3 weeks ago 
Hi Andrei,

On Mon, Jun 16, 2025 at 01:31:43PM +0000, Andrei Kuchynski wrote:
> This reverts commit b4b38ffb38c91afd4dc387608db26f6fc34ed40b.
> 
> The commit introduced a deadlock with the cros_ec_typec driver.
> The deadlock occurs due to a recursive lock acquisition of
> `cros_typec_altmode_work::mutex`.
> The call chain is as follows: 
> 1. cros_typec_altmode_work() acquires the mutex
> 2. typec_altmode_vdm() -> dp_altmode_vdm() ->
> 3. typec_altmode_exit() -> cros_typec_altmode_exit()
> 4. cros_typec_altmode_exit() attempts to acquire the mutex again
> 
> This revert is considered safe as no other known driver sends back
> DP_CMD_STATUS_UPDATE command with the NAK flag.
> 
> Signed-off-by: Andrei Kuchynski <akuchynski@chromium.org>
> ---
>  drivers/usb/typec/altmodes/displayport.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index b09b58d7311d..ac84a6d64c2f 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -393,10 +393,6 @@ static int dp_altmode_vdm(struct typec_altmode *alt,
>  		break;
>  	case CMDT_RSP_NAK:
>  		switch (cmd) {
> -		case DP_CMD_STATUS_UPDATE:
> -			if (typec_altmode_exit(alt))
> -				dev_err(&dp->alt->dev, "Exit Mode Failed!\n");
> -			break;

Commit b4b38ffb38c9 ("usb: typec: displayport: Receive DP Status
Update NAK request exit dp altmode") addressed a very real problem
with failure to execute data role swap. You are not really offering
anything else for that issue here.

Is it not an option to just schedule the mode exit here instead to
solve the problem?

diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index b09b58d7311d..2abbe4de3216 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -394,8 +394,7 @@ static int dp_altmode_vdm(struct typec_altmode *alt,
        case CMDT_RSP_NAK:
                switch (cmd) {
                case DP_CMD_STATUS_UPDATE:
-                       if (typec_altmode_exit(alt))
-                               dev_err(&dp->alt->dev, "Exit Mode Failed!\n");
+                       dp->state = DP_STATE_EXIT;
                        break;
                case DP_CMD_CONFIGURE:
                        dp->data.conf = 0;


-- 
heikki
Re: [PATCH 06/10] Revert "usb: typec: displayport: Receive DP Status Update NAK request exit dp altmode"
Posted by Andrei Kuchynski 3 months, 3 weeks ago 
On Tue, Jun 17, 2025 at 10:54 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Andrei,
>
> On Mon, Jun 16, 2025 at 01:31:43PM +0000, Andrei Kuchynski wrote:
> > This reverts commit b4b38ffb38c91afd4dc387608db26f6fc34ed40b.
> >
> > The commit introduced a deadlock with the cros_ec_typec driver.
> > The deadlock occurs due to a recursive lock acquisition of
> > `cros_typec_altmode_work::mutex`.
> > The call chain is as follows:
> > 1. cros_typec_altmode_work() acquires the mutex
> > 2. typec_altmode_vdm() -> dp_altmode_vdm() ->
> > 3. typec_altmode_exit() -> cros_typec_altmode_exit()
> > 4. cros_typec_altmode_exit() attempts to acquire the mutex again
> >
> > This revert is considered safe as no other known driver sends back
> > DP_CMD_STATUS_UPDATE command with the NAK flag.
> >
> > Signed-off-by: Andrei Kuchynski <akuchynski@chromium.org>
> > ---
> >  drivers/usb/typec/altmodes/displayport.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> > index b09b58d7311d..ac84a6d64c2f 100644
> > --- a/drivers/usb/typec/altmodes/displayport.c
> > +++ b/drivers/usb/typec/altmodes/displayport.c
> > @@ -393,10 +393,6 @@ static int dp_altmode_vdm(struct typec_altmode *alt,
> >               break;
> >       case CMDT_RSP_NAK:
> >               switch (cmd) {
> > -             case DP_CMD_STATUS_UPDATE:
> > -                     if (typec_altmode_exit(alt))
> > -                             dev_err(&dp->alt->dev, "Exit Mode Failed!\n");
> > -                     break;
>
> Commit b4b38ffb38c9 ("usb: typec: displayport: Receive DP Status
> Update NAK request exit dp altmode") addressed a very real problem
> with failure to execute data role swap. You are not really offering
> anything else for that issue here.

Thanks, I see the problem now. Reverting the patch is not feasible.

>
> Is it not an option to just schedule the mode exit here instead to
> solve the problem?

Of course, that's an option. Alternatively, maybe I could resolve the
deadlock within the `cros_ec_typec` driver. Regardless, this seems like
a separate patch.

>
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index b09b58d7311d..2abbe4de3216 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -394,8 +394,7 @@ static int dp_altmode_vdm(struct typec_altmode *alt,
>         case CMDT_RSP_NAK:
>                 switch (cmd) {
>                 case DP_CMD_STATUS_UPDATE:
> -                       if (typec_altmode_exit(alt))
> -                               dev_err(&dp->alt->dev, "Exit Mode Failed!\n");
> +                       dp->state = DP_STATE_EXIT;
>                         break;
>                 case DP_CMD_CONFIGURE:
>                         dp->data.conf = 0;
>
>
> --
> heikki
Re: [PATCH 06/10] Revert "usb: typec: displayport: Receive DP Status Update NAK request exit dp altmode"
Posted by Greg Kroah-Hartman 3 months, 3 weeks ago 
On Mon, Jun 16, 2025 at 01:31:43PM +0000, Andrei Kuchynski wrote:
> This reverts commit b4b38ffb38c91afd4dc387608db26f6fc34ed40b.
> 
> The commit introduced a deadlock with the cros_ec_typec driver.
> The deadlock occurs due to a recursive lock acquisition of
> `cros_typec_altmode_work::mutex`.
> The call chain is as follows: 
> 1. cros_typec_altmode_work() acquires the mutex
> 2. typec_altmode_vdm() -> dp_altmode_vdm() ->
> 3. typec_altmode_exit() -> cros_typec_altmode_exit()
> 4. cros_typec_altmode_exit() attempts to acquire the mutex again
> 
> This revert is considered safe as no other known driver sends back
> DP_CMD_STATUS_UPDATE command with the NAK flag.
> 
> Signed-off-by: Andrei Kuchynski <akuchynski@chromium.org>
> ---
>  drivers/usb/typec/altmodes/displayport.c | 4 ----
>  1 file changed, 4 deletions(-)

Why isn't this being sent as a separate patch for 6.16-final?  And why
not put a fixes: line?

thanks,

greg k-h
Re: [PATCH 06/10] Revert "usb: typec: displayport: Receive DP Status Update NAK request exit dp altmode"
Posted by Andrei Kuchynski 3 months, 3 weeks ago 
On Mon, Jun 16, 2025 at 4:15 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 16, 2025 at 01:31:43PM +0000, Andrei Kuchynski wrote:
> > This reverts commit b4b38ffb38c91afd4dc387608db26f6fc34ed40b.
> >
> > The commit introduced a deadlock with the cros_ec_typec driver.
> > The deadlock occurs due to a recursive lock acquisition of
> > `cros_typec_altmode_work::mutex`.
> > The call chain is as follows:
> > 1. cros_typec_altmode_work() acquires the mutex
> > 2. typec_altmode_vdm() -> dp_altmode_vdm() ->
> > 3. typec_altmode_exit() -> cros_typec_altmode_exit()
> > 4. cros_typec_altmode_exit() attempts to acquire the mutex again
> >
> > This revert is considered safe as no other known driver sends back
> > DP_CMD_STATUS_UPDATE command with the NAK flag.
> >
> > Signed-off-by: Andrei Kuchynski <akuchynski@chromium.org>
> > ---
> >  drivers/usb/typec/altmodes/displayport.c | 4 ----
> >  1 file changed, 4 deletions(-)
>
> Why isn't this being sent as a separate patch for 6.16-final?  And why
> not put a fixes: line?
>

Hi Greg,

The issue will emerge only after this series is applied, so 6.16
remains secure as this code is not executable.
I will submit it as a separate patch, with the relevant tags included.

Thanks,
Andrei

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.