linux-kernel - Re: [PATCH v4] clk: allow reentrant calls into the clk framework

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: <alpine.LFD.2.02.1303271209490.22263@ionos>
Date:	Wed, 27 Mar 2013 12:24:12 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Mike Turquette <mturquette@...aro.org>
cc:	linux-kernel@...r.kernel.org, linaro-kernel@...ts.linaro.org,
	patches@...aro.org,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	David Brown <davidb@...eaurora.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4] clk: allow reentrant calls into the clk framework

On Wed, 27 Mar 2013, Mike Turquette wrote:
> +/***  locking & reentrancy ***/
> +
> +static void clk_fwk_lock(void)

This function name sucks as much as the whole implementation does.

> +{
> +	/* hold the framework-wide lock, context == NULL */
> +	mutex_lock(&prepare_lock);
> +
> +	/* set context for any reentrant calls */
> +	atomic_set(&prepare_context, (int) get_current());

And what's the point of the atomic here? There is no need for an
atomic if you hold the lock. Neither here nor on the reader side.

Aside of that, the cast to (int) and the one below to (void *) are
blantantly wrong on 64 bit.

> +}
> +
> +static void clk_fwk_unlock(void)
> +{
> +	/* clear the context */
> +	atomic_set(&prepare_context, 0);
> +
> +	/* release the framework-wide lock, context == NULL */
> +	mutex_unlock(&prepare_lock);
> +}
> +
> +static bool clk_is_reentrant(void)
> +{
> +	if (mutex_is_locked(&prepare_lock))
> +		if ((void *) atomic_read(&prepare_context) == get_current())

Mooo.

> +			return true;
> +
> +	return false;
> +}

Why the heck do you need this function?

Just to sprinkle all these ugly constructs into the code:

> -	mutex_lock(&prepare_lock);
> +	/* re-enter if call is from the same context */
> +	if (clk_is_reentrant()) {
> +		__clk_unprepare(clk);
> +		return;
> +	}

Sigh. Why not doing the obvious?

Step 1/2: Wrap locking in helper functions

+static void clk_prepare_lock(void)
+{
+	mutex_lock(&prepare_lock);
+}
+
+static void clk_prepare_unlock(void)
+{
+	mutex_unlock(&prepare_lock);
+}

That way the whole change in the existing code boils down to:

-	mutex_lock(&prepare_lock);
+	clk_prepare_lock();
...
-	mutex_unlock(&prepare_lock);
+	clk_prepare_unlock();

Ditto for the spinlock.

And there is no pointless reshuffling of functions required.


Step 2/2: Implement reentrancy

+static struct task_struct *prepare_owner;
+static int prepare_refcnt;

static void clk_prepare_lock()
{
-	mutex_lock(&prepare_lock);
+	if (!mutex_trylock(&prepare_lock)) {
+		if (prepare_owner == current) {
+		   	prepare_refcnt++;
+			return;
+		}
+		mutex_lock(&prepare_lock);
+	}
+	WARN_ON_ONCE(prepare_owner != NULL);
+	WARN_ON_ONCE(prepare_refcnt != 0);
+	prepare_owner = current;
+	prepare_refcnt = 1;
}

static void clk_prepare_unlock(void)
{
-	mutex_unlock(&prepare_lock);
+	WARN_ON_ONCE(prepare_owner != current);
+	WARN_ON_ONCE(prepare_refcnt == 0);
+
+	if (--prepare_refcnt)
+		return;
+	prepare_owner = NULL;
+	mutex_unlock(&prepare_lock);
}

Ditto for the spinlock.

That step requires ZERO change to the functions. They simply work and
you don't need all this ugly reentrancy hackery.

Thanks,

	tglx
--
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/