[PATCH v3 00/10] Allow opal-async waiters to get interrupted

[PATCH v3 00/10] Allow opal-async waiters to get interrupted

[Cyril Bur]

V3: export opal_error_code() so that powernv_flash can be built=m

Hello,

Version one of this series ignored that OPAL may continue to use
buffers passed to it after Linux kfree()s the buffer. This version
addresses this, not in a particularly nice way - future work could
make this better. This version also includes a few cleanups and fixups
to powernv_flash driver one along the course of this work that I
thought I would just send.

The problem we're trying to solve here is that currently all users of
the opal-async calls must use wait_event(), this may be undesirable
when there is a userspace process behind the request for the opal
call, if OPAL takes too long to complete the call then hung task
warnings will appear.

In order to solve the problem callers should use
wait_event_interruptible(), due to the interruptible nature of this
call the opal-async infrastructure needs to track extra state
associated with each async token, this is prepared for in patch 6/10.

While I was working on the opal-async infrastructure improvements
Stewart fixed another problem and he relies on the corrected behaviour
of opal-async so I've sent it here.

Hello MTD folk, traditionally Michael Ellerman takes powernv_flash
driver patches through the powerpc tree, as always your feedback is
very welcome.

Thanks,

Cyril

Cyril Bur (9):
  mtd: powernv_flash: Use WARN_ON_ONCE() rather than BUG_ON()
  mtd: powernv_flash: Lock around concurrent access to OPAL
  mtd: powernv_flash: Don't treat OPAL_SUCCESS as an error
  mtd: powernv_flash: Remove pointless goto in driver init
  powerpc/opal: Make __opal_async_{get,release}_token() static
  powerpc/opal: Rework the opal-async interface
  powerpc/opal: Add opal_async_wait_response_interruptible() to
    opal-async
  powerpc/powernv: Add OPAL_BUSY to opal_error_code()
  mtd: powernv_flash: Use opal_async_wait_response_interruptible()

Stewart Smith (1):
  powernv/opal-sensor: remove not needed lock

 arch/powerpc/include/asm/opal.h              |   4 +-
 arch/powerpc/platforms/powernv/opal-async.c  | 188 +++++++++++++++++++--------
 arch/powerpc/platforms/powernv/opal-sensor.c |  17 +--
 arch/powerpc/platforms/powernv/opal.c        |   2 +
 drivers/mtd/devices/powernv_flash.c          |  66 +++++++---
 5 files changed, 191 insertions(+), 86 deletions(-)

-- 
2.13.2

[PATCH v3 01/10] mtd: powernv_flash: Use WARN_ON_ONCE() rather than BUG_ON()

[Cyril Bur]

BUG_ON() should be reserved in situations where we can not longer
guarantee the integrity of the system. In the case where
powernv_flash_async_op() receives an impossible op, we can still
guarantee the integrity of the system.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/mtd/devices/powernv_flash.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index f5396f26ddb4..a9a20c00687c 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -78,7 +78,8 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		rc = opal_flash_erase(info->id, offset, len, token);
 		break;
 	default:
-		BUG_ON(1);
+		WARN_ON_ONCE(1);
+		return -EIO;
 	}
 
 	if (rc != OPAL_ASYNC_COMPLETION) {
-- 
2.13.2

[PATCH v3 02/10] mtd: powernv_flash: Lock around concurrent access to OPAL

[Cyril Bur]

OPAL can only manage one flash access at a time and will return an
OPAL_BUSY error for each concurrent access to the flash. The simplest
way to prevent this from happening is with a mutex.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/mtd/devices/powernv_flash.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index a9a20c00687c..7b41af06f4fe 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -38,6 +38,7 @@
 
 struct powernv_flash {
 	struct mtd_info	mtd;
+	struct mutex lock;
 	u32 id;
 };
 
@@ -59,12 +60,15 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 	dev_dbg(dev, "%s(op=%d, offset=0x%llx, len=%zu)\n",
 			__func__, op, offset, len);
 
+	mutex_lock(&info->lock);
+
 	token = opal_async_get_token_interruptible();
 	if (token < 0) {
 		if (token != -ERESTARTSYS)
 			dev_err(dev, "Failed to get an async token\n");
 
-		return token;
+		rc = token;
+		goto out;
 	}
 
 	switch (op) {
@@ -79,18 +83,21 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		break;
 	default:
 		WARN_ON_ONCE(1);
-		return -EIO;
+		rc = -EIO;
+		goto out;
 	}
 
 	if (rc != OPAL_ASYNC_COMPLETION) {
 		dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
 				op, rc);
 		opal_async_release_token(token);
-		return -EIO;
+		rc = -EIO;
+		goto out;
 	}
 
 	rc = opal_async_wait_response(token, &msg);
 	opal_async_release_token(token);
+	mutex_unlock(&info->lock);
 	if (rc) {
 		dev_err(dev, "opal async wait failed (rc %d)\n", rc);
 		return -EIO;
@@ -106,6 +113,9 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 	}
 
 	return rc;
+out:
+	mutex_unlock(&info->lock);
+	return rc;
 }
 
 /**
@@ -237,6 +247,8 @@ static int powernv_flash_probe(struct platform_device *pdev)
 	if (ret)
 		goto out;
 
+	mutex_init(&data->lock);
+
 	dev_set_drvdata(dev, data);
 
 	/*
-- 
2.13.2

[PATCH v3 03/10] mtd: powernv_flash: Don't treat OPAL_SUCCESS as an error

[Cyril Bur]

While this driver expects to interact asynchronously, OPAL is well
within its rights to return OPAL_SUCCESS to indicate that the operation
completed without the need for a callback. We shouldn't treat
OPAL_SUCCESS as an error rather we should wrap up and return promptly to
the caller.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
I'll note here that currently no OPAL exists that will return
OPAL_SUCCESS so there isn't the possibility of a bug today.

 drivers/mtd/devices/powernv_flash.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index 7b41af06f4fe..d50b5f200f73 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -66,9 +66,8 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 	if (token < 0) {
 		if (token != -ERESTARTSYS)
 			dev_err(dev, "Failed to get an async token\n");
-
-		rc = token;
-		goto out;
+		mutex_unlock(&info->lock);
+		return token;
 	}
 
 	switch (op) {
@@ -87,23 +86,25 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		goto out;
 	}
 
+	if (rc == OPAL_SUCCESS)
+		goto out_success;
+
 	if (rc != OPAL_ASYNC_COMPLETION) {
 		dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
 				op, rc);
-		opal_async_release_token(token);
 		rc = -EIO;
 		goto out;
 	}
 
 	rc = opal_async_wait_response(token, &msg);
-	opal_async_release_token(token);
-	mutex_unlock(&info->lock);
 	if (rc) {
 		dev_err(dev, "opal async wait failed (rc %d)\n", rc);
-		return -EIO;
+		rc = -EIO;
+		goto out;
 	}
 
 	rc = opal_get_async_rc(msg);
+out_success:
 	if (rc == OPAL_SUCCESS) {
 		rc = 0;
 		if (retlen)
@@ -112,8 +113,8 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		rc = -EIO;
 	}
 
-	return rc;
 out:
+	opal_async_release_token(token);
 	mutex_unlock(&info->lock);
 	return rc;
 }
-- 
2.13.2

[PATCH v3 04/10] mtd: powernv_flash: Remove pointless goto in driver init

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/mtd/devices/powernv_flash.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index d50b5f200f73..d7243b72ba6e 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -232,21 +232,20 @@ static int powernv_flash_probe(struct platform_device *pdev)
 	int ret;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
-	if (!data) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!data)
+		return -ENOMEM;
+
 	data->mtd.priv = data;
 
 	ret = of_property_read_u32(dev->of_node, "ibm,opal-id", &(data->id));
 	if (ret) {
 		dev_err(dev, "no device property 'ibm,opal-id'\n");
-		goto out;
+		return ret;
 	}
 
 	ret = powernv_flash_set_driver_info(dev, &data->mtd);
 	if (ret)
-		goto out;
+		return ret;
 
 	mutex_init(&data->lock);
 
@@ -257,10 +256,7 @@ static int powernv_flash_probe(struct platform_device *pdev)
 	 * with an ffs partition at the start, it should prove easier for users
 	 * to deal with partitions or not as they see fit
 	 */
