| 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: <CAPTae5Kt4eZdEX-4GNYnxUApS20FMTbQKPod8xkfSj40EfazjQ@mail.gmail.com>
Date: Fri, 8 Sep 2017 10:43:40 -0700
From: Badhri Jagan Sridharan <badhri@...gle.com>
To: Dan Carpenter <dan.carpenter@...cle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Guenter Roeck <linux@...ck-us.net>, devel@...verdev.osuosl.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] staging: typec: tcpm: Only request matching pdos
On Fri, Sep 8, 2017 at 2:36 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> On Thu, Sep 07, 2017 at 06:22:14PM -0700, Badhri Jagan Sridharan wrote:
>> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
>> index 58a2c279f7d1..df0986d9f756 100644
>> --- a/drivers/staging/typec/tcpm.c
>> +++ b/drivers/staging/typec/tcpm.c
>> @@ -262,6 +262,9 @@ struct tcpm_port {
>> unsigned int nr_src_pdo;
>> u32 snk_pdo[PDO_MAX_OBJECTS];
>> unsigned int nr_snk_pdo;
>> + unsigned int nr_snk_fixed_pdo;
>> + unsigned int nr_snk_var_pdo;
>> + unsigned int nr_snk_batt_pdo;
>
> These names are too long. The extra words obscure the parts of the
> variable names which have information. It hurts readability. Do this:
>
> 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;
>>
>> @@ -1780,37 +1783,77 @@ static int tcpm_pd_check_request(struct tcpm_port *port)
>> return 0;
>> }
>>
>> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
>> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo)
>> {
>> - unsigned int i, max_mw = 0, max_mv = 0;
>> + 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. Prefer PDO providing exp
> ^^^^^^^ ^^^^^^^^^^^^^
> "matching". What does "providing exp" mean?
Actually that was moved forward from the existing code.
So not sure what that means ? I can remove it, if needed.
>
>> */
>> 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;
>> + unsigned int mv = 0, ma = 0, mw = 0, selected = 0;
>> +
>> + if (type == PDO_TYPE_FIXED) {
>> + for (j = 0; j < port->nr_snk_fixed_pdo; j++) {
>> + if (pdo_fixed_voltage(pdo) ==
>> + pdo_fixed_voltage(port->snk_pdo[j])) {
>> + mv = pdo_fixed_voltage(pdo);
>> + selected = j;
>> + break;
>> + }
>> + }
>> + } else if (type == PDO_TYPE_BATT && port->nr_snk_batt_pdo) {
>
> The test for nr_snk_batt_pdo isn't required. If it's zero then the for
> loop is just a no-op. Remove it.
>
>> + for (j = port->nr_snk_fixed_pdo;
>> + j < port->nr_snk_fixed_pdo +
>> + port->nr_snk_batt_pdo;
>
> This should be aligned like this:
>
> for (j = port->nr_snk_fixed_pdo;
> j < port->nr_snk_fixed_pdo +
> port->nr_snk_batt_pdo;
>
>> + j++) {
>> + if ((pdo_min_voltage(pdo) >=
>> + pdo_min_voltage(port->snk_pdo[j])) &&
>> + pdo_max_voltage(pdo) <=
>> + pdo_max_voltage(port->snk_pdo[j])) {
>
> No need for the extra parentheses around the first part of the &&. The
> condition isn't indented right either because the last two lines are
> indented one space more than they should be. Just do:
>
> if (pdo_min_voltage(pdo) >=
> pdo_min_voltage(port->snk_pdo[j]) &&
> pdo_max_voltage(pdo) <=
> pdo_max_voltage(port->snk_pdo[j])) {
>
>
>> + mv = pdo_min_voltage(pdo);
>> + selected = j;
>> + break;
>
> We always select the first valid voltage?
Good catch ! Should be evaluating against all matching sink_caps
not just the first one which matches. Will make amends in the next
version.
>
>> + }
>> + }
>> + } else if (type == PDO_TYPE_VAR && port->nr_snk_var_pdo) {
>> + for (j = port->nr_snk_fixed_pdo +
>> + port->nr_snk_batt_pdo;
>> + j < port->nr_snk_fixed_pdo +
>> + port->nr_snk_batt_pdo +
>> + port->nr_snk_var_pdo;
>> + j++) {
>> + if ((pdo_min_voltage(pdo) >=
>> + pdo_min_voltage(port->snk_pdo[j])) &&
>> + pdo_max_voltage(pdo) <=
>> + pdo_max_voltage(port->snk_pdo[j])) {
>> + mv = pdo_min_voltage(pdo);
>> + selected = j;
>> + break;
>> + }
>> + }
>
> Same stuff.
>
>> }
>>
>> + if (mv != 0) {
>
> Flip this condition around.
>
> if (mv == 0)
> continue;
>
>> + if (type == PDO_TYPE_BATT) {
>> + mw = min(pdo_max_power(pdo),
>> + pdo_max_power(port->snk_pdo[selected]
>> + ));
>> + } else {
>> + ma = min(pdo_max_current(pdo),
>> + pdo_max_current(
>> + port->snk_pdo[selected]));
>> + mw = ma * mv / 1000;
>> + }
>
> Then pull this code in one indent level and it looks nicer.
>
>> + }
>> /* Perfer higher voltages if available */
>> - if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
>> - mv <= port->max_snk_mv) {
>> + if (mw > max_mw || (mw == max_mw && mv > max_mv)) {
>> ret = i;
>> + *sink_pdo = selected;
>> max_mw = mw;
>> max_mv = mv;
>> }
>
> [ snip ]
>
>> @@ -3609,6 +3659,17 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>> }
>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>>
>> +static int tcpm_get_nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
>
> This function name is awkward. It's quite long and that means we keep
> bumping into the 80 character limit. Often times "get_" functions need
> to be followed by a "put_" but so the name is a little misleading. It's
> static so we don't really need the tcpm_ prefix. Just call it
> nr_type_pdos().
>
>> + enum pd_pdo_type type)
>> +{
>> + unsigned int i = 0;
>> +
>> + for (i = 0; i < nr_pdo; i++)
>> + if (pdo_type(pdo[i] == type))
>
> Parentheses are in the wrong place so this is always false. It should
> say:
> if (pdo_type(pdo[i]) == type)
>
>> + i++;
>
> The "i" variable is the iterator. We should be saying "count++"; This
> function always returns nr_pdo. Write it like this:
>
> static int nr_type_pdos(const u32 *pdo, unsigned int nr_pdo,
> enum pd_pdo_type type)
> {
> int count = 0;
> int i;
>
> for (i = 0; i < nr_pdo; i++) {
> if (pdo_type(pdo[i]) == type)
> count++;
> }
> return count;
> }
Sure. Will make the about amends and all other readability changes
in the next version.
>
> regards,
> dan carpenter
>
Powered by blists - more mailing lists