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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200430100733.4e0dc578@collabora.com>
Date:   Thu, 30 Apr 2020 10:07:33 +0200
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     Parshuram Thombare <pthombar@...ence.com>
Cc:     <bbrezillon@...nel.org>, <vitor.soares@...opsys.com>,
        <pgaj@...ence.com>, <linux-i3c@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <mparab@...ence.com>,
        <praneeth@...com>
Subject: Re: [PATCH v6 3/8] i3c: master: i3c mastership request and handover

On Fri, 17 Apr 2020 18:21:02 +0200
Parshuram Thombare <pthombar@...ence.com> 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);