LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] usb: typec: tcpm: always rediscover when swapping DR
@ 2021-08-13  4:31 Icenowy Zheng
  2021-08-17  9:36 ` Heikki Krogerus
  0 siblings, 1 reply; 16+ messages in thread
From: Icenowy Zheng @ 2021-08-13  4:31 UTC (permalink / raw)
  To: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Icenowy Zheng

Currently, TCPM code omits discover when swapping to gadget, and assume
that no altmodes are available when swapping from gadget. However, we do
send discover when we get attached as gadget -- this leads to modes to be
discovered twice when attached as gadget and then swap to host.

Always re-send discover when swapping DR, regardless of what change is
being made; and because of this, the assumption that no altmodes are
registered with gadget role is broken, and altmodes de-registeration is
always needed now.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/usb/typec/tcpm/tcpm.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index b9bb63d749ec..ab6d0d51ee1c 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -4495,15 +4495,14 @@ static void run_state_machine(struct tcpm_port *port)
 		tcpm_set_state(port, ready_state(port), 0);
 		break;
 	case DR_SWAP_CHANGE_DR:
-		if (port->data_role == TYPEC_HOST) {
-			tcpm_unregister_altmodes(port);
+		tcpm_unregister_altmodes(port);
+		if (port->data_role == TYPEC_HOST)
 			tcpm_set_roles(port, true, port->pwr_role,
 				       TYPEC_DEVICE);
-		} else {
+		else
 			tcpm_set_roles(port, true, port->pwr_role,
 				       TYPEC_HOST);
-			port->send_discover = true;
-		}
+		port->send_discover = true;
 		tcpm_ams_finish(port);
 		tcpm_set_state(port, ready_state(port), 0);
 		break;
-- 
2.30.2

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-13  4:31 [PATCH] usb: typec: tcpm: always rediscover when swapping DR Icenowy Zheng
@ 2021-08-17  9:36 ` Heikki Krogerus
  2021-08-17  9:41   ` Icenowy Zheng
  2021-08-17 15:13   ` Guenter Roeck
  0 siblings, 2 replies; 16+ messages in thread
From: Heikki Krogerus @ 2021-08-17  9:36 UTC (permalink / raw)
  To: Icenowy Zheng; +Cc: Guenter Roeck, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Aug 13, 2021 at 12:31:31PM +0800, Icenowy Zheng wrote:
> Currently, TCPM code omits discover when swapping to gadget, and assume
> that no altmodes are available when swapping from gadget. However, we do
> send discover when we get attached as gadget -- this leads to modes to be
> discovered twice when attached as gadget and then swap to host.
> 
> Always re-send discover when swapping DR, regardless of what change is
> being made; and because of this, the assumption that no altmodes are
> registered with gadget role is broken, and altmodes de-registeration is
> always needed now.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index b9bb63d749ec..ab6d0d51ee1c 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4495,15 +4495,14 @@ static void run_state_machine(struct tcpm_port *port)
>  		tcpm_set_state(port, ready_state(port), 0);
>  		break;
>  	case DR_SWAP_CHANGE_DR:
> -		if (port->data_role == TYPEC_HOST) {
> -			tcpm_unregister_altmodes(port);
> +		tcpm_unregister_altmodes(port);
> +		if (port->data_role == TYPEC_HOST)
>  			tcpm_set_roles(port, true, port->pwr_role,
>  				       TYPEC_DEVICE);
> -		} else {
> +		else
>  			tcpm_set_roles(port, true, port->pwr_role,
>  				       TYPEC_HOST);
> -			port->send_discover = true;
> -		}
> +		port->send_discover = true;
>  		tcpm_ams_finish(port);
>  		tcpm_set_state(port, ready_state(port), 0);
>  		break;

Why is it necessary to do discovery with data role swap in general?

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-17  9:36 ` Heikki Krogerus
@ 2021-08-17  9:41   ` Icenowy Zheng
  2021-08-17 11:00     ` Heikki Krogerus
  2021-08-17 15:13   ` Guenter Roeck
  1 sibling, 1 reply; 16+ messages in thread
From: Icenowy Zheng @ 2021-08-17  9:41 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Guenter Roeck, Greg Kroah-Hartman, linux-usb, linux-kernel



