Re: [PATCH v5 1/3] usb: typec: tcpm: AMS and Collision Avoidance

From: Kyle Tso <kyletso@google.com>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	Greg KH <gregkh@linuxfoundation.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Badhri Jagan Sridharan <badhri@google.com>,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Will McVicker <willmcvicker@google.com>
Subject: Re: [PATCH v5 1/3] usb: typec: tcpm: AMS and Collision Avoidance
Date: Wed, 13 Jan 2021 22:44:14 +0800	[thread overview]
Message-ID: <CAGZ6i=2j2U6C_gtdz73vMPn_UAdZu=cU+pZguW=d8iZecKE=0g@mail.gmail.com> (raw)
In-Reply-To: <20210112132925.GC2020859@kuha.fi.intel.com>

On Tue, Jan 12, 2021 at 9:29 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Jan 06, 2021 at 12:39:25AM +0800, Kyle Tso wrote:
> > 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 <kyletso@google.com>
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
>
> 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.
>

> > +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.


> > +                     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.

> > +                             break;
> > +                     case CMD_EXIT_MODE:
> > +                             res = tcpm_ams_start(port,
> > +                                                  DFP_TO_UFP_EXIT_MODE);
>
> Ditto.
>
will fix this in the next patch version.

> > +                             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.


> > +             /*
> > +              * 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


> > +             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