From 44e2aa2b16a872fa8aa4901b3793132e6cfd293a Mon Sep 17 00:00:00 2001 From: Cyril Bur Date: Fri, 3 Nov 2017 13:41:37 +1100 Subject: mtd: powernv_flash: Use WARN_ON_ONCE() rather than BUG_ON() BUG_ON() should be reserved in situations where we can not longer guarantee the integrity of the system. In the case where powernv_flash_async_op() receives an impossible op, we can still guarantee the integrity of the system. Signed-off-by: Cyril Bur Acked-by: Boris Brezillon Signed-off-by: Michael Ellerman --- drivers/mtd/devices/powernv_flash.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c index f5396f26ddb4..f9ec38281ff2 100644 --- a/drivers/mtd/devices/powernv_flash.c +++ b/drivers/mtd/devices/powernv_flash.c @@ -78,7 +78,9 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op, rc = opal_flash_erase(info->id, offset, len, token); break; default: - BUG_ON(1); + WARN_ON_ONCE(1); + opal_async_release_token(token); + return -EIO; } if (rc != OPAL_ASYNC_COMPLETION) { -- cgit v1.2.3 From 25ee52e66949b6e5f041aedff4db9a7d84a6fb2b Mon Sep 17 00:00:00 2001 From: Cyril Bur Date: Fri, 3 Nov 2017 13:41:38 +1100 Subject: mtd: powernv_flash: Don't treat OPAL_SUCCESS as an error 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 Acked-by: Boris Brezillon Signed-off-by: Michael Ellerman --- drivers/mtd/devices/powernv_flash.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c index f9ec38281ff2..ca3ca6adf71e 100644 --- a/drivers/mtd/devices/powernv_flash.c +++ b/drivers/mtd/devices/powernv_flash.c @@ -63,7 +63,6 @@ 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"); - return token; } @@ -83,21 +82,25 @@ 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) { dev_err(dev, "opal_flash_async_op(op=%d) failed (rc %d)\n", op, rc); - opal_async_release_token(token); - return -EIO; + rc = -EIO; + goto out; } rc = opal_async_wait_response(token, &msg); - opal_async_release_token(token); 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) @@ -106,6 +109,8 @@ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op, rc = -EIO; } +out: + opal_async_release_token(token); return rc; } -- cgit v1.2.3 From e32ec15a2d57977f9f69b222b7b0395a2d60a71c Mon Sep 17 00:00:00 2001 From: Cyril Bur Date: Fri, 3 Nov 2017 13:41:39 +1100 Subject: mtd: powernv_flash: Remove pointless goto in driver init powernv_flash_probe() has pointless goto statements which jump to the end of the function to simply return a variable. Rather than checking for error and going to the label, just return the error as soon as it is detected. Signed-off-by: Cyril Bur Acked-by: Boris Brezillon Signed-off-by: Michael Ellerman --- drivers/mtd/devices/powernv_flash.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c index ca3ca6adf71e..4dd3b5d2feb2 100644 --- a/drivers/mtd/devices/powernv_flash.c +++ b/drivers/mtd/devices/powernv_flash.c @@ -227,21 +227,20 @@ static int powernv_flash_probe(struct platform_device *pdev) int ret; data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); - if (!data) { - ret = -ENOMEM; - goto out; - } + if (!data) + return -ENOMEM; + data->mtd.priv = data; ret = of_property_read_u32(dev->of_node, "ibm,opal-id", &(data->id)); if (ret) { dev_err(dev, "no device property 'ibm,opal-id'\n"); - goto out; + return ret; } ret = powernv_flash_set_driver_info(dev, &data->mtd); if (ret) - goto out; + return ret; dev_set_drvdata(dev, data); @@ -250,10 +249,7 @@ static int powernv_flash_probe(struct platform_device *pdev) * with an ffs partition at the start, it should prove easier for users * to deal with partitions or not as they see fit */ - ret = mtd_device_register(&data->mtd, NULL, 0); - -out: - return ret; + return mtd_device_register(&data->mtd, NULL, 0); } /** -- cgit v1.2.3 From efe6941450b80927d3a5721594b7f103cb0c7428 Mon Sep 17 00:00:00 2001 From: Cyril Bur Date: Fri, 3 Nov 2017 13:41:40 +1100 Subject: mtd: powernv_flash: Don't return -ERESTARTSYS on interrupted token acquisition Because the MTD core might split up a read() or write() from userspace into several calls to the driver, we may fail to get a token but already have done some work, best to return -EINTR back to userspace and have them decide what to do. Signed-off-by: Cyril Bur Acked-by: Boris Brezillon Signed-off-by: Michael Ellerman --- drivers/mtd/devices/powernv_flash.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'drivers/mtd') diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c index 4dd3b5d2feb2..3343d4f5c4f3 100644 --- a/drivers/mtd/devices/powernv_flash.c +++ b/drivers/mtd/devices/powernv_flash.c @@ -47,6 +47,11 @@ enum flash_op { FLASH_OP_ERASE, }; +/* + * Don't return -ERESTARTSYS if we can't get a token, the MTD core + * might have split up the call from userspace and called into the + * driver more than once, we'll already have done some amount of work. + */ static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op, loff_t offset, size_t len, size_t *retlen, u_char *buf) { @@ -63,6 +68,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"); + else + token = -EINTR; return token; } -- cgit v1.2.3 From 6f469b67ff8a3ec2698ef1041229e111065013ce Mon Sep 17 00:00:00 2001 From: Cyril Bur Date: Fri, 3 Nov 2017 13:41:46 +1100 Subject: mtd: powernv_flash: Use opal_async_wait_response_interruptible() 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 Acked-by: Boris Brezillon Signed-off-by: Michael Ellerman --- drivers/mtd/devices/powernv_flash.c | 57 +++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 22 deletions(-) (limited to 'drivers/mtd') diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c index 3343d4f5c4f3..26f9feaa5d17 100644 --- a/drivers/mtd/devices/powernv_flash.c +++ b/drivers/mtd/devices/powernv_flash.c @@ -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 + * it to 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) - *retlen = len; - } else { - rc = -EIO; - } + if (rc == OPAL_SUCCESS && retlen) + *retlen = len; + rc = opal_error_code(rc); out: opal_async_release_token(token); return rc; -- cgit v1.2.3