于 2021年8月17日 GMT+08:00 下午5:36:27, Heikki Krogerus <heikki.krogerus@linux.intel.com> 写到:
>On Fri, Aug 13, 2021 at 12:31:31PM +0800, Icenowy Zheng wrote:
>> Currently, TCPM code omits discover when swapping to gadget, and assume
>> that no altmodes are available when swapping from gadget. However, we do
>> send discover when we get attached as gadget -- this leads to modes to be
>> discovered twice when attached as gadget and then swap to host.
>> 
>> Always re-send discover when swapping DR, regardless of what change is
>> being made; and because of this, the assumption that no altmodes are
>> registered with gadget role is broken, and altmodes de-registeration is
>> always needed now.
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>>  drivers/usb/typec/tcpm/tcpm.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index b9bb63d749ec..ab6d0d51ee1c 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -4495,15 +4495,14 @@ static void run_state_machine(struct tcpm_port *port)
>>  		tcpm_set_state(port, ready_state(port), 0);
>>  		break;
>>  	case DR_SWAP_CHANGE_DR:
>> -		if (port->data_role == TYPEC_HOST) {
>> -			tcpm_unregister_altmodes(port);
>> +		tcpm_unregister_altmodes(port);
>> +		if (port->data_role == TYPEC_HOST)
>>  			tcpm_set_roles(port, true, port->pwr_role,
>>  				       TYPEC_DEVICE);
>> -		} else {
>> +		else
>>  			tcpm_set_roles(port, true, port->pwr_role,
>>  				       TYPEC_HOST);
>> -			port->send_discover = true;
>> -		}
>> +		port->send_discover = true;
>>  		tcpm_ams_finish(port);
>>  		tcpm_set_state(port, ready_state(port), 0);
>>  		break;
>
>Why is it necessary to do discovery with data role swap in general?

I think it could be possible for devices to expose different altmode
with different role.

>
>thanks,
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-17  9:41   ` Icenowy Zheng
@ 2021-08-17 11:00     ` Heikki Krogerus
  2021-08-18 13:56       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 16+ messages in thread
From: Heikki Krogerus @ 2021-08-17 11:00 UTC (permalink / raw)
  To: Icenowy Zheng; +Cc: Guenter Roeck, Greg Kroah-Hartman, linux-usb, linux-kernel

> >Why is it necessary to do discovery with data role swap in general?
> 
> I think it could be possible for devices to expose different altmode
> with different role.

OK. FWIW:

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-17  9:36 ` Heikki Krogerus
  2021-08-17  9:41   ` Icenowy Zheng
@ 2021-08-17 15:13   ` Guenter Roeck
  2021-08-18  8:02     ` Kyle Tso
  1 sibling, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2021-08-17 15:13 UTC (permalink / raw)
  To: Heikki Krogerus, Icenowy Zheng
  Cc: Greg Kroah-Hartman, linux-usb, linux-kernel

On 8/17/21 2:36 AM, Heikki Krogerus wrote:
> On Fri, Aug 13, 2021 at 12:31:31PM +0800, Icenowy Zheng wrote:
>> Currently, TCPM code omits discover when swapping to gadget, and assume
>> that no altmodes are available when swapping from gadget. However, we do
>> send discover when we get attached as gadget -- this leads to modes to be
>> discovered twice when attached as gadget and then swap to host.
>>
>> Always re-send discover when swapping DR, regardless of what change is
>> being made; and because of this, the assumption that no altmodes are
>> registered with gadget role is broken, and altmodes de-registeration is
>> always needed now.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>>   drivers/usb/typec/tcpm/tcpm.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index b9bb63d749ec..ab6d0d51ee1c 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -4495,15 +4495,14 @@ static void run_state_machine(struct tcpm_port *port)
>>   		tcpm_set_state(port, ready_state(port), 0);
>>   		break;
>>   	case DR_SWAP_CHANGE_DR:
>> -		if (port->data_role == TYPEC_HOST) {
>> -			tcpm_unregister_altmodes(port);
>> +		tcpm_unregister_altmodes(port);
>> +		if (port->data_role == TYPEC_HOST)
>>   			tcpm_set_roles(port, true, port->pwr_role,
>>   				       TYPEC_DEVICE);
>> -		} else {
>> +		else
>>   			tcpm_set_roles(port, true, port->pwr_role,
>>   				       TYPEC_HOST);
>> -			port->send_discover = true;
>> -		}
>> +		port->send_discover = true;
>>   		tcpm_ams_finish(port);
>>   		tcpm_set_state(port, ready_state(port), 0);
>>   		break;
> 
> Why is it necessary to do discovery with data role swap in general?
> 
> thanks,
> 

