linux-kernel - RE: [PATCH v5 1/5] typec: tcpm: Add core support for sink side PPS

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2E89032DDAA8B9408CB92943514A0337014C1E2864@SW-EX-MBX01.diasemi.com>
Date:   Thu, 22 Mar 2018 09:44:49 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sebastian Reichel <sre@...nel.org>,
        "Hans de Goede" <hdegoede@...hat.com>, Jun Li <jun.li@....com>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Support Opensource" <Support.Opensource@...semi.com>
Subject: RE: [PATCH v5 1/5] typec: tcpm: Add core support for sink side PPS

On 22 March 2018 04:03, Guenter Roeck wrote:

> >   static enum pdo_err tcpm_caps_err(struct tcpm_port *port, const u32 *pdo,
> > @@ -1308,6 +1347,26 @@ static enum pdo_err tcpm_caps_err(struct tcpm_port
> *port, const u32 *pdo,
> >   					  pdo_min_voltage(pdo[i - 1])))
> >   					return PDO_ERR_DUPE_PDO;
> >   				break;
> > +			/*
> > +			 * The Programmable Power Supply APDOs, if present,
> > +			 * shall be sent in Maximum Voltage order;
> > +			 * lowest to highest.
> > +			 */
> > +			case PDO_TYPE_APDO:
> > +				if (pdo_apdo_type(pdo[i]) != APDO_TYPE_PPS)
> > +					break;
> > +
> > +				if (pdo_pps_apdo_max_current(pdo[i]) <
> > +				    pdo_pps_apdo_max_current(pdo[i - 1]))
> > +					return
> PDO_ERR_PPS_APDO_NOT_SORTED;
> > +				else if ((pdo_pps_apdo_min_voltage(pdo[i]) ==
> > +					  pdo_pps_apdo_min_voltage(pdo[i - 1]))
> &&
> > +					 (pdo_pps_apdo_max_voltage(pdo[i]) ==
> > +					  pdo_pps_apdo_max_voltage(pdo[i - 1]))
> &&
> > +					 (pdo_pps_apdo_max_current(pdo[i]) ==
> > +					  pdo_pps_apdo_max_current(pdo[i - 1])))
>
> Unnecessary ( )

I have to say I think it's neater/clearer with than without but if that's
something you really don't like then I'll remove them.


> > +static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
> > +{
> > +	unsigned int target_mw;
> > +	int ret = 0;
> > +
>
> Unnecessary initialization.

Ok, will remove.


> > +static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
> > +{
> > +	unsigned int target_mw;
> > +	int ret = 0;
> > +
> Unnecessary initialization.

Ditto

> > +	mutex_lock(&port->swap_lock);
> > +	mutex_lock(&port->lock);
> > +
> > +	if (!port->pps_data.active) {
> > +		ret = -EOPNOTSUPP;
> > +		goto port_unlock;
> > +	}
> > +
> > +	if (port->state != SNK_READY) {
> > +		ret = -EAGAIN;
> > +		goto port_unlock;
> > +	}
> > +
> > +	if ((out_volt < port->pps_data.min_volt) ||
> > +	    (out_volt > port->pps_data.max_volt)) {
>
> Unnecessary ( )

Ok.

> > +		ret = -EINVAL;
> > +		goto port_unlock;
> > +	}
> > +
> > +	target_mw = (port->pps_data.op_curr * out_volt) / 1000;
> > +	if (target_mw < port->operating_snk_mw) {
> > +		ret = -EINVAL;
> > +		goto port_unlock;
> > +	}
> > +
> > +	reinit_completion(&port->pps_complete);
> > +	port->pps_data.out_volt = out_volt;
> > +	port->pps_status = 0;
> > +	port->pps_pending = true;
> > +	tcpm_set_state(port, SNK_NEGOTIATE_PPS_CAPABILITIES, 0);
> > +	mutex_unlock(&port->lock);
> > +
> > +	if (!wait_for_completion_timeout(&port->pps_complete,
> > +				msecs_to_jiffies(PD_STATE_MACHINE_TIMEOUT)))
> > +		ret = -ETIMEDOUT;
> > +	else
> > +		ret = port->pps_status;
> > +
> > +	goto swap_unlock;
> > +
> > +port_unlock:
> > +	mutex_unlock(&port->lock);
> > +swap_unlock:
> > +	mutex_unlock(&port->swap_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int tcpm_pps_activate(struct tcpm_port *port, bool activate)
> > +{
> > +	int ret = 0;
> > +
> > +	mutex_lock(&port->swap_lock);
> > +	mutex_lock(&port->lock);
> > +
> > +	if (!port->pps_data.supported) {
> > +		ret = -EOPNOTSUPP;
> > +		goto port_unlock;
> > +	}
> > +
> > +	/* Trying to deactivate PPS when already deactivated so just bail */
> > +	if ((!port->pps_data.active) && (!activate))
>
> Unnecessary ( )

Actually agree on this one :)