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

From: Heikki Krogerus
Date: Tue Oct 10 2017 - 04:06:39 EST

Next message: Ulf Hansson: "Re: [PATCH v3 01/12] mmc: mediatek: add support of mt2701/mt2712"
Previous message: Marek Szyprowski: "Re: [PATCH] media: vb2: unify calling of set_page_dirty_lock"
In reply to: Badhri Jagan Sridharan: "[PATCH 2/2 v2] typec: tcpm: Only request matching pdos"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On Mon, Oct 09, 2017 at 03:13:43PM -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>
> ---
> Changelog since v1:
> - Rebased the patch on top of drivers/usb/type/tcpm.c
> - Added duplicate pdo check for variable/batt pdo.
> - Fixed tabs as suggested by dan.carpenter@xxxxxxxxxx
>
> drivers/usb/typec/tcpm.c | 114 +++++++++++++++++++++++++++++++++++++++++++----
> include/linux/usb/pd.h | 2 +
> include/linux/usb/tcpm.h | 4 +-
> 3 files changed, 109 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 8483d3e33853..75deac3ee58d 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -1256,6 +1256,82 @@ static void vdm_state_machine_work(struct work_struct *work)
> mutex_unlock(&port->lock);
> }
>
> +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])) {
> + 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])) {
> + 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;
> + } else if ((pdo_min_voltage(pdo[i]) ==
> + pdo_min_voltage(pdo[i - 1])) &&
> + (pdo_max_voltage(pdo[i]) ==
> + pdo_min_voltage(pdo[i - 1]))) {
> + tcpm_log_force(port,
> + " err: Variable/Batt supply pdos cannot have same min/max voltage, pdo index:%u"
> + , i);
> + return -EINVAL;
> + }
> + break;
> + default:
> + tcpm_log_force(port, " Unknown pdo type");
> + }
> + }
> + }
> +
> + return 0;
> +}

That is a bit difficult to read. Please group the error messages
somehow. Here's an example:

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: Variable supply pdos should be in increasing order.",
"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 4;
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 5;
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 +1354,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 +3523,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");

No need for the extra log entry.

> + return -EINVAL;
> + }
> mutex_lock(&port->lock);
> port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, pdo, nr_pdo);
> switch (port->state) {
> @@ -3466,16 +3549,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");

Ditto.

> + 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 +3582,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 +3618,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 +3681,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..bc419f93c90e 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -183,9 +183,9 @@ 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,
> +int 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,
> +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,

Thanks,

--
heikki

Next message: Ulf Hansson: "Re: [PATCH v3 01/12] mmc: mediatek: add support of mt2701/mt2712"
Previous message: Marek Szyprowski: "Re: [PATCH] media: vb2: unify calling of set_page_dirty_lock"
In reply to: Badhri Jagan Sridharan: "[PATCH 2/2 v2] typec: tcpm: Only request matching pdos"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]