Additional question: There are two patches pending related to DR_SWAP
and discovery. Are they both needed, or do they both solve the same
problem ?

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-17 15:13   ` Guenter Roeck
@ 2021-08-18  8:02     ` Kyle Tso
  2021-08-18  8:16       ` Kyle Tso
                         ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Kyle Tso @ 2021-08-18  8:02 UTC (permalink / raw)
  To: Guenter Roeck, Icenowy Zheng
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
	Badhri Jagan Sridharan

On Tue, Aug 17, 2021 at 11:13 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/17/21 2:36 AM, Heikki Krogerus wrote:
> > On Fri, Aug 13, 2021 at 12:31:31PM +0800, Icenowy Zheng wrote:
> >> Currently, TCPM code omits discover when swapping to gadget, and assume
> >> that no altmodes are available when swapping from gadget. However, we do
> >> send discover when we get attached as gadget -- this leads to modes to be
> >> discovered twice when attached as gadget and then swap to host.
> >>
> >> Always re-send discover when swapping DR, regardless of what change is
> >> being made; and because of this, the assumption that no altmodes are
> >> registered with gadget role is broken, and altmodes de-registeration is
> >> always needed now.
> >>
> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >> ---
> >>   drivers/usb/typec/tcpm/tcpm.c | 9 ++++-----
> >>   1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> >> index b9bb63d749ec..ab6d0d51ee1c 100644
> >> --- a/drivers/usb/typec/tcpm/tcpm.c
> >> +++ b/drivers/usb/typec/tcpm/tcpm.c
> >> @@ -4495,15 +4495,14 @@ static void run_state_machine(struct tcpm_port *port)
> >>              tcpm_set_state(port, ready_state(port), 0);
> >>              break;
> >>      case DR_SWAP_CHANGE_DR:
> >> -            if (port->data_role == TYPEC_HOST) {
> >> -                    tcpm_unregister_altmodes(port);
> >> +            tcpm_unregister_altmodes(port);
> >> +            if (port->data_role == TYPEC_HOST)
> >>                      tcpm_set_roles(port, true, port->pwr_role,
> >>                                     TYPEC_DEVICE);
> >> -            } else {
> >> +            else
> >>                      tcpm_set_roles(port, true, port->pwr_role,
> >>                                     TYPEC_HOST);
> >> -                    port->send_discover = true;
> >> -            }
> >> +            port->send_discover = true;
> >>              tcpm_ams_finish(port);
> >>              tcpm_set_state(port, ready_state(port), 0);
> >>              break;
> >
> > Why is it necessary to do discovery with data role swap in general?
> >
> > thanks,
> >
>
> Additional question: There are two patches pending related to DR_SWAP
> and discovery. Are they both needed, or do they both solve the same
> problem ?
>
> Thanks,
> Guenter

Hi, I just noticed this patch.

Part of this patch and part of my patch
https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com
are to solve the same problem that Discover_Identity is not sent in a
case where the port becomes UFP after DR_SWAP while in PD3.

The difference (for the DR_SWAP part) is that my patch does not set
the flag "send_discover" if the port becomes UFP after PD2 DR_SWAP.
That is because in PD2 Spec, UFP is not allowed to be the SVDM
Initiator.

This patch indeed solves another problem where
tcpm_unregister_altmodes should be called during PD3 DR_SWAP because
the port partner may return mode data in the latest Discover_Mode. For
the PD2 case, I don't think it needs to be called because PD2 DFP will
always return NAK for Discover_Mode. However it is fine because it is
safe to call tcpm_unregister_altmodes even if there is no mode data.

In fact, when I was tracing the code I found another bug. PD2 UFP is
not allowed to send Discover_Identity and Discover_Mode. I can send
another patch to address this problem.

thanks,
Kyle

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-18  8:02     ` Kyle Tso
@ 2021-08-18  8:16       ` Kyle Tso
  2021-08-18  9:20       ` Icenowy Zheng
  2021-08-18  9:27       ` Icenowy Zheng
  2 siblings, 0 replies; 16+ messages in thread
From: Kyle Tso @ 2021-08-18  8:16 UTC (permalink / raw)
  To: Guenter Roeck, Icenowy Zheng
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
	Badhri Jagan Sridharan

On Wed, Aug 18, 2021 at 4:02 PM Kyle Tso <kyletso@google.com> wrote:
>
> On Tue, Aug 17, 2021 at 11:13 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 8/17/21 2:36 AM, Heikki Krogerus wrote:
> > > On Fri, Aug 13, 2021 at 12:31:31PM +0800, Icenowy Zheng wrote:
> > >> Currently, TCPM code omits discover when swapping to gadget, and assume
> > >> that no altmodes are available when swapping from gadget. However, we do
> > >> send discover when we get attached as gadget -- this leads to modes to be
> > >> discovered twice when attached as gadget and then swap to host.
> > >>
> > >> Always re-send discover when swapping DR, regardless of what change is
> > >> being made; and because of this, the assumption that no altmodes are
> > >> registered with gadget role is broken, and altmodes de-registeration is
> > >> always needed now.
> > >>
> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > >> ---
> > >>   drivers/usb/typec/tcpm/tcpm.c | 9 ++++-----
> > >>   1 file changed, 4 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > >> index b9bb63d749ec..ab6d0d51ee1c 100644
> > >> --- a/drivers/usb/typec/tcpm/tcpm.c
> > >> +++ b/drivers/usb/typec/tcpm/tcpm.c
> > >> @@ -4495,15 +4495,14 @@ static void run_state_machine(struct tcpm_port *port)
> > >>              tcpm_set_state(port, ready_state(port), 0);
> > >>              break;
> > >>      case DR_SWAP_CHANGE_DR:
> > >> -            if (port->data_role == TYPEC_HOST) {
> > >> -                    tcpm_unregister_altmodes(port);
> > >> +            tcpm_unregister_altmodes(port);
> > >> +            if (port->data_role == TYPEC_HOST)
> > >>                      tcpm_set_roles(port, true, port->pwr_role,
> > >>                                     TYPEC_DEVICE);
> > >> -            } else {
> > >> +            else
> > >>                      tcpm_set_roles(port, true, port->pwr_role,
> > >>                                     TYPEC_HOST);
> > >> -                    port->send_discover = true;
> > >> -            }
> > >> +            port->send_discover = true;
> > >>              tcpm_ams_finish(port);
> > >>              tcpm_set_state(port, ready_state(port), 0);
> > >>              break;
> > >
> > > Why is it necessary to do discovery with data role swap in general?
> > >
> > > thanks,
> > >
> >
> > Additional question: There are two patches pending related to DR_SWAP
> > and discovery. Are they both needed, or do they both solve the same
> > problem ?
> >
> > Thanks,
> > Guenter
>
> Hi, I just noticed this patch.
>
> Part of this patch and part of my patch
> https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com
> are to solve the same problem that Discover_Identity is not sent in a
> case where the port becomes UFP after DR_SWAP while in PD3.
>
> The difference (for the DR_SWAP part) is that my patch does not set
> the flag "send_discover" if the port becomes UFP after PD2 DR_SWAP.
> That is because in PD2 Spec, UFP is not allowed to be the SVDM
> Initiator.
>

"in PD2 Spec, UFP is not allowed to be the SVDM Initiator."
Sorry this is not correct. The exception is for the cable discovery.
But it doesn't matter here because tcpm.c doesn't support cable
discovery.

thanks,
Kyle

> This patch indeed solves another problem where
> tcpm_unregister_altmodes should be called during PD3 DR_SWAP because
> the port partner may return mode data in the latest Discover_Mode. For
> the PD2 case, I don't think it needs to be called because PD2 DFP will
> always return NAK for Discover_Mode. However it is fine because it is
> safe to call tcpm_unregister_altmodes even if there is no mode data.
>
> In fact, when I was tracing the code I found another bug. PD2 UFP is
> not allowed to send Discover_Identity and Discover_Mode. I can send
> another patch to address this problem.
>
> thanks,
> Kyle

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-18  8:02     ` Kyle Tso
  2021-08-18  8:16       ` Kyle Tso
@ 2021-08-18  9:20       ` Icenowy Zheng
  2021-08-18  9:27       ` Icenowy Zheng
  2 siblings, 0 replies; 16+ messages in thread
From: Icenowy Zheng @ 2021-08-18  9:20 UTC (permalink / raw)
  To: Kyle Tso, Guenter Roeck
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
	Badhri Jagan Sridharan



于 2021年8月18日 GMT+08:00 下午4:02:24, Kyle Tso <kyletso@google.com> 写到:
>On Tue, Aug 17, 2021 at 11:13 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 8/17/21 2:36 AM, Heikki Krogerus wrote:
>> > On Fri, Aug 13, 2021 at 12:31:31PM +0800, Icenowy Zheng wrote:
>> >> Currently, TCPM code omits discover when swapping to gadget, and assume
>> >> that no altmodes are available when swapping from gadget. However, we do
>> >> send discover when we get attached as gadget -- this leads to modes to be
>> >> discovered twice when attached as gadget and then swap to host.
>> >>
>> >> Always re-send discover when swapping DR, regardless of what change is
>> >> being made; and because of this, the assumption that no altmodes are
>> >> registered with gadget role is broken, and altmodes de-registeration is
>> >> always needed now.
>> >>
>> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> >> ---
>> >>   drivers/usb/typec/tcpm/tcpm.c | 9 ++++-----
>> >>   1 file changed, 4 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> >> index b9bb63d749ec..ab6d0d51ee1c 100644
>> >> --- a/drivers/usb/typec/tcpm/tcpm.c
>> >> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> >> @@ -4495,15 +4495,14 @@ static void run_state_machine(struct tcpm_port *port)
>> >>              tcpm_set_state(port, ready_state(port), 0);
>> >>              break;
>> >>      case DR_SWAP_CHANGE_DR:
>> >> -            if (port->data_role == TYPEC_HOST) {
>> >> -                    tcpm_unregister_altmodes(port);
>> >> +            tcpm_unregister_altmodes(port);
>> >> +            if (port->data_role == TYPEC_HOST)
>> >>                      tcpm_set_roles(port, true, port->pwr_role,
>> >>                                     TYPEC_DEVICE);
>> >> -            } else {
>> >> +            else
>> >>                      tcpm_set_roles(port, true, port->pwr_role,
>> >>                                     TYPEC_HOST);
>> >> -                    port->send_discover = true;
>> >> -            }
>> >> +            port->send_discover = true;
>> >>              tcpm_ams_finish(port);
>> >>              tcpm_set_state(port, ready_state(port), 0);
>> >>              break;
>> >
>> > Why is it necessary to do discovery with data role swap in general?
>> >
>> > thanks,
>> >
>>
>> Additional question: There are two patches pending related to DR_SWAP
>> and discovery. Are they both needed, or do they both solve the same
>> problem ?
>>
>> Thanks,
>> Guenter
>
>Hi, I just noticed this patch.
>
>Part of this patch and part of my patch
>https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com
>are to solve the same problem that Discover_Identity is not sent in a
>case where the port becomes UFP after DR_SWAP while in PD3.
>
>The difference (for the DR_SWAP part) is that my patch does not set
>the flag "send_discover" if the port becomes UFP after PD2 DR_SWAP.
>That is because in PD2 Spec, UFP is not allowed to be the SVDM
>Initiator.
>
>This patch indeed solves another problem where
>tcpm_unregister_altmodes should be called during PD3 DR_SWAP because
>the port partner may return mode data in the latest Discover_Mode. For
>the PD2 case, I don't think it needs to be called because PD2 DFP will
>always return NAK for Discover_Mode. However it is fine because it is
>safe to call tcpm_unregister_altmodes even if there is no mode data.
>
>In fact, when I was tracing the code I found another bug. PD2 UFP is
>not allowed to send Discover_Identity and Discover_Mode. I can send
>another patch to address this problem.

Well, to be honest, it's why I send this patch.

I didn't read PD spec before, so I assumed UFP is okay to send
discover, and this is what I got wrong. I should remove the
discover sending flag when we attach as sink.

Will it be okay for me to send this patch? It should help my device here.

>
>thanks,
>Kyle

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-18  8:02     ` Kyle Tso
  2021-08-18  8:16       ` Kyle Tso
  2021-08-18  9:20       ` Icenowy Zheng