-	ret = mtd_device_register(&data->mtd, NULL, 0);
-
-out:
-	return ret;
+	return mtd_device_register(&data->mtd, NULL, 0);
 }
 
 /**
-- 
2.13.2

[PATCH v3 05/10] powerpc/opal: Make __opal_async_{get, release}_token() static

[Cyril Bur]

There are no callers of both __opal_async_get_token() and
__opal_async_release_token().

This patch also removes the possibility of "emergency through
synchronous call to __opal_async_get_token()" as such it makes more
sense to initialise opal_sync_sem for the maximum number of async
tokens.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/powerpc/include/asm/opal.h             |  2 --
 arch/powerpc/platforms/powernv/opal-async.c | 10 +++-------
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 588fb1c23af9..5553ad2f3e53 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -291,9 +291,7 @@ extern void opal_notifier_enable(void);
 extern void opal_notifier_disable(void);
 extern void opal_notifier_update_evt(uint64_t evt_mask, uint64_t evt_val);
 
-extern int __opal_async_get_token(void);
 extern int opal_async_get_token_interruptible(void);
-extern int __opal_async_release_token(int token);
 extern int opal_async_release_token(int token);
 extern int opal_async_wait_response(uint64_t token, struct opal_msg *msg);
 extern int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data);
diff --git a/arch/powerpc/platforms/powernv/opal-async.c b/arch/powerpc/platforms/powernv/opal-async.c
index 83bebeec0fea..1d56ac9da347 100644
--- a/arch/powerpc/platforms/powernv/opal-async.c
+++ b/arch/powerpc/platforms/powernv/opal-async.c
@@ -33,7 +33,7 @@ static struct semaphore opal_async_sem;
 static struct opal_msg *opal_async_responses;
 static unsigned int opal_max_async_tokens;
 
-int __opal_async_get_token(void)
+static int __opal_async_get_token(void)
 {
 	unsigned long flags;
 	int token;
@@ -73,7 +73,7 @@ int opal_async_get_token_interruptible(void)
 }
 EXPORT_SYMBOL_GPL(opal_async_get_token_interruptible);
 
-int __opal_async_release_token(int token)
+static int __opal_async_release_token(int token)
 {
 	unsigned long flags;
 
@@ -199,11 +199,7 @@ int __init opal_async_comp_init(void)
 		goto out_opal_node;
 	}
 
-	/* Initialize to 1 less than the maximum tokens available, as we may
-	 * require to pop one during emergency through synchronous call to
-	 * __opal_async_get_token()
-	 */
-	sema_init(&opal_async_sem, opal_max_async_tokens - 1);
+	sema_init(&opal_async_sem, opal_max_async_tokens);
 
 out_opal_node:
 	of_node_put(opal_node);
-- 
2.13.2

[PATCH v3 06/10] powerpc/opal: Rework the opal-async interface

[Cyril Bur]

Future work will add an opal_async_wait_response_interruptible()
which will call wait_event_interruptible(). This work requires extra
token state to be tracked as wait_event_interruptible() can return and
the caller could release the token before OPAL responds.

Currently token state is tracked with two bitfields which are 64 bits
big but may not need to be as OPAL informs Linux how many async tokens
there are. It also uses an array indexed by token to store response
messages for each token.

The bitfields make it difficult to add more state and also provide a
hard maximum as to how many tokens there can be - it is possible that
OPAL will inform Linux that there are more than 64 tokens.

Rather than add a bitfield to track the extra state, rework the
internals slightly.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/powerpc/platforms/powernv/opal-async.c | 97 ++++++++++++++++-------------
 1 file changed, 53 insertions(+), 44 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-async.c b/arch/powerpc/platforms/powernv/opal-async.c
