[PATCH 0/5] Allow opal-async waiters to get interrupted

[PATCH 0/5] Allow opal-async waiters to get interrupted

[Cyril Bur]

Hello,

This patchset aims to solve the problem 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 2/5.

Patch 3/5 attempts to provide a solution to the problem that the
powernv_flash MTD driver can use in patch 5/5.

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 (5):
  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()

 arch/powerpc/include/asm/opal.h             |   3 +-
 arch/powerpc/platforms/powernv/opal-async.c | 190 ++++++++++++++++++++--------
 arch/powerpc/platforms/powernv/opal.c       |   1 +
 drivers/mtd/devices/powernv_flash.c         |  18 ++-
 4 files changed, 151 insertions(+), 61 deletions(-)

-- 
2.13.2

[PATCH 1/5] 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 2/5] 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 | 99 ++++++++++++++++-------------
 1 file changed, 55 insertions(+), 44 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-async.c b/arch/powerpc/platforms/powernv/opal-async.c
index 1d56ac9da347..ed888686925b 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,48 @@
 #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;
+	for (token = 0; token < opal_max_async_tokens; token++) {
+		if (opal_async_tokens[token].state == ASYNC_TOKEN_FREE) {
+			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);
+		}
 	}
 
-	if (__test_and_set_bit(token, opal_async_token_map)) {
-		token = -EBUSY;
-		goto out;
-	}
-
-	__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 +84,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 +93,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 +112,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 +136,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 +156,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 +194,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 = kzalloc(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 3/5] 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             |  1 +
 arch/powerpc/platforms/powernv/opal-async.c | 87 +++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 5553ad2f3e53..3070a66b2c51 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -294,6 +294,7 @@ 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);
+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 ed888686925b..b028896da4da 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
 };
 
@@ -61,8 +63,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)
@@ -99,6 +103,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;
 	}
@@ -131,7 +145,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.
 	 */
@@ -144,11 +162,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;
 
@@ -156,11 +229,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 4/5] powerpc/powernv: Add OPAL_BUSY to opal_error_code()

[Cyril Bur]

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

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 59684b4af4d1..f87e000e7c28 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;
-- 
2.13.2

[PATCH 5/5] 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()
also 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 | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index 7b41af06f4fe..a7c5ae2b2898 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -88,19 +88,23 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 	}
 
 	if (rc != OPAL_ASYNC_COMPLETION) {
-		dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
-				op, rc);
+		if (rc != OPAL_BUSY)
+			dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
+					op, rc);
 		opal_async_release_token(token);
-		rc = -EIO;
+		rc = opal_error_code(rc);
 		goto out;
 	}
 
-	rc = opal_async_wait_response(token, &msg);
+	rc = opal_async_wait_response_interruptible(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;
+		if (rc == -ERESTARTSYS)
+			rc = -EINTR;
+		else
+			dev_err(dev, "opal async wait failed (rc %d)\n", rc);
+		return rc;
 	}
 
 	rc = opal_get_async_rc(msg);
@@ -109,7 +113,7 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		if (retlen)
 			*retlen = len;
 	} else {
-		rc = -EIO;
+		rc = opal_error_code(rc);
 	}
 
 	return rc;
-- 
2.13.2

Re: [PATCH 5/5] mtd: powernv_flash: Use opal_async_wait_response_interruptible()

[Brian Norris]

On Thu, Jun 29, 2017 at 04:54:13PM +1000, Cyril Bur wrote:
> 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()
> also 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 | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)

Seems OK:

Acked-by: Brian Norris <computersforpeace@gmail.com>

Re: [PATCH 5/5] mtd: powernv_flash: Use opal_async_wait_response_interruptible()

[Benjamin Herrenschmidt]

On Thu, 2017-06-29 at 16:54 +1000, Cyril Bur wrote:
> 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()
> also 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.

So what happens when you get a signal then ? The async op is still in
progress, will the callers properly handle that ?

For example, for a read, OPAL will potentially still be writing to
the destination buffer. Are the callers expecting this ?

That sounds very broken to me ....

You should simply limit the amount of flash that can be affected by
a single call to avoid long hung tasks.

> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  drivers/mtd/devices/powernv_flash.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
> index 7b41af06f4fe..a7c5ae2b2898 100644
> --- a/drivers/mtd/devices/powernv_flash.c
> +++ b/drivers/mtd/devices/powernv_flash.c
> @@ -88,19 +88,23 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
>  	}
>  
>  	if (rc != OPAL_ASYNC_COMPLETION) {
> -		dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
> -				op, rc);
> +		if (rc != OPAL_BUSY)
> +			dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
> +					op, rc);
>  		opal_async_release_token(token);
> -		rc = -EIO;
> +		rc = opal_error_code(rc);
>  		goto out;
>  	}
>  
> -	rc = opal_async_wait_response(token, &msg);
> +	rc = opal_async_wait_response_interruptible(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;
> +		if (rc == -ERESTARTSYS)
> +			rc = -EINTR;
> +		else
> +			dev_err(dev, "opal async wait failed (rc %d)\n", rc);
> +		return rc;
>  	}
>  
>  	rc = opal_get_async_rc(msg);
> @@ -109,7 +113,7 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
>  		if (retlen)
>  			*retlen = len;
>  	} else {
> -		rc = -EIO;
> +		rc = opal_error_code(rc);
>  	}
>  
>  	return rc;

Re: [PATCH 5/5] mtd: powernv_flash: Use opal_async_wait_response_interruptible()

[Cyril Bur]

On Sat, 2017-07-08 at 14:59 -0500, Benjamin Herrenschmidt wrote:
> On Thu, 2017-06-29 at 16:54 +1000, Cyril Bur wrote:
> > 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()
> > also 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.
> 
> So what happens when you get a signal then ? The async op is still in
> progress, will the callers properly handle that ?
> 
> For example, for a read, OPAL will potentially still be writing to
> the destination buffer. Are the callers expecting this ?
> 
> That sounds very broken to me ....
> 

Correct - I have a v2 that I let stew in my head over the weekend
before sending. I think its good.

Thanks,

Cyril

> You should simply limit the amount of flash that can be affected by
> a single call to avoid long hung tasks.
> 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  drivers/mtd/devices/powernv_flash.c | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
> > index 7b41af06f4fe..a7c5ae2b2898 100644
> > --- a/drivers/mtd/devices/powernv_flash.c
> > +++ b/drivers/mtd/devices/powernv_flash.c
> > @@ -88,19 +88,23 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
> >  	}
> >  
> >  	if (rc != OPAL_ASYNC_COMPLETION) {
> > -		dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
> > -				op, rc);
> > +		if (rc != OPAL_BUSY)
> > +			dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
> > +					op, rc);
> >  		opal_async_release_token(token);
> > -		rc = -EIO;
> > +		rc = opal_error_code(rc);
> >  		goto out;
> >  	}
> >  
> > -	rc = opal_async_wait_response(token, &msg);
> > +	rc = opal_async_wait_response_interruptible(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;
> > +		if (rc == -ERESTARTSYS)
> > +			rc = -EINTR;
> > +		else
> > +			dev_err(dev, "opal async wait failed (rc %d)\n", rc);
> > +		return rc;
> >  	}
> >  
> >  	rc = opal_get_async_rc(msg);
> > @@ -109,7 +113,7 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
> >  		if (retlen)
> >  			*retlen = len;
> >  	} else {
> > -		rc = -EIO;
> > +		rc = opal_error_code(rc);
> >  	}
> >  
> >  	return rc;