MIME-Version: 1.0
In-Reply-To: <20170908093404.7dfikzfgrcytvbp7@mwanda>
References: <20170908012214.19047-1-Badhri@google.com> <20170908093404.7dfikzfgrcytvbp7@mwanda>
From: Badhri Jagan Sridharan <badhri@google.com>
Date: Fri, 8 Sep 2017 10:27:27 -0700
Message-ID: <CAPTae5KRDXJnbJZVwm2y+y0TcRM5hrMFEOrUod_Oaigay5x5RQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] staging: typec: tcpm: Validate source and sink caps
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Guenter Roeck <linux@roeck-us.net>, devel@driverdev.osuosl.org,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 9790
Lines: 228

Thanks for the comments. Replies inline.

On Fri, Sep 8, 2017 at 2:34 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Sep 07, 2017 at 06:22:13PM -0700, Badhri Jagan Sridharan wrote:
>> +static int tcpm_validate_caps(struct tcpm_port *port, const u32 *pdo,
>> +                           unsigned int nr_pdo)
>> +{
>> +     unsigned int i;
>> +
>> +     /* Should at least contain vSafe5v */
>> +     if (nr_pdo < 1) {
>> +             tcpm_log_force(port,
>> +                            " err: source/sink caps should atleast have vSafe5V");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* 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) {
>> +             tcpm_log_force(port,
>> +                            " err: vSafe5V Fixed Supply Object Shall always be the first object");
>> +             return -EINVAL;
>> +     }
>> +
>> +     for (i = 1; i < nr_pdo; i++) {
>> +             if (pdo_type(pdo[i]) < pdo_type(pdo[i - 1])) {
>> +                     tcpm_log_force(port,
>> +                                    " err:PDOs should be in the following order: Fixed; Battery; Variable. pdo index:%u"
>> +                                    , i);
>> +                     return -EINVAL;
>> +             } else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1])) {
>
> The else statement isn't needed. Pull this in one indent level.

Just to clarify, you are suggesting,
"if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1]))"
instead of "else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1]))"
right ?

>
>> +                     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])) {
>
> We can't have 2 fixed voltages which are the same?  The <= is < in the
> section below and the spec just says lowest to highest for everything.
> (I am similar to John Snow in that I know nothing).

The PD3.0 spec says the following:
A Source Shall Not offer multiple Power Data Objects of
the same type (fixed, variable, Battery) and the same voltage
but Shall instead offer one Power Data Object with the
highest available current for that Source capability and voltage.

I forgot to add the duplicate check(same min/max voltage for VAR/BATT.
Will add it in the next version.

>
>> +                                     tcpm_log_force(port,
>> +                                                    " err: Fixed supply pdos should be in increasing order, pdo index:%u"
>> +                                                    , i);
>> +                                     return -EINVAL;
>> +                             }
>> +                             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])) {
>> +                                     tcpm_log_force(port,
>> +                                                    " err: Variable supply pdos should be in increasing order, pdo index:%u"
>> +                                                    , i);
>> +                                     return -EINVAL;
>> +                             }
>> +                             break;
>> +                     default:
>> +                             tcpm_log_force(port, " Unknown pdo type");
>> +                     }
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  /*
>>   * PD (data, control) command handling functions
>>   */
>> @@ -1308,6 +1376,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);
>
> It's strange that this isn't checked.  We just want to print errors?
> Can this ever be true here because it feels like we won't load if it's
> invalid.  Is this really the right place to check?

The source_caps here is from the port partner which might be running
a different codebase. Most of the chargers run their custom
code which implies that we should atleast be aware if any of the charger
does not comply to the spec. The TCPM code would still manage to
negotiate the contract without the strict ordering from the charger
side. So, just printing errors should be suffice. Not negotiating a
contract might be little too extreme.

>
>> +
>>               /*
>>                * This message may be received even if VBUS is not
>>                * present. This is quite unexpected; see USB PD
>> @@ -3475,9 +3546,13 @@ 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)) {
>> +             tcpm_log_force(port, "Invalid source caps");
>> +             return -EINVAL;
>> +     }
>>       mutex_lock(&port->lock);
>>       port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, pdo, nr_pdo);
>>       switch (port->state) {
>> @@ -3497,16 +3572,21 @@ 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)) {
>> +             tcpm_log_force(port, "Invalid source caps");
>> +             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;
>> @@ -3525,6 +3605,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);
>>
>> @@ -3560,7 +3641,16 @@ 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);
>
> Why are we moving debugfs init for this patch?  I'm too lazy to figure
> out how this relates to the patch.  :/

The tcpm_validate_caps prints error messages into the logbuffer.
There is already an error in the TOT code, where the tcpm_log(line 3613) is
called before tcpm_debugfs_init is called. This would make the kernel
come down due to null pointer deference if the execution takes this
path as logbuffer_lock is not yet initialized. I am also not removing the
debugfs dir if tcpm_register_port fails to help retrieve
the error messages in the logbuffer. I could dump the error message into
kernel dmesg but didnt take that path to keep it in the same logbuffer.
>
>>
>> +     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)) {
>
> This is indented too far.  It should be:
>
>         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,
>> @@ -3620,7 +3710,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);
>
> regards,
> dan carpenter
>