@ 2021-08-18  9:27       ` Icenowy Zheng
  2021-08-18 10:47         ` Kyle Tso
  2 siblings, 1 reply; 16+ messages in thread
From: Icenowy Zheng @ 2021-08-18  9:27 UTC (permalink / raw)
  To: Kyle Tso, Guenter Roeck
  Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
	Badhri Jagan Sridharan



于 2021年8月18日 GMT+08:00 下午4:02:24, Kyle Tso <kyletso@google.com> 写到:
>On Tue, Aug 17, 2021 at 11:13 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 8/17/21 2:36 AM, Heikki Krogerus wrote:
>> > On Fri, Aug 13, 2021 at 12:31:31PM +0800, Icenowy Zheng wrote:
>> >> Currently, TCPM code omits discover when swapping to gadget, and assume
>> >> that no altmodes are available when swapping from gadget. However, we do
>> >> send discover when we get attached as gadget -- this leads to modes to be
>> >> discovered twice when attached as gadget and then swap to host.
>> >>
>> >> Always re-send discover when swapping DR, regardless of what change is
>> >> being made; and because of this, the assumption that no altmodes are
>> >> registered with gadget role is broken, and altmodes de-registeration is
>> >> always needed now.
>> >>
>> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> >> ---
>> >>   drivers/usb/typec/tcpm/tcpm.c | 9 ++++-----
>> >>   1 file changed, 4 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> >> index b9bb63d749ec..ab6d0d51ee1c 100644
>> >> --- a/drivers/usb/typec/tcpm/tcpm.c
>> >> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> >> @@ -4495,15 +4495,14 @@ static void run_state_machine(struct tcpm_port *port)
>> >>              tcpm_set_state(port, ready_state(port), 0);
>> >>              break;
>> >>      case DR_SWAP_CHANGE_DR:
>> >> -            if (port->data_role == TYPEC_HOST) {
>> >> -                    tcpm_unregister_altmodes(port);
>> >> +            tcpm_unregister_altmodes(port);
>> >> +            if (port->data_role == TYPEC_HOST)
>> >>                      tcpm_set_roles(port, true, port->pwr_role,
>> >>                                     TYPEC_DEVICE);
>> >> -            } else {
>> >> +            else
>> >>                      tcpm_set_roles(port, true, port->pwr_role,
>> >>                                     TYPEC_HOST);
>> >> -                    port->send_discover = true;
>> >> -            }
>> >> +            port->send_discover = true;
>> >>              tcpm_ams_finish(port);
>> >>              tcpm_set_state(port, ready_state(port), 0);
>> >>              break;
>> >
>> > Why is it necessary to do discovery with data role swap in general?
>> >
>> > thanks,
>> >
>>
>> Additional question: There are two patches pending related to DR_SWAP
>> and discovery. Are they both needed, or do they both solve the same
>> problem ?
>>
>> Thanks,
>> Guenter
>
>Hi, I just noticed this patch.
>
>Part of this patch and part of my patch
>https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com
>are to solve the same problem that Discover_Identity is not sent in a
>case where the port becomes UFP after DR_SWAP while in PD3.
>
>The difference (for the DR_SWAP part) is that my patch does not set
>the flag "send_discover" if the port becomes UFP after PD2 DR_SWAP.
>That is because in PD2 Spec, UFP is not allowed to be the SVDM
>Initiator.
>
>This patch indeed solves another problem where
>tcpm_unregister_altmodes should be called during PD3 DR_SWAP because
>the port partner may return mode data in the latest Discover_Mode. For
>the PD2 case, I don't think it needs to be called because PD2 DFP will
>always return NAK for Discover_Mode. However it is fine because it is

