[v3,03/10] mtd: powernv_flash: Don't treat OPAL_SUCCESS as an error

Message ID 20170712042304.19745-4-cyrilbur@gmail.com
State Superseded
Headers show

Commit Message

Cyril Bur July 12, 2017, 4:22 a.m.
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 <cyrilbur@gmail.com>
---
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 | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Balbir Singh July 17, 2017, 8:50 a.m. | #1
On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> 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 <cyrilbur@gmail.com>
> ---
> I'll note here that currently no OPAL exists that will return
> OPAL_SUCCESS so there isn't the possibility of a bug today.

It would help if you mentioned OPAL_SUCCESS to the async call. So effectively
what we expected to be an asynchronous call with callback, but OPAL returned
immediately with success.

Balbir Singh.
Cyril Bur July 18, 2017, 12:42 a.m. | #2
On Mon, 2017-07-17 at 18:50 +1000, Balbir Singh wrote:
> On Wed, 2017-07-12 at 14:22 +1000, Cyril Bur wrote:
> > 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 <cyrilbur@gmail.com>
> > ---
> > I'll note here that currently no OPAL exists that will return
> > OPAL_SUCCESS so there isn't the possibility of a bug today.
> 
> It would help if you mentioned OPAL_SUCCESS to the async call. So effectively
> what we expected to be an asynchronous call with callback, but OPAL returned
> immediately with success.
> 

Ah my favourite problems, commit message.

Thanks,

Cyril

> Balbir Singh.
>

Patch

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index 7b41af06f4fe..d50b5f200f73 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -66,9 +66,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");
-
-		rc = token;
-		goto out;
+		mutex_unlock(&info->lock);
+		return token;
 	}
 
 	switch (op) {
@@ -87,23 +86,25 @@  static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		goto out;
 	}
 
+	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);
 		rc = -EIO;
 		goto out;
 	}
 
 	rc = opal_async_wait_response(token, &msg);
-	opal_async_release_token(token);
-	mutex_unlock(&info->lock);
 	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)
@@ -112,8 +113,8 @@  static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 		rc = -EIO;
 	}
 
-	return rc;
 out:
+	opal_async_release_token(token);
 	mutex_unlock(&info->lock);
 	return rc;
 }