[PATCH v4] UBI: block: Continue creating ubiblocks after an initialization error

[PATCH v4] UBI: block: Continue creating ubiblocks after an initialization error

[Dan Ehrenberg]

If one ubi volume is corrupted but another is not, it should be
possible to initialize that ubiblock from a kernel commandline which
includes both of them. This patch changes the error handling behavior
in initializing ubiblock to ensure that all parameters are attempted
even if one fails. If there is a failure, it is logged on dmesg.
It also makes error messages more descriptive by including the
name of the UBI volume that failed.

Tested: Formatted ubi volume /dev/ubi5_0 in a corrupt way and
dev/ubi3_0 properly and included "ubi.block=5,0 ubi.block=3,0" on
the kernel command line. At boot, I see the following in the console:
[   21.082420] UBI error: ubiblock_create_from_param: block: can't open volume on ubi5_0, err=-19
[   21.084268] UBI: ubiblock3_0 created from ubi3:0(rootfs)

Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
---
Changes in v4:
- Removed Change-Id line
Changes in v3:
- Tried to fix the wraparound in sending the email
- Moved the 'changes' section to below the ---
Changes in v2:
- Added comments in the code explaning the intent of the error
  handling strategy.
- Fixed the code surrounding ubiblock initialization to not
  shut down the block devices after starting them up. In my
  test case, the initialization happened to not get called
  because the return value happened to be overwritten by the
  second successful case, but if the cases were reversed it
  would be removed. In the new code, ubiblock_create_from_param
  is simply a void function because no caller cares if one of
  the blocks failed to initialize.

 drivers/mtd/ubi/block.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 8876c7d..f9fd9048 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -584,22 +584,28 @@ open_volume_desc(const char *name, int ubi_num, int vol_id)
 		return ubi_open_volume(ubi_num, vol_id, UBI_READONLY);
 }
 
-static int __init ubiblock_create_from_param(void)
+static void __init ubiblock_create_from_param(void)
 {
-	int i, ret;
+	int i, ret = 0;
 	struct ubiblock_param *p;
 	struct ubi_volume_desc *desc;
 	struct ubi_volume_info vi;
 
+	/*
+	 * If there is an error creating one of the ubiblocks, continue on to
+	 * create the following ubiblocks. This helps in a circumstance where
+	 * the kernel command-line specifies multiple block devices and some
+	 * may be broken, but we still want the working ones to come up.
+	 */
 	for (i = 0; i < ubiblock_devs; i++) {
 		p = &ubiblock_param[i];
 
 		desc = open_volume_desc(p->name, p->ubi_num, p->vol_id);
 		if (IS_ERR(desc)) {
-			ubi_err("block: can't open volume, err=%ld\n",
-				PTR_ERR(desc));
-			ret = PTR_ERR(desc);
-			break;
+			ubi_err(
+				"block: can't open volume on ubi%d_%d, err=%ld",
+				p->ubi_num, p->vol_id, PTR_ERR(desc));
+			continue;
 		}
 
 		ubi_get_volume_info(desc, &vi);
@@ -607,12 +613,12 @@ static int __init ubiblock_create_from_param(void)
 
 		ret = ubiblock_create(&vi);
 		if (ret) {
-			ubi_err("block: can't add '%s' volume, err=%d\n",
-				vi.name, ret);
-			break;
+			ubi_err(
+				"block: can't add '%s' volume on ubi%d_%d, err=%d",
+				vi.name, p->ubi_num, p->vol_id, ret);
+			continue;
 		}
 	}
-	return ret;
 }
 
 static void ubiblock_remove_all(void)
@@ -640,10 +646,12 @@ int __init ubiblock_init(void)
 	if (ubiblock_major < 0)
 		return ubiblock_major;
 