index 1d56ac9da347..d692372a0363 100644
--- a/arch/powerpc/platforms/powernv/opal-async.c
+++ b/arch/powerpc/platforms/powernv/opal-async.c
@@ -1,7 +1,7 @@
 /*
  * PowerNV OPAL asynchronous completion interfaces
  *
- * Copyright 2013 IBM Corp.
+ * Copyright 2013-2017 IBM Corp.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -23,40 +23,46 @@
 #include <asm/machdep.h>
 #include <asm/opal.h>
 
-#define N_ASYNC_COMPLETIONS	64
+enum opal_async_token_state {
+	ASYNC_TOKEN_FREE,
+	ASYNC_TOKEN_ALLOCATED,
+	ASYNC_TOKEN_COMPLETED
+};
+
+struct opal_async_token {
+	enum opal_async_token_state state;
+	struct opal_msg response;
+};
 
-static DECLARE_BITMAP(opal_async_complete_map, N_ASYNC_COMPLETIONS) = {~0UL};
-static DECLARE_BITMAP(opal_async_token_map, N_ASYNC_COMPLETIONS);
 static DECLARE_WAIT_QUEUE_HEAD(opal_async_wait);
 static DEFINE_SPINLOCK(opal_async_comp_lock);
 static struct semaphore opal_async_sem;
-static struct opal_msg *opal_async_responses;
 static unsigned int opal_max_async_tokens;
+static struct opal_async_token *opal_async_tokens;
 
 static int __opal_async_get_token(void)
 {
 	unsigned long flags;
 	int token;
 
-	spin_lock_irqsave(&opal_async_comp_lock, flags);
-	token = find_first_bit(opal_async_complete_map, opal_max_async_tokens);
-	if (token >= opal_max_async_tokens) {
-		token = -EBUSY;
-		goto out;
-	}
-
-	if (__test_and_set_bit(token, opal_async_token_map)) {
-		token = -EBUSY;
-		goto out;
+	for (token = 0; token < opal_max_async_tokens; token++) {
+		spin_lock_irqsave(&opal_async_comp_lock, flags);
+		if (opal_async_tokens[token].state == ASYNC_TOKEN_FREE) {
+			opal_async_tokens[token].state = ASYNC_TOKEN_ALLOCATED;
+			spin_unlock_irqrestore(&opal_async_comp_lock, flags);
+			return token;
+		}
+		spin_unlock_irqrestore(&opal_async_comp_lock, flags);
 	}
 
-	__clear_bit(token, opal_async_complete_map);
-
-out:
-	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
-	return token;
+	return -EBUSY;
 }
 
+/*
+ * Note: If the returned token is used in an opal call and opal returns
+ * OPAL_ASYNC_COMPLETION you MUST opal_async_wait_response() before
+ * calling another other opal_async_* function
+ */
 int opal_async_get_token_interruptible(void)
 {
 	int token;
@@ -76,6 +82,7 @@ EXPORT_SYMBOL_GPL(opal_async_get_token_interruptible);
 static int __opal_async_release_token(int token)
 {
 	unsigned long flags;
+	int rc;
 
 	if (token < 0 || token >= opal_max_async_tokens) {
 		pr_err("%s: Passed token is out of range, token %d\n",
@@ -84,11 +91,18 @@ static int __opal_async_release_token(int token)
 	}
 
 	spin_lock_irqsave(&opal_async_comp_lock, flags);
-	__set_bit(token, opal_async_complete_map);
-	__clear_bit(token, opal_async_token_map);
+	switch (opal_async_tokens[token].state) {
+	case ASYNC_TOKEN_COMPLETED:
+	case ASYNC_TOKEN_ALLOCATED:
+		opal_async_tokens[token].state = ASYNC_TOKEN_FREE;
+		rc = 0;
+		break;
+	default:
+		rc = 1;
+	}
 	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
 
-	return 0;
+	return rc;
 }
 
 int opal_async_release_token(int token)
@@ -96,12 +110,10 @@ int opal_async_release_token(int token)
 	int ret;
 
 	ret = __opal_async_release_token(token);
-	if (ret)
-		return ret;
-
-	up(&opal_async_sem);
+	if (!ret)
+		up(&opal_async_sem);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(opal_async_release_token);
 
@@ -122,13 +134,15 @@ int opal_async_wait_response(uint64_t token, struct opal_msg *msg)
 	 * functional.
 	 */
 	opal_wake_poller();
-	wait_event(opal_async_wait, test_bit(token, opal_async_complete_map));
-	memcpy(msg, &opal_async_responses[token], sizeof(*msg));
+	wait_event(opal_async_wait, opal_async_tokens[token].state
+			== ASYNC_TOKEN_COMPLETED);
+	memcpy(msg, &opal_async_tokens[token].response, sizeof(*msg));
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(opal_async_wait_response);
 
+/* Called from interrupt context */
 static int opal_async_comp_event(struct notifier_block *nb,
 		unsigned long msg_type, void *msg)
 {
@@ -140,9 +154,9 @@ static int opal_async_comp_event(struct notifier_block *nb,
 		return 0;
 
 	token = be64_to_cpu(comp_msg->params[0]);
-	memcpy(&opal_async_responses[token], comp_msg, sizeof(*comp_msg));
+	memcpy(&opal_async_tokens[token].response, comp_msg, sizeof(*comp_msg));
 	spin_lock_irqsave(&opal_async_comp_lock, flags);
-	__set_bit(token, opal_async_complete_map);
+	opal_async_tokens[token].state = ASYNC_TOKEN_COMPLETED;
 	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
 
 	wake_up(&opal_async_wait);
@@ -178,24 +192,19 @@ int __init opal_async_comp_init(void)
 	}
 
 	opal_max_async_tokens = be32_to_cpup(async);
-	if (opal_max_async_tokens > N_ASYNC_COMPLETIONS)
-		opal_max_async_tokens = N_ASYNC_COMPLETIONS;
+	opal_async_tokens = kcalloc(opal_max_async_tokens,
+			sizeof(*opal_async_tokens), GFP_KERNEL);
+	if (!opal_async_tokens) {
+		err = -ENOMEM;
+		goto out_opal_node;
+	}
 
 	err = opal_message_notifier_register(OPAL_MSG_ASYNC_COMP,
 			&opal_async_comp_nb);
 	if (err) {
 		pr_err("%s: Can't register OPAL event notifier (%d)\n",
 				__func__, err);
-		goto out_opal_node;
-	}
-
-	opal_async_responses = kzalloc(
-			sizeof(*opal_async_responses) * opal_max_async_tokens,
-			GFP_KERNEL);
-	if (!opal_async_responses) {
-		pr_err("%s: Out of memory, failed to do asynchronous "
-				"completion init\n", __func__);
-		err = -ENOMEM;
+		kfree(opal_async_tokens);
 		goto out_opal_node;
 	}
 
-- 
2.13.2

[PATCH v3 07/10] powernv/opal-sensor: remove not needed lock

[Cyril Bur]

From: Stewart Smith <stewart@linux.vnet.ibm.com>

Parallel sensor reads could run out of async tokens due to
opal_get_sensor_data grabbing tokens but then doing the sensor
read behind a mutex, essentially serializing the (possibly
asynchronous and relatively slow) sensor read.

It turns out that the mutex isn't needed at all, not only
should the OPAL interface allow concurrent reads, the implementation
is certainly safe for that, and if any sensor we were reading
from somewhere isn't, doing the mutual exclusion in the kernel
is the wrong place to do it, OPAL should be doing it for the kernel.

So, remove the mutex.

Additionally, we shouldn't be printing out an error when we don't
get a token as the only way this should happen is if we've been
interrupted in down_interruptible() on the semaphore.

Reported-by: Robert Lippert <rlippert@google.com>
Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com>
Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/powerpc/platforms/powernv/opal-sensor.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c b/arch/powerpc/platforms/powernv/opal-sensor.c
index aa267f120033..0a7074bb91dc 100644
--- a/arch/powerpc/platforms/powernv/opal-sensor.c
+++ b/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -19,13 +19,10 @@
  */
 
 #include <linux/delay.h>
-#include <linux/mutex.h>
 #include <linux/of_platform.h>
 #include <asm/opal.h>
 #include <asm/machdep.h>
 
-static DEFINE_MUTEX(opal_sensor_mutex);
-
 /*
  * This will return sensor information to driver based on the requested sensor
  * handle. A handle is an opaque id for the powernv, read by the driver from the
@@ -38,13 +35,9 @@ int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data)
 	__be32 data;
 
 	token = opal_async_get_token_interruptible();
-	if (token < 0) {
-		pr_err("%s: Couldn't get the token, returning\n", __func__);
-		ret = token;
-		goto out;
-	}
+	if (token < 0)
+		return token;
 
-	mutex_lock(&opal_sensor_mutex);
 	ret = opal_sensor_read(sensor_hndl, token, &data);
 	switch (ret) {
 	case OPAL_ASYNC_COMPLETION:
@@ -52,7 +45,7 @@ int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data)
 		if (ret) {
 			pr_err("%s: Failed to wait for the async response, %d\n",
 			       __func__, ret);
-			goto out_token;
+			goto out;
 		}
 
 		ret = opal_error_code(opal_get_async_rc(msg));
@@ -73,10 +66,8 @@ int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data)
 		break;
 	}
 
-out_token:
-	mutex_unlock(&opal_sensor_mutex);
-	opal_async_release_token(token);
 out:
+	opal_async_release_token(token);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(opal_get_sensor_data);
-- 
2.13.2

[PATCH v3 08/10] powerpc/opal: Add opal_async_wait_response_interruptible() to opal-async

[Cyril Bur]

This patch adds an _interruptible version of opal_async_wait_response().
This is useful when a long running OPAL call is performed on behalf of a
userspace thread, for example, the opal_flash_{read,write,erase}
functions performed by the powernv-flash MTD driver.

It is foreseeable that these functions would take upwards of two minutes
causing the wait_event() to block long enough to cause hung task
warnings. Furthermore, wait_event_interruptible() is preferable as
otherwise there is no way for signals to stop the process which is going
to be confusing in userspace.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/powerpc/include/asm/opal.h             |  2 +
 arch/powerpc/platforms/powernv/opal-async.c | 87 +++++++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 5553ad2f3e53..6e9e53d744f3 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -294,6 +294,8 @@ extern void opal_notifier_update_evt(uint64_t evt_mask, uint64_t evt_val);
 extern int opal_async_get_token_interruptible(void);
 extern int opal_async_release_token(int token);
 extern int opal_async_wait_response(uint64_t token, struct opal_msg *msg);
+extern int opal_async_wait_response_interruptible(uint64_t token,
+		struct opal_msg *msg);
 extern int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data);
 
 struct rtc_time;
diff --git a/arch/powerpc/platforms/powernv/opal-async.c b/arch/powerpc/platforms/powernv/opal-async.c
index d692372a0363..f6b30cfceb8f 100644
--- a/arch/powerpc/platforms/powernv/opal-async.c
+++ b/arch/powerpc/platforms/powernv/opal-async.c
@@ -26,6 +26,8 @@
 enum opal_async_token_state {
 	ASYNC_TOKEN_FREE,
 	ASYNC_TOKEN_ALLOCATED,
+	ASYNC_TOKEN_DISPATCHED,
+	ASYNC_TOKEN_ABANDONED,
 	ASYNC_TOKEN_COMPLETED
 };
 
@@ -59,8 +61,10 @@ static int __opal_async_get_token(void)
 }
 
 /*
- * Note: If the returned token is used in an opal call and opal returns
- * OPAL_ASYNC_COMPLETION you MUST opal_async_wait_response() before
+ * Note: If the returned token is used in an opal call and opal
+ * returns OPAL_ASYNC_COMPLETION you MUST one of
+ * opal_async_wait_response() or
+ * opal_async_wait_response_interruptible() at least once before
  * calling another other opal_async_* function
  */
 int opal_async_get_token_interruptible(void)
@@ -97,6 +101,16 @@ static int __opal_async_release_token(int token)
 		opal_async_tokens[token].state = ASYNC_TOKEN_FREE;
 		rc = 0;
 		break;
+	/*
+	 * DISPATCHED and ABANDONED tokens must wait for OPAL to
+	 * respond.
+	 * Mark a DISPATCHED token as ABANDONED so that the response
+	 * response handling code knows no one cares and that it can
+	 * free it then.
+	 */
+	case ASYNC_TOKEN_DISPATCHED:
+		opal_async_tokens[token].state = ASYNC_TOKEN_ABANDONED;
+		/* Fall through */
 	default:
 		rc = 1;
 	}
@@ -129,7 +143,11 @@ int opal_async_wait_response(uint64_t token, struct opal_msg *msg)
 		return -EINVAL;
 	}
 
-	/* Wakeup the poller before we wait for events to speed things
+	/*
+	 * There is no need to mark the token as dispatched, wait_event()
+	 * will block until the token completes.
+	 *
+	 * Wakeup the poller before we wait for events to speed things
 	 * up on platforms or simulators where the interrupts aren't
 	 * functional.
 	 */
@@ -142,11 +160,66 @@ int opal_async_wait_response(uint64_t token, struct opal_msg *msg)
 }
 EXPORT_SYMBOL_GPL(opal_async_wait_response);
 
