[PATCH 1/2] ubi: move constants for ubi vol parsing from kernel param to ubi.h

[PATCH 1/2] ubi: move constants for ubi vol parsing from kernel param to ubi.h

[Quentin Schulz]

The constants used to parse ubi.block parameters from the kernel command
line are not specific to UBI block devices but rather to UBI volumes
(on which block devices are "based").

So let's rename UBIBLOCK_* constants used in parameter parsing to
UBIVOL_*.

Let's move the constants to ubi.h so that they can be used to parse
other parameters impacting UBI volumes.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/mtd/ubi/block.c | 33 +++++++++------------------------
 drivers/mtd/ubi/ubi.h   | 15 +++++++++++++++
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index d0b63bb..836272c 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -54,21 +54,6 @@
 #include "ubi-media.h"
 #include "ubi.h"
 
-/* Maximum number of supported devices */
-#define UBIBLOCK_MAX_DEVICES 32
-
-/* Maximum length of the 'block=' parameter */
-#define UBIBLOCK_PARAM_LEN 63
-
-/* Maximum number of comma-separated items in the 'block=' parameter */
-#define UBIBLOCK_PARAM_COUNT 2
-
-struct ubiblock_param {
-	int ubi_num;
-	int vol_id;
-	char name[UBIBLOCK_PARAM_LEN+1];
-};
-
 struct ubiblock_pdu {
 	struct work_struct work;
 	struct ubi_sgl usgl;
@@ -78,7 +63,7 @@ struct ubiblock_pdu {
 static int ubiblock_devs __initdata;
 
 /* MTD devices specification parameters */
-static struct ubiblock_param ubiblock_param[UBIBLOCK_MAX_DEVICES] __initdata;
+static struct ubivol_param ubiblock_param[UBIVOL_MAX_DEVICES] __initdata;
 
 struct ubiblock {
 	struct ubi_volume_desc *desc;
@@ -109,23 +94,23 @@ static int __init ubiblock_set_param(const char *val,
 {
 	int i, ret;
 	size_t len;
-	struct ubiblock_param *param;
-	char buf[UBIBLOCK_PARAM_LEN];
+	struct ubivol_param *param;
+	char buf[UBIVOL_PARAM_LEN];
 	char *pbuf = &buf[0];
-	char *tokens[UBIBLOCK_PARAM_COUNT];
+	char *tokens[UBIVOL_PARAM_COUNT];
 
 	if (!val)
 		return -EINVAL;
 
-	len = strnlen(val, UBIBLOCK_PARAM_LEN);
+	len = strnlen(val, UBIVOL_PARAM_LEN);
 	if (len == 0) {
 		pr_warn("UBI: block: empty 'block=' parameter - ignored\n");
 		return 0;
 	}
 
-	if (len == UBIBLOCK_PARAM_LEN) {
+	if (len == UBIVOL_PARAM_LEN) {
 		pr_err("UBI: block: parameter \"%s\" is too long, max. is %d\n",
-		       val, UBIBLOCK_PARAM_LEN);
+		       val, UBIVOL_PARAM_LEN);
 		return -EINVAL;
 	}
 
@@ -135,7 +120,7 @@ static int __init ubiblock_set_param(const char *val,
 	if (buf[len - 1] == '\n')
 		buf[len - 1] = '\0';
 
-	for (i = 0; i < UBIBLOCK_PARAM_COUNT; i++)
+	for (i = 0; i < UBIVOL_PARAM_COUNT; i++)
 		tokens[i] = strsep(&pbuf, ",");
 
 	param = &ubiblock_param[ubiblock_devs];
@@ -599,7 +584,7 @@ open_volume_desc(const char *name, int ubi_num, int vol_id)
 static void __init ubiblock_create_from_param(void)
 {
 	int i, ret = 0;
-	struct ubiblock_param *p;
+	struct ubivol_param *p;
 	struct ubi_volume_desc *desc;
 	struct ubi_volume_info vi;
 
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 5fe6265..4cc6ec9 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -810,6 +810,21 @@ struct ubi_work {
 	int anchor;
 };
 
+/* Maximum number of supported devices */
+#define UBIVOL_MAX_DEVICES 32
+
+/* Maximum length of the vol parameter */
+#define UBIVOL_PARAM_LEN 63
+
+/* Maximum number of comma-separated items in a vol parameter */
+#define UBIVOL_PARAM_COUNT 2
+
+struct ubivol_param {
+	int ubi_num;
+	int vol_id;
+	char name[UBIVOL_PARAM_LEN + 1];
+};
+
 #include "debug.h"
 
 extern struct kmem_cache *ubi_wl_entry_slab;

base-commit: 559d97996f1b75731195d813158eee7cc8a1f316
-- 
git-series 0.9.1

[PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Quentin Schulz]

There's already ECC on NAND pages so there may be no need for one to
check the CRC of a UBI volume.

Let's introduce a ubi.nocheck parameter that let one skip the CRC check
when attaching a UBI volume.

This also drastically speeds kernel boot by removing a potentially
useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board
for a ~20MB UBI volume used as rootfs.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/mtd/ubi/kapi.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/mtd/ubi/ubi.h  |  6 ++++-
 drivers/mtd/ubi/vtbl.c | 24 ++++++++++++++-
 3 files changed, 99 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
index d4b2e87..d604cd5 100644
--- a/drivers/mtd/ubi/kapi.c
+++ b/drivers/mtd/ubi/kapi.c
@@ -28,6 +28,9 @@
 #include <asm/div64.h>
 #include "ubi.h"
 
+struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
+int ubinocheck_devs;
+
 /**
  * ubi_do_get_device_info - get information about UBI device.
  * @ubi: UBI device description object
@@ -865,3 +868,70 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb)
 	return blocking_notifier_chain_unregister(&ubi_notifiers, nb);
 }
 EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier);
+
+static int __init ubinocheckvol_set_param(const char *val,
+					  const struct kernel_param *kp)
+{
+	int i, ret;
+	size_t len;
+	struct ubivol_param *param;
+	char buf[UBIVOL_PARAM_LEN];
+	char *pbuf = &buf[0];
+	char *tokens[UBIVOL_PARAM_COUNT];
+
+	if (!val)
+		return -EINVAL;
+
+	len = strnlen(val, UBIVOL_PARAM_LEN);
+	if (len == 0) {
+		pr_warn("UBI: nocheck: empty 'nocheck=' parameter - ignored\n");
+		return 0;
+	}
+
+	if (len == UBIVOL_PARAM_LEN) {
+		pr_err("UBI: nocheck: parameter \"%s\" is too long, max. is %d\n",
+		       val, UBIVOL_PARAM_LEN);
+		return -EINVAL;
+	}
+
+	strcpy(buf, val);
+
+	/* Get rid of the final newline */
+	if (buf[len - 1] == '\n')
+		buf[len - 1] = '\0';
+
+	for (i = 0; i < UBIVOL_PARAM_COUNT; i++)
+		tokens[i] = strsep(&pbuf, ",");
+
+	param = &ubinocheck_param[ubinocheck_devs];
+	if (tokens[1]) {
+		/* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */
+		ret = kstrtoint(tokens[0], 10, &param->ubi_num);
+		if (ret < 0)
+			return -EINVAL;
+
+		/* Second param can be a number or a name */
+		ret = kstrtoint(tokens[1], 10, &param->vol_id);
+		if (ret < 0) {
+			param->vol_id = -1;
+			strcpy(param->name, tokens[1]);
+		}
+	}
+
+	ubinocheck_devs++;
+
+	return 0;
+}
+
+static const struct kernel_param_ops ubinocheckvol_param_ops = {
+	.set	= ubinocheckvol_set_param,
+};
+module_param_cb(nocheck, &ubinocheckvol_param_ops, NULL, 0);
+MODULE_PARM_DESC(nocheck,
+		 "Disable CRC check for UBI volumes. Parameter format: nocheck=dev,[num|name]>.\n"
+		 "Multiple \"nocheck\" parameters may be specified.\n"
+		 "Examples\n"
+		 "Using the UBI device, and the volume name:\n"
+		 "ubi.nocheck=0,rootfs\n"
+		 "Using both UBI device number and UBI volume number:\n"
+		 "ubi.nocheck=0,0\n");
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 4cc6ec9..2bd89b4 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -825,6 +825,12 @@ struct ubivol_param {
 	char name[UBIVOL_PARAM_LEN + 1];
 };
 
+/* Numbers of elements set in the @ubinocheck_param array */
+extern int ubinocheck_devs;
+
+/* MTD devices specification parameters */
+extern struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
+
 #include "debug.h"
 
 extern struct kmem_cache *ubi_wl_entry_slab;
diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
index 263743e..06b6cfd 100644
--- a/drivers/mtd/ubi/vtbl.c
+++ b/drivers/mtd/ubi/vtbl.c
@@ -534,9 +534,10 @@ static int init_volumes(struct ubi_device *ubi,
 			const struct ubi_attach_info *ai,
 			const struct ubi_vtbl_record *vtbl)
 {
-	int i, reserved_pebs = 0;
+	int i, j, reserved_pebs = 0;
 	struct ubi_ainf_volume *av;
 	struct ubi_volume *vol;
+	struct ubivol_param *param;
 
 	for (i = 0; i < ubi->vtbl_slots; i++) {
 		cond_resched();
@@ -620,6 +621,27 @@ static int init_volumes(struct ubi_device *ubi,
 			(long long)(vol->used_ebs - 1) * vol->usable_leb_size;
 		vol->used_bytes += av->last_data_size;
 		vol->last_eb_bytes = av->last_data_size;
+
+		for (j = 0; j < ubinocheck_devs; j++) {
+			param = &ubinocheck_param[j];
+
+			if (vol->ubi->ubi_num != param->ubi_num)
+				continue;
+			if (vol->vol_id == param->vol_id) {
+				ubi_msg(vol->ubi,
+					"skipping CRC check for volume %d",
+					vol->vol_id);
+				vol->checked = true;
+				break;
+			}
+			if (!strcmp(vol->name, param->name)) {
+				ubi_msg(vol->ubi,
+					"skipping CRC check for volume %s",
+					param->name);
+				vol->checked = true;
+				break;
+			}
+		}
 	}
 
 	/* And add the layout volume */
-- 
git-series 0.9.1

Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Richard Weinberger]

Quentin,

Am Freitag, 20. April 2018, 10:52:41 CEST schrieb Quentin Schulz:
> There's already ECC on NAND pages so there may be no need for one to
> check the CRC of a UBI volume.
> 
> Let's introduce a ubi.nocheck parameter that let one skip the CRC check
> when attaching a UBI volume.
> 
> This also drastically speeds kernel boot by removing a potentially
> useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board
> for a ~20MB UBI volume used as rootfs.

You mean at *open* time of a *static* UBI volume?
So I guess the use case is having a read-only filesystem on top of ubiblock?

> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> ---
>  drivers/mtd/ubi/kapi.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/mtd/ubi/ubi.h  |  6 ++++-
>  drivers/mtd/ubi/vtbl.c | 24 ++++++++++++++-
>  3 files changed, 99 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
> index d4b2e87..d604cd5 100644
> --- a/drivers/mtd/ubi/kapi.c
> +++ b/drivers/mtd/ubi/kapi.c
> @@ -28,6 +28,9 @@
>  #include <asm/div64.h>
>  #include "ubi.h"
>  
> +struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
> +int ubinocheck_devs;
> +
>  /**
>   * ubi_do_get_device_info - get information about UBI device.
>   * @ubi: UBI device description object
> @@ -865,3 +868,70 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb)
>  	return blocking_notifier_chain_unregister(&ubi_notifiers, nb);
>  }
>  EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier);
> +
> +static int __init ubinocheckvol_set_param(const char *val,
> +					  const struct kernel_param *kp)
> +{
> +	int i, ret;
> +	size_t len;
> +	struct ubivol_param *param;
> +	char buf[UBIVOL_PARAM_LEN];
> +	char *pbuf = &buf[0];
> +	char *tokens[UBIVOL_PARAM_COUNT];
> +
> +	if (!val)
> +		return -EINVAL;
> +
> +	len = strnlen(val, UBIVOL_PARAM_LEN);
> +	if (len == 0) {
> +		pr_warn("UBI: nocheck: empty 'nocheck=' parameter - ignored\n");
> +		return 0;
> +	}
> +
> +	if (len == UBIVOL_PARAM_LEN) {
> +		pr_err("UBI: nocheck: parameter \"%s\" is too long, max. is %d\n",
> +		       val, UBIVOL_PARAM_LEN);
> +		return -EINVAL;
> +	}
> +
> +	strcpy(buf, val);
> +
> +	/* Get rid of the final newline */
> +	if (buf[len - 1] == '\n')
> +		buf[len - 1] = '\0';
> +
> +	for (i = 0; i < UBIVOL_PARAM_COUNT; i++)
> +		tokens[i] = strsep(&pbuf, ",");
> +
> +	param = &ubinocheck_param[ubinocheck_devs];
> +	if (tokens[1]) {
> +		/* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */
> +		ret = kstrtoint(tokens[0], 10, &param->ubi_num);
> +		if (ret < 0)
> +			return -EINVAL;
> +
> +		/* Second param can be a number or a name */
> +		ret = kstrtoint(tokens[1], 10, &param->vol_id);
> +		if (ret < 0) {
> +			param->vol_id = -1;
> +			strcpy(param->name, tokens[1]);
> +		}
> +	}
>

Do we really need this per volume? If your flash is trustworthy, it should not matter.
Having it per UBI instance instead of volume would make the code less complicated.

> +	ubinocheck_devs++;
> +
> +	return 0;
> +}
> +
> +static const struct kernel_param_ops ubinocheckvol_param_ops = {
> +	.set	= ubinocheckvol_set_param,
> +};
> +module_param_cb(nocheck, &ubinocheckvol_param_ops, NULL, 0);
> +MODULE_PARM_DESC(nocheck,
> +		 "Disable CRC check for UBI volumes. Parameter format: nocheck=dev,[num|name]>.\n"
> +		 "Multiple \"nocheck\" parameters may be specified.\n"
> +		 "Examples\n"
> +		 "Using the UBI device, and the volume name:\n"
> +		 "ubi.nocheck=0,rootfs\n"
> +		 "Using both UBI device number and UBI volume number:\n"
> +		 "ubi.nocheck=0,0\n");
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index 4cc6ec9..2bd89b4 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -825,6 +825,12 @@ struct ubivol_param {
>  	char name[UBIVOL_PARAM_LEN + 1];
>  };
>  
> +/* Numbers of elements set in the @ubinocheck_param array */
> +extern int ubinocheck_devs;
> +
> +/* MTD devices specification parameters */
> +extern struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
> +
>  #include "debug.h"
>  
>  extern struct kmem_cache *ubi_wl_entry_slab;
> diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
> index 263743e..06b6cfd 100644
> --- a/drivers/mtd/ubi/vtbl.c
> +++ b/drivers/mtd/ubi/vtbl.c
> @@ -534,9 +534,10 @@ static int init_volumes(struct ubi_device *ubi,
>  			const struct ubi_attach_info *ai,
>  			const struct ubi_vtbl_record *vtbl)
>  {
> -	int i, reserved_pebs = 0;
> +	int i, j, reserved_pebs = 0;
>  	struct ubi_ainf_volume *av;
>  	struct ubi_volume *vol;
> +	struct ubivol_param *param;
>  
>  	for (i = 0; i < ubi->vtbl_slots; i++) {
>  		cond_resched();
> @@ -620,6 +621,27 @@ static int init_volumes(struct ubi_device *ubi,
>  			(long long)(vol->used_ebs - 1) * vol->usable_leb_size;
>  		vol->used_bytes += av->last_data_size;
>  		vol->last_eb_bytes = av->last_data_size;
> +
> +		for (j = 0; j < ubinocheck_devs; j++) {
> +			param = &ubinocheck_param[j];
> +
> +			if (vol->ubi->ubi_num != param->ubi_num)
> +				continue;
> +			if (vol->vol_id == param->vol_id) {
> +				ubi_msg(vol->ubi,
> +					"skipping CRC check for volume %d",
> +					vol->vol_id);
> +				vol->checked = true;

Please don't abuse the checked flag.
A new one a la "skip_check" does not hurt.
But again, I don't think we need it per volume.

Also don't forget to add the nocheck parameter to the ioctl() interface, such that
you can specify it also with ubiattach...

Thanks,
//richard

Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 6312 bytes --]

Hi Richard,

On Fri, Apr 20, 2018 at 11:37:07AM +0200, Richard Weinberger wrote:
> Quentin,
> 
> Am Freitag, 20. April 2018, 10:52:41 CEST schrieb Quentin Schulz:
> > There's already ECC on NAND pages so there may be no need for one to
> > check the CRC of a UBI volume.
> > 
> > Let's introduce a ubi.nocheck parameter that let one skip the CRC check
> > when attaching a UBI volume.
> > 
> > This also drastically speeds kernel boot by removing a potentially
> > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board
> > for a ~20MB UBI volume used as rootfs.
> 
> You mean at *open* time of a *static* UBI volume?
> So I guess the use case is having a read-only filesystem on top of ubiblock?
> 

Turns out I forgot some important information and poorly worded my
commit log :)

Yes sorry, I have a squashfs (read-only by design) as the image of one
static UBI volume.

And yes, I meant when opening a static UBI volume.

Quentin

> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
> >  drivers/mtd/ubi/kapi.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/mtd/ubi/ubi.h  |  6 ++++-
> >  drivers/mtd/ubi/vtbl.c | 24 ++++++++++++++-
> >  3 files changed, 99 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
> > index d4b2e87..d604cd5 100644
> > --- a/drivers/mtd/ubi/kapi.c
> > +++ b/drivers/mtd/ubi/kapi.c
> > @@ -28,6 +28,9 @@
> >  #include <asm/div64.h>
> >  #include "ubi.h"
> >  
> > +struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
> > +int ubinocheck_devs;
> > +
> >  /**
> >   * ubi_do_get_device_info - get information about UBI device.
> >   * @ubi: UBI device description object
> > @@ -865,3 +868,70 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb)
> >  	return blocking_notifier_chain_unregister(&ubi_notifiers, nb);
> >  }
> >  EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier);
> > +
> > +static int __init ubinocheckvol_set_param(const char *val,
> > +					  const struct kernel_param *kp)
> > +{
> > +	int i, ret;
> > +	size_t len;
> > +	struct ubivol_param *param;
> > +	char buf[UBIVOL_PARAM_LEN];
> > +	char *pbuf = &buf[0];
> > +	char *tokens[UBIVOL_PARAM_COUNT];
> > +
> > +	if (!val)
> > +		return -EINVAL;
> > +
> > +	len = strnlen(val, UBIVOL_PARAM_LEN);
> > +	if (len == 0) {
> > +		pr_warn("UBI: nocheck: empty 'nocheck=' parameter - ignored\n");
> > +		return 0;
> > +	}
> > +
> > +	if (len == UBIVOL_PARAM_LEN) {
> > +		pr_err("UBI: nocheck: parameter \"%s\" is too long, max. is %d\n",
> > +		       val, UBIVOL_PARAM_LEN);
> > +		return -EINVAL;
> > +	}
> > +
> > +	strcpy(buf, val);
> > +
> > +	/* Get rid of the final newline */
> > +	if (buf[len - 1] == '\n')
> > +		buf[len - 1] = '\0';
> > +
> > +	for (i = 0; i < UBIVOL_PARAM_COUNT; i++)
> > +		tokens[i] = strsep(&pbuf, ",");
> > +
> > +	param = &ubinocheck_param[ubinocheck_devs];
> > +	if (tokens[1]) {
> > +		/* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */
> > +		ret = kstrtoint(tokens[0], 10, &param->ubi_num);
> > +		if (ret < 0)
> > +			return -EINVAL;
> > +
> > +		/* Second param can be a number or a name */
> > +		ret = kstrtoint(tokens[1], 10, &param->vol_id);
> > +		if (ret < 0) {
> > +			param->vol_id = -1;
> > +			strcpy(param->name, tokens[1]);
> > +		}
> > +	}
> >
> 
> Do we really need this per volume? If your flash is trustworthy, it should not matter.
> Having it per UBI instance instead of volume would make the code less complicated.
> 
> > +	ubinocheck_devs++;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct kernel_param_ops ubinocheckvol_param_ops = {
> > +	.set	= ubinocheckvol_set_param,
> > +};
> > +module_param_cb(nocheck, &ubinocheckvol_param_ops, NULL, 0);
> > +MODULE_PARM_DESC(nocheck,
> > +		 "Disable CRC check for UBI volumes. Parameter format: nocheck=dev,[num|name]>.\n"
> > +		 "Multiple \"nocheck\" parameters may be specified.\n"
> > +		 "Examples\n"
> > +		 "Using the UBI device, and the volume name:\n"
> > +		 "ubi.nocheck=0,rootfs\n"
> > +		 "Using both UBI device number and UBI volume number:\n"
> > +		 "ubi.nocheck=0,0\n");
> > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> > index 4cc6ec9..2bd89b4 100644
> > --- a/drivers/mtd/ubi/ubi.h
> > +++ b/drivers/mtd/ubi/ubi.h
> > @@ -825,6 +825,12 @@ struct ubivol_param {
> >  	char name[UBIVOL_PARAM_LEN + 1];
> >  };
> >  
> > +/* Numbers of elements set in the @ubinocheck_param array */
> > +extern int ubinocheck_devs;
> > +
> > +/* MTD devices specification parameters */
> > +extern struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
> > +
> >  #include "debug.h"
> >  
> >  extern struct kmem_cache *ubi_wl_entry_slab;
> > diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
> > index 263743e..06b6cfd 100644
> > --- a/drivers/mtd/ubi/vtbl.c
> > +++ b/drivers/mtd/ubi/vtbl.c
> > @@ -534,9 +534,10 @@ static int init_volumes(struct ubi_device *ubi,
> >  			const struct ubi_attach_info *ai,
> >  			const struct ubi_vtbl_record *vtbl)
> >  {
> > -	int i, reserved_pebs = 0;
> > +	int i, j, reserved_pebs = 0;
> >  	struct ubi_ainf_volume *av;
> >  	struct ubi_volume *vol;
> > +	struct ubivol_param *param;
> >  
> >  	for (i = 0; i < ubi->vtbl_slots; i++) {
> >  		cond_resched();
> > @@ -620,6 +621,27 @@ static int init_volumes(struct ubi_device *ubi,
> >  			(long long)(vol->used_ebs - 1) * vol->usable_leb_size;
> >  		vol->used_bytes += av->last_data_size;
> >  		vol->last_eb_bytes = av->last_data_size;
> > +
> > +		for (j = 0; j < ubinocheck_devs; j++) {
> > +			param = &ubinocheck_param[j];
> > +
> > +			if (vol->ubi->ubi_num != param->ubi_num)
> > +				continue;
> > +			if (vol->vol_id == param->vol_id) {
> > +				ubi_msg(vol->ubi,
> > +					"skipping CRC check for volume %d",
> > +					vol->vol_id);
> > +				vol->checked = true;
> 
> Please don't abuse the checked flag.
> A new one a la "skip_check" does not hurt.
> But again, I don't think we need it per volume.
> 
> Also don't forget to add the nocheck parameter to the ioctl() interface, such that
> you can specify it also with ubiattach...
> 
> Thanks,
> //richard

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Boris Brezillon]

On Fri, 20 Apr 2018 10:52:41 +0200
Quentin Schulz <quentin.schulz@bootlin.com> wrote:

> There's already ECC on NAND pages so there may be no need for one to
> check the CRC of a UBI volume.

That's true that ECC can help detecting corruptions, but I don't think
this is the actual reason for disabling CRC check at volume open time.
The actual reason for doing that is when the UBI volume user (in our
case the squashfs FS) is also checking data consistency on its own
(which is the case for squashfs).

> 
> Let's introduce a ubi.nocheck parameter that let one skip the CRC check
> when attaching a UBI volume.
> 
> This also drastically speeds kernel boot by removing a potentially
> useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board
> for a ~20MB UBI volume used as rootfs.

Can you give the old and new open/mount time instead of telling how
much you gained on the whole boot-time?

Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 3382 bytes --]

Hi Boris,

On Fri, Apr 20, 2018 at 07:19:24PM +0200, Boris Brezillon wrote:
> On Fri, 20 Apr 2018 10:52:41 +0200
> Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> 
> > There's already ECC on NAND pages so there may be no need for one to
> > check the CRC of a UBI volume.
> 
> That's true that ECC can help detecting corruptions, but I don't think
> this is the actual reason for disabling CRC check at volume open time.
> The actual reason for doing that is when the UBI volume user (in our
> case the squashfs FS) is also checking data consistency on its own
> (which is the case for squashfs).
> 
> > 
> > Let's introduce a ubi.nocheck parameter that let one skip the CRC check
> > when attaching a UBI volume.
> > 
> > This also drastically speeds kernel boot by removing a potentially
> > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board
> > for a ~20MB UBI volume used as rootfs.
> 
> Can you give the old and new open/mount time instead of telling how
> much you gained on the whole boot-time?

# grabserial -v -d /dev/ttyUSB0 -t "Starting kernel"

Kernel command line is:
rootfstype=squashfs root=/dev/ubiblock0_0 ubi.mtd=ubi ubi.block=0,rootfs mtdparts=nand:92M@36M(ubi)

Current behaviour (CRC checking):
[2.335558 0.017867] ubi0: attaching mtd7
[2.504021 0.168463] ubi0: scanning is finished
[2.519824 0.015803] ubi0: attached mtd7 (name "ubi", size 92 MiB)
[2.523987 0.004163] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes
[2.530447 0.006460] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 512
[2.535836 0.005389] ubi0: VID header offset: 512 (aligned 512), data offset: 2048
[2.541601 0.005765] ubi0: good PEBs: 736, bad PEBs: 0, corrupted PEBs: 0
[2.547923 0.006322] ubi0: user volume: 3, internal volumes: 1, max. volumes count: 128
[2.554223 0.006300] ubi0: max/mean erase counter: 1/0, WL threshold: 4096, image sequence number: 463835917
[2.562460 0.008237] ubi0: available PEBs: 0, total reserved PEBs: 736, PEBs reserved for bad PEB handling: 20
[2.569779 0.007319] ubi0: background thread "ubi_bgt0d" started, PID 546
[4.550142 1.980363] block ubiblock0_0: created from ubi0:0(rootfs)

Skipping CRC check (ubi.nocheck=0,rootfs):
[2.349903 0.019126] ubi0: attaching mtd7
[2.516995 0.167092] ubi0: scanning is finished
[2.527101 0.010106] ubi0: skipping CRC check for volume rootfs
[2.536719 0.009618] ubi0: attached mtd7 (name "ubi", size 92 MiB)
[2.541859 0.005140] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes
[2.547744 0.005885] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 512
[2.552990 0.005246] ubi0: VID header offset: 512 (aligned 512), data offset: 2048
[2.560497 0.007507] ubi0: good PEBs: 736, bad PEBs: 0, corrupted PEBs: 0
[2.568010 0.007513] ubi0: user volume: 3, internal volumes: 1, max. volumes count: 128
[2.574723 0.006713] ubi0: max/mean erase counter: 1/0, WL threshold: 4096, image sequence number: 463835917
[2.586223 0.011500] ubi0: available PEBs: 0, total reserved PEBs: 736, PEBs reserved for bad PEB handling: 20
[2.592365 0.006142] ubi0: background thread "ubi_bgt0d" started, PID 545
[2.595960 0.003595] block ubiblock0_0: created from ubi0:0(rootfs)

It seems like it's more in the magnitude of 2s rather than 3, but still.

This is retrieved from the boot log.

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Boris Brezillon]

On Mon, 23 Apr 2018 11:40:09 +0200
Quentin Schulz <quentin.schulz@bootlin.com> wrote:

> Hi Boris,
> 
> On Fri, Apr 20, 2018 at 07:19:24PM +0200, Boris Brezillon wrote:
> > On Fri, 20 Apr 2018 10:52:41 +0200
> > Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> >   
> > > There's already ECC on NAND pages so there may be no need for one to
> > > check the CRC of a UBI volume.  
> > 
> > That's true that ECC can help detecting corruptions, but I don't think
> > this is the actual reason for disabling CRC check at volume open time.
> > The actual reason for doing that is when the UBI volume user (in our
> > case the squashfs FS) is also checking data consistency on its own
> > (which is the case for squashfs).
> >   
> > > 
> > > Let's introduce a ubi.nocheck parameter that let one skip the CRC check
> > > when attaching a UBI volume.
> > > 
> > > This also drastically speeds kernel boot by removing a potentially
> > > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board
> > > for a ~20MB UBI volume used as rootfs.  
> > 
> > Can you give the old and new open/mount time instead of telling how
> > much you gained on the whole boot-time?  
> 
> # grabserial -v -d /dev/ttyUSB0 -t "Starting kernel"
> 
> Kernel command line is:
> rootfstype=squashfs root=/dev/ubiblock0_0 ubi.mtd=ubi ubi.block=0,rootfs mtdparts=nand:92M@36M(ubi)
> 
> Current behaviour (CRC checking):
> [2.335558 0.017867] ubi0: attaching mtd7
> [2.504021 0.168463] ubi0: scanning is finished
> [2.519824 0.015803] ubi0: attached mtd7 (name "ubi", size 92 MiB)
> [2.523987 0.004163] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes
> [2.530447 0.006460] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 512
> [2.535836 0.005389] ubi0: VID header offset: 512 (aligned 512), data offset: 2048
> [2.541601 0.005765] ubi0: good PEBs: 736, bad PEBs: 0, corrupted PEBs: 0
> [2.547923 0.006322] ubi0: user volume: 3, internal volumes: 1, max. volumes count: 128
> [2.554223 0.006300] ubi0: max/mean erase counter: 1/0, WL threshold: 4096, image sequence number: 463835917
> [2.562460 0.008237] ubi0: available PEBs: 0, total reserved PEBs: 736, PEBs reserved for bad PEB handling: 20
> [2.569779 0.007319] ubi0: background thread "ubi_bgt0d" started, PID 546
> [4.550142 1.980363] block ubiblock0_0: created from ubi0:0(rootfs)
> 
> Skipping CRC check (ubi.nocheck=0,rootfs):
> [2.349903 0.019126] ubi0: attaching mtd7
> [2.516995 0.167092] ubi0: scanning is finished
> [2.527101 0.010106] ubi0: skipping CRC check for volume rootfs
> [2.536719 0.009618] ubi0: attached mtd7 (name "ubi", size 92 MiB)
> [2.541859 0.005140] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes
> [2.547744 0.005885] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 512
> [2.552990 0.005246] ubi0: VID header offset: 512 (aligned 512), data offset: 2048
> [2.560497 0.007507] ubi0: good PEBs: 736, bad PEBs: 0, corrupted PEBs: 0
> [2.568010 0.007513] ubi0: user volume: 3, internal volumes: 1, max. volumes count: 128
> [2.574723 0.006713] ubi0: max/mean erase counter: 1/0, WL threshold: 4096, image sequence number: 463835917
> [2.586223 0.011500] ubi0: available PEBs: 0, total reserved PEBs: 736, PEBs reserved for bad PEB handling: 20
> [2.592365 0.006142] ubi0: background thread "ubi_bgt0d" started, PID 545
> [2.595960 0.003595] block ubiblock0_0: created from ubi0:0(rootfs)
> 
> It seems like it's more in the magnitude of 2s rather than 3, but still.

No, I meant add this information to the commit message since telling
how many seconds are saved on the absolute boot-time does not give any
information on the before vs after ratio.

Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Quentin Schulz]

[-- Attachment #1: Type: text/plain, Size: 7078 bytes --]

Hi Richard,

On Fri, Apr 20, 2018 at 11:37:07AM +0200, Richard Weinberger wrote:
> Quentin,
> 
> Am Freitag, 20. April 2018, 10:52:41 CEST schrieb Quentin Schulz:
> > There's already ECC on NAND pages so there may be no need for one to
> > check the CRC of a UBI volume.
> > 
> > Let's introduce a ubi.nocheck parameter that let one skip the CRC check
> > when attaching a UBI volume.
> > 
> > This also drastically speeds kernel boot by removing a potentially
> > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board
> > for a ~20MB UBI volume used as rootfs.
> 
> You mean at *open* time of a *static* UBI volume?
> So I guess the use case is having a read-only filesystem on top of ubiblock?
> 
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
> >  drivers/mtd/ubi/kapi.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/mtd/ubi/ubi.h  |  6 ++++-
> >  drivers/mtd/ubi/vtbl.c | 24 ++++++++++++++-
> >  3 files changed, 99 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
> > index d4b2e87..d604cd5 100644
> > --- a/drivers/mtd/ubi/kapi.c
> > +++ b/drivers/mtd/ubi/kapi.c
> > @@ -28,6 +28,9 @@
> >  #include <asm/div64.h>
> >  #include "ubi.h"
> >  
> > +struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
> > +int ubinocheck_devs;
> > +
> >  /**
> >   * ubi_do_get_device_info - get information about UBI device.
> >   * @ubi: UBI device description object
> > @@ -865,3 +868,70 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb)
> >  	return blocking_notifier_chain_unregister(&ubi_notifiers, nb);
> >  }
> >  EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier);
> > +
> > +static int __init ubinocheckvol_set_param(const char *val,
> > +					  const struct kernel_param *kp)
> > +{
> > +	int i, ret;
> > +	size_t len;
> > +	struct ubivol_param *param;
> > +	char buf[UBIVOL_PARAM_LEN];
> > +	char *pbuf = &buf[0];
> > +	char *tokens[UBIVOL_PARAM_COUNT];
> > +
> > +	if (!val)
> > +		return -EINVAL;
> > +
> > +	len = strnlen(val, UBIVOL_PARAM_LEN);
> > +	if (len == 0) {
> > +		pr_warn("UBI: nocheck: empty 'nocheck=' parameter - ignored\n");
> > +		return 0;
> > +	}
> > +
> > +	if (len == UBIVOL_PARAM_LEN) {
> > +		pr_err("UBI: nocheck: parameter \"%s\" is too long, max. is %d\n",
> > +		       val, UBIVOL_PARAM_LEN);
> > +		return -EINVAL;
> > +	}
> > +
> > +	strcpy(buf, val);
> > +
> > +	/* Get rid of the final newline */
> > +	if (buf[len - 1] == '\n')
> > +		buf[len - 1] = '\0';
> > +
> > +	for (i = 0; i < UBIVOL_PARAM_COUNT; i++)
> > +		tokens[i] = strsep(&pbuf, ",");
> > +
> > +	param = &ubinocheck_param[ubinocheck_devs];
> > +	if (tokens[1]) {
> > +		/* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */
> > +		ret = kstrtoint(tokens[0], 10, &param->ubi_num);
> > +		if (ret < 0)
> > +			return -EINVAL;
> > +
> > +		/* Second param can be a number or a name */
> > +		ret = kstrtoint(tokens[1], 10, &param->vol_id);
> > +		if (ret < 0) {
> > +			param->vol_id = -1;
> > +			strcpy(param->name, tokens[1]);
> > +		}
> > +	}
> >
> 
> Do we really need this per volume? If your flash is trustworthy, it should not matter.
> Having it per UBI instance instead of volume would make the code less complicated.
> 

In my opinion, yes.

Here is my use case: I have one "big" UBI image with different
volumes and filesystems. One of those is a squashfs that is
mounted/checked by dm-verity. The hash check done by dm-verity is heavy
and makes the CRC check done by UBI useless.

I would like to still be able to check the CRC of the other UBI volumes
but also have one or more volumes for which I bypass the CRC check done
by UBI since it's useless and time consuming. By putting a flag per UBI
image would require me to complexify the layout of my storage medium to
have one UBI image for the volumes that don't need CRC check and one for
the volumes that do. It's cumbersome and can be avoided IMHO.

> > +	ubinocheck_devs++;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct kernel_param_ops ubinocheckvol_param_ops = {
> > +	.set	= ubinocheckvol_set_param,
> > +};
> > +module_param_cb(nocheck, &ubinocheckvol_param_ops, NULL, 0);
> > +MODULE_PARM_DESC(nocheck,
> > +		 "Disable CRC check for UBI volumes. Parameter format: nocheck=dev,[num|name]>.\n"
> > +		 "Multiple \"nocheck\" parameters may be specified.\n"
> > +		 "Examples\n"
> > +		 "Using the UBI device, and the volume name:\n"
> > +		 "ubi.nocheck=0,rootfs\n"
> > +		 "Using both UBI device number and UBI volume number:\n"
> > +		 "ubi.nocheck=0,0\n");
> > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> > index 4cc6ec9..2bd89b4 100644
> > --- a/drivers/mtd/ubi/ubi.h
> > +++ b/drivers/mtd/ubi/ubi.h
> > @@ -825,6 +825,12 @@ struct ubivol_param {
> >  	char name[UBIVOL_PARAM_LEN + 1];
> >  };
> >  
> > +/* Numbers of elements set in the @ubinocheck_param array */
> > +extern int ubinocheck_devs;
> > +
> > +/* MTD devices specification parameters */
> > +extern struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES];
> > +
> >  #include "debug.h"
> >  
> >  extern struct kmem_cache *ubi_wl_entry_slab;
> > diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
> > index 263743e..06b6cfd 100644
> > --- a/drivers/mtd/ubi/vtbl.c
> > +++ b/drivers/mtd/ubi/vtbl.c
> > @@ -534,9 +534,10 @@ static int init_volumes(struct ubi_device *ubi,
> >  			const struct ubi_attach_info *ai,
> >  			const struct ubi_vtbl_record *vtbl)
> >  {
> > -	int i, reserved_pebs = 0;
> > +	int i, j, reserved_pebs = 0;
> >  	struct ubi_ainf_volume *av;
> >  	struct ubi_volume *vol;
> > +	struct ubivol_param *param;
> >  
> >  	for (i = 0; i < ubi->vtbl_slots; i++) {
> >  		cond_resched();
> > @@ -620,6 +621,27 @@ static int init_volumes(struct ubi_device *ubi,
> >  			(long long)(vol->used_ebs - 1) * vol->usable_leb_size;
> >  		vol->used_bytes += av->last_data_size;
> >  		vol->last_eb_bytes = av->last_data_size;
> > +
> > +		for (j = 0; j < ubinocheck_devs; j++) {
> > +			param = &ubinocheck_param[j];
> > +
> > +			if (vol->ubi->ubi_num != param->ubi_num)
> > +				continue;
> > +			if (vol->vol_id == param->vol_id) {
> > +				ubi_msg(vol->ubi,
> > +					"skipping CRC check for volume %d",
> > +					vol->vol_id);
> > +				vol->checked = true;
> 
> Please don't abuse the checked flag.
> A new one a la "skip_check" does not hurt.

I'll do that.

> But again, I don't think we need it per volume.
> 
> Also don't forget to add the nocheck parameter to the ioctl() interface, such that
> you can specify it also with ubiattach...
> 

If we go for a per-image flag, adding nocheck to the ioctl makes sense,
otherwise we have to find a way to select only one or more volumes for
which the nocheck flag should be set. That being said, I'm not sure a
per-image flag is the way we want to go.

What are your thoughts on this?

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Richard Weinberger]

Quentin,

Am Montag, 11. Juni 2018, 12:20:37 CEST schrieb Quentin Schulz:
> If we go for a per-image flag, adding nocheck to the ioctl makes sense,
> otherwise we have to find a way to select only one or more volumes for
> which the nocheck flag should be set. That being said, I'm not sure a
> per-image flag is the way we want to go.
> 
> What are your thoughts on this?

I like that approach a lot more, please send a patch. :-)
Having a volume flag will reduce complexity a lot.

Thanks,
//richard

Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Boris Brezillon]

On Thu, 14 Jun 2018 09:29:42 +0200
Richard Weinberger <richard@nod.at> wrote:

> Quentin,
> 
> Am Montag, 11. Juni 2018, 12:20:37 CEST schrieb Quentin Schulz:
> > If we go for a per-image flag, adding nocheck to the ioctl makes sense,
> > otherwise we have to find a way to select only one or more volumes for
> > which the nocheck flag should be set. That being said, I'm not sure a
> > per-image flag is the way we want to go.
> > 
> > What are your thoughts on this?  
> 
> I like that approach a lot more, please send a patch. :-)

I don't think the approach you're talking about has been mentioned by
Quentin in this email :-). To clarify the discussion, we're talking
about adding a new flag to VTBL entries telling whether the CRC should
be skipped or not.

> Having a volume flag will reduce complexity a lot.

That's my opinion as well.

Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Richard Weinberger]

Am Donnerstag, 14. Juni 2018, 10:04:36 CEST schrieb Boris Brezillon:
> On Thu, 14 Jun 2018 09:29:42 +0200
> Richard Weinberger <richard@nod.at> wrote:
> 
> > Quentin,
> > 
> > Am Montag, 11. Juni 2018, 12:20:37 CEST schrieb Quentin Schulz:
> > > If we go for a per-image flag, adding nocheck to the ioctl makes sense,
> > > otherwise we have to find a way to select only one or more volumes for
> > > which the nocheck flag should be set. That being said, I'm not sure a
> > > per-image flag is the way we want to go.
> > > 
> > > What are your thoughts on this?  
> > 
> > I like that approach a lot more, please send a patch. :-)
> 
> I don't think the approach you're talking about has been mentioned by
> Quentin in this email :-). To clarify the discussion, we're talking
> about adding a new flag to VTBL entries telling whether the CRC should
> be skipped or not.

Hehe yes. Too many discussions on and off-list in parallel. ;)

Thanks,
//richard