Allow opal-async waiters to get interrupted

PATCH Superseded Cyril Bur 3 participants 14 messages Jul 10, 2017 – Jul 11, 2017

Subsystems (2)

PowerPC Memory technology devices (MTD)

lore

Share & tools

lore

Cyril Bur

author

2017-07-10 01:31 UTC #0 lore raw

v2 cover Allow opal-async waiters to get interrupted

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        |   1 +
 drivers/mtd/devices/powernv_flash.c          |  66 +++++++---
 5 files changed, 190 insertions(+), 86 deletions(-)

Signature · 1 line

2.13.2

Cyril Bur

author

2017-07-10 01:31 UTC #1 lore raw

PATCH v2 1/10 mtd: powernv_flash: Use WARN_ON_ONCE() rather than BUG_ON()

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 <[email protected]>

---
 drivers/mtd/devices/powernv_flash.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/mtd/devices/powernv_flash.c +2 -1

index f5396f26ddb4..a9a20c00687c 100644
@@ -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) {

Signature · 1 line

2.13.2

Cyril Bur

author

2017-07-10 01:31 UTC #2 lore raw

PATCH v2 2/10 mtd: powernv_flash: Lock around concurrent access to OPAL

In reply to 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 <[email protected]>

---
 drivers/mtd/devices/powernv_flash.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

drivers/mtd/devices/powernv_flash.c +15 -3

index a9a20c00687c..7b41af06f4fe 100644
@@ -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);
 
 	/*

Signature · 1 line

2.13.2

Cyril Bur

author

2017-07-10 01:31 UTC #3 lore raw

PATCH v2 3/10 mtd: powernv_flash: Don't treat OPAL_SUCCESS as an error

In reply to 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 <[email protected]>

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

drivers/mtd/devices/powernv_flash.c +9 -8

index 7b41af06f4fe..d50b5f200f73 100644
@@ -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;
 }

Signature · 1 line

2.13.2

Cyril Bur

author

2017-07-10 01:31 UTC #4 lore raw

PATCH v2 4/10 mtd: powernv_flash: Remove pointless goto in driver init

In reply to Cyril Bur

Signed-off-by Cyril Bur <[email protected]>

---
 drivers/mtd/devices/powernv_flash.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

drivers/mtd/devices/powernv_flash.c +6 ~~-10~~

index d50b5f200f73..d7243b72ba6e 100644
@@ -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);
 }
 
 /**

Signature · 1 line

2.13.2

Cyril Bur

author

2017-07-10 01:31 UTC #5 lore raw

PATCH v2 5/10 powerpc/opal: Make __opal_async_{get, release}_token() static

In reply to 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 <[email protected]>

---
 arch/powerpc/include/asm/opal.h             |  2 --
 arch/powerpc/platforms/powernv/opal-async.c | 10 +++-------
 2 files changed, 3 insertions(+), 9 deletions(-)

arch/powerpc/include/asm/opal.h +0 -2

index 588fb1c23af9..5553ad2f3e53 100644
@@ -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);

arch/powerpc/platforms/powernv/opal-async.c +3 -7

index 83bebeec0fea..1d56ac9da347 100644
@@ -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);

Signature · 1 line

2.13.2

Cyril Bur

author

2017-07-10 01:31 UTC #6 lore raw

PATCH v2 6/10 powerpc/opal: Rework the opal-async interface

In reply to 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 <[email protected]>

---
 arch/powerpc/platforms/powernv/opal-async.c | 97 ++++++++++++++++-------------
 1 file changed, 53 insertions(+), 44 deletions(-)

arch/powerpc/platforms/powernv/opal-async.c +53 ~~-44~~

index 1d56ac9da347..d692372a0363 100644
@@ -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;
 	}

Signature · 1 line

2.13.2

Cyril Bur

author

2017-07-10 01:31 UTC #7 lore raw

PATCH v2 7/10 powernv/opal-sensor: remove not needed lock

In reply to Cyril Bur

From: Stewart Smith <[email protected]>

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 <[email protected]>

Signed-off-by Stewart Smith <[email protected]>

Signed-off-by Cyril Bur <[email protected]>

---
 arch/powerpc/platforms/powernv/opal-sensor.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

arch/powerpc/platforms/powernv/opal-sensor.c +4 ~~-13~~

index aa267f120033..0a7074bb91dc 100644
@@ -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);

Signature · 1 line

2.13.2

Cyril Bur

author

2017-07-10 01:31 UTC #8 lore raw

PATCH v2 8/10 powerpc/opal: Add opal_async_wait_response_interruptible() to opal-async

In reply to 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 <[email protected]>

---
 arch/powerpc/include/asm/opal.h             |  2 +
 arch/powerpc/platforms/powernv/opal-async.c | 87 +++++++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 4 deletions(-)

arch/powerpc/include/asm/opal.h +2 -0

index 5553ad2f3e53..6e9e53d744f3 100644
@@ -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;

arch/powerpc/platforms/powernv/opal-async.c +83 -4

132 lines not on this page · raw message

Signature · 1 line

2.13.2

David Laight

2017-07-10 14:05 UTC #9 lore raw

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

From: Cyril Bur

Quoted 11 lines

Sent: 10 July 2017 02:31
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.
=20
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.

ISTM that if you are doing (something like) a flash full device erase (that really can take minutes) it isn't actually an interruptible operation - the flash chip will still be busy.
So allowing the user process be interrupted just leaves a big mess.

OTOH the 'hung task' warning isn't the only problem with uninterruptible sleeps - the processes also count towards the 'load average'. Some software believes the 'load average' is a meaningful value.

It would be more generally useful for tasks to be able to sleep uninterruptibly without counting towards the 'load average' or triggering the 'task stuck' warning.
(I've code somewhere that sleeps interruptibly unless there is a signal pending when it sleeps uninterruptibly.)

WRT flash erases, 'whole device' erases aren't significantly quicker than sector by sector erases.
The latter can be interrupted between sectors.
I'm not sure you'd want to do writes than lock down enough kernel memory to take even a second to complete.

David

Cyril Bur

author

2017-07-11 00:48 UTC #10 lore raw

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

Quoted 13 lines · David Laight

From: Cyril Bur
Quoted 11 lines
Sent: 10 July 2017 02:31
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.

Quoted 5 lines

ISTM that if you are doing (something like) a flash full device erase (that really can take minutes) it isn't actually an interruptible operation - the flash chip will still be busy.
So allowing the user process be interrupted just leaves a big mess.

Agreed.

Quoted 4 lines

OTOH the 'hung task' warning isn't the only problem with uninterruptible sleeps - the processes also count towards the 'load average'. Some software believes the 'load average' is a meaningful value.

Yes, and because the read write and erase ops are actually calls into firmware which in some cases completely emulates flash we can mitigate the mess of allowing the process to be interrupted that you mention above.

Quoted 6 lines

It would be more generally useful for tasks to be able to sleep uninterruptibly without counting towards the 'load average' or triggering the 'task stuck' warning.
(I've code somewhere that sleeps interruptibly unless there is a signal pending when it sleeps uninterruptibly.)

I'm not sure what you mean here, if I understand correctly, this is what I'm doing. In the patch to the powernv_flash driver which uses opal_async_wait_response_interruptible() I essentially do the interruptible and if it breaks early the driver determines if it is safe to return to userspace otherwise it sleeps uninterruptibly (hopefully not for long). I was hesitant to put that logic here and prefered to leave it up to the caller to make the decision as to what to do.

Quoted 2 lines

WRT flash erases, 'whole device' erases aren't significantly quicker than sector by sector erases.

Yes, I'm rewriting some of the userspace tools we have to do everything sector by sector. MTD interfaces allow big reads/writes and erases so we should still handle it as best we can.

Quoted 4 lines

The latter can be interrupted between sectors.
I'm not sure you'd want to do writes than lock down enough kernel memory to take even a second to complete.

The MTD core actually chunks them up before passing them to the to backing driver so I don't think holding too much memory is a problem. Because the time spent in OPAL servicing flash calls is hard to determine and might not even be that related to the size of the operation, even smaller ops might still spend 'time' in OPAL.

Cyril

Quoted 2 lines

David

Cyril Bur

author

2017-07-10 01:31 UTC #11 lore raw

PATCH v2 9/10 powerpc/powernv: Add OPAL_BUSY to opal_error_code()

In reply to Cyril Bur

Signed-off-by Cyril Bur <[email protected]>

---
 arch/powerpc/platforms/powernv/opal.c | 1 +
 1 file changed, 1 insertion(+)

arch/powerpc/platforms/powernv/opal.c +1 -0

index 59684b4af4d1..f87e000e7c28 100644
@@ -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;

Signature · 1 line

2.13.2

Cyril Bur

author

2017-07-10 01:31 UTC #12 lore raw

PATCH v2 10/10 mtd: powernv_flash: Use opal_async_wait_response_interruptible()

In reply to 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 <[email protected]>

---
 drivers/mtd/devices/powernv_flash.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

drivers/mtd/devices/powernv_flash.c +23 -5

index d7243b72ba6e..cfa274ba7e40 100644
@@ -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;
 	}

Signature · 1 line

2.13.2

kbuild test robot

build bot

2017-07-11 04:20 UTC #13 lore raw

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

reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross…

Hi Cyril,

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.12 next-20170710]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Cyril-Bur/Allow-opal-async-waiters-to-get-interrupted/20170710-095504
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allmodconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705

reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc

All errors (new ones prefixed by >>):

>> ERROR: ".opal_error_code" [drivers/mtd/devices/powernv_flash.ko] undefined!

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation