[PATCH] usb: typec: altmodes/displayport: Update active state

[PATCH] usb: typec: altmodes/displayport: Update active state

[Prashant Malani]

Update the altmode "active" state when we receive Acks for Enter and
Exit Mode commands. Having the right state is necessary to change Pin
Assignments using the 'pin_assignment" sysfs file.

Fixes: 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
Cc: stable@vger.kernel.org
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/usb/typec/altmodes/displayport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 06fb4732f8cd..bc1c556944d6 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -277,9 +277,11 @@ static int dp_altmode_vdm(struct typec_altmode *alt,
 	case CMDT_RSP_ACK:
 		switch (cmd) {
 		case CMD_ENTER_MODE:
+			typec_altmode_update_active(alt, true);
 			dp->state = DP_STATE_UPDATE;
 			break;
 		case CMD_EXIT_MODE:
+			typec_altmode_update_active(alt, false);
 			dp->data.status = 0;
 			dp->data.conf = 0;
 			break;
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

Hi Prashant,

On Wed, Jan 18, 2023 at 03:15:15AM +0000, Prashant Malani wrote:
> Update the altmode "active" state when we receive Acks for Enter and
> Exit Mode commands. Having the right state is necessary to change Pin
> Assignments using the 'pin_assignment" sysfs file.
> 
> Fixes: 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
> Cc: stable@vger.kernel.org
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Reviewed-by: Benson Leung <bleung@chromium.org>


> ---
>  drivers/usb/typec/altmodes/displayport.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 06fb4732f8cd..bc1c556944d6 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -277,9 +277,11 @@ static int dp_altmode_vdm(struct typec_altmode *alt,
>  	case CMDT_RSP_ACK:
>  		switch (cmd) {
>  		case CMD_ENTER_MODE:
> +			typec_altmode_update_active(alt, true);
>  			dp->state = DP_STATE_UPDATE;
>  			break;
>  		case CMD_EXIT_MODE:
> +			typec_altmode_update_active(alt, false);
>  			dp->data.status = 0;
>  			dp->data.conf = 0;
>  			break;
> -- 
> 2.39.0.314.g84b9a713c41-goog
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Heikki Krogerus]

On Wed, Jan 18, 2023 at 03:15:15AM +0000, Prashant Malani wrote:
> Update the altmode "active" state when we receive Acks for Enter and
> Exit Mode commands. Having the right state is necessary to change Pin
> Assignments using the 'pin_assignment" sysfs file.

The idea was that the port drivers take care of this, not the altmode
drivers.

thanks,

> Fixes: 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
> Cc: stable@vger.kernel.org
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  drivers/usb/typec/altmodes/displayport.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 06fb4732f8cd..bc1c556944d6 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -277,9 +277,11 @@ static int dp_altmode_vdm(struct typec_altmode *alt,
>  	case CMDT_RSP_ACK:
>  		switch (cmd) {
>  		case CMD_ENTER_MODE:
> +			typec_altmode_update_active(alt, true);
>  			dp->state = DP_STATE_UPDATE;
>  			break;
>  		case CMD_EXIT_MODE:
> +			typec_altmode_update_active(alt, false);
>  			dp->data.status = 0;
>  			dp->data.conf = 0;
>  			break;
> -- 
> 2.39.0.314.g84b9a713c41-goog

-- 
heikki

Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Prashant Malani]

Hi Heikki,

Thanks for reviewing the patch.

On Wed, Jan 18, 2023 at 1:39 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Jan 18, 2023 at 03:15:15AM +0000, Prashant Malani wrote:
> > Update the altmode "active" state when we receive Acks for Enter and
> > Exit Mode commands. Having the right state is necessary to change Pin
> > Assignments using the 'pin_assignment" sysfs file.
>
> The idea was that the port drivers take care of this, not the altmode
> drivers.

For the port's typec_altmode struct, that makes sense.
Should the port driver be taking care of the state for the partner's altmode
too, i.e "/sys/class/typec/port1-partner/port1-partner.0/active" ?

It seemed like the port driver should be forwarding the VDMs without snooping
the header, or IOW, it should let the altmode driver parse the VDMs (which it's
doing in this case) and manage the partner altmode state.

"pin_assignment_store" seems to only work if the partner's altmode
"active" bit is set to active [1]

FWIW, I think we can make the typec_altmode_update_active() calls from
our (cros-ec-typec) port driver too, but displayport.c is parsing the header
anyway, so it seemed repetitive. Just wanted to clarify the intention here.

BR,

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/typec/altmodes/displayport.c#n474

Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Heikki Krogerus]

Hi Prashant,

On Wed, Jan 18, 2023 at 10:26:21AM -0800, Prashant Malani wrote:
> Hi Heikki,
> 
> Thanks for reviewing the patch.
> 
> On Wed, Jan 18, 2023 at 1:39 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Wed, Jan 18, 2023 at 03:15:15AM +0000, Prashant Malani wrote:
> > > Update the altmode "active" state when we receive Acks for Enter and
> > > Exit Mode commands. Having the right state is necessary to change Pin
> > > Assignments using the 'pin_assignment" sysfs file.
> >
> > The idea was that the port drivers take care of this, not the altmode
> > drivers.
> 
> For the port's typec_altmode struct, that makes sense.
> Should the port driver be taking care of the state for the partner's altmode
> too, i.e "/sys/class/typec/port1-partner/port1-partner.0/active" ?
> 
> It seemed like the port driver should be forwarding the VDMs without snooping
> the header, or IOW, it should let the altmode driver parse the VDMs (which it's
> doing in this case) and manage the partner altmode state.
> 
> "pin_assignment_store" seems to only work if the partner's altmode
> "active" bit is set to active [1]
> 
> FWIW, I think we can make the typec_altmode_update_active() calls from
> our (cros-ec-typec) port driver too, but displayport.c is parsing the header
> anyway, so it seemed repetitive. Just wanted to clarify the intention here.

The alt modes may have been entered even if there are no drivers for
them, if for example the PD controller handles the mode entry. In
those cases the port driver needs to update the active state of the
partner alt mode.

Since the port drivers have to handle that in some cases, for the sake
of consistency I thought that they might as well take care of it in
every case.

On the other hand, it should be safe to do it in both the port driver
and the altmode driver.

If you prefer that the altmode drivers always do this, I'm not against
it. But in that case could you patch tcpm.c while at it - in the same
series:

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 904c7b4ce2f0c..0f5a9d4db105a 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1693,14 +1693,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
                        }
                        break;
                case CMD_ENTER_MODE:
-                       if (adev && pdev) {
-                               typec_altmode_update_active(pdev, true);
+                       if (adev && pdev)
                                *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL;
-                       }
                        return 0;
                case CMD_EXIT_MODE:
                        if (adev && pdev) {
-                               typec_altmode_update_active(pdev, false);
                                /* Back to USB Operation */
                                *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
                                return 0;

That's the only driver that will definitely always requires the
altmode drivers, so perhaps it would be good to drop the calls
from it at the same time.

thanks,

-- 
heikki

Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Prashant Malani]

On Thu, Jan 19, 2023 at 1:26 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Prashant,
>
> On Wed, Jan 18, 2023 at 10:26:21AM -0800, Prashant Malani wrote:
> > Hi Heikki,
> >
> > Thanks for reviewing the patch.
> >
> > On Wed, Jan 18, 2023 at 1:39 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Wed, Jan 18, 2023 at 03:15:15AM +0000, Prashant Malani wrote:
> > FWIW, I think we can make the typec_altmode_update_active() calls from
> > our (cros-ec-typec) port driver too, but displayport.c is parsing the header
> > anyway, so it seemed repetitive. Just wanted to clarify the intention here.
>
> The alt modes may have been entered even if there are no drivers for
> them, if for example the PD controller handles the mode entry. In
> those cases the port driver needs to update the active state of the
> partner alt mode.

Ack. Thanks for explaining the rationale here.

>
> Since the port drivers have to handle that in some cases, for the sake
> of consistency I thought that they might as well take care of it in
> every case.
>
> On the other hand, it should be safe to do it in both the port driver
> and the altmode driver.
>
> If you prefer that the altmode drivers always do this, I'm not against
> it. But in that case could you patch tcpm.c while at it - in the same
> series:

Sure, I will send out a v2 with the below diff as Patch 2/2 (I will mark you as
"Suggested-by" but as always LMK if you prefer another way to
denote attribution).

>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 904c7b4ce2f0c..0f5a9d4db105a 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1693,14 +1693,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>                         }
>                         break;
>                 case CMD_ENTER_MODE:
> -                       if (adev && pdev) {
> -                               typec_altmode_update_active(pdev, true);
> +                       if (adev && pdev)
>                                 *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL;
> -                       }
>                         return 0;
>                 case CMD_EXIT_MODE:
>                         if (adev && pdev) {
> -                               typec_altmode_update_active(pdev, false);
>                                 /* Back to USB Operation */
>                                 *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
>                                 return 0;
>
> That's the only driver that will definitely always requires the
> altmode drivers, so perhaps it would be good to drop the calls
> from it at the same time.
>
> thanks,
>
> --
> heikki

Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Prashant Malani]

On Thu, Jan 19, 2023 at 1:55 AM Prashant Malani <pmalani@chromium.org> wrote:
>
> On Thu, Jan 19, 2023 at 1:26 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Prashant,
> >
> > On Wed, Jan 18, 2023 at 10:26:21AM -0800, Prashant Malani wrote:
> > > Hi Heikki,
> > >
> > > Thanks for reviewing the patch.
> > >
> > > On Wed, Jan 18, 2023 at 1:39 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > On Wed, Jan 18, 2023 at 03:15:15AM +0000, Prashant Malani wrote:
> > > FWIW, I think we can make the typec_altmode_update_active() calls from
> > > our (cros-ec-typec) port driver too, but displayport.c is parsing the header
> > > anyway, so it seemed repetitive. Just wanted to clarify the intention here.
> >
> > The alt modes may have been entered even if there are no drivers for
> > them, if for example the PD controller handles the mode entry. In
> > those cases the port driver needs to update the active state of the
> > partner alt mode.
>
> Ack. Thanks for explaining the rationale here.
>
> >
> > Since the port drivers have to handle that in some cases, for the sake
> > of consistency I thought that they might as well take care of it in
> > every case.
> >
> > On the other hand, it should be safe to do it in both the port driver
> > and the altmode driver.
> >
> > If you prefer that the altmode drivers always do this, I'm not against
> > it. But in that case could you patch tcpm.c while at it - in the same
> > series:
>
> Sure, I will send out a v2 with the below diff as Patch 2/2 (I will mark you as
> "Suggested-by" but as always LMK if you prefer another way to
> denote attribution).
>
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 904c7b4ce2f0c..0f5a9d4db105a 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -1693,14 +1693,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
> >                         }
> >                         break;
> >                 case CMD_ENTER_MODE:
> > -                       if (adev && pdev) {
> > -                               typec_altmode_update_active(pdev, true);
> > +                       if (adev && pdev)
> >                                 *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL;
> > -                       }
> >                         return 0;
> >                 case CMD_EXIT_MODE:
> >                         if (adev && pdev) {
> > -                               typec_altmode_update_active(pdev, false);
> >                                 /* Back to USB Operation */
> >                                 *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
> >                                 return 0;
> >
> > That's the only driver that will definitely always requires the
> > altmode drivers, so perhaps it would be good to drop the calls
> > from it at the same time.

On 2nd thought, would it be safe to drop the calls in tcpm.c ? Following
on from your PD controller example above, TCPM might be updating
the active state for an altmode which doesn't have an altmode driver
registered? Or does it only send out ENTER_MODE for alt modes
which have an altmode driver?

(Sorry if this is obvious to TCPM users, but I wanted to confirm before
proceeding with a v2).

> >
> > thanks,
> >
> > --
> > heikki

Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Heikki Krogerus]

