On Tue, 10 Oct 2017 14:33:02 +1100
Cyril Bur <cyril...@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 <cyril...@gmail.com>
Have some minor comments (see below). Once addressed you can add
Acked-by: Boris Brezillon <boris.brezil...@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;
