Re: [PATCH v4 4/7] typec: tcpm: Add core support for sink side PPS

[Heikki Krogerus]

On Tue, Feb 06, 2018 at 02:33:08PM +0000, Adam Thomson wrote:
> 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.

OK. Then I'm fine with this patch as well. FWIW:

Acked-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

-- 
heikki