On Thu, Jan 19, 2023 at 02:12:20AM -0800, Prashant Malani wrote:
> On Thu, Jan 19, 2023 at 1:55 AM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > On Thu, Jan 19, 2023 at 1:26 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > Hi Prashant,
> > >
> > > On Wed, Jan 18, 2023 at 10:26:21AM -0800, Prashant Malani wrote:
> > > > Hi Heikki,
> > > >
> > > > Thanks for reviewing the patch.
> > > >
> > > > On Wed, Jan 18, 2023 at 1:39 AM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > >
> > > > > On Wed, Jan 18, 2023 at 03:15:15AM +0000, Prashant Malani wrote:
> > > > FWIW, I think we can make the typec_altmode_update_active() calls from
> > > > our (cros-ec-typec) port driver too, but displayport.c is parsing the header
> > > > anyway, so it seemed repetitive. Just wanted to clarify the intention here.
> > >
> > > The alt modes may have been entered even if there are no drivers for
> > > them, if for example the PD controller handles the mode entry. In
> > > those cases the port driver needs to update the active state of the
> > > partner alt mode.
> >
> > Ack. Thanks for explaining the rationale here.
> >
> > >
> > > Since the port drivers have to handle that in some cases, for the sake
> > > of consistency I thought that they might as well take care of it in
> > > every case.
> > >
> > > On the other hand, it should be safe to do it in both the port driver
> > > and the altmode driver.
> > >
> > > If you prefer that the altmode drivers always do this, I'm not against
> > > it. But in that case could you patch tcpm.c while at it - in the same
> > > series:
> >
> > Sure, I will send out a v2 with the below diff as Patch 2/2 (I will mark you as
> > "Suggested-by" but as always LMK if you prefer another way to
> > denote attribution).
> >
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > index 904c7b4ce2f0c..0f5a9d4db105a 100644
> > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > @@ -1693,14 +1693,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
> > >                         }
> > >                         break;
> > >                 case CMD_ENTER_MODE:
> > > -                       if (adev && pdev) {
> > > -                               typec_altmode_update_active(pdev, true);
> > > +                       if (adev && pdev)
> > >                                 *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL;
> > > -                       }
> > >                         return 0;
> > >                 case CMD_EXIT_MODE:
> > >                         if (adev && pdev) {
> > > -                               typec_altmode_update_active(pdev, false);
> > >                                 /* Back to USB Operation */
> > >                                 *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM;
> > >                                 return 0;
> > >
> > > That's the only driver that will definitely always requires the
> > > altmode drivers, so perhaps it would be good to drop the calls
> > > from it at the same time.
> 
> On 2nd thought, would it be safe to drop the calls in tcpm.c ? Following
> on from your PD controller example above, TCPM might be updating
> the active state for an altmode which doesn't have an altmode driver
> registered? Or does it only send out ENTER_MODE for alt modes
> which have an altmode driver?
> 
> (Sorry if this is obvious to TCPM users, but I wanted to confirm before
> proceeding with a v2).

It's not be possible to enter a mode with tcpm.c unless there is
a driver for the altmode currently. Something has to take care of the
altmode, and if that something is not the altmode driver it would need
to be the user space. Right now we don't have an interface for that.

In any case, if there's no driver for the altmode, then the partner
altmode "active" file should not be visible.

thanks,

-- 
heikki

Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Heikki Krogerus]

On Fri, Jan 20, 2023 at 11:16:43AM +0200, Heikki Krogerus wrote:
> It's not be possible to enter a mode with tcpm.c unless there is
> a driver for the altmode currently. Something has to take care of the
> altmode, and if that something is not the altmode driver it would need
> to be the user space. Right now we don't have an interface for that.
> 
> In any case, if there's no driver for the altmode, then the partner
> altmode "active" file should not be visible.

I meant read-only :-).

thanks,

-- 
heikki

Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Prashant Malani]

On Fri, Jan 20, 2023 at 1:23 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Fri, Jan 20, 2023 at 11:16:43AM +0200, Heikki Krogerus wrote:
> > It's not be possible to enter a mode with tcpm.c unless there is
> > a driver for the altmode currently. Something has to take care of the
> > altmode, and if that something is not the altmode driver it would need
> > to be the user space. Right now we don't have an interface for that.
> >
> > In any case, if there's no driver for the altmode, then the partner
> > altmode "active" file should not be visible.
>
> I meant read-only :-).

Got it. Thank you for explaining this to me :)

I will send out a v2 soon.

BR,

-Prashant