linux-kernel - RE: [PATCH v4 4/7] 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]

Date:   Tue, 6 Feb 2018 14:33:08 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>
CC:     Guenter Roeck <linux@...ck-us.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sebastian Reichel <sre@...nel.org>,
        "Hans de Goede" <hdegoede@...hat.com>,
        Yueyao Zhu <yueyao.zhu@...il.com>,
        "Rui Miguel Silva" <rmfrfs@...il.com>,
        "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 v4 4/7] typec: tcpm: Add core support for sink side PPS

On 30 January 2018 12:47, Heikki Krogerus wrote:

> > +static int tcpm_pps_set_op_curr(struct tcpm_port *port, u16 op_curr)
> > +{
> > +	unsigned int target_mw;
> > +	int ret = 0;
> > +
> > +	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 (op_curr > port->pps_data.max_curr) {
> > +		ret = -EINVAL;
> > +		goto port_unlock;
> > +	}
> > +
> > +	target_mw = (op_curr * port->pps_data.out_volt) / 1000;
> > +	if (target_mw < port->operating_snk_mw) {
> > +		ret = -EINVAL;
> > +		goto port_unlock;
> > +	}
> > +
> > +	reinit_completion(&port->pps_complete);
> > +	port->pps_data.op_curr = op_curr;
> > +	port->pps_status = 0;
> > +	port->pps_pending = true;
> > +	tcpm_set_state(port, SNK_NEGOTIATE_PPS_CAPABILITIES, 0);
>
> Why not just take the swap_lock here..

I believe this would result in deadlock. All of the existing uses of swap_lock
acquire it first before the port->lock is then acquired (and vice-versa for
unlock). We don't want the power role to change during this procedure, so we
hold the swap_lock for the whole process. Have a look at tcpm_dr_set() and
tcpm_pr_set() as examples of existing usage.

> > +	return ret;
> > +}
> > +
> > +static int tcpm_pps_set_out_volt(struct tcpm_port *port, u16 out_volt)
> > +{
> > +	unsigned int target_mw;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&port->swap_lock);
> > +	mutex_lock(&port->lock);
> > +
> > +	if (!port->pps_data.active) {
> > +		ret = -EOPNOTSUPP;
> > +		goto port_unlock;
>
> Or, on top of what I said above, you could actually consider releasing
> the port lock here and just returning. Then you would not need those
> port_unlock and swap_unlock labels at all..
>
>                 mutex_unlock(&port->lock);
>                 return -EOPNOTSUPP;

Based on the comment above, I don't think this makes sense as you'd still need
to release the swap_lock as well, so there would be quite a lot of duplicated
code. Would prefer to stick with the present implementation.