Re: [PATCH v6 3/8] i3c: master: i3c mastership request and handover

[Boris Brezillon]

On Fri, 17 Apr 2020 18:21:02 +0200
Parshuram Thombare <pthombar@xxxxxxxxxxx> wrote:

> +
> +/* This function is expected to be called with normaluse_lock */
> +int i3c_master_acquire_bus(struct i3c_master_controller *master)
> +{
> +	int ret = 0;
> +
> +	if (!master->this || master->this != master->bus.cur_master) {

Let's limit the number of indentations by doing

	if (master->this == master->bus.cur_master)
		return 0;


> +		if (master->mr_state == I3C_MR_IDLE) {
> +			master->mr_state = I3C_MR_WAIT_DA;
> +			init_completion(&master->mr_comp);
> +			queue_work(master->wq, &master->sec_mst_work);
> +			/*
> +			 * Bus acquire procedure may need write lock
> +			 * so release read lock before yielding
> +			 * to bus acquire state machine
> +			 */
> +			i3c_bus_normaluse_unlock(&master->bus);
> +			wait_for_completion(&master->mr_comp);
> +			i3c_bus_normaluse_lock(&master->bus);

Is that really enough? I remember we had something a bit more complex
to handle the case where bus is acquired to send a message to a device,
and another master on the bus re-acquires it before we have a chance to
send this message message. i3c_master_acquire_bus_ownership() was
dealing with that in Przemek series. It seems you've rewritten a lot of
these things. Would you mind explaining why, and how that works?

> +			if (master->mr_state != I3C_MR_DONE)
> +				ret = -EAGAIN;
> +			master->mr_state = I3C_MR_IDLE;
> +		} else {
> +			/*
> +			 * MR request is already in process for
> +			 * this master
> +			 */
> +			ret = -EAGAIN;
> +		}
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_acquire_bus);