It sounds like my dock is doing something against the specification...

It sends altmodes rather than NAK when my board gets attached as
UFP and send bogus discovers (because of bugs of current TCPM
code). These altmodes are then registered in TCPM and never get
cleaned up within it, which leads to conflict when the dock is
removed and then re-inserted.

(BTW is it neceesary for data role and power role to be the same
when attaching? My board now gets attached as UFP to drain
power, and then do DR_SWAP to become USB host.)

>safe to call tcpm_unregister_altmodes even if there is no mode data.
>
>In fact, when I was tracing the code I found another bug. PD2 UFP is
>not allowed to send Discover_Identity and Discover_Mode. I can send
>another patch to address this problem.
>
>thanks,
>Kyle

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-18  9:27       ` Icenowy Zheng
@ 2021-08-18 10:47         ` Kyle Tso
  0 siblings, 0 replies; 16+ messages in thread
From: Kyle Tso @ 2021-08-18 10:47 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: Guenter Roeck, Heikki Krogerus, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Badhri Jagan Sridharan

On Wed, Aug 18, 2021 at 5:27 PM Icenowy Zheng <icenowy@aosc.io> wrote:
>
>
>
> 于 2021年8月18日 GMT+08:00 下午4:02:24, Kyle Tso <kyletso@google.com> 写到:
> >On Tue, Aug 17, 2021 at 11:13 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 8/17/21 2:36 AM, Heikki Krogerus wrote:
> >> > On Fri, Aug 13, 2021 at 12:31:31PM +0800, Icenowy Zheng wrote:
> >> >> Currently, TCPM code omits discover when swapping to gadget, and assume
> >> >> that no altmodes are available when swapping from gadget. However, we do
> >> >> send discover when we get attached as gadget -- this leads to modes to be
> >> >> discovered twice when attached as gadget and then swap to host.
> >> >>
> >> >> Always re-send discover when swapping DR, regardless of what change is
> >> >> being made; and because of this, the assumption that no altmodes are
> >> >> registered with gadget role is broken, and altmodes de-registeration is
> >> >> always needed now.
> >> >>
> >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >> >> ---
> >> >>   drivers/usb/typec/tcpm/tcpm.c | 9 ++++-----
> >> >>   1 file changed, 4 insertions(+), 5 deletions(-)
> >> >>
> >> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> >> >> index b9bb63d749ec..ab6d0d51ee1c 100644
> >> >> --- a/drivers/usb/typec/tcpm/tcpm.c
> >> >> +++ b/drivers/usb/typec/tcpm/tcpm.c
> >> >> @@ -4495,15 +4495,14 @@ static void run_state_machine(struct tcpm_port *port)
> >> >>              tcpm_set_state(port, ready_state(port), 0);
> >> >>              break;
> >> >>      case DR_SWAP_CHANGE_DR:
> >> >> -            if (port->data_role == TYPEC_HOST) {
> >> >> -                    tcpm_unregister_altmodes(port);
> >> >> +            tcpm_unregister_altmodes(port);
> >> >> +            if (port->data_role == TYPEC_HOST)
> >> >>                      tcpm_set_roles(port, true, port->pwr_role,
> >> >>                                     TYPEC_DEVICE);
> >> >> -            } else {
> >> >> +            else
> >> >>                      tcpm_set_roles(port, true, port->pwr_role,
> >> >>                                     TYPEC_HOST);
> >> >> -                    port->send_discover = true;
> >> >> -            }
> >> >> +            port->send_discover = true;
> >> >>              tcpm_ams_finish(port);
> >> >>              tcpm_set_state(port, ready_state(port), 0);
> >> >>              break;
> >> >
> >> > Why is it necessary to do discovery with data role swap in general?
> >> >
> >> > thanks,
> >> >
> >>
> >> Additional question: There are two patches pending related to DR_SWAP
> >> and discovery. Are they both needed, or do they both solve the same
> >> problem ?
> >>
> >> Thanks,
> >> Guenter
> >
> >Hi, I just noticed this patch.
> >
> >Part of this patch and part of my patch
> >https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com
> >are to solve the same problem that Discover_Identity is not sent in a
> >case where the port becomes UFP after DR_SWAP while in PD3.
> >
> >The difference (for the DR_SWAP part) is that my patch does not set
> >the flag "send_discover" if the port becomes UFP after PD2 DR_SWAP.
> >That is because in PD2 Spec, UFP is not allowed to be the SVDM
> >Initiator.
> >
> >This patch indeed solves another problem where
> >tcpm_unregister_altmodes should be called during PD3 DR_SWAP because
> >the port partner may return mode data in the latest Discover_Mode. For
> >the PD2 case, I don't think it needs to be called because PD2 DFP will
> >always return NAK for Discover_Mode. However it is fine because it is
>
> It sounds like my dock is doing something against the specification...
>
> It sends altmodes rather than NAK when my board gets attached as
> UFP and send bogus discovers (because of bugs of current TCPM
> code). These altmodes are then registered in TCPM and never get
> cleaned up within it, which leads to conflict when the dock is
> removed and then re-inserted.
>

