Allow opal-async waiters to get interrupted

PATCH In review Cyril Bur 3 participants 14 messages Nov 3, 2017 – Nov 7, 2017

Subsystems (2)

PowerPC Memory technology devices (MTD)

lore

Share & tools

lore

Cyril Bur

author

2017-11-03 02:42 UTC #0 lore raw

v5 cover Allow opal-async waiters to get interrupted

V5: Address review from Boris Brezillon, thanks! Minor cleanups and descriptions - no functional changes.

V4: Rework and rethink.

To recap:
Userspace MTD read()s/write()s and erases to powernv_flash become calls into the OPAL firmware which subsequently handles flash access. Because the read()s, write()s or erases can be large (bounded of course my the size of flash) OPAL may take some time to service the request, this causes the powernv_flash driver to sit in a wait_event() for potentially minutes. This causes two problems, firstly, tools appear to hang for the entire time as they cannot be interrupted by signals and secondly, this can trigger hung task warnings. The correct solution is to use wait_event_interruptible() which my rework (as part of this series) of the opal-async infrastructure provides.

The final patch in this series achieves this. It should eliminate both hung tasks and threads locking up.

Included in this series are other simpler fixes for powernv_flash:

Don't always return EIO on error. OPAL does mutual exclusion on the flash and also knows when the service processor takes control of the flash, in both of these cases it will return OPAL_BUSY, translating this to EIO is misleading to userspace.

Handle receiving OPAL_SUCCESS when it expects OPAL_ASYNC_COMPLETION and don't treat it as an error. Unfortunately there are too many drivers out there with the incorrect behaviour so this means OPAL can never return anything but OPAL_ASYNC_COMPLETION, this shouldn't prevent the code from being correct.

Don't return ERESTARTSYS if token acquisition is interrupted as powernv_flash can't be sure it hasn't already performed some work, let userspace deal with the problem.

Change the incorrect use of BUG_ON() to WARN_ON() in powernv_flash.

Not for powernv_flash, a fix from Stewart Smith which fits into this series as it relies on my improvements to the opal-async infrastructure.

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: Don't treat OPAL_SUCCESS as an error
mtd: powernv_flash: Remove pointless goto in driver init
mtd: powernv_flash: Don't return -ERESTARTSYS on interrupted token
acquisition
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  | 183 +++++++++++++++++++--------
 arch/powerpc/platforms/powernv/opal-sensor.c |  17 +--
 arch/powerpc/platforms/powernv/opal.c        |   2 +
 drivers/mtd/devices/powernv_flash.c          |  83 +++++++-----
 5 files changed, 194 insertions(+), 95 deletions(-)

Signature · 1 line

2.15.0

Cyril Bur

author

2017-11-03 02:42 UTC #1 lore raw

PATCH v5 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]>

Acked-by Boris Brezillon <[email protected]>

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

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

index f5396f26ddb4..f9ec38281ff2 100644
@@ -78,7 +78,9 @@ 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);
+		opal_async_release_token(token);
+		return -EIO;
 	}
 
 	if (rc != OPAL_ASYNC_COMPLETION) {

Signature · 1 line

2.15.0

Michael Ellerman

2017-11-07 23:30 UTC #2 lore raw

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

Quoted 7 lines · 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 <[email protected]>
Acked-by Boris Brezillon <[email protected]>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/44e2aa2b16a872fa8aa4901b379313

cheers

Cyril Bur

author

2017-11-03 02:42 UTC #3 lore raw

PATCH v5 2/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]>

Acked-by Boris Brezillon <[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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

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

index f9ec38281ff2..ca3ca6adf71e 100644
@@ -63,7 +63,6 @@ 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");
-
 		return token;
 	}

@@ -83,21 +82,25 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		return -EIO;
 	}
 
+	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);
-		return -EIO;
+		rc = -EIO;
+		goto out;
 	}
 
 	rc = opal_async_wait_response(token, &msg);
