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

[Boris Brezillon <bbrezillon@kernel.org>]

On Wed, 30 Jan 2019 08:29:45 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:

> The 01/30/2019 08:56, Boris Brezillon wrote:
> > EXTERNAL MAIL
> > 
> > 
> > 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.  
> 
> Thank you for the explanation. It's clear now.
> 

Just realize we want a fast path for the likely "bus is already owned"
case, so we should actually do something like:

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

	i3c_bus_normaluse_lock(dev->bus);
	if (!i3c_master_owns_bus(master)) {
		i3c_bus_normaluse_unlock(dev->bus);
		i3c_bus_maintenance_lock(dev->bus);
		ret = i3c_master_request_mastership(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;
} 

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