On Tue, Jan 12, 2021 at 9:29 PM Heikki Krogerus
[off-list ref] wrote:

On Wed, Jan 06, 2021 at 12:39:25AM +0800, Kyle Tso wrote:

quoted

This patch provides the implementation of Collision Avoidance introduced
in PD3.0. The start of each Atomic Message Sequence (AMS) initiated by
the port will be denied if the current AMS is not interruptible. The
Source port will set the CC to SinkTxNG if it is going to initiate an
AMS, and SinkTxOk otherwise. Meanwhile, any AMS initiated by a Sink port
will be denied in TCPM if the port partner (Source) sets SinkTxNG except
for HARD_RESET and SOFT_RESET.

Signed-off-by: Kyle Tso <redacted>
Signed-off-by: Will McVicker <redacted>

So did you and Will develop this patch together?

Not really.
Will cherry-picked the patch from our old branch to a later one which
is more close to Upstream.
And I cherry-picked his version so the signed-off is here.
I will remove the signed-off if that is the right move.

Few nitpicks below.

quoted

+static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status cc)
+{
+     tcpm_log(port, "cc:=%d", cc);
+     port->cc_req = cc;
+     port->tcpc->set_cc(port->tcpc, cc);
+}
+
+static enum typec_cc_status tcpm_rp_cc(struct tcpm_port *port);

I think you should move the function here instead of adding the
prototype for it.

will fix this in the next patch version.

quoted

+                     case CMD_DISCOVER_MODES:
+                             res = tcpm_ams_start(port, DISCOVER_MODES);
+                             break;
+                     case CMD_ENTER_MODE:
+                             res = tcpm_ams_start(port,
+                                                  DFP_TO_UFP_ENTER_MODE);

One line is enough:

                                res = tcpm_ams_start(port, DFP_TO_UFP_ENTER_MODE);

will fix this in the next patch version.

quoted

+                             break;
+                     case CMD_EXIT_MODE:
+                             res = tcpm_ams_start(port,
+                                                  DFP_TO_UFP_EXIT_MODE);

Ditto.

will fix this in the next patch version.

quoted

+                             break;
+                     case CMD_ATTENTION:
+                             res = tcpm_ams_start(port, ATTENTION);
+                             break;
+                     case VDO_CMD_VENDOR(0) ... VDO_CMD_VENDOR(15):
+                             res = tcpm_ams_start(port, STRUCTURED_VDMS);
+                             break;
+                     default:
+                             res = -EOPNOTSUPP;
+                             break;
+                     }

-                     port->vdm_retries = 0;
-                     port->vdm_state = VDM_STATE_BUSY;
-                     timeout = vdm_ready_timeout(port->vdo_data[0]);
-                     mod_vdm_delayed_work(port, timeout);
+                     if (res < 0)
+                             return;
              }
+
+             port->vdm_state = VDM_STATE_SEND_MESSAGE;
+             mod_vdm_delayed_work(port, (port->negotiated_rev >= PD_REV30) &&
+                                        (port->pwr_role == TYPEC_SOURCE) &&
+                                        (PD_VDO_SVDM(vdo_hdr)) &&
+                                        (PD_VDO_CMDT(vdo_hdr) == CMDT_INIT) ?
+                                        PD_T_SINK_TX : 0);

I don't think you need all those brackets. This would look better, and
I bet it would make also scripts/checkpatch.pl happy:

                mod_vdm_delayed_work(port, (port->negotiated_rev >= PD_REV30 &&
                                            port->pwr_role == TYPEC_SOURCE &&
                                            PD_VDO_SVDM(vdo_hdr) &&
                                            PD_VDO_CMDT(vdo_hdr) == CMDT_INIT) ?
                                           PD_T_SINK_TX : 0);

will fix this in the next patch version.

quoted

+             /*
+              * If previous AMS is interrupted, switch to the upcoming
+              * state.
+              */
+             upcoming_state = port->upcoming_state;
+             if (port->upcoming_state != INVALID_STATE) {
+                     port->upcoming_state = INVALID_STATE;
+                     tcpm_set_state(port, upcoming_state, 0);
+                     break;
+             }

I don't see the local upcoming_state variable is being used anywhere
outside of these conditions, so please set it inside the condition
block:

                if (port->upcoming_state != INVALID_STATE) {
                        upcoming_state = port->upcoming_state;
                        port->upcoming_state = INVALID_STATE;
                        tcpm_set_state(port, upcoming_state, 0);
                        break;
                }

will do

quoted

+             upcoming_state = port->upcoming_state;
+             if (port->upcoming_state != INVALID_STATE) {
+                     port->upcoming_state = INVALID_STATE;
+                     tcpm_set_state(port, upcoming_state, 0);
+                     break;
+             }

Same here.

will do

thanks,
Kyle