-	opal_async_release_token(token);
 	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)

@@ -106,6 +109,8 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		rc = -EIO;
 	}
 
+out:
+	opal_async_release_token(token);
 	return rc;
 }

Signature · 1 line

2.15.0

Cyril Bur

author

2017-11-03 02:42 UTC #4 lore raw

PATCH v5 3/10 mtd: powernv_flash: Remove pointless goto in driver init

In reply to Cyril Bur

powernv_flash_probe() has pointless goto statements which jump to the end of the function to simply return a variable. Rather than checking for error and going to the label, just return the error as soon as it is detected.

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

Acked-by Boris Brezillon <[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 ca3ca6adf71e..4dd3b5d2feb2 100644
@@ -227,21 +227,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;
 
 	dev_set_drvdata(dev, data);

@@ -250,10 +249,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.15.0

Cyril Bur

author

2017-11-03 02:42 UTC #5 lore raw

PATCH v5 4/10 mtd: powernv_flash: Don't return -ERESTARTSYS on interrupted token acquisition

In reply to Cyril Bur

Because the MTD core might split up a read() or write() from userspace into several calls to the driver, we may fail to get a token but already have done some work, best to return -EINTR back to userspace and have them decide what to do.

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

Acked-by Boris Brezillon <[email protected]>

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

drivers/mtd/devices/powernv_flash.c +7 -0

index 4dd3b5d2feb2..3343d4f5c4f3 100644
@@ -47,6 +47,11 @@ enum flash_op {
 	FLASH_OP_ERASE,
 };
 
+/*
+ * Don't return -ERESTARTSYS if we can't get a token, the MTD core
+ * might have split up the call from userspace and called into the
+ * driver more than once, we'll already have done some amount of work.
+ */
 static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		loff_t offset, size_t len, size_t *retlen, u_char *buf)
 {

@@ -63,6 +68,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");
+		else
+			token = -EINTR;
 		return token;
 	}

Signature · 1 line

2.15.0

Cyril Bur

author

2017-11-03 02:42 UTC #6 lore raw

PATCH v5 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 726c23304a57..0078eb5acf98 100644
@@ -304,9 +304,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 cf33769a7b72..c43421ab2d2f 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.15.0

Cyril Bur

author

2017-11-03 02:42 UTC #7 lore raw

PATCH v5 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 | 92 ++++++++++++++++-------------
 1 file changed, 50 insertions(+), 42 deletions(-)

arch/powerpc/platforms/powernv/opal-async.c +50 ~~-42~~

index c43421ab2d2f..fbae8a37ce2c 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,45 @@
 #include <asm/machdep.h>
 #include <asm/opal.h>
 
-#define N_ASYNC_COMPLETIONS	64
+enum opal_async_token_state {
+	ASYNC_TOKEN_UNALLOCATED = 0,
+	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;
+	int token = -EBUSY;
 
 	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_UNALLOCATED) {
+			opal_async_tokens[token].state = ASYNC_TOKEN_ALLOCATED;
+			goto out;
+		}
 	}
-
-	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;
 }
 
