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

[Przemyslaw Gaj <pgaj@cadence.com>]

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.

-- 
-- 
Przemyslaw Gaj

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