| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPTae5KJ_zV2n_KUHsMiK1U60sAc171_Q6zya=ugSBSR-UcDKQ@mail.gmail.com>
Date: Sat, 4 Nov 2017 11:53:32 -0700
From: Badhri Jagan Sridharan <badhri@...gle.com>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
USB <linux-usb@...r.kernel.org>,
Dan Carpenter <dan.carpenter@...cle.com>,
Guenter Roeck <linux@...ck-us.net>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] typec: tcpm: Only request matching pdos
On Thu, Nov 2, 2017 at 4:00 AM, Heikki Krogerus
<heikki.krogerus@...ux.intel.com> wrote:
> Hi,
>
> +Dan and Guenter
>
> I'm sorry for the late reply. These slipped under my radar. I do
> have a one more proposal below, and a few nits..
>
> On Wed, Oct 18, 2017 at 01:22:48PM -0700, Badhri Jagan Sridharan wrote:
>> At present, TCPM code assumes that local device supports
>> variable/batt pdos and always selects the pdo with highest
>> possible power within the board limit. This assumption
>> might not hold good for all devices. To overcome this,
>> this patch makes TCPM only accept a source_pdo when there is
>> a matching sink pdo.
>>
>> For Fixed pdos: The voltage should match between the
>> incoming source_cap and the registered snk_pdo
>> For Variable/Batt pdos: The incoming source_cap voltage
>> range should fall within the registered snk_pdo's voltage
>> range.
>>
>> Also, when the cap_mismatch bit is set, the max_power/current
>> should be set to the max_current/power of the sink_pdo.
>> This is according to:
>>
>> "If the Capability Mismatch bit is set to one
>> The Maximum Operating Current/Power field may contain a value
>> larger than the maximum current/power offered in the Source
>> Capabilities message???s PDO as referenced by the Object position field.
>> This enables the Sink to indicate that it requires more current/power
>> than is being offered. If the Sink requires a different voltage this
>> will be indicated by its Sink Capabilities message.
>>
>> Signed-off-by: Badhri Jagan Sridharan <Badhri@...gle.com>
>> ---
>> 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@...cle.com
>>
>> Changelog since v2:
>> - Rebase
>>
>> drivers/usb/typec/tcpm.c | 155 ++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 120 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
>> index e3f8b3da46b0..1b37333dc63b 100644
>> --- a/drivers/usb/typec/tcpm.c
>> +++ b/drivers/usb/typec/tcpm.c
>> @@ -261,6 +261,9 @@ struct tcpm_port {
>> unsigned int nr_src_pdo;
>> u32 snk_pdo[PDO_MAX_OBJECTS];
>> unsigned int nr_snk_pdo;
>> + unsigned int nr_fixed; /* number of fixed sink PDOs */
>> + unsigned int nr_var; /* number of variable sink PDOs */
>> + unsigned int nr_batt; /* number of battery sink PDOs */
>> u32 snk_vdo[VDO_MAX_OBJECTS];
>> unsigned int nr_snk_vdo;
>>
>> @@ -1762,39 +1765,92 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
>> return 0;
>> }
>>
>> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
>> +static void tcpm_pd_evaluate_pdo(struct tcpm_port *port, int src_pdo_index,
>> + int snk_pdo_index, int *max_mw, int *max_mv,
>> + int *selected_src_pdo,
>> + int *matching_snk_pdo)
>> {
>> - unsigned int i, max_mw = 0, max_mv = 0;
>> + unsigned int mv = 0, ma = 0, mw = 0;
>> + u32 src_pdo = port->source_caps[src_pdo_index];
>> + u32 snk_pdo = port->snk_pdo[snk_pdo_index];
>> + enum pd_pdo_type type = pdo_type(src_pdo);
>> +
>> + mv = (type == PDO_TYPE_FIXED) ? pdo_fixed_voltage(src_pdo) :
>> + pdo_min_voltage(src_pdo);
>> +
>> + if (type == PDO_TYPE_BATT) {
>> + mw = min(pdo_max_power(src_pdo), pdo_max_power(snk_pdo));
>> + } else {
>> + ma = min(pdo_max_current(src_pdo), pdo_max_current(snk_pdo));
>> + mw = ma * mv / 1000;
>> + }
>> +
>> + /* Perfer higher voltages if available */
>> + if (mw > *max_mw || (mw == *max_mw && mv > *max_mv)) {
>> + *selected_src_pdo = src_pdo_index;
>> + *matching_snk_pdo = snk_pdo_index;
>> + *max_mw = mw;
>> + *max_mv = mv;
>> + }
>> +}
>> +
>> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo)
>> +{
>> + unsigned int i, j, max_mw = 0, max_mv = 0;
>> int ret = -EINVAL;
>>
>> /*
>> - * Select the source PDO providing the most power while staying within
>> - * the board's voltage limits. Prefer PDO providing exp
>> + * Select the source PDO providing the most power which has a
>> + * matchig sink cap.
>> */
>> for (i = 0; i < port->nr_source_caps; i++) {
>> u32 pdo = port->source_caps[i];
>> enum pd_pdo_type type = pdo_type(pdo);
>> - unsigned int mv, ma, mw;
>> -
>> - if (type == PDO_TYPE_FIXED)
>> - mv = pdo_fixed_voltage(pdo);
>> - else
>> - mv = pdo_min_voltage(pdo);
>> -
>> - if (type == PDO_TYPE_BATT) {
>> - mw = pdo_max_power(pdo);
>> - } else {
>> - ma = min(pdo_max_current(pdo),
>> - port->max_snk_ma);
>> - mw = ma * mv / 1000;
>> - }
>>
>> - /* Perfer higher voltages if available */
>> - if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
>> - mv <= port->max_snk_mv) {
>> - ret = i;
>> - max_mw = mw;
>> - max_mv = mv;
>> + if (type == PDO_TYPE_FIXED) {
>> + for (j = 0; j < port->nr_fixed; j++) {
>> + if (pdo_fixed_voltage(pdo) ==
>> + pdo_fixed_voltage(port->snk_pdo[j])) {
>> + tcpm_pd_evaluate_pdo(port, i, j,
>> + &max_mw, &max_mv,
>> + &ret, sink_pdo);
>> + /* There could only be one fixed pdo
>> + * at a specific voltage level.
>> + * So breaking here.
>> + */
>> + break;
>> + }
>> + }
>> + } else if (type == PDO_TYPE_BATT) {
>> + for (j = port->nr_fixed;
>> + j < port->nr_fixed +
>> + port->nr_batt;
>> + j++) {
>> + if (pdo_min_voltage(pdo) >=
>> + pdo_min_voltage(port->snk_pdo[j]) &&
>> + pdo_max_voltage(pdo) <=
>> + pdo_max_voltage(port->snk_pdo[j])) {
>> + tcpm_pd_evaluate_pdo(port, i, j,
>> + &max_mw, &max_mv,
>> + &ret, sink_pdo);
>> + }
>> + }
>> + } else if (type == PDO_TYPE_VAR) {
>> + for (j = port->nr_fixed +
>> + port->nr_batt;
>> + j < port->nr_fixed +
>> + port->nr_batt +
>> + port->nr_var;
>> + j++) {
>> + if (pdo_min_voltage(pdo) >=
>> + pdo_min_voltage(port->snk_pdo[j]) &&
>> + pdo_max_voltage(pdo) <=
>> + pdo_max_voltage(port->snk_pdo[j])) {
>> + tcpm_pd_evaluate_pdo(port, i, j,
>> + &max_mw, &max_mv,
>> + &ret, sink_pdo);
>> + }
>> + }
>> }
>
> Wouldn't it make sense to call tcpm_pd_evaluate_pdo() here ones and
> run those nested loops there? You are already checking the type there
> in any case.
Combined into one function and introduced couple of macros to keep up with
the character limit instead. Let me know your thoughts.
>
>> }
>>
>> @@ -1806,13 +1862,14 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>> unsigned int mv, ma, mw, flags;
>> unsigned int max_ma, max_mw;
>> enum pd_pdo_type type;
>> - int index;
>> - u32 pdo;
>> + int index, snk_pdo_index;
>> + u32 pdo, matching_snk_pdo;
>>
>> - index = tcpm_pd_select_pdo(port);
>> + index = tcpm_pd_select_pdo(port, &snk_pdo_index);
>> if (index < 0)
>> return -EINVAL;
>> pdo = port->source_caps[index];
>> + matching_snk_pdo = port->snk_pdo[snk_pdo_index];
>> type = pdo_type(pdo);
>>
>> if (type == PDO_TYPE_FIXED)
>> @@ -1820,26 +1877,32 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
>> else
>> mv = pdo_min_voltage(pdo);
>>
>> - /* Select maximum available current within the board's power limit */
>> + /* Select maximum available current within the sink pdo's limit */
>> if (type == PDO_TYPE_BATT) {
>> - mw = pdo_max_power(pdo);
>> - ma = 1000 * min(mw, port->max_snk_mw) / mv;
>> + mw = min(pdo_max_power(pdo), pdo_max_power(matching_snk_pdo));
>> + ma = 1000 * mw / mv;
>> } else {
>> ma = min(pdo_max_current(pdo),
>> - 1000 * port->max_snk_mw / mv);
>> + pdo_max_current(matching_snk_pdo));
>> + mw = ma * mv / 1000;
>> }
>> - ma = min(ma, port->max_snk_ma);
>>
>> flags = RDO_USB_COMM | RDO_NO_SUSPEND;
>>
>> /* Set mismatch bit if offered power is less than operating power */
>> - mw = ma * mv / 1000;
>> max_ma = ma;
>> max_mw = mw;
>> if (mw < port->operating_snk_mw) {
>> flags |= RDO_CAP_MISMATCH;
>> - max_mw = port->operating_snk_mw;
>> - max_ma = max_mw * 1000 / mv;
>> + if (type == PDO_TYPE_BATT) {
>> + if (pdo_max_power(matching_snk_pdo) >
>> + pdo_max_power(pdo))
>> + max_mw = pdo_max_power(matching_snk_pdo);
>> + } else {
>> + if (pdo_max_current(matching_snk_pdo) >
>> + pdo_max_current(pdo))
>> + max_ma = pdo_max_current(matching_snk_pdo);
>> + }
>
> How about:
>
> if ((type == PDO_TYPE_BATT) &&
> (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
> max_mw = pdo_max_power(matching_snk_pdo);
> else if (pdo_max_current(matching_snk_pdo) > pdo_max_current(pdo))
> max_ma = pdo_max_current(matching_snk_pdo);
>
> Though, that does not make it any more readable. Just a proposal.
Sure. Will do in next patch.
>
>> }
>>
>> tcpm_log(port, "cc=%d cc1=%d cc2=%d vbus=%d vconn=%s polarity=%d",
>> @@ -3588,6 +3651,19 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>> }
>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>>
>> +static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
>> + enum pd_pdo_type type)
>
> Please align that properly.
>
>> +{
>> + int count = 0;
>> + int i;
>> +
>> + for (i = 0; i < nr_pdo; i++) {
>> + if (pdo_type(pdo[i]) == type)
>> + count++;
>> + }
>> + return count;
>> +}
>> +
>> struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>> {
>> struct tcpm_port *port;
>> @@ -3633,6 +3709,15 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>> tcpc->config->nr_src_pdo);
>> port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
>> tcpc->config->nr_snk_pdo);
>> + port->nr_fixed = nr_type_pdos(port->snk_pdo,
>> + port->nr_snk_pdo,
>> + PDO_TYPE_FIXED);
>
> Ditto.
>
>> + port->nr_var = nr_type_pdos(port->snk_pdo,
>> + port->nr_snk_pdo,
>> + PDO_TYPE_VAR);
>
> Ditto.
>
>> + port->nr_batt = nr_type_pdos(port->snk_pdo,
>> + port->nr_snk_pdo,
>> + PDO_TYPE_BATT);
>
> Ditto.
Will fix other styling issues as well.
Thanks.
>
>
> Thanks,
>
> --
> heikki
Powered by blists - more mailing lists