+/*
+ * 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 +81,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 +90,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_UNALLOCATED;
+		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 +109,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 +133,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 +153,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 +191,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.15.0

Michael Ellerman

reviewer

2017-11-06 09:41 UTC #8 lore raw

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

Quoted 48 lines · Cyril Bur

arch/powerpc/platforms/powernv/opal-async.c +17 ~~-11~~

index c43421ab2d2f..fbae8a37ce2c 100644
@@ -23,40 +23,45 @@
 #include <asm/machdep.h>
 #include <asm/opal.h>
 
-#define N_ASYNC_COMPLETIONS	64
+enum opal_async_token_state {
+	ASYNC_TOKEN_UNALLOCATED = 0,
+	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;
+	int token = -EBUSY;
 
 	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_UNALLOCATED) {
+			opal_async_tokens[token].state = ASYNC_TOKEN_ALLOCATED;
+			goto out;
+		}
 	}
-
-	if (__test_and_set_bit(token, opal_async_token_map)) {

arch/powerpc/platforms/powernv/opal-async.c line 58

Quoted 10 lines

arch/powerpc/platforms/powernv/opal-async.c +0 -6

-		token = -EBUSY;
-		goto out;
-	}
-
-	__clear_bit(token, opal_async_complete_map);
-
 out:
 	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
 	return token;
 }

Resulting in:

static int __opal_async_get_token(void)
{
unsigned long flags;
+ int token = -EBUSY;

spin_lock_irqsave(&opal_async_comp_lock, flags);
+ for (token = 0; token < opal_max_async_tokens; token++) {
+ if (opal_async_tokens[token].state == ASYNC_TOKEN_UNALLOCATED) {
+ opal_async_tokens[token].state = ASYNC_TOKEN_ALLOCATED;
+ goto out;
+ }
}
out:
spin_unlock_irqrestore(&opal_async_comp_lock, flags); return token;
}

So when no unallocated token is found we return opal_max_async_tokens :(

I changed it to:

static int __opal_async_get_token(void)

{
	unsigned long flags;
	int i, token = -EBUSY;

	spin_lock_irqsave(&opal_async_comp_lock, flags);

	for (i = 0; i < opal_max_async_tokens; i++) {
		if (opal_async_tokens[i].state == ASYNC_TOKEN_UNALLOCATED) {
			opal_async_tokens[i].state = ASYNC_TOKEN_ALLOCATED;
			token = i;
			break;
		}
	}

	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
	return token;
}

arch/powerpc/platforms/powernv/opal-async.c line 62

Quoted 4 lines

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

+/*
+ * Note: If the returned token is used in an opal call and opal returns
+ * OPAL_ASYNC_COMPLETION you MUST opal_async_wait_response() before

                                     ^
                                     call

cheers

Cyril Bur

author

2017-11-06 23:38 UTC #9 lore raw

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

Quoted 94 lines · Michael Ellerman

Quoted 48 lines · Cyril Bur

arch/powerpc/platforms/powernv/opal-async.c +17 ~~-11~~

index c43421ab2d2f..fbae8a37ce2c 100644
@@ -23,40 +23,45 @@
 #include <asm/machdep.h>
 #include <asm/opal.h>
 
-#define N_ASYNC_COMPLETIONS	64
+enum opal_async_token_state {
+	ASYNC_TOKEN_UNALLOCATED = 0,
+	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;
+	int token = -EBUSY;
 
 	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_UNALLOCATED) {
+			opal_async_tokens[token].state = ASYNC_TOKEN_ALLOCATED;
+			goto out;
+		}
 	}
-
-	if (__test_and_set_bit(token, opal_async_token_map)) {

arch/powerpc/platforms/powernv/opal-async.c line 58

Quoted 10 lines

arch/powerpc/platforms/powernv/opal-async.c +0 -6

-		token = -EBUSY;
-		goto out;
-	}
-
-	__clear_bit(token, opal_async_complete_map);
-
 out:
 	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
 	return token;
 }

Resulting in:

static int __opal_async_get_token(void)
{

arch/powerpc/platforms/powernv/opal-async.c +6 -0

 	unsigned long flags;
+	int token = -EBUSY;
 
 	spin_lock_irqsave(&opal_async_comp_lock, flags);
+	for (token = 0; token < opal_max_async_tokens; token++) {
+		if (opal_async_tokens[token].state == ASYNC_TOKEN_UNALLOCATED) {
+			opal_async_tokens[token].state = ASYNC_TOKEN_ALLOCATED;
+			goto out;
+		}
 	}
 out:
 	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
 	return token;
 }

So when no unallocated token is found we return opal_max_async_tokens :(

I changed it to:

static int __opal_async_get_token(void)

{
	unsigned long flags;
	int i, token = -EBUSY;

	spin_lock_irqsave(&opal_async_comp_lock, flags);

	for (i = 0; i < opal_max_async_tokens; i++) {
		if (opal_async_tokens[i].state == ASYNC_TOKEN_UNALLOCATED) {
			opal_async_tokens[i].state = ASYNC_TOKEN_ALLOCATED;

Quoted 10 lines

			token = i;
			break;
		}
	}

	spin_unlock_irqrestore(&opal_async_comp_lock, flags);
	return token;
}

Thanks!!

Quoted 10 lines

arch/powerpc/platforms/powernv/opal-async.c line 62

Quoted 4 lines

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

+/*
+ * Note: If the returned token is used in an opal call and opal returns
+ * OPAL_ASYNC_COMPLETION you MUST opal_async_wait_response() before

                                     ^
                                     call

cheers

Cyril Bur

author

2017-11-03 02:42 UTC #10 lore raw

PATCH v5 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.15.0

Cyril Bur

author

2017-11-03 02:42 UTC #11 lore raw

PATCH v5 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 0078eb5acf98..f95ca4560bfa 100644
@@ -307,6 +307,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.15.0

Cyril Bur

author

2017-11-03 02:42 UTC #12 lore raw

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

In reply to Cyril Bur

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

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

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

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

index 65c79ecf5a4d..041ddbd1fc57 100644
@@ -998,6 +998,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;

@@ -1037,3 +1038,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);

Signature · 1 line

2.15.0

Cyril Bur

author

2017-11-03 02:42 UTC #13 lore raw

PATCH v5 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]>

Acked-by Boris Brezillon <[email protected]>

---
 drivers/mtd/devices/powernv_flash.c | 57 +++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 22 deletions(-)

drivers/mtd/devices/powernv_flash.c +35 ~~-22~~

index 3343d4f5c4f3..26f9feaa5d17 100644
@@ -89,33 +89,46 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		return -EIO;
 	}
 
-	if (rc == OPAL_SUCCESS)
-		goto out_success;
+	if (rc == OPAL_ASYNC_COMPLETION) {
+		rc = opal_async_wait_response_interruptible(token, &msg);
+		if (rc) {
+			/*
+			 * If we 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.
+			 * 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 need
+			 * it to know we've been interrupted.
+			 */
+			rc = -EINTR;
+			if (opal_async_wait_response(token, &msg))
+				dev_err(dev, "opal_async_wait_response() failed\n");
+			goto out;
+		}
+		rc = opal_get_async_rc(msg);
+	}
 
-	if (rc != OPAL_ASYNC_COMPLETION) {
+	/*
+	 * OPAL does mutual exclusion on the flash, it will return
+	 * OPAL_BUSY.
+	 * During firmware updates by the service processor OPAL may
+	 * be (temporarily) prevented from accessing the flash, in
+	 * this case OPAL will also return OPAL_BUSY.
+	 * Both cases aren't errors exactly but the flash could have
+	 * changed, userspace should be informed.
+	 */
+	if (rc != OPAL_SUCCESS && rc != OPAL_BUSY)
 		dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n",
 				op, rc);
-		rc = -EIO;
-		goto out;
-	}
 
-	rc = opal_async_wait_response(token, &msg);
-	if (rc) {
-		dev_err(dev, "opal async wait failed (rc %d)\n", rc);
-		rc = -EIO;
-		goto out;
-	}
-
-	rc = opal_get_async_rc(msg);
-out_success:
-	if (rc == OPAL_SUCCESS) {
-		rc = 0;
-		if (retlen)
-			*retlen = len;
-	} else {
-		rc = -EIO;
-	}
+	if (rc == OPAL_SUCCESS && retlen)
+		*retlen = len;
 
+	rc = opal_error_code(rc);
 out:
 	opal_async_release_token(token);
 	return rc;

Signature · 1 line

2.15.0