Could you provide more details?
1. Is the connection in PD3 ?
2. Could you provide the tcpm logs?

> (BTW is it neceesary for data role and power role to be the same
> when attaching? My board now gets attached as UFP to drain
> power, and then do DR_SWAP to become USB host.)
>

Either Sink/UFP or Source/DFP in the beginning. Then DR_SWAP is okay
when both ports are in ready state. Your dock looks like a Sourcing
Device.

Type-C Spec:
```
4.8.4 Sourcing Device

A Sourcing Device is a special sub-class of a DRP that is capable of
supplying power, but is not capable of acting as a USB host. For
example a hub’s UFP or a monitor’s UFP that operates as a device but
not as a host.

The Sourcing Device shall follow the rules for a DRP (See Section
4.5.1.4 and Figure 4-15). It shall also follow the requirements for
the Source as Power Source (See Section 4.8.1). The Sourcing Device
shall support USB PD and shall support the DR_Swap command in order to
enable the Source to assume the UFP data role.
```

thanks,
Kyle

> >safe to call tcpm_unregister_altmodes even if there is no mode data.
> >
> >In fact, when I was tracing the code I found another bug. PD2 UFP is
> >not allowed to send Discover_Identity and Discover_Mode. I can send
> >another patch to address this problem.
> >
> >thanks,
> >Kyle

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-17 11:00     ` Heikki Krogerus
@ 2021-08-18 13:56       ` Greg Kroah-Hartman
  2021-08-18 14:02         ` Icenowy Zheng
  0 siblings, 1 reply; 16+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-18 13:56 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: Icenowy Zheng, Guenter Roeck, linux-usb, linux-kernel

On Tue, Aug 17, 2021 at 02:00:33PM +0300, Heikki Krogerus wrote:
> > >Why is it necessary to do discovery with data role swap in general?
> > 
> > I think it could be possible for devices to expose different altmode
> > with different role.
> 
> OK. FWIW:
> 
> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Is this conflicting with https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com ?

Which of these two should I take, both?  Neither?

confused,

greg k-h

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-18 13:56       ` Greg Kroah-Hartman
@ 2021-08-18 14:02         ` Icenowy Zheng
  2021-08-18 14:40           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 16+ messages in thread
From: Icenowy Zheng @ 2021-08-18 14:02 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Heikki Krogerus
  Cc: Guenter Roeck, linux-usb, linux-kernel



于 2021年8月18日 GMT+08:00 下午9:56:38, Greg Kroah-Hartman <gregkh@linuxfoundation.org> 写到:
>On Tue, Aug 17, 2021 at 02:00:33PM +0300, Heikki Krogerus wrote:
>> > >Why is it necessary to do discovery with data role swap in general?
>> > 
>> > I think it could be possible for devices to expose different altmode
>> > with different role.
>> 
>> OK. FWIW:
>> 
>> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>
>Is this conflicting with https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com ?
>
>Which of these two should I take, both?  Neither?

Don't take this, it's against spec.

Thanks,
Icenowy

>
>confused,
>
>greg k-h

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-18 14:02         ` Icenowy Zheng
@ 2021-08-18 14:40           ` Greg Kroah-Hartman
  2021-08-18 16:13             ` Kyle Tso
  0 siblings, 1 reply; 16+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-18 14:40 UTC (permalink / raw)
  To: Icenowy Zheng; +Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel

