| Message ID | 20171010033302.20854-11-cyrilbur@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow opal-async waiters to get interrupted | expand |
On Tue, 10 Oct 2017 14:33:02 +1100 Cyril Bur <cyrilbur@gmail.com> 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 <cyrilbur@gmail.com> Have some minor comments (see below). Once addressed you can add Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > 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. > * > * 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;
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 * * 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. + */ + 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; - } else { - rc = -EIO; - } + rc = opal_error_code(rc); out: opal_async_release_token(token); return rc;
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 | 57 +++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 22 deletions(-)