linux-kernel - Shared i2c adapter locking (Was: linux-next: manual merge of the net tree with the i2c tree)

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20091029154317.651904b9@hyperion.delvare>
Date:	Thu, 29 Oct 2009 15:43:17 +0100
From:	Jean Delvare <khali@...ux-fr.org>
To:	Stephen Rothwell <sfr@...b.auug.org.au>
Cc:	David Miller <davem@...emloft.net>, <netdev@...r.kernel.org>,
	linux-next@...r.kernel.org, linux-kernel@...r.kernel.org,
	Mika Kuoppala <mika.kuoppala@...ia.com>,
	Ben Hutchings <bhutchings@...arflare.com>,
	Linux I2C <linux-i2c@...r.kernel.org>
Subject: Shared i2c adapter locking (Was: linux-next: manual merge of the
 net tree with the i2c tree)

Hi Stephen,

On Mon, 26 Oct 2009 13:37:57 +1100, Stephen Rothwell wrote:
> Today's linux-next merge of the net tree got a conflict in
> drivers/net/sfc/sfe4001.c between commit
> 3f7c0648f727a6d5baf6117653e4001dc877b90b ("i2c: Prevent priority
> inversion on top of bus lock") from the i2c tree and commit
> c9597d4f89565b6562bd3026adbe6eac6c317f47 ("sfc: Merge sfe4001.c into
> falcon_boards.c") from the net tree.
> 
> I have applied the following merge fixup patch (after removing
> drivers/net/sfc/sfe4001.c) and can carry it as necessary.

Thanks for fixing it. The core problem here IMHO is that the sfc
network driver touches i2c internals which it would rather leave alone.
This is the only driver I know of which does this.

I can think of 3 different ways to address the issue.

Method #1: add a public API to grab/release an I2C segment.

void i2c_adapter_lock(struct i2c_adapter *adapter)
{
	rt_mutex_lock(&adapter->bus_lock);
}

void i2c_adapter_unlock(struct i2c_adapter *adapter)
{
	rt_mutex_unlock(&adapter->bus_lock);
}

It has the advantage of simplicity. The problem is that it is not
symmetric. Whatever shares the pins with I2C, I2C is considered the
main user in that its mutex is used to guarantee mutual exclusion. If
the other subsystem also has a mandatory locking mechanism, there will
have to be a decision on who is locked first. If not all drivers agree,
lockdep will get confused.

Method #2: let the driver implement its own "main" locking, and have
all pin users (i2c etc.) take it in addition to any subsystem-specific
mutex. As far as the sfc network driver is concerned, that would mean
adding pre-xfer and post-xfer hooks to i2c-algo-bit, where the "main"
mutex in question would be taken resp. released.

It has the advantage of symmetry. The problem is performance, as
regular operation will require to take and release two mutexes instead
of just one. But it is a fairly generic approach which could solve
other issues too.

Method #3: let individual bus drivers implement segment locking
themselves, with custom locking/unlocking hooks. This way, a device
sharing pins between several functions can have its own mutex
protecting these pins globally, and all user subsystems (i2c etc.) use
this single mutex.

It has the advantage of performance and symmetry, at the price of
fragility. If the i2c bus driver implements locking improperly... I am
also worried by inconsistencies that may arise, if different i2c
adapters are protected by different mutex types (mutex vs. real-time
mutex vs. spinlock etc.) but maybe this will be seen as an advantage
rather than a problem.

I'm not really sure if I have a preference yet, so please speak up if
you do.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/