On Wed, Aug 18, 2021 at 10:02:24PM +0800, Icenowy Zheng wrote:
> 
> 
> 于 2021年8月18日 GMT+08:00 下午9:56:38, Greg Kroah-Hartman <gregkh@linuxfoundation.org> 写到:
> >On Tue, Aug 17, 2021 at 02:00:33PM +0300, Heikki Krogerus wrote:
> >> > >Why is it necessary to do discovery with data role swap in general?
> >> > 
> >> > I think it could be possible for devices to expose different altmode
> >> > with different role.
> >> 
> >> OK. FWIW:
> >> 
> >> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> >Is this conflicting with https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com ?
> >
> >Which of these two should I take, both?  Neither?
> 
> Don't take this, it's against spec.

Ok, now dropped.  What about the linked patch above?  Does that work for
you instead?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-18 14:40           ` Greg Kroah-Hartman
@ 2021-08-18 16:13             ` Kyle Tso
  2021-08-18 16:17               ` Icenowy Zheng
  2021-08-18 16:24               ` Icenowy Zheng
  0 siblings, 2 replies; 16+ messages in thread
From: Kyle Tso @ 2021-08-18 16:13 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Icenowy Zheng, Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel

On Wed, Aug 18, 2021 at 10:40 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Aug 18, 2021 at 10:02:24PM +0800, Icenowy Zheng wrote:
> >
> >
> > 于 2021年8月18日 GMT+08:00 下午9:56:38, Greg Kroah-Hartman <gregkh@linuxfoundation.org> 写到:
> > >On Tue, Aug 17, 2021 at 02:00:33PM +0300, Heikki Krogerus wrote:
> > >> > >Why is it necessary to do discovery with data role swap in general?
> > >> >
> > >> > I think it could be possible for devices to expose different altmode
> > >> > with different role.
> > >>
> > >> OK. FWIW:
> > >>
> > >> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > >
> > >Is this conflicting with https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com ?
> > >
> > >Which of these two should I take, both?  Neither?
> >
> > Don't take this, it's against spec.
>
> Ok, now dropped.  What about the linked patch above?  Does that work for
> you instead?
>
> thanks,
>
> greg k-h

