Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem

[Boris Brezillon <bbrezillon@kernel.org>]

On Tue, 29 Jan 2019 18:13:37 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:

> The 01/15/2019 22:09, Boris Brezillon wrote:
> > EXTERNAL MAIL
> > 
> > 
> > On Fri, 11 Jan 2019 17:43:35 +0000
> > Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >   
> > > +		i3c_bus_normaluse_unlock(i3cbus);
> > > +	}
> > > +
> > >  	i3c_bus_normaluse_lock(i3cbus);
> > >  	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > >  		      i3cbus->cur_master->info.pid);
> > > @@ -663,6 +728,12 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
> > >  		    !rwsem_is_locked(&master->bus.lock)))
> > >  		return -EINVAL;
> > >  
> > > +	if (!i3c_master_owns_bus(master)) {
> > > +		ret = i3c_master_request_mastership(master);
> > > +		if (ret)
> > > +			return ret;
> > > +	}  
> > 
> > As I said above, I think bus ownership should be requested in
> > maintenance mode, which means it has to be done before entering this
> > function. You can probably start acquiring the lock in write (AKA
> > maintenance) mode and downgrade it to read (AKA normal) mode before we
> > start sending the frame (CCC, SDR, I2C or HDR). 
> >   
> 
> So, I'll have to call all the _locked functions with maintenance lock.
> Inside i3c_master_send_ccc_cmd_locked() I will downgrade the lock to read
> (normal use).

No, definitely not. The prefix _locked() means that the lock has been
acquired by the caller, and the function itself is not responsible for
downgrading the lock. What I meant is that the
i3c_master_owns_bus()+i3c_master_request_mastership() sequence does not
belong in xxxx_locked() funcs. It should be done by the caller with the
lock held in write/maintenance mode, and then downgraded to a read/normal
lock once the bus has been acquired. i3c_master_send_ccc_cmd_locked()
was probably not the best example, as this function might be
intentionally called with the lock held in write/maintenance mode
sometimes (depends on the CCC command), but for other funcs, it should
be pretty easy to do:

int i3c_master_acquire_bus_ownership_locked(struct i3c_master_controller *master)
{
	if (i3c_master_owns_bus(master))
		return 0;

	return i3c_master_request_mastership(master);
}

int i3c_device_do_priv_xfers(struct i3c_device *dev,
			     struct i3c_priv_xfer *xfers,
			     int nxfers)
{
	...

	i3c_bus_maintenance_lock(dev->bus);
	ret = i3c_master_acquire_bus_ownership(master);
	if (ret) {
		i3c_bus_maintenance_unlock(dev->bus);
		return ret;
	}

	i3c_bus_downgrade_maintenance_lock(dev->bus);
	ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
	i3c_bus_normaluse_unlock(dev->bus);

	return ret;
} 

> Isn't it inconsistent that after _locked functions I will
> unlock the bus from normal use, even if I locked it for maintenance?> 
> Also, I will have to downgrade the lock in case of failure in all _locked
> functions to unlock the bus properly using i3c_bus_normaluse_lock(). DO you
> think this looks good?

No, see above for an explanation.

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c