Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20171104185953.9532-1-Badhri@google.com>
References: <20171104185953.9532-1-Badhri@google.com>
From:   Badhri Jagan Sridharan <badhri@google.com>
Date:   Sat, 4 Nov 2017 12:13:31 -0700
Message-ID: <CAPTae5J0C5nev9iv19G8qyDEbHgJs2S6ftQp_iBDubjvHDn1eg@mail.gmail.com>
Subject: Re: [PATCH 1/2 v4] typec: tcpm: Validate source and sink caps
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Guenter Roeck <linux@roeck-us.net>
Cc:     USB <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Badhri Jagan Sridharan <Badhri@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Please disregard v4. Going to send another version again.

On Sat, Nov 4, 2017 at 11:59 AM, Badhri Jagan Sridharan
<badhri@google.com> wrote:
> The source and sink caps should follow the following rules.
> This patch validates whether the src_caps/snk_caps adheres
> to it.
>
> 6.4.1 Capabilities Message
> A Capabilities message (Source Capabilities message or Sink
> Capabilities message) shall have at least one Power
> Data Object for vSafe5V. The Capabilities message shall also
> contain the sending Port=E2=80=99s information followed by up to
> 6 additional Power Data Objects. Power Data Objects in a
> Capabilities message shall be sent in the following order:
>
> 1. The vSafe5V Fixed Supply Object shall always be the first object.
> 2. The remaining Fixed Supply Objects, if present, shall be sent
>    in voltage order; lowest to highest.
> 3. The Battery Supply Objects, if present shall be sent in Minimum
>    Voltage order; lowest to highest.
> 4. The Variable Supply (non-battery) Objects, if present, shall be
>    sent in Minimum Voltage order; lowest to highest.
>
> Errors in source/sink_caps of the local port will prevent
> the port registration. Whereas, errors in source caps of partner
> device would only log them.
>
> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> ---
> Changelog since v1:
> - rebased on top drivers/usb/typec/tcpm.c as suggested by
>   gregkh@linuxfoundation.org
> - renamed nr_snk_*_pdo as suggested by dan.carpenter@oracle.com
> - removed stale comment as suggested by linux@roeck-us.net
> - removed the tests for nr_snk_*_pdo as suggested by
>   dan.carpenter@oracle.com
> - Fixed sytling as suggested by dan.carpenter@oracle.com
> - renamed tcpm_get_nr_type_pdos to nr_type_pdos as suggested by
>   dan.carpenter@oracle.com
> - fixed nr_type_pdos as suggested by dan.carpenter@oracle.com
> - tcpm_pd_select_pdo now checks for all matching variable/batt pdos
>   instead of the first matching one.
>
> Changelog since v2:
> - refactored error messages as suggested by
>   heikki.krogerus@linux.intel.com
>
> Changelog since v3:
> - fixed formatting errors as suggested by
>   heikki.krogerus@linux.intel.com
>
>  drivers/usb/typec/tcpm.c | 115 +++++++++++++++++++++++++++++++++++++++++=
++----
>  include/linux/usb/pd.h   |   2 +
>  include/linux/usb/tcpm.h |  16 +++----
>  3 files changed, 116 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 8483d3e33853..ecdad71a6159 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -1256,6 +1256,85 @@ static void vdm_state_machine_work(struct work_str=
uct *work)
>         mutex_unlock(&port->lock);
>  }
>
> +static const char * const pdo_err[] =3D {
> +       "",
> +       " err: source/sink caps should atleast have vSafe5V",
> +       " err: vSafe5V Fixed Supply Object Shall always be the first obje=
ct",
> +       " err: PDOs should be in the following order: Fixed; Battery; Var=
iable",
> +       " err: Fixed supply pdos should be in increasing order of their f=
ixed voltage",
> +       " err: Variable/Battery supply pdos should be in increasing order=
 of their minimum voltage",
> +       " err: Variable/Batt supply pdos cannot have same min/max voltage=
",
> +};
> +
> +static int tcpm_caps_err(struct tcpm_port *port, const u32 *pdo,
> +                        unsigned int nr_pdo)
> +{
> +       unsigned int i;
> +
> +       /* Should at least contain vSafe5v */
> +       if (nr_pdo < 1)
> +               return 1;
> +
> +       /* The vSafe5V Fixed Supply Object Shall always be the first obje=
ct */
> +       if (pdo_type(pdo[0]) !=3D PDO_TYPE_FIXED ||
> +           pdo_fixed_voltage(pdo[0]) !=3D VSAFE5V)
> +               return 2;
> +
> +       for (i =3D 1; i < nr_pdo; i++) {
> +               if (pdo_type(pdo[i]) < pdo_type(pdo[i - 1])) {
> +                       return 3;
> +               } else if (pdo_type(pdo[i]) =3D=3D pdo_type(pdo[i - 1])) =
{
> +                       enum pd_pdo_type type =3D pdo_type(pdo[i]);
> +
> +                       switch (type) {
> +                       /*
> +                        * The remaining Fixed Supply Objects, if
> +                        * present, shall be sent in voltage order;
> +                        * lowest to highest.
> +                        */
> +                       case PDO_TYPE_FIXED:
> +                               if (pdo_fixed_voltage(pdo[i]) <=3D
> +                                   pdo_fixed_voltage(pdo[i - 1]))
> +                                       return 4;
> +                               break;
> +                       /*
> +                        * The Battery Supply Objects and Variable
> +                        * supply, if present shall be sent in Minimum
> +                        * Voltage order; lowest to highest.
> +                        */
> +                       case PDO_TYPE_VAR:
> +                       case PDO_TYPE_BATT:
> +                               if (pdo_min_voltage(pdo[i]) <
> +                                   pdo_min_voltage(pdo[i - 1]))
> +                                       return 5;
> +                               else if ((pdo_min_voltage(pdo[i]) =3D=3D
> +                                         pdo_min_voltage(pdo[i - 1])) &&
> +                                        (pdo_max_voltage(pdo[i]) =3D=3D
> +                                         pdo_min_voltage(pdo[i - 1])))
> +                                       return 6;
> +                               break;
> +                       default:
> +                               tcpm_log_force(port, " Unknown pdo type")=
;
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int tcpm_validate_caps(struct tcpm_port *port, const u32 *pdo,
> +                             unsigned int nr_pdo)
> +{
> +       int err_index =3D tcpm_caps_err(port, pdo, nr_pdo);
> +
> +       if (err_index) {
> +               tcpm_log_force(port, " %s", pdo_err[err_index]);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   * PD (data, control) command handling functions
>   */
> @@ -1278,6 +1357,9 @@ static void tcpm_pd_data_request(struct tcpm_port *=
port,
>
>                 tcpm_log_source_caps(port);
>
> +               tcpm_validate_caps(port, port->source_caps,
> +                                  port->nr_source_caps);
> +
>                 /*
>                  * This message may be received even if VBUS is not
>                  * present. This is quite unexpected; see USB PD
> @@ -3444,9 +3526,12 @@ static int tcpm_copy_vdos(u32 *dest_vdo, const u32=
 *src_vdo,
>         return nr_vdo;
>  }
>
> -void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *=
pdo,
> -                                    unsigned int nr_pdo)
> +int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *p=
do,
> +                                   unsigned int nr_pdo)
>  {
> +       if (tcpm_validate_caps(port, pdo, nr_pdo))
> +               return -EINVAL;
> +
>         mutex_lock(&port->lock);
>         port->nr_src_pdo =3D tcpm_copy_pdos(port->src_pdo, pdo, nr_pdo);
>         switch (port->state) {
> @@ -3466,16 +3551,20 @@ void tcpm_update_source_capabilities(struct tcpm_=
port *port, const u32 *pdo,
>                 break;
>         }
>         mutex_unlock(&port->lock);
> +       return 0;
>  }
>  EXPORT_SYMBOL_GPL(tcpm_update_source_capabilities);
>
> -void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pd=
o,
> -                                  unsigned int nr_pdo,
> -                                  unsigned int max_snk_mv,
> -                                  unsigned int max_snk_ma,
> -                                  unsigned int max_snk_mw,
> -                                  unsigned int operating_snk_mw)
> +int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo=
,
> +                                 unsigned int nr_pdo,
> +                                 unsigned int max_snk_mv,
> +                                 unsigned int max_snk_ma,
> +                                 unsigned int max_snk_mw,
> +                                 unsigned int operating_snk_mw)
>  {
> +       if (tcpm_validate_caps(port, pdo, nr_pdo))
> +               return -EINVAL;
> +
>         mutex_lock(&port->lock);
>         port->nr_snk_pdo =3D tcpm_copy_pdos(port->snk_pdo, pdo, nr_pdo);
>         port->max_snk_mv =3D max_snk_mv;
> @@ -3494,6 +3583,7 @@ void tcpm_update_sink_capabilities(struct tcpm_port=
 *port, const u32 *pdo,
>                 break;
>         }
>         mutex_unlock(&port->lock);
> +       return 0;
>  }
>  EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>
> @@ -3529,7 +3619,15 @@ struct tcpm_port *tcpm_register_port(struct device=
 *dev, struct tcpc_dev *tcpc)
>
>         init_completion(&port->tx_complete);
>         init_completion(&port->swap_complete);
> +       tcpm_debugfs_init(port);
>
> +       if (tcpm_validate_caps(port, tcpc->config->src_pdo,
> +                              tcpc->config->nr_src_pdo) ||
> +           tcpm_validate_caps(port, tcpc->config->snk_pdo,
> +                              tcpc->config->nr_snk_pdo)) {
> +               err =3D -EINVAL;
> +               goto out_destroy_wq;
> +       }
>         port->nr_src_pdo =3D tcpm_copy_pdos(port->src_pdo, tcpc->config->=
src_pdo,
>                                           tcpc->config->nr_src_pdo);
>         port->nr_snk_pdo =3D tcpm_copy_pdos(port->snk_pdo, tcpc->config->=
snk_pdo,
> @@ -3584,7 +3682,6 @@ struct tcpm_port *tcpm_register_port(struct device =
*dev, struct tcpc_dev *tcpc)
>                 }
>         }
>
> -       tcpm_debugfs_init(port);
>         mutex_lock(&port->lock);
>         tcpm_init(port);
>         mutex_unlock(&port->lock);
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index e00051ced806..b3d41d7409b3 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -148,6 +148,8 @@ enum pd_pdo_type {
>         (PDO_TYPE(PDO_TYPE_FIXED) | (flags) |           \
>          PDO_FIXED_VOLT(mv) | PDO_FIXED_CURR(ma))
>
> +#define VSAFE5V 5000 /* mv units */
> +
>  #define PDO_BATT_MAX_VOLT_SHIFT        20      /* 50mV units */
>  #define PDO_BATT_MIN_VOLT_SHIFT        10      /* 50mV units */
>  #define PDO_BATT_MAX_PWR_SHIFT 0       /* 250mW units */
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 073197f0d2bb..ca1c0b57f03f 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -183,14 +183,14 @@ struct tcpm_port;
>  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev=
 *tcpc);
>  void tcpm_unregister_port(struct tcpm_port *port);
>
> -void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *=
pdo,
> -                                    unsigned int nr_pdo);
> -void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pd=
o,
> -                                  unsigned int nr_pdo,
> -                                  unsigned int max_snk_mv,
> -                                  unsigned int max_snk_ma,
> -                                  unsigned int max_snk_mw,
> -                                  unsigned int operating_snk_mw);
> +int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *p=
do,
> +                                   unsigned int nr_pdo);
> +int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo=
,
> +                                 unsigned int nr_pdo,
> +                                 unsigned int max_snk_mv,
> +                                 unsigned int max_snk_ma,
> +                                 unsigned int max_snk_mw,
> +                                 unsigned int operating_snk_mw);
>
>  void tcpm_vbus_change(struct tcpm_port *port);
>  void tcpm_cc_change(struct tcpm_port *port);
> --
> 2.15.0.403.gc27cc4dac6-goog
>

From 1583163198448891335@xxx Sat Nov 04 19:01:21 +0000 2017
X-GM-THRID: 1583163198448891335
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread