Re: [PATCH v4 10/10] mtd: powernv_flash: Use opal_async_wait_response_interruptible()
From: Boris Brezillon <hidden>
Date: 2017-10-30 09:14:43
On Tue, 10 Oct 2017 14:33:02 +1100 Cyril Bur [off-list ref] 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() 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 <redacted>
Have some minor comments (see below). Once addressed you can add Acked-by: Boris Brezillon <redacted>
quoted hunk ↗ jump to hunk
--- drivers/mtd/devices/powernv_flash.c | 57 +++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 22 deletions(-)diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c index 3343d4f5c4f3..42383dbca5a6 100644 --- a/drivers/mtd/devices/powernv_flash.c +++ b/drivers/mtd/devices/powernv_flash.c@@ -1,7 +1,7 @@ /* * OPAL PNOR flash MTD abstraction * - * Copyright IBM 2015 + * Copyright IBM 2015-2017
Not a big deal, but this copyright update is not really related to the change you describe in your commit message. Maybe this should be done in a separate commit.
quoted hunk ↗ jump to hunk
* * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by@@ -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 + * to it know we've been interrupted.
^ 'it to' ?
+ */
+ 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)
+ if (rc == OPAL_SUCCESS && retlen)
*retlen = len;There's one too many tabs here.
- } else {
- rc = -EIO;
- }
+ rc = opal_error_code(rc);
out:
opal_async_release_token(token);
return rc;