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: <20171107110030.GD18063@kuha.fi.intel.com>
References: <20171104192213.14117-1-Badhri@google.com> <20171107110030.GD18063@kuha.fi.intel.com>
From:   Badhri Jagan Sridharan <badhri@google.com>
Date:   Sun, 12 Nov 2017 16:05:40 -0800
Message-ID: <CAPTae5LrFZ6N5B_JGZ1RykAu6v5C=xSaXB8oBz=HrPFjkpnW2Q@mail.gmail.com>
Subject: Re: [PATCH 1/2 v5] typec: tcpm: Validate source and sink caps
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Guenter Roeck <linux@roeck-us.net>,
        USB <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks Heikki !
I am making another version addressing your nits as well.

Guenter, Are you fine with the patch as well ?

Regards,
Badhri

On Tue, Nov 7, 2017 at 3:00 AM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Sat, Nov 04, 2017 at 12:22:12PM -0700, Badhri Jagan Sridharan 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???s 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>
>
> It would have been nice to have enum for the return values used in
> tcpm_cap_err() IMHO, but never mind. Is Guenter OK with these?
>
> FWIW:
>
> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>
>
> Thanks,
>
>> ---
>> hangelog 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
>>
>> Changelog since v4:
>> - Reusing the macro
>>
>>  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 2b735f3e5765..8b30ab69ed79 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_struct *work)
>>       mutex_unlock(&port->lock);
>>  }
>>
>> +static const char * const pdo_err[] = {
>> +     "",
>> +     " err: source/sink caps should atleast have vSafe5V",
>> +     " err: vSafe5V Fixed Supply Object Shall always be the first object",
>> +     " err: PDOs should be in the following order: Fixed; Battery; Variable",
>> +     " err: Fixed supply pdos should be in increasing order of their fixed 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 object */
>> +     if (pdo_type(pdo[0]) != PDO_TYPE_FIXED ||
>> +         pdo_fixed_voltage(pdo[0]) != VSAFE5V)
>> +             return 2;
>> +
>> +     for (i = 1; i < nr_pdo; i++) {
>> +             if (pdo_type(pdo[i]) < pdo_type(pdo[i - 1])) {
>> +                     return 3;
>> +             } else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1])) {
>> +                     enum pd_pdo_type type = 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]) <=
>> +                                 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]) ==
>> +                                       pdo_min_voltage(pdo[i - 1])) &&
>> +                                      (pdo_max_voltage(pdo[i]) ==
>> +                                       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 = 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 *pdo,
>> +                                 unsigned int nr_pdo)
>>  {
>> +     if (tcpm_validate_caps(port, pdo, nr_pdo))
>> +             return -EINVAL;
>> +
>>       mutex_lock(&port->lock);
>>       port->nr_src_pdo = 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 *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)
>> +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 = tcpm_copy_pdos(port->snk_pdo, pdo, nr_pdo);
>>       port->max_snk_mv = 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 = -EINVAL;
>> +             goto out_destroy_wq;
>> +     }
>>       port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, tcpc->config->src_pdo,
>>                                         tcpc->config->nr_src_pdo);
>>       port->nr_snk_pdo = 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 *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);
>> +int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
>> +                                 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
>
> --
> heikki

From 1583411413298104815@xxx Tue Nov 07 12:46:37 +0000 2017
X-GM-THRID: 1583163198448891335
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread