Re: [PATCH 1/2 v5] typec: tcpm: Validate source and sink caps

From: Badhri Jagan Sridharan
Date: Sun Nov 12 2017 - 19:06:27 EST

Next message: Hyunchul Lee: "Re: [f2fs-dev] [RFC PATHC 2/2] f2fs: apply write hints to select the type of segment for direct write"
Previous message: Stephen Hemminger: "Re: [PATCH 6/7] net: core: maybe return -EEXIST in __dev_alloc_name"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
>
>
> Thanks,
>
>> ---
>> hangelog since v1:
>> - rebased on top drivers/usb/typec/tcpm.c as suggested by
>> gregkh@xxxxxxxxxxxxxxxxxxx
>> - renamed nr_snk_*_pdo as suggested by dan.carpenter@xxxxxxxxxx
>> - removed stale comment as suggested by linux@xxxxxxxxxxxx
>> - removed the tests for nr_snk_*_pdo as suggested by
>> dan.carpenter@xxxxxxxxxx
>> - Fixed sytling as suggested by dan.carpenter@xxxxxxxxxx
>> - renamed tcpm_get_nr_type_pdos to nr_type_pdos as suggested by
>> dan.carpenter@xxxxxxxxxx
>> - fixed nr_type_pdos as suggested by dan.carpenter@xxxxxxxxxx
>> - 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@xxxxxxxxxxxxxxx
>>
>> Changelog since v3:
>> - fixed formatting errors as suggested by
>> heikki.krogerus@xxxxxxxxxxxxxxx
>>
>> 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

Next message: Hyunchul Lee: "Re: [f2fs-dev] [RFC PATHC 2/2] f2fs: apply write hints to select the type of segment for direct write"
Previous message: Stephen Hemminger: "Re: [PATCH 6/7] net: core: maybe return -EEXIST in __dev_alloc_name"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]