-	/* Attach block devices from 'block=' module param */
-	ret = ubiblock_create_from_param();
-	if (ret)
-		goto err_remove;
+	/*
+	 * Attach block devices from 'block=' module param.
+	 * Even if one block device in the param list fails to come up,
+	 * still allow the module to load and leave any others up.
+	 */
+	ubiblock_create_from_param();
 
 	/*
 	 * Block devices are only created upon user requests, so we ignore
@@ -656,7 +664,6 @@ int __init ubiblock_init(void)
 
 err_unreg:
 	unregister_blkdev(ubiblock_major, "ubiblock");
-err_remove:
 	ubiblock_remove_all();
 	return ret;
 }
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v4] UBI: block: Continue creating ubiblocks after an initialization error

[Ezequiel Garcia]

Hey Dan,

On 12/18/2014 08:33 PM, Dan Ehrenberg wrote:
> If one ubi volume is corrupted but another is not, it should be
> possible to initialize that ubiblock from a kernel commandline which
> includes both of them. This patch changes the error handling behavior
> in initializing ubiblock to ensure that all parameters are attempted
> even if one fails. If there is a failure, it is logged on dmesg.
> It also makes error messages more descriptive by including the
> name of the UBI volume that failed.
> 
> Tested: Formatted ubi volume /dev/ubi5_0 in a corrupt way and
> dev/ubi3_0 properly and included "ubi.block=5,0 ubi.block=3,0" on
> the kernel command line. At boot, I see the following in the console:
> [   21.082420] UBI error: ubiblock_create_from_param: block: can't open volume on ubi5_0, err=-19
> [   21.084268] UBI: ubiblock3_0 created from ubi3:0(rootfs)
> 
> Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
> ---
> Changes in v4:

I think you should re-send on top of linux-next. Commit
326087033108e7806e24974f2c8271f95cddaf3a has changed the way
user messages are printed.

Other than that, the patch looks good:

Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks!
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v4] UBI: block: Continue creating ubiblocks after an initialization error

[Daniel Ehrenberg]

On Thu, Dec 18, 2014 at 3:44 PM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> I think you should re-send on top of linux-next. Commit
> 326087033108e7806e24974f2c8271f95cddaf3a has changed the way
> user messages are printed.
>
> Other than that, the patch looks good:
>
> Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Hi,

When you say linux-next, do you mean Linus's linux-next? I had based
my patch on linux-mtd.git's master branch (based on what I saw at
http://www.linux-mtd.infradead.org/source.html#kernelversions) but
apparently that doesn't include the patch you mention, whereas Linus
does have it. Is Linus's linux-next where to find the most up-to-date
version of the MTD code, or is there another repository that includes
bleeding-edge stuff?

Thanks,
Dan

Re: [PATCH v4] UBI: block: Continue creating ubiblocks after an initialization error

[Richard Weinberger]

On Fri, Dec 19, 2014 at 1:52 AM, Daniel Ehrenberg <dehrenberg@google.com> wrote:
> On Thu, Dec 18, 2014 at 3:44 PM, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
>> I think you should re-send on top of linux-next. Commit
>> 326087033108e7806e24974f2c8271f95cddaf3a has changed the way
>> user messages are printed.
>>
>> Other than that, the patch looks good:
>>
>> Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
> Hi,
>
> When you say linux-next, do you mean Linus's linux-next? I had based
> my patch on linux-mtd.git's master branch (based on what I saw at
> http://www.linux-mtd.infradead.org/source.html#kernelversions) but
> apparently that doesn't include the patch you mention, whereas Linus
> does have it. Is Linus's linux-next where to find the most up-to-date
> version of the MTD code, or is there another repository that includes
> bleeding-edge stuff?

You want the most current UBI stuff. :-)
git://git.infradead.org/linux-ubifs.git

-- 
Thanks,
//richard

Re: [PATCH v4] UBI: block: Continue creating ubiblocks after an initialization error

[Ezequiel Garcia]

On 12/19/2014 04:16 AM, Richard Weinberger wrote:
> On Fri, Dec 19, 2014 at 1:52 AM, Daniel Ehrenberg <dehrenberg@google.com> wrote:
>> On Thu, Dec 18, 2014 at 3:44 PM, Ezequiel Garcia
>> <ezequiel@vanguardiasur.com.ar> wrote:
>>> I think you should re-send on top of linux-next. Commit
>>> 326087033108e7806e24974f2c8271f95cddaf3a has changed the way
>>> user messages are printed.
>>>
>>> Other than that, the patch looks good:
>>>
>>> Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>
>> Hi,
>>
>> When you say linux-next, do you mean Linus's linux-next?

This is what I meant:
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

> I had based
>> my patch on linux-mtd.git's master branch (based on what I saw at
>> http://www.linux-mtd.infradead.org/source.html#kernelversions) but
>> apparently that doesn't include the patch you mention, whereas Linus
>> does have it. Is Linus's linux-next where to find the most up-to-date
>> version of the MTD code, or is there another repository that includes
>> bleeding-edge stuff?
> 
> You want the most current UBI stuff. :-)
> git://git.infradead.org/linux-ubifs.git
> 

Exactly. MTD and UBI/UBIFS are different subsystems with different trees
and maintainers.

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar