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

Message ID 20170710013106.27276-11-cyrilbur@gmail.com
State Superseded
Headers show

Commit Message

Cyril Bur July 10, 2017, 1:31 a.m.
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(-)


kbuild test robot July 11, 2017, 4:19 a.m. | #1
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
        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


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;
-		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;