+int opal_async_wait_response_interruptible(uint64_t token, struct opal_msg *msg)
+{
+	unsigned long flags;
+	int ret;
+
+	if (token >= opal_max_async_tokens) {
+		pr_err("%s: Invalid token passed\n", __func__);
+		return -EINVAL;
+	}
+
+	if (!msg) {
+		pr_err("%s: Invalid message pointer passed\n", __func__);
+		return -EINVAL;
+	}
+
+	/*
+	 * The first time this gets called we mark the token as DISPATCHED
+	 * so that if wait_event_interruptible() returns not zero and the
+	 * caller frees the token, we know not to actually free the token
+	 * until the response comes.
+	 *
+	 * Only change if the token is ALLOCATED - it may have been
+	 * completed even before the caller gets around to calling this
+	 * the first time.
+	 *
+	 * There is also a dirty great comment at the token allocation
+	 * function that if the opal call returns OPAL_ASYNC_COMPLETION to
+	 * the caller then the caller *must* call this or the not
+	 * interruptible version before doing anything else with the
+	 * token.
+	 */
+	if (opal_async_tokens[token].state == ASYNC_TOKEN_ALLOCATED) {
+		spin_lock_irqsave(&opal_async_comp_lock, flags);
+		if (opal_async_tokens[token].state == ASYNC_TOKEN_ALLOCATED)
+			opal_async_tokens[token].state = ASYNC_TOKEN_DISPATCHED;
+		spin_unlock_irqrestore(&opal_async_comp_lock, flags);
+	}
+
+	/*
+	 * Wakeup the poller before we wait for events to speed things
+	 * up on platforms or simulators where the interrupts aren't
+	 * functional.
+	 */
+	opal_wake_poller();
+	ret = wait_event_interruptible(opal_async_wait,
+			opal_async_tokens[token].state ==
+			ASYNC_TOKEN_COMPLETED);
+	if (!ret)
+		memcpy(msg, &opal_async_tokens[token].response, sizeof(*msg));
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(opal_async_wait_response_interruptible);
+
 /* Called from interrupt context */
 static int opal_async_comp_event(struct notifier_block *nb,
 		unsigned long msg_type, void *msg)
 {
 	struct opal_msg *comp_msg = msg;
+	enum opal_async_token_state state;
 	unsigned long flags;
 	uint64_t token;
 
@@ -154,11 +227,17 @@ static int opal_async_comp_event(struct notifier_block *nb,
 		return 0;
 
 	token = be64_to_cpu(comp_msg->params[0]);
-	memcpy(&opal_async_tokens[token].response, comp_msg, sizeof(*comp_msg));
 	spin_lock_irqsave(&opal_async_comp_lock, flags);
+	state = opal_async_tokens[token].state;
 	opal_async_tokens[token].state = ASYNC_TOKEN_COMPLETED;
 	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
 
+	if (state == ASYNC_TOKEN_ABANDONED) {
+		/* Free the token, no one else will */
+		opal_async_release_token(token);
+		return 0;
+	}
+	memcpy(&opal_async_tokens[token].response, comp_msg, sizeof(*comp_msg));
 	wake_up(&opal_async_wait);
 
 	return 0;
-- 
2.13.2

[PATCH v3 09/10] powerpc/powernv: Add OPAL_BUSY to opal_error_code()

[Cyril Bur]

Also export opal_error_code() so that it can be used in modules

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/powerpc/platforms/powernv/opal.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 59684b4af4d1..c4008a612473 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -930,6 +930,7 @@ int opal_error_code(int rc)
 
 	case OPAL_PARAMETER:		return -EINVAL;
 	case OPAL_ASYNC_COMPLETION:	return -EINPROGRESS;
+	case OPAL_BUSY:
 	case OPAL_BUSY_EVENT:		return -EBUSY;
 	case OPAL_NO_MEM:		return -ENOMEM;
 	case OPAL_PERMISSION:		return -EPERM;
@@ -968,3 +969,4 @@ EXPORT_SYMBOL_GPL(opal_write_oppanel_async);
 /* Export this for KVM */
 EXPORT_SYMBOL_GPL(opal_int_set_mfrr);
 EXPORT_SYMBOL_GPL(opal_int_eoi);
+EXPORT_SYMBOL_GPL(opal_error_code);
-- 
2.13.2

[PATCH v3 10/10] mtd: powernv_flash: Use opal_async_wait_response_interruptible()

[Cyril Bur]

The OPAL calls performed in this driver shouldn't be using
opal_async_wait_response() as this performs a wait_event() which, on
long running OPAL calls could result in hung task warnings. wait_event()
prevents timely signal delivery which is also undesirable.

This patch also attempts to quieten down the use of dev_err() when
errors haven't actually occurred and also to return better information up
the stack rather than always -EIO.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/mtd/devices/powernv_flash.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index d7243b72ba6e..cfa274ba7e40 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -90,16 +90,34 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		goto out_success;
 
 	if (rc != OPAL_ASYNC_COMPLETION) {
-		dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
+		if (rc != OPAL_BUSY)
+			dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
 				op, rc);
-		rc = -EIO;
+		rc = opal_error_code(rc);
 		goto out;
 	}
 
-	rc = opal_async_wait_response(token, &msg);
+	rc = opal_async_wait_response_interruptible(token, &msg);
 	if (rc) {
-		dev_err(dev, "opal async wait failed (rc %d)\n", rc);
-		rc = -EIO;
+		/*
+		 * Awkward, we've been interrupted but we cannot return. If we
+		 * do return the mtd core will free the buffer we've just
+		 * passed to OPAL but OPAL will continue to read or write from
+		 * that memory.
+		 * Future work will introduce a call to tell OPAL to stop
+		 * using the buffer.
+		 * It may be tempting to ultimately return 0 if we're doing a
+		 * read or a write since we are going to end up waiting until
+		 * OPAL is done. However, because the MTD core sends us the
+		 * userspace request in chunks, we must report EINTR so that
+		 * it doesn't just send us the next chunk, thus defeating the
+		 * point of the _interruptible wait.
+		 */
+		rc = -EINTR;
+		if (op == FLASH_OP_READ || op == FLASH_OP_WRITE) {
+			if (opal_async_wait_response(token, &msg))
+				dev_err(dev, "opal async wait failed (rc %d)\n", rc);
+		}
 		goto out;
 	}
 
-- 
2.13.2

Re: [PATCH v3 01/10] mtd: powernv_flash: Use WARN_ON_ONCE() rather than BUG_ON()

[Balbir Singh]

On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> BUG_ON() should be reserved in situations where we can not longer
> guarantee the integrity of the system. In the case where
> powernv_flash_async_op() receives an impossible op, we can still
> guarantee the integrity of the system.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---

Looks reasonable

Acked-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH v3 02/10] mtd: powernv_flash: Lock around concurrent access to OPAL

[Balbir Singh]

On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> OPAL can only manage one flash access at a time and will return an
> OPAL_BUSY error for each concurrent access to the flash. The simplest
> way to prevent this from happening is with a mutex.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---

Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
the mutex while other operations with the lock are busy?

Balbir Singh.

Re: [PATCH v3 02/10] mtd: powernv_flash: Lock around concurrent access to OPAL

[Cyril Bur]

On Mon, 2017-07-17 at 17:34 +1000, Balbir Singh wrote:
> On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> > OPAL can only manage one flash access at a time and will return an
> > OPAL_BUSY error for each concurrent access to the flash. The simplest
> > way to prevent this from happening is with a mutex.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> 
> Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
> the mutex while other operations with the lock are busy?
> 

This is a good question. My best interpretation is that
_interruptible() should be used when you'll only be coming from a user
context. Which is mostly true for this driver, however, MTD does
provide kernel interfaces, so I was hesitant, there isn't a great deal
of use of _interruptible() in drivers/mtd. 

Thoughts?

Cyril

> Balbir Singh.
> 

Re: [PATCH v3 03/10] mtd: powernv_flash: Don't treat OPAL_SUCCESS as an error

[Balbir Singh]

On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> While this driver expects to interact asynchronously, OPAL is well
> within its rights to return OPAL_SUCCESS to indicate that the operation
> completed without the need for a callback. We shouldn't treat
> OPAL_SUCCESS as an error rather we should wrap up and return promptly to
> the caller.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> I'll note here that currently no OPAL exists that will return
> OPAL_SUCCESS so there isn't the possibility of a bug today.

It would help if you mentioned OPAL_SUCCESS to the async call. So effectively
what we expected to be an asynchronous call with callback, but OPAL returned
immediately with success.

Balbir Singh.

Re: [PATCH v3 02/10] mtd: powernv_flash: Lock around concurrent access to OPAL

[Balbir Singh]

On Mon, 2017-07-17 at 17:55 +1000, Cyril Bur wrote:
> On Mon, 2017-07-17 at 17:34 +1000, Balbir Singh wrote:
> > On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> > > OPAL can only manage one flash access at a time and will return an
> > > OPAL_BUSY error for each concurrent access to the flash. The simplest
> > > way to prevent this from happening is with a mutex.
> > > 
> > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > > ---
> > 
> > Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
> > the mutex while other operations with the lock are busy?
> > 
> 
> This is a good question. My best interpretation is that
> _interruptible() should be used when you'll only be coming from a user
> context. Which is mostly true for this driver, however, MTD does
> provide kernel interfaces, so I was hesitant, there isn't a great deal
> of use of _interruptible() in drivers/mtd. 
> 
> Thoughts?

What are the kernel interfaces (I have not read through mtd in detail)?
I would still like to see us not blocked in mutex_lock() across threads
for parallel calls, one option is to use mutex_trylock() and return if
someone already holds the mutex with -EBUSY, but you'll need to evaluate
what that means for every call.

Balbir Singh.

Re: [PATCH v3 06/10] powerpc/opal: Rework the opal-async interface

[Balbir Singh]

On Wed, 2017-07-12 at 14:23 +1000, Cyril Bur wrote:
> Future work will add an opal_async_wait_response_interruptible()
> which will call wait_event_interruptible(). This work requires extra
> token state to be tracked as wait_event_interruptible() can return and
> the caller could release the token before OPAL responds.
> 
> Currently token state is tracked with two bitfields which are 64 bits
> big but may not need to be as OPAL informs Linux how many async tokens
> there are. It also uses an array indexed by token to store response
> messages for each token.
> 
> The bitfields make it difficult to add more state and also provide a
> hard maximum as to how many tokens there can be - it is possible that
> OPAL will inform Linux that there are more than 64 tokens.
> 
> Rather than add a bitfield to track the extra state, rework the
> internals slightly.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  arch/powerpc/platforms/powernv/opal-async.c | 97 ++++++++++++++++-------------
>  1 file changed, 53 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/opal-async.c b/arch/powerpc/platforms/powernv/opal-async.c
> index 1d56ac9da347..d692372a0363 100644
> --- a/arch/powerpc/platforms/powernv/opal-async.c
> +++ b/arch/powerpc/platforms/powernv/opal-async.c
> @@ -1,7 +1,7 @@
>  /*
>   * PowerNV OPAL asynchronous completion interfaces
>   *
> - * Copyright 2013 IBM Corp.
> + * Copyright 2013-2017 IBM Corp.
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -23,40 +23,46 @@
>  #include <asm/machdep.h>
>  #include <asm/opal.h>
>  
> -#define N_ASYNC_COMPLETIONS	64
> +enum opal_async_token_state {
> +	ASYNC_TOKEN_FREE,
> +	ASYNC_TOKEN_ALLOCATED,
> +	ASYNC_TOKEN_COMPLETED
> +};

Are these states mutually exclusive? Does _COMPLETED imply that it is also
_ALLOCATED? ALLOCATED and FREE are confusing, I would use IN_USE and NOT_IN_USE
for tokens. If these are mutually exclusive then you can use IN_USE and !IN_USE

> +
> +struct opal_async_token {
> +	enum opal_async_token_state state;
> +	struct opal_msg response;
> +};
>  
> -static DECLARE_BITMAP(opal_async_complete_map, N_ASYNC_COMPLETIONS) = {~0UL};
> -static DECLARE_BITMAP(opal_async_token_map, N_ASYNC_COMPLETIONS);
>  static DECLARE_WAIT_QUEUE_HEAD(opal_async_wait);
>  static DEFINE_SPINLOCK(opal_async_comp_lock);
>  static struct semaphore opal_async_sem;
> -static struct opal_msg *opal_async_responses;
>  static unsigned int opal_max_async_tokens;
> +static struct opal_async_token *opal_async_tokens;
>  
>  static int __opal_async_get_token(void)
>  {
>  	unsigned long flags;
>  	int token;
>  
> -	spin_lock_irqsave(&opal_async_comp_lock, flags);
> -	token = find_first_bit(opal_async_complete_map, opal_max_async_tokens);
> -	if (token >= opal_max_async_tokens) {
> -		token = -EBUSY;
> -		goto out;
> -	}
> -
> -	if (__test_and_set_bit(token, opal_async_token_map)) {
> -		token = -EBUSY;
> -		goto out;
> +	for (token = 0; token < opal_max_async_tokens; token++) {
> +		spin_lock_irqsave(&opal_async_comp_lock, flags);

Why is the spin lock inside the for loop? If the last token is free, the
number of times we'll take and release a lock is extensive, why are we
doing it this way?

> +		if (opal_async_tokens[token].state == ASYNC_TOKEN_FREE) {
> +			opal_async_tokens[token].state = ASYNC_TOKEN_ALLOCATED;
> +			spin_unlock_irqrestore(&opal_async_comp_lock, flags);
> +			return token;
> +		}
> +		spin_unlock_irqrestore(&opal_async_comp_lock, flags);
>  	}
>  
> -	__clear_bit(token, opal_async_complete_map);
> -
> -out:
> -	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
> -	return token;
> +	return -EBUSY;
>  }
>  
> +/*
> + * Note: If the returned token is used in an opal call and opal returns
> + * OPAL_ASYNC_COMPLETION you MUST opal_async_wait_response() before
> + * calling another other opal_async_* function
> + */
>  int opal_async_get_token_interruptible(void)
>  {
>  	int token;
> @@ -76,6 +82,7 @@ EXPORT_SYMBOL_GPL(opal_async_get_token_interruptible);
>  static int __opal_async_release_token(int token)
>  {
>  	unsigned long flags;
> +	int rc;
>  
>  	if (token < 0 || token >= opal_max_async_tokens) {
>  		pr_err("%s: Passed token is out of range, token %d\n",
> @@ -84,11 +91,18 @@ static int __opal_async_release_token(int token)
>  	}
>  
>  	spin_lock_irqsave(&opal_async_comp_lock, flags);
> -	__set_bit(token, opal_async_complete_map);
> -	__clear_bit(token, opal_async_token_map);
> +	switch (opal_async_tokens[token].state) {
> +	case ASYNC_TOKEN_COMPLETED:
> +	case ASYNC_TOKEN_ALLOCATED:
> +		opal_async_tokens[token].state = ASYNC_TOKEN_FREE;

So we can go from

_COMPLETED | _ALLOCATED to _FREE on release_token, why would be release
an _ALLOCATED token, in the case the callback is not really called?

> +		rc = 0;
> +		break;
> +	default:
> +		rc = 1;
> +	}
>  	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
>  
> -	return 0;
> +	return rc;
>  }
>  
>  int opal_async_release_token(int token)
> @@ -96,12 +110,10 @@ int opal_async_release_token(int token)
>  	int ret;
>  
>  	ret = __opal_async_release_token(token);
> -	if (ret)
> -		return ret;
> -
> -	up(&opal_async_sem);
> +	if (!ret)
> +		up(&opal_async_sem);

So we up the semaphore only if we made a transition and freed the token, right?
What happens otherwise?

>  
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(opal_async_release_token);
>  
> @@ -122,13 +134,15 @@ int opal_async_wait_response(uint64_t token, struct opal_msg *msg)
>  	 * functional.
>  	 */
>  	opal_wake_poller();
> -	wait_event(opal_async_wait, test_bit(token, opal_async_complete_map));
> -	memcpy(msg, &opal_async_responses[token], sizeof(*msg));
> +	wait_event(opal_async_wait, opal_async_tokens[token].state
> +			== ASYNC_TOKEN_COMPLETED);

Since wait_event is a macro, I'd recommend parenthesis around the second
argument. I think there is also an inbuilt assumption that the barriers
in schedule() called by wait_event() will make the write to the token
state visible.

> +	memcpy(msg, &opal_async_tokens[token].response, sizeof(*msg));
>  
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(opal_async_wait_response);
>  
> +/* Called from interrupt context */
>  static int opal_async_comp_event(struct notifier_block *nb,
>  		unsigned long msg_type, void *msg)
>  {
> @@ -140,9 +154,9 @@ static int opal_async_comp_event(struct notifier_block *nb,
>  		return 0;
>  
>  	token = be64_to_cpu(comp_msg->params[0]);
> -	memcpy(&opal_async_responses[token], comp_msg, sizeof(*comp_msg));
> +	memcpy(&opal_async_tokens[token].response, comp_msg, sizeof(*comp_msg));
>  	spin_lock_irqsave(&opal_async_comp_lock, flags);
> -	__set_bit(token, opal_async_complete_map);
> +	opal_async_tokens[token].state = ASYNC_TOKEN_COMPLETED;
>  	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
>  
>  	wake_up(&opal_async_wait);
> @@ -178,24 +192,19 @@ int __init opal_async_comp_init(void)
>  	}
>  
>  	opal_max_async_tokens = be32_to_cpup(async);
> -	if (opal_max_async_tokens > N_ASYNC_COMPLETIONS)
> -		opal_max_async_tokens = N_ASYNC_COMPLETIONS;
> +	opal_async_tokens = kcalloc(opal_max_async_tokens,
> +			sizeof(*opal_async_tokens), GFP_KERNEL);
> +	if (!opal_async_tokens) {
> +		err = -ENOMEM;
> +		goto out_opal_node;
> +	}
>  
>  	err = opal_message_notifier_register(OPAL_MSG_ASYNC_COMP,
>  			&opal_async_comp_nb);
>  	if (err) {
>  		pr_err("%s: Can't register OPAL event notifier (%d)\n",
>  				__func__, err);
> -		goto out_opal_node;
> -	}
> -
> -	opal_async_responses = kzalloc(
> -			sizeof(*opal_async_responses) * opal_max_async_tokens,
> -			GFP_KERNEL);
> -	if (!opal_async_responses) {
> -		pr_err("%s: Out of memory, failed to do asynchronous "
> -				"completion init\n", __func__);
> -		err = -ENOMEM;
> +		kfree(opal_async_tokens);
>  		goto out_opal_node;
>  	}
>  

Balbir Singh.

Re: [PATCH v3 01/10] mtd: powernv_flash: Use WARN_ON_ONCE() rather than BUG_ON()

[Frans Klaver]

On Wed, Jul 12, 2017 at 6:22 AM, Cyril Bur <cyrilbur@gmail.com> wrote:
> BUG_ON() should be reserved in situations where we can not longer
> guarantee the integrity of the system. In the case where
> powernv_flash_async_op() receives an impossible op, we can still
> guarantee the integrity of the system.
>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  drivers/mtd/devices/powernv_flash.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
> index f5396f26ddb4..a9a20c00687c 100644
> --- a/drivers/mtd/devices/powernv_flash.c
> +++ b/drivers/mtd/devices/powernv_flash.c
> @@ -78,7 +78,8 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
>                 rc = opal_flash_erase(info->id, offset, len, token);
>                 break;
>         default:
> -               BUG_ON(1);
> +               WARN_ON_ONCE(1);
> +               return -EIO;

Based on the fact that all three values in enum flash_op are handled,
I would go as far as stating that the default lemma adds no value and
can be removed.

Frans

Re: [PATCH v3 01/10] mtd: powernv_flash: Use WARN_ON_ONCE() rather than BUG_ON()

[Cyril Bur]

On Mon, 2017-07-17 at 13:33 +0200, Frans Klaver wrote:
> On Wed, Jul 12, 2017 at 6:22 AM, Cyril Bur <cyrilbur@gmail.com> wrote:
> > BUG_ON() should be reserved in situations where we can not longer
> > guarantee the integrity of the system. In the case where
> > powernv_flash_async_op() receives an impossible op, we can still
> > guarantee the integrity of the system.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  drivers/mtd/devices/powernv_flash.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
> > index f5396f26ddb4..a9a20c00687c 100644
> > --- a/drivers/mtd/devices/powernv_flash.c
> > +++ b/drivers/mtd/devices/powernv_flash.c
> > @@ -78,7 +78,8 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
> >                 rc = opal_flash_erase(info->id, offset, len, token);
> >                 break;
> >         default:
> > -               BUG_ON(1);
> > +               WARN_ON_ONCE(1);
> > +               return -EIO;
> 
> Based on the fact that all three values in enum flash_op are handled,
> I would go as far as stating that the default lemma adds no value and
> can be removed.
> 

The way I see it is that it isn't doing any harm being there and in
cases of future programmer error or during corruption events, that
WARN_ON might prove useful.

> Frans

Re: [PATCH v3 06/10] powerpc/opal: Rework the opal-async interface

[Cyril Bur]

On Mon, 2017-07-17 at 21:30 +1000, Balbir Singh wrote:
> On Wed, 2017-07-12 at 14:23 +1000, Cyril Bur wrote:
> > Future work will add an opal_async_wait_response_interruptible()
> > which will call wait_event_interruptible(). This work requires extra
> > token state to be tracked as wait_event_interruptible() can return and
> > the caller could release the token before OPAL responds.
> > 
> > Currently token state is tracked with two bitfields which are 64 bits
> > big but may not need to be as OPAL informs Linux how many async tokens
> > there are. It also uses an array indexed by token to store response
> > messages for each token.
> > 
> > The bitfields make it difficult to add more state and also provide a
> > hard maximum as to how many tokens there can be - it is possible that
> > OPAL will inform Linux that there are more than 64 tokens.
> > 
> > Rather than add a bitfield to track the extra state, rework the
> > internals slightly.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  arch/powerpc/platforms/powernv/opal-async.c | 97 ++++++++++++++++-------------
> >  1 file changed, 53 insertions(+), 44 deletions(-)
> > 
> > diff --git a/arch/powerpc/platforms/powernv/opal-async.c b/arch/powerpc/platforms/powernv/opal-async.c
> > index 1d56ac9da347..d692372a0363 100644
> > --- a/arch/powerpc/platforms/powernv/opal-async.c
> > +++ b/arch/powerpc/platforms/powernv/opal-async.c
> > @@ -1,7 +1,7 @@
> >  /*
> >   * PowerNV OPAL asynchronous completion interfaces
> >   *
> > - * Copyright 2013 IBM Corp.
> > + * Copyright 2013-2017 IBM Corp.
> >   *
> >   * This program is free software; you can redistribute it and/or
> >   * modify it under the terms of the GNU General Public License
> > @@ -23,40 +23,46 @@
> >  #include <asm/machdep.h>
> >  #include <asm/opal.h>
> >  
> > -#define N_ASYNC_COMPLETIONS	64
> > +enum opal_async_token_state {
> > +	ASYNC_TOKEN_FREE,
> > +	ASYNC_TOKEN_ALLOCATED,
> > +	ASYNC_TOKEN_COMPLETED
> > +};
> 
> Are these states mutually exclusive? Does _COMPLETED imply that it is also
> _ALLOCATED? 

Yes

> ALLOCATED and FREE are confusing, I would use IN_USE and NOT_IN_USE
> for tokens. If these are mutually exclusive then you can use IN_USE and !IN_USE
> 

Perhaps instead of _FREE it could be _UNALLOCATED ?

> > +
> > +struct opal_async_token {
> > +	enum opal_async_token_state state;
> > +	struct opal_msg response;
> > +};
> >  
> > -static DECLARE_BITMAP(opal_async_complete_map, N_ASYNC_COMPLETIONS) = {~0UL};
> > -static DECLARE_BITMAP(opal_async_token_map, N_ASYNC_COMPLETIONS);
> >  static DECLARE_WAIT_QUEUE_HEAD(opal_async_wait);
> >  static DEFINE_SPINLOCK(opal_async_comp_lock);
> >  static struct semaphore opal_async_sem;
> > -static struct opal_msg *opal_async_responses;
> >  static unsigned int opal_max_async_tokens;
> > +static struct opal_async_token *opal_async_tokens;
> >  
> >  static int __opal_async_get_token(void)
> >  {
> >  	unsigned long flags;
> >  	int token;
> >  
> > -	spin_lock_irqsave(&opal_async_comp_lock, flags);
> > -	token = find_first_bit(opal_async_complete_map, opal_max_async_tokens);
> > -	if (token >= opal_max_async_tokens) {
> > -		token = -EBUSY;
> > -		goto out;
> > -	}
> > -
> > -	if (__test_and_set_bit(token, opal_async_token_map)) {
> > -		token = -EBUSY;
> > -		goto out;
> > +	for (token = 0; token < opal_max_async_tokens; token++) {
> > +		spin_lock_irqsave(&opal_async_comp_lock, flags);
> 
> Why is the spin lock inside the for loop? If the last token is free, the
> number of times we'll take and release a lock is extensive, why are we
> doing it this way?
> 

Otherwise we might hold the lock for quite some time. At the moment I
think it isn't a bit deal since OPAL gives 8 but there is current work
to increase that number and while it seems the number might only grow
to 16, for a while it was looking like it might grow more.

In a previous iteration I had a check inside the loop but outside the
lock for if (token == ASYNC_TOKEN_FREE) which would then proceed to
take the lock, check again and mark it allocated...

Or I could put the lock around the loop, I'm not attached to any
particular approach.

> > +		if (opal_async_tokens[token].state == ASYNC_TOKEN_FREE) {
> > +			opal_async_tokens[token].state = ASYNC_TOKEN_ALLOCATED;
> > +			spin_unlock_irqrestore(&opal_async_comp_lock, flags);
> > +			return token;
> > +		}
> > +		spin_unlock_irqrestore(&opal_async_comp_lock, flags);
> >  	}
> >  
> > -	__clear_bit(token, opal_async_complete_map);
> > -
> > -out:
> > -	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
> > -	return token;
> > +	return -EBUSY;
> >  }
> >  
> > +/*
> > + * Note: If the returned token is used in an opal call and opal returns
> > + * OPAL_ASYNC_COMPLETION you MUST opal_async_wait_response() before
> > + * calling another other opal_async_* function
> > + */
> >  int opal_async_get_token_interruptible(void)
> >  {
> >  	int token;
> > @@ -76,6 +82,7 @@ EXPORT_SYMBOL_GPL(opal_async_get_token_interruptible);
> >  static int __opal_async_release_token(int token)
> >  {
> >  	unsigned long flags;
> > +	int rc;
> >  
> >  	if (token < 0 || token >= opal_max_async_tokens) {
> >  		pr_err("%s: Passed token is out of range, token %d\n",
> > @@ -84,11 +91,18 @@ static int __opal_async_release_token(int token)
> >  	}
> >  
> >  	spin_lock_irqsave(&opal_async_comp_lock, flags);
> > -	__set_bit(token, opal_async_complete_map);
> > -	__clear_bit(token, opal_async_token_map);
> > +	switch (opal_async_tokens[token].state) {
> > +	case ASYNC_TOKEN_COMPLETED:
> > +	case ASYNC_TOKEN_ALLOCATED:
> > +		opal_async_tokens[token].state = ASYNC_TOKEN_FREE;
> 
> So we can go from
> 
> _COMPLETED | _ALLOCATED to _FREE on release_token, why would be release
> an _ALLOCATED token, in the case the callback is not really called?
> 

If the OPAL call fails, the caller can either choose to retry the OPAL
call or just abandon, if it abandons then it must free the token (which
will never complete since the OPAL call failed).

> > +		rc = 0;
> > +		break;
> > +	default:
> > +		rc = 1;
> > +	}
> >  	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
> >  
> > -	return 0;
> > +	return rc;
> >  }
> >  
> >  int opal_async_release_token(int token)
> > @@ -96,12 +110,10 @@ int opal_async_release_token(int token)
> >  	int ret;
> >  
> >  	ret = __opal_async_release_token(token);
> > -	if (ret)
> > -		return ret;
> > -
> > -	up(&opal_async_sem);
> > +	if (!ret)
> > +		up(&opal_async_sem);
> 
> So we up the semaphore only if we made a transition and freed the token, right?
> What happens otherwise?
> 
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(opal_async_release_token);
> >  
> > @@ -122,13 +134,15 @@ int opal_async_wait_response(uint64_t token, struct opal_msg *msg)
> >  	 * functional.
> >  	 */
> >  	opal_wake_poller();
> > -	wait_event(opal_async_wait, test_bit(token, opal_async_complete_map));
> > -	memcpy(msg, &opal_async_responses[token], sizeof(*msg));
> > +	wait_event(opal_async_wait, opal_async_tokens[token].state
> > +			== ASYNC_TOKEN_COMPLETED);
> 
> Since wait_event is a macro, I'd recommend parenthesis around the second
> argument. I think there is also an inbuilt assumption that the barriers
> in schedule() called by wait_event() will make the write to the token
> state visible.

Yes good point.

> 
> > +	memcpy(msg, &opal_async_tokens[token].response, sizeof(*msg));
> >  
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(opal_async_wait_response);
> >  
> > +/* Called from interrupt context */
> >  static int opal_async_comp_event(struct notifier_block *nb,
> >  		unsigned long msg_type, void *msg)
> >  {
> > @@ -140,9 +154,9 @@ static int opal_async_comp_event(struct notifier_block *nb,
> >  		return 0;
> >  
> >  	token = be64_to_cpu(comp_msg->params[0]);
> > -	memcpy(&opal_async_responses[token], comp_msg, sizeof(*comp_msg));
> > +	memcpy(&opal_async_tokens[token].response, comp_msg, sizeof(*comp_msg));
> >  	spin_lock_irqsave(&opal_async_comp_lock, flags);
> > -	__set_bit(token, opal_async_complete_map);
> > +	opal_async_tokens[token].state = ASYNC_TOKEN_COMPLETED;
> >  	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
> >  
> >  	wake_up(&opal_async_wait);
> > @@ -178,24 +192,19 @@ int __init opal_async_comp_init(void)
> >  	}
> >  
> >  	opal_max_async_tokens = be32_to_cpup(async);
> > -	if (opal_max_async_tokens > N_ASYNC_COMPLETIONS)
> > -		opal_max_async_tokens = N_ASYNC_COMPLETIONS;
> > +	opal_async_tokens = kcalloc(opal_max_async_tokens,
> > +			sizeof(*opal_async_tokens), GFP_KERNEL);
> > +	if (!opal_async_tokens) {
> > +		err = -ENOMEM;
> > +		goto out_opal_node;
> > +	}
> >  
> >  	err = opal_message_notifier_register(OPAL_MSG_ASYNC_COMP,
> >  			&opal_async_comp_nb);
> >  	if (err) {
> >  		pr_err("%s: Can't register OPAL event notifier (%d)\n",
> >  				__func__, err);
> > -		goto out_opal_node;
> > -	}
> > -
> > -	opal_async_responses = kzalloc(
> > -			sizeof(*opal_async_responses) * opal_max_async_tokens,
> > -			GFP_KERNEL);
> > -	if (!opal_async_responses) {
> > -		pr_err("%s: Out of memory, failed to do asynchronous "
> > -				"completion init\n", __func__);
> > -		err = -ENOMEM;
> > +		kfree(opal_async_tokens);
> >  		goto out_opal_node;
> >  	}
> >  
> 
> Balbir Singh.
> 

Re: [PATCH v3 03/10] mtd: powernv_flash: Don't treat OPAL_SUCCESS as an error

[Cyril Bur]

On Mon, 2017-07-17 at 18:50 +1000, Balbir Singh wrote:
> On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> > While this driver expects to interact asynchronously, OPAL is well
> > within its rights to return OPAL_SUCCESS to indicate that the operation
> > completed without the need for a callback. We shouldn't treat
> > OPAL_SUCCESS as an error rather we should wrap up and return promptly to
> > the caller.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> > I'll note here that currently no OPAL exists that will return
> > OPAL_SUCCESS so there isn't the possibility of a bug today.
> 
> It would help if you mentioned OPAL_SUCCESS to the async call. So effectively
> what we expected to be an asynchronous call with callback, but OPAL returned
> immediately with success.
> 

Ah my favourite problems, commit message.

Thanks,

Cyril

> Balbir Singh.
> 

Re: [PATCH v3 02/10] mtd: powernv_flash: Lock around concurrent access to OPAL

[Cyril Bur]

On Mon, 2017-07-17 at 19:29 +1000, Balbir Singh wrote:
> On Mon, 2017-07-17 at 17:55 +1000, Cyril Bur wrote:
> > On Mon, 2017-07-17 at 17:34 +1000, Balbir Singh wrote:
> > > On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> > > > OPAL can only manage one flash access at a time and will return an
> > > > OPAL_BUSY error for each concurrent access to the flash. The simplest
> > > > way to prevent this from happening is with a mutex.
> > > > 
> > > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > > > ---
> > > 
> > > Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
> > > the mutex while other operations with the lock are busy?
> > > 
> > 
> > This is a good question. My best interpretation is that
> > _interruptible() should be used when you'll only be coming from a user
> > context. Which is mostly true for this driver, however, MTD does
> > provide kernel interfaces, so I was hesitant, there isn't a great deal
> > of use of _interruptible() in drivers/mtd. 
> > 
> > Thoughts?
> 
> What are the kernel interfaces (I have not read through mtd in detail)?
> I would still like to see us not blocked in mutex_lock() across threads
> for parallel calls, one option is to use mutex_trylock() and return if
> someone already holds the mutex with -EBUSY, but you'll need to evaluate
> what that means for every call.
> 

Yeah maybe mutex_trylock() is the way to go, thinking quickly, I don't
see how it could be a problem for userspace using powernv_flash. I'm
honestly not too sure about the depths of the mtd kernel interfaces but
I've seen a tonne of cool stuff you could do, hence my reluctance to go
with _interruptible()

Cyril
> Balbir Singh.
> 

Re: [PATCH v3 02/10] mtd: powernv_flash: Lock around concurrent access to OPAL

[Michael Ellerman]

Cyril Bur <cyrilbur@gmail.com> writes:

> On Mon, 2017-07-17 at 19:29 +1000, Balbir Singh wrote:
>> On Mon, 2017-07-17 at 17:55 +1000, Cyril Bur wrote:
>> > On Mon, 2017-07-17 at 17:34 +1000, Balbir Singh wrote:
>> > > On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
>> > > > OPAL can only manage one flash access at a time and will return an
>> > > > OPAL_BUSY error for each concurrent access to the flash. The simplest
>> > > > way to prevent this from happening is with a mutex.
>> > > > 
>> > > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
>> > > > ---
>> > > 
>> > > Should the mutex_lock() be mutex_lock_interruptible()? Are we OK waiting on
>> > > the mutex while other operations with the lock are busy?
>> > > 
>> > 
>> > This is a good question. My best interpretation is that
>> > _interruptible() should be used when you'll only be coming from a user
>> > context. Which is mostly true for this driver, however, MTD does
>> > provide kernel interfaces, so I was hesitant, there isn't a great deal
>> > of use of _interruptible() in drivers/mtd. 
>> > 
>> > Thoughts?
>> 
>> What are the kernel interfaces (I have not read through mtd in detail)?
>> I would still like to see us not blocked in mutex_lock() across threads
>> for parallel calls, one option is to use mutex_trylock() and return if
>> someone already holds the mutex with -EBUSY, but you'll need to evaluate
>> what that means for every call.
>
> Yeah maybe mutex_trylock() is the way to go, thinking quickly, I don't
> see how it could be a problem for userspace using powernv_flash. I'm
> honestly not too sure about the depths of the mtd kernel interfaces but
> I've seen a tonne of cool stuff you could do, hence my reluctance to go
> with _interruptible()

If you use trylock that means all your callers now need to handle EBUSY,
which I doubt they do. Which means it goes up to userspace, which most
users will just treat as a hard error.

So that sounds like a bad plan to me.

cheers

Re: [PATCH v3 06/10] powerpc/opal: Rework the opal-async interface

[Michael Ellerman]

Cyril Bur <cyrilbur@gmail.com> writes:
> On Mon, 2017-07-17 at 21:30 +1000, Balbir Singh wrote:
>> On Wed, 2017-07-12 at 14:23 +1000, Cyril Bur wrote:
>> >  static int __opal_async_get_token(void)
>> >  {
>> >  	unsigned long flags;
>> >  	int token;
>> >  
>> > -	spin_lock_irqsave(&opal_async_comp_lock, flags);
>> > -	token = find_first_bit(opal_async_complete_map, opal_max_async_tokens);
>> > -	if (token >= opal_max_async_tokens) {
>> > -		token = -EBUSY;
>> > -		goto out;
>> > -	}
>> > -
>> > -	if (__test_and_set_bit(token, opal_async_token_map)) {
>> > -		token = -EBUSY;
>> > -		goto out;
>> > +	for (token = 0; token < opal_max_async_tokens; token++) {
>> > +		spin_lock_irqsave(&opal_async_comp_lock, flags);
>> 
>> Why is the spin lock inside the for loop? If the last token is free, the
>> number of times we'll take and release a lock is extensive, why are we
>> doing it this way?
>
> Otherwise we might hold the lock for quite some time.

No we won't. A loop over a few 10s or 100s of tokens is not going to
take long, compared to the overhead of taking the lock every iteration
through the loop.

If the linear search with the lock held is a bottle neck, then you can
keep a hint variable which tracks the last freed token and start
searching from there.

cheers