Re: Converting dev->mutex into dev->spinlock ?

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

On 2023/02/05 10:23, Alan Stern wrote:
> I suppose we could create separate lockdep classes for every bus_type 
> and device_type combination, as well as for the different sorts of 
> devices -- treat things like class devices separately from normal 
> devices, and so on.  But even then there would be trouble.

Sorry, since I'm not familiar with devices, I can't interpret what you
are talking about in this response. But why don't you try test5() approach
in an example module shown below (i.e. treat all dev->mutex instances
independent to each other) ?

Sharing mutex_init() (like test2() approach) causes false positives,
but allocating a key on each dev->mutex (like test5() approach) should
avoid false positives.

----------
#include <linux/module.h>
#include <linux/slab.h>

static struct mutex *alloc_mutex(void)
{
	return kzalloc(sizeof(struct mutex), GFP_KERNEL | __GFP_NOFAIL);
}

static void free_mutex(struct mutex *m)
{
	kfree(m);
}

static void test1(void)
{
	struct mutex *parent_mutex;
	struct mutex *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex();
	mutex_init(parent_mutex);

	peer_mutex[0] = alloc_mutex();
	mutex_init(peer_mutex[0]);
	peer_mutex[1] = alloc_mutex();
	mutex_init(peer_mutex[1]);
	peer_mutex[2] = alloc_mutex();
	mutex_init(peer_mutex[2]);
	peer_mutex[3] = alloc_mutex();
	mutex_init(peer_mutex[3]);

	mutex_lock(parent_mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(peer_mutex[i]);
	for (i = 0; i < 4; i++)
		mutex_unlock(peer_mutex[i]);
	mutex_unlock(parent_mutex);
	for (i = 0; i < 4; i++)
		free_mutex(peer_mutex[i]);
	free_mutex(parent_mutex);
}

static void test2(void)
{
	struct mutex *parent_mutex;
	struct mutex *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex();
	mutex_init(parent_mutex);
	for (i = 0; i < 4; i++) {
		peer_mutex[i] = alloc_mutex();
		mutex_init(peer_mutex[i]);
	}
	mutex_lock(parent_mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(peer_mutex[i]);
	for (i = 0; i < 4; i++)
		mutex_unlock(peer_mutex[i]);
	mutex_unlock(parent_mutex);
	for (i = 0; i < 4; i++)
		free_mutex(peer_mutex[i]);
	free_mutex(parent_mutex);
}

static void test3(void)
{
	struct mutex *parent_mutex;
	struct mutex *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex();
	mutex_init(parent_mutex);
	for (i = 0; i < 4; i++) {
		peer_mutex[i] = alloc_mutex();
		switch (i) {
		case 0:
			mutex_init(peer_mutex[i]);
			break;
		case 1:
			mutex_init(peer_mutex[i]);
			break;
		case 2:
			mutex_init(peer_mutex[i]);
			break;
		case 3:
			mutex_init(peer_mutex[i]);
			break;
		}
	}
	mutex_lock(parent_mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(peer_mutex[i]);
	for (i = 0; i < 4; i++)
		mutex_unlock(peer_mutex[i]);
	mutex_unlock(parent_mutex);
	for (i = 0; i < 4; i++)
		free_mutex(peer_mutex[i]);
	free_mutex(parent_mutex);
}

static void test4(void)
{
	struct mutex *parent_mutex;
	struct mutex *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex();
	mutex_init(parent_mutex);
	for (i = 0; i < 4; i++) {
		peer_mutex[i] = alloc_mutex();
		switch (i) {
		case 0:
			mutex_init(peer_mutex[i]);
			break;
		case 1:
			mutex_init(peer_mutex[i]);
			break;
		case 2:
			mutex_init(peer_mutex[i]);
			break;
		case 3:
			mutex_init(peer_mutex[i]);
			break;
		}
	}
	mutex_lock(parent_mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(peer_mutex[i]);
	for (i = 0; i < 4; i++)
		mutex_unlock(peer_mutex[i]);
	for (i = 3; i >= 0; i--)
		mutex_lock(peer_mutex[i]);
	for (i = 3; i >= 0; i--)
		mutex_unlock(peer_mutex[i]);
	mutex_unlock(parent_mutex);
	for (i = 0; i < 4; i++)
		free_mutex(peer_mutex[i]);
	free_mutex(parent_mutex);
}

struct mutex2 {
	struct mutex mutex;
	struct lock_class_key key;
};

static struct mutex2 *alloc_mutex2(void)
{
	struct mutex2 *m = kzalloc(sizeof(struct mutex2), GFP_KERNEL | __GFP_NOFAIL);

	lockdep_register_key(&m->key);
	mutex_init(&m->mutex);
	lockdep_set_class(&m->mutex, &m->key);
	return m;
}

static void free_mutex2(struct mutex2 *m)
{
	mutex_destroy(&m->mutex);
	lockdep_unregister_key(&m->key);
	kfree(m);
}

static void test5(void)
{
	struct mutex2 *parent_mutex;
	struct mutex2 *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex2();
	for (i = 0; i < 4; i++)
		peer_mutex[i] = alloc_mutex2();
	mutex_lock(&parent_mutex->mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(&peer_mutex[i]->mutex);
	for (i = 0; i < 4; i++)
		mutex_unlock(&peer_mutex[i]->mutex);
	mutex_unlock(&parent_mutex->mutex);
	for (i = 0; i < 4; i++)
		free_mutex2(peer_mutex[i]);
	free_mutex2(parent_mutex);
}

static void test6(void)
{
	struct mutex2 *parent_mutex;
	struct mutex2 *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex2();
	for (i = 0; i < 4; i++)
		peer_mutex[i] = alloc_mutex2();
	mutex_lock(&parent_mutex->mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(&peer_mutex[i]->mutex);
	for (i = 0; i < 4; i++)
		mutex_unlock(&peer_mutex[i]->mutex);
	for (i = 3; i >= 0; i--)
		mutex_lock(&peer_mutex[i]->mutex);
	for (i = 3; i >= 0; i--)
		mutex_unlock(&peer_mutex[i]->mutex);
	mutex_unlock(&parent_mutex->mutex);
	for (i = 0; i < 4; i++)
		free_mutex2(peer_mutex[i]);
	free_mutex2(parent_mutex);
}

static int __init lockdep_test_init(void)
{
	if (1)
		test1(); // safe and lockdep does not complain
	if (0)
		test2(); // safe but lockdep complains
	if (1)
		test3(); // safe and lockdep does not complain
	if (0)
		test4(); // unsafe and lockdep complains
	if (1)
		test5(); // safe and lockdep does not complain
	if (0)
		test6(); // unsafe and lockdep complains
        return -EINVAL;
}

module_init(lockdep_test_init);
MODULE_LICENSE("GPL");
----------