Hi Icenowy,

Could you revisit the patch for the tcpm_unregister_altmodes part?
I think that is still a problem.

thanks,
Kyle

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-18 16:13             ` Kyle Tso
@ 2021-08-18 16:17               ` Icenowy Zheng
  2021-08-18 16:24               ` Icenowy Zheng
  1 sibling, 0 replies; 16+ messages in thread
From: Icenowy Zheng @ 2021-08-18 16:17 UTC (permalink / raw)
  To: Kyle Tso, Greg Kroah-Hartman
  Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel

在 2021-08-19星期四的 00:13 +0800,Kyle Tso写道:
> On Wed, Aug 18, 2021 at 10:40 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > 
> > On Wed, Aug 18, 2021 at 10:02:24PM +0800, Icenowy Zheng wrote:
> > > 
> > > 
> > > 于 2021年8月18日 GMT+08:00 下午9:56:38, Greg Kroah-Hartman < 
> > > gregkh@linuxfoundation.org> 写到:
> > > > On Tue, Aug 17, 2021 at 02:00:33PM +0300, Heikki Krogerus
> > > > wrote:
> > > > > > > Why is it necessary to do discovery with data role swap
> > > > > > > in general?
> > > > > > 
> > > > > > I think it could be possible for devices to expose
> > > > > > different altmode
> > > > > > with different role.
> > > > > 
> > > > > OK. FWIW:
> > > > > 
> > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > 
> > > > Is this conflicting with  
> > > > https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com
> > > >  ?
> > > > 
> > > > Which of these two should I take, both?  Neither?
> > > 
> > > Don't take this, it's against spec.
> > 
> > Ok, now dropped.  What about the linked patch above?  Does that
> > work for
> > you instead?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Icenowy,
> 
> Could you revisit the patch for the tcpm_unregister_altmodes part?
> I think that is still a problem.

Well I think it's okay, but I know none about PD3, and I have no PD3
device either.

So I prefer to continue on fix on dropping the discover sent on
snk_attach instead of working on this, because I know nothing about
this, and in my limited knowledge the code looks fine here.

> 
> thanks,
> Kyle


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR
  2021-08-18 16:13             ` Kyle Tso
  2021-08-18 16:17               ` Icenowy Zheng
@ 2021-08-18 16:24               ` Icenowy Zheng
  1 sibling, 0 replies; 16+ messages in thread
From: Icenowy Zheng @ 2021-08-18 16:24 UTC (permalink / raw)
  To: Kyle Tso, Greg Kroah-Hartman
  Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel

在 2021-08-19星期四的 00:13 +0800,Kyle Tso写道:
> On Wed, Aug 18, 2021 at 10:40 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > 
> > On Wed, Aug 18, 2021 at 10:02:24PM +0800, Icenowy Zheng wrote:
> > > 
> > > 
> > > 于 2021年8月18日 GMT+08:00 下午9:56:38, Greg Kroah-Hartman < 
> > > gregkh@linuxfoundation.org> 写到:
> > > > On Tue, Aug 17, 2021 at 02:00:33PM +0300, Heikki Krogerus
> > > > wrote:
> > > > > > > Why is it necessary to do discovery with data role swap
> > > > > > > in general?
> > > > > > 
> > > > > > I think it could be possible for devices to expose
> > > > > > different altmode
> > > > > > with different role.
> > > > > 
> > > > > OK. FWIW:
> > > > > 
> > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > 
> > > > Is this conflicting with  
> > > > https://lore.kernel.org/r/20210816075449.2236547-1-kyletso@google.com
> > > >  ?
> > > > 
> > > > Which of these two should I take, both?  Neither?
> > > 
> > > Don't take this, it's against spec.
> > 
> > Ok, now dropped.  What about the linked patch above?  Does that
> > work for
> > you instead?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Icenowy,
> 
> Could you revisit the patch for the tcpm_unregister_altmodes part?
> I think that is still a problem.

Well, I must admit that only doing a tcpm_unregister_altmodes() should
be harmless.

> 
> thanks,
> Kyle


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-08-18 16:25 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13  4:31 [PATCH] usb: typec: tcpm: always rediscover when swapping DR Icenowy Zheng
2021-08-17  9:36 ` Heikki Krogerus
2021-08-17  9:41   ` Icenowy Zheng
2021-08-17 11:00     ` Heikki Krogerus
2021-08-18 13:56       ` Greg Kroah-Hartman
2021-08-18 14:02         ` Icenowy Zheng
2021-08-18 14:40           ` Greg Kroah-Hartman
2021-08-18 16:13             ` Kyle Tso
2021-08-18 16:17               ` Icenowy Zheng
2021-08-18 16:24               ` Icenowy Zheng
2021-08-17 15:13   ` Guenter Roeck
2021-08-18  8:02     ` Kyle Tso
2021-08-18  8:16       ` Kyle Tso
2021-08-18  9:20       ` Icenowy Zheng
2021-08-18  9:27       ` Icenowy Zheng
2021-08-18 10:47         ` Kyle Tso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).