[PATCH 1/2] ubi: mount partitions specified in device tree

[PATCH 1/2] ubi: mount partitions specified in device tree

[Hauke Mehrtens]

This makes it possible to open a ubi layer in device tree, this is
helpful when the rootfs is on a ubi layer. It loops though all mtd
partitions and mounts the partition which is compatible with
"ubi,volume". The same was only possible with kernel command line
arguments before.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 Documentation/devicetree/bindings/mtd/ubi.txt | 33 ++++++++++++++
 drivers/mtd/ubi/block.c                       | 63 +++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/ubi.txt

diff --git a/Documentation/devicetree/bindings/mtd/ubi.txt b/Documentation/devicetree/bindings/mtd/ubi.txt
new file mode 100644
index 0000000..5fcd47e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/ubi.txt
@@ -0,0 +1,33 @@
+UBI - Unsorted block images
+
+Describe of a UBI layer in device tree.
+
+ - compatible:		"ubi,device", This marks a partition that contains
+ 			a ubi layer.
+ - vid_hdr_offs:	Optional parameter specifies UBI VID header position
+ 			to be used by UBI. (default value if 0)
+ - max_beb_per1024:	Optional parameter specifies the maximum expected bad
+ 			eraseblock per 1024 eraseblocks.
+ 			(default value CONFIG_MTD_UBI_BEB_LIMIT)
+ -ubi_num:		Optional parameter specifies UBI device number
+ 			which have to be assigned to the newly created UBI
+ 			device (assigned automatically by default)
+
+Example:
+
+partitions {
+	compatible = "fixed-partitions";
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	partition@0 {
+		label = "uboot";
+		reg = <0x00000 0x100000>;
+	};
+
+	partition@1c0000 {
+		label = "system_sw";
+		reg = <0x1c0000 0xc800000>;
+		compatible = "ubi,device";
+	};
+};
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index ebf46ad..5ed390d 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -1,6 +1,7 @@
 /*
  * Copyright (c) 2014 Ezequiel Garcia
  * Copyright (c) 2011 Free Electrons
+ * Copyright (c) 2016 Hauke Mehrtens <hauke@hauke-m.de>
  *
  * Driver parameter handling strongly based on drivers/mtd/ubi/build.c
  *   Copyright (c) International Business Machines Corp., 2006
@@ -41,6 +42,7 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/mtd/ubi.h>
 #include <linux/workqueue.h>
@@ -628,6 +630,64 @@ static void __init ubiblock_create_from_param(void)
 	}
 }
 
+static void __init ubiblock_create_from_device_tree(void)
+{
+	int ubi_num;
+	const char *name;
+	u32 mode;
+	struct ubi_device *ubi;
+	struct ubi_volume_desc *desc;
+	struct ubi_volume_info vi;
+	struct mtd_info *mtd;
+	struct device_node *volume;
+	int ret;
+
+	for (ubi_num = 0; ubi_num < UBI_MAX_DEVICES; ubi_num++) {
+		ubi = ubi_get_device(ubi_num);
+		if (!ubi)
+			continue;
+		mtd = ubi->mtd;
+		if (!mtd || !of_device_is_compatible(mtd->dev.of_node,
+						     "ubi,device")) {
+			ubi_put_device(ubi);
+			continue;
+		}
+
+		for_each_child_of_node(mtd->dev.of_node, volume) {
+			if (!of_device_is_compatible(volume, "ubi,volume"))
+				continue;
+
+			ret = of_property_read_string(volume, "name", &name);
+			if (ret)
+				continue;
+
+			ret = of_property_read_u32(volume, "ubi-mode", &mode);
+			if (ret)
+				continue;
+
+			desc = ubi_open_volume_nm(ubi_num, name, mode);
+			if (IS_ERR(desc)) {
+				pr_err(
+				       "UBI: block: can't open volume %s on ubi%d, err=%ld",
+				       name, ubi_num, PTR_ERR(desc));
+				continue;
+			}
+
+			ubi_get_volume_info(desc, &vi);
+			ubi_close_volume(desc);
+
+			ret = ubiblock_create(&vi);
+			if (ret) {
+				pr_err(
+				       "UBI: block: can't add '%s' volume on ubi%d, err=%d",
+				       vi.name, ubi_num, ret);
+				continue;
+			}
+		}
+		ubi_put_device(ubi);
+	}
+}
+
 static void ubiblock_remove_all(void)
 {
 	struct ubiblock *next;
@@ -658,6 +718,9 @@ int __init ubiblock_init(void)
 	 */
 	ubiblock_create_from_param();
 
+	/* Attach block devices from device tree */
+	ubiblock_create_from_device_tree();
+
 	/*
 	 * Block devices are only created upon user requests, so we ignore
 	 * existing volumes.
-- 
2.8.1

[PATCH 2/2] ubi: open volumes define in device tree

[Hauke Mehrtens]

This makes it possible to define a volume to open in device tree. This
is helpful when a root file system is on a ubi layer. Without this
patch it is only possible to device with with kernel command line
parameters.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 Documentation/devicetree/bindings/mtd/ubi.txt | 22 +++++++++++++++++
 drivers/mtd/ubi/build.c                       | 34 +++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/ubi.txt b/Documentation/devicetree/bindings/mtd/ubi.txt
index 5fcd47e..f87a7d8 100644
--- a/Documentation/devicetree/bindings/mtd/ubi.txt
+++ b/Documentation/devicetree/bindings/mtd/ubi.txt
@@ -13,6 +13,16 @@ Describe of a UBI layer in device tree.
  			which have to be assigned to the newly created UBI
  			device (assigned automatically by default)
 
+Partitions on a UBI layer can be defined:
+
+ - compatible: 		"ubi,volume"
+ - name:		name of the ubi volume to open
+ - ubi-mode:		mode of the ubi volume to open value:
+ 			UBI_READONLY:	1
+ 			UBI_READWRITE:	2
+ 			UBI_EXCLUSIVE:	3
+ 			UBI_METAONLY:	4
+
 Example:
 
 partitions {
@@ -29,5 +39,17 @@ partitions {
 		label = "system_sw";
 		reg = <0x1c0000 0xc800000>;
 		compatible = "ubi,device";
+
+		rootfsA@0 {
+			compatible = "ubi,volume";
+			name = "rootfsA";
+			ubi-mode = <1>;
+		};
+
+		data_vol@0 {
+			compatible = "ubi,volume";
+			name = "data_vol";
+			ubi-mode = <2>;
+		};
 	};
 };
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 16baeb5..1c22533 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1,6 +1,7 @@
 /*
  * Copyright (c) International Business Machines Corp., 2006
  * Copyright (c) Nokia Corporation, 2007
+ * Copyright (c) 2016 Hauke Mehrtens <hauke@hauke-m.de>
  *
  * 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
@@ -33,6 +34,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/stringify.h>
+#include <linux/of.h>
 #include <linux/namei.h>
 #include <linux/stat.h>
 #include <linux/miscdevice.h>
@@ -43,6 +45,7 @@
 #include <linux/slab.h>
 #include <linux/major.h>
 #include "ubi.h"
+#include "../mtdcore.h"
 
 /* Maximum length of the 'mtd=' parameter */
 #define MTD_PARAM_LEN_MAX 64
@@ -1205,6 +1208,7 @@ static struct mtd_info * __init open_mtd_device(const char *mtd_dev)
 static int __init ubi_init(void)
 {
 	int err, i, k;
+	struct mtd_info *mtd_it;
 
 	/* Ensure that EC and VID headers have correct size */
 	BUILD_BUG_ON(sizeof(struct ubi_ec_hdr) != 64);
@@ -1285,6 +1289,36 @@ static int __init ubi_init(void)
 		}
 	}
 
+	mtd_for_each_device(mtd_it) {
+		struct mtd_info *mtd;
+		u32 ubi_num = 0;
+		u32 vid_hdr_offs = 0;
+		u32 max_beb_per1024 = CONFIG_MTD_UBI_BEB_LIMIT;
+
+		mtd = get_mtd_device(mtd_it, -1);
+
+		if (!of_device_is_compatible(mtd->dev.of_node, "ubi,device")) {
+			put_mtd_device(mtd);
+			continue;
+		}
+
+		of_property_read_u32(mtd->dev.of_node, "vid_hdr_offs",
+				     &vid_hdr_offs);
+		of_property_read_u32(mtd->dev.of_node, "max_beb_per1024",
+				     &max_beb_per1024);
+		of_property_read_u32(mtd->dev.of_node, "ubi_num", &ubi_num);
+
+		mutex_lock(&ubi_devices_mutex);
+		err = ubi_attach_mtd_dev(mtd, ubi_num, vid_hdr_offs,
+					 max_beb_per1024);
+		mutex_unlock(&ubi_devices_mutex);
+		if (err < 0) {
+			pr_err("UBI error: cannot attach mtd%d",
+			       mtd->index);
+		}
+		put_mtd_device(mtd);
+	}
+
 	err = ubiblock_init();
 	if (err) {
 		pr_err("UBI error: block: cannot initialize, error %d", err);
-- 
2.8.1

Re: [PATCH 2/2] ubi: open volumes define in device tree

[Daniel Golle]

On Sat, Jun 18, 2016 at 09:17:56PM +0200, Hauke Mehrtens wrote:
> This makes it possible to define a volume to open in device tree. This
> is helpful when a root file system is on a ubi layer. Without this
> patch it is only possible to device with with kernel command line
> parameters.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

The example in the documentation doesn't belong to that patch, see
below. Apart from this single [PATCH 2/2]:
Acked-by: Daniel Golle <daniel@makrotopia.org>

> ---
>  Documentation/devicetree/bindings/mtd/ubi.txt | 22 +++++++++++++++++
>  drivers/mtd/ubi/build.c                       | 34 +++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/ubi.txt b/Documentation/devicetree/bindings/mtd/ubi.txt
> index 5fcd47e..f87a7d8 100644
> --- a/Documentation/devicetree/bindings/mtd/ubi.txt
> +++ b/Documentation/devicetree/bindings/mtd/ubi.txt
> @@ -13,6 +13,16 @@ Describe of a UBI layer in device tree.
>   			which have to be assigned to the newly created UBI
>   			device (assigned automatically by default)
>  
> +Partitions on a UBI layer can be defined:
> +
> + - compatible: 		"ubi,volume"
> + - name:		name of the ubi volume to open
> + - ubi-mode:		mode of the ubi volume to open value:
> + 			UBI_READONLY:	1
> + 			UBI_READWRITE:	2
> + 			UBI_EXCLUSIVE:	3
> + 			UBI_METAONLY:	4
> +
>  Example:
>  
>  partitions {
> @@ -29,5 +39,17 @@ partitions {
>  		label = "system_sw";
>  		reg = <0x1c0000 0xc800000>;
>  		compatible = "ubi,device";
> +
> +		rootfsA@0 {
> +			compatible = "ubi,volume";
> +			name = "rootfsA";
> +			ubi-mode = <1>;
> +		};
> +
> +		data_vol@0 {
> +			compatible = "ubi,volume";
> +			name = "data_vol";
> +			ubi-mode = <2>;
> +		};

This is an example for the 'ubi,volume' while the patch introduces
support for 'ubi,device'.


>  	};
>  };
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 16baeb5..1c22533 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -1,6 +1,7 @@
>  /*
>   * Copyright (c) International Business Machines Corp., 2006
>   * Copyright (c) Nokia Corporation, 2007
> + * Copyright (c) 2016 Hauke Mehrtens <hauke@hauke-m.de>
>   *
>   * 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
> @@ -33,6 +34,7 @@
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/stringify.h>
> +#include <linux/of.h>
>  #include <linux/namei.h>
>  #include <linux/stat.h>
>  #include <linux/miscdevice.h>
> @@ -43,6 +45,7 @@
>  #include <linux/slab.h>
>  #include <linux/major.h>
>  #include "ubi.h"
> +#include "../mtdcore.h"
>  
>  /* Maximum length of the 'mtd=' parameter */
>  #define MTD_PARAM_LEN_MAX 64
> @@ -1205,6 +1208,7 @@ static struct mtd_info * __init open_mtd_device(const char *mtd_dev)
>  static int __init ubi_init(void)
>  {
>  	int err, i, k;
> +	struct mtd_info *mtd_it;
>  
>  	/* Ensure that EC and VID headers have correct size */
>  	BUILD_BUG_ON(sizeof(struct ubi_ec_hdr) != 64);
> @@ -1285,6 +1289,36 @@ static int __init ubi_init(void)
>  		}
>  	}
>  
> +	mtd_for_each_device(mtd_it) {
> +		struct mtd_info *mtd;
> +		u32 ubi_num = 0;
> +		u32 vid_hdr_offs = 0;
> +		u32 max_beb_per1024 = CONFIG_MTD_UBI_BEB_LIMIT;
> +
> +		mtd = get_mtd_device(mtd_it, -1);
> +
> +		if (!of_device_is_compatible(mtd->dev.of_node, "ubi,device")) {
> +			put_mtd_device(mtd);
> +			continue;
> +		}
> +
> +		of_property_read_u32(mtd->dev.of_node, "vid_hdr_offs",
> +				     &vid_hdr_offs);
> +		of_property_read_u32(mtd->dev.of_node, "max_beb_per1024",
> +				     &max_beb_per1024);
> +		of_property_read_u32(mtd->dev.of_node, "ubi_num", &ubi_num);
> +
> +		mutex_lock(&ubi_devices_mutex);
> +		err = ubi_attach_mtd_dev(mtd, ubi_num, vid_hdr_offs,
> +					 max_beb_per1024);
> +		mutex_unlock(&ubi_devices_mutex);
> +		if (err < 0) {
> +			pr_err("UBI error: cannot attach mtd%d",
> +			       mtd->index);
> +		}
> +		put_mtd_device(mtd);
> +	}
> +
>  	err = ubiblock_init();
>  	if (err) {
>  		pr_err("UBI error: block: cannot initialize, error %d", err);
> -- 
> 2.8.1
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] ubi: open volumes define in device tree

[Hauke Mehrtens]

On 06/19/2016 01:36 AM, Daniel Golle wrote:
> On Sat, Jun 18, 2016 at 09:17:56PM +0200, Hauke Mehrtens wrote:
>> This makes it possible to define a volume to open in device tree. This
>> is helpful when a root file system is on a ubi layer. Without this
>> patch it is only possible to device with with kernel command line
>> parameters.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> 
> The example in the documentation doesn't belong to that patch, see
> below. Apart from this single [PATCH 2/2]:
> Acked-by: Daniel Golle <daniel@makrotopia.org>
> 
>> ---
>>  Documentation/devicetree/bindings/mtd/ubi.txt | 22 +++++++++++++++++
>>  drivers/mtd/ubi/build.c                       | 34 +++++++++++++++++++++++++++
>>  2 files changed, 56 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/ubi.txt b/Documentation/devicetree/bindings/mtd/ubi.txt
>> index 5fcd47e..f87a7d8 100644
>> --- a/Documentation/devicetree/bindings/mtd/ubi.txt
>> +++ b/Documentation/devicetree/bindings/mtd/ubi.txt
>> @@ -13,6 +13,16 @@ Describe of a UBI layer in device tree.
>>   			which have to be assigned to the newly created UBI
>>   			device (assigned automatically by default)
>>  
>> +Partitions on a UBI layer can be defined:
>> +
>> + - compatible: 		"ubi,volume"
>> + - name:		name of the ubi volume to open
>> + - ubi-mode:		mode of the ubi volume to open value:
>> + 			UBI_READONLY:	1
>> + 			UBI_READWRITE:	2
>> + 			UBI_EXCLUSIVE:	3
>> + 			UBI_METAONLY:	4
>> +
>>  Example:
>>  
>>  partitions {
>> @@ -29,5 +39,17 @@ partitions {
>>  		label = "system_sw";
>>  		reg = <0x1c0000 0xc800000>;
>>  		compatible = "ubi,device";
>> +
>> +		rootfsA@0 {
>> +			compatible = "ubi,volume";
>> +			name = "rootfsA";
>> +			ubi-mode = <1>;
>> +		};
>> +
>> +		data_vol@0 {
>> +			compatible = "ubi,volume";
>> +			name = "data_vol";
>> +			ubi-mode = <2>;
>> +		};
> 
> This is an example for the 'ubi,volume' while the patch introduces
> support for 'ubi,device'.

Sorry this documentation should be for the code added in the previous
patch and vise versa.

>>  	};
>>  };
>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
>> index 16baeb5..1c22533 100644
>> --- a/drivers/mtd/ubi/build.c
>> +++ b/drivers/mtd/ubi/build.c
>> @@ -1,6 +1,7 @@
>>  /*
>>   * Copyright (c) International Business Machines Corp., 2006
>>   * Copyright (c) Nokia Corporation, 2007
>> + * Copyright (c) 2016 Hauke Mehrtens <hauke@hauke-m.de>
>>   *
>>   * 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
>> @@ -33,6 +34,7 @@
>>  #include <linux/module.h>
>>  #include <linux/moduleparam.h>
>>  #include <linux/stringify.h>
>> +#include <linux/of.h>
>>  #include <linux/namei.h>
>>  #include <linux/stat.h>
>>  #include <linux/miscdevice.h>
>> @@ -43,6 +45,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/major.h>
>>  #include "ubi.h"
>> +#include "../mtdcore.h"
>>  
>>  /* Maximum length of the 'mtd=' parameter */
>>  #define MTD_PARAM_LEN_MAX 64
>> @@ -1205,6 +1208,7 @@ static struct mtd_info * __init open_mtd_device(const char *mtd_dev)
>>  static int __init ubi_init(void)
>>  {
>>  	int err, i, k;
>> +	struct mtd_info *mtd_it;
>>  
>>  	/* Ensure that EC and VID headers have correct size */
>>  	BUILD_BUG_ON(sizeof(struct ubi_ec_hdr) != 64);
>> @@ -1285,6 +1289,36 @@ static int __init ubi_init(void)
>>  		}
>>  	}
>>  
>> +	mtd_for_each_device(mtd_it) {
>> +		struct mtd_info *mtd;
>> +		u32 ubi_num = 0;
>> +		u32 vid_hdr_offs = 0;
>> +		u32 max_beb_per1024 = CONFIG_MTD_UBI_BEB_LIMIT;
>> +
>> +		mtd = get_mtd_device(mtd_it, -1);
>> +
>> +		if (!of_device_is_compatible(mtd->dev.of_node, "ubi,device")) {
>> +			put_mtd_device(mtd);
>> +			continue;
>> +		}
>> +
>> +		of_property_read_u32(mtd->dev.of_node, "vid_hdr_offs",
>> +				     &vid_hdr_offs);
>> +		of_property_read_u32(mtd->dev.of_node, "max_beb_per1024",
>> +				     &max_beb_per1024);
>> +		of_property_read_u32(mtd->dev.of_node, "ubi_num", &ubi_num);
>> +
>> +		mutex_lock(&ubi_devices_mutex);
>> +		err = ubi_attach_mtd_dev(mtd, ubi_num, vid_hdr_offs,
>> +					 max_beb_per1024);
>> +		mutex_unlock(&ubi_devices_mutex);
>> +		if (err < 0) {
>> +			pr_err("UBI error: cannot attach mtd%d",
>> +			       mtd->index);
>> +		}
>> +		put_mtd_device(mtd);
>> +	}
>> +
>>  	err = ubiblock_init();
>>  	if (err) {
>>  		pr_err("UBI error: block: cannot initialize, error %d", err);
>> -- 
>> 2.8.1
>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] ubi: open volumes define in device tree

[Ezequiel Garcia]

On 18 June 2016 at 16:17, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> This makes it possible to define a volume to open in device tree. This
> is helpful when a root file system is on a ubi layer. Without this
> patch it is only possible to device with with kernel command line
> parameters.
>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  Documentation/devicetree/bindings/mtd/ubi.txt | 22 +++++++++++++++++
>  drivers/mtd/ubi/build.c                       | 34 +++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/ubi.txt b/Documentation/devicetree/bindings/mtd/ubi.txt
> index 5fcd47e..f87a7d8 100644
> --- a/Documentation/devicetree/bindings/mtd/ubi.txt
> +++ b/Documentation/devicetree/bindings/mtd/ubi.txt
> @@ -13,6 +13,16 @@ Describe of a UBI layer in device tree.
>                         which have to be assigned to the newly created UBI
>                         device (assigned automatically by default)
>
> +Partitions on a UBI layer can be defined:
> +
> + - compatible:                 "ubi,volume"
> + - name:               name of the ubi volume to open
> + - ubi-mode:           mode of the ubi volume to open value:
> +                       UBI_READONLY:   1
> +                       UBI_READWRITE:  2
> +                       UBI_EXCLUSIVE:  3
> +                       UBI_METAONLY:   4
> +
>  Example:
>
>  partitions {
> @@ -29,5 +39,17 @@ partitions {
>                 label = "system_sw";
>                 reg = <0x1c0000 0xc800000>;
>                 compatible = "ubi,device";
> +
> +               rootfsA@0 {
> +                       compatible = "ubi,volume";
> +                       name = "rootfsA";
> +                       ubi-mode = <1>;
> +               };
> +
> +               data_vol@0 {
> +                       compatible = "ubi,volume";
> +                       name = "data_vol";
> +                       ubi-mode = <2>;
> +               };
>         };
>  };
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 16baeb5..1c22533 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -1,6 +1,7 @@
>  /*
>   * Copyright (c) International Business Machines Corp., 2006
>   * Copyright (c) Nokia Corporation, 2007
> + * Copyright (c) 2016 Hauke Mehrtens <hauke@hauke-m.de>
>   *

A tangential comment for this and other patches.

There's no need to add your copyright when you are submitting
small contributions.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 2/2] ubi: open volumes define in device tree

[Hauke Mehrtens]

On 06/24/2016 08:45 PM, Ezequiel Garcia wrote:
> On 18 June 2016 at 16:17, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>> This makes it possible to define a volume to open in device tree. This
>> is helpful when a root file system is on a ubi layer. Without this
>> patch it is only possible to device with with kernel command line
>> parameters.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  Documentation/devicetree/bindings/mtd/ubi.txt | 22 +++++++++++++++++
>>  drivers/mtd/ubi/build.c                       | 34 +++++++++++++++++++++++++++
>>  2 files changed, 56 insertions(+)
>>
....
>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
>> index 16baeb5..1c22533 100644
>> --- a/drivers/mtd/ubi/build.c
>> +++ b/drivers/mtd/ubi/build.c
>> @@ -1,6 +1,7 @@
>>  /*
>>   * Copyright (c) International Business Machines Corp., 2006
>>   * Copyright (c) Nokia Corporation, 2007
>> + * Copyright (c) 2016 Hauke Mehrtens <hauke@hauke-m.de>
>>   *
> 
> A tangential comment for this and other patches.
> 
> There's no need to add your copyright when you are submitting
> small contributions.
> 
The GPL v2 [0] says in section 1 a):
> You must cause the modified files to carry prominent notices stating
> that you changed the files and the date of any change.

My assumption is that my changes are copyrightable and then I have to
add a notice accordingly to the GPL. I do not really care about this and
can remove this.

Hauke

[0]: https://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Am 18.06.2016 um 21:17 schrieb Hauke Mehrtens:
> This makes it possible to open a ubi layer in device tree, this is
> helpful when the rootfs is on a ubi layer. It loops though all mtd
> partitions and mounts the partition which is compatible with
> "ubi,volume". The same was only possible with kernel command line
> arguments before.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  Documentation/devicetree/bindings/mtd/ubi.txt | 33 ++++++++++++++
>  drivers/mtd/ubi/block.c                       | 63 +++++++++++++++++++++++++++
>  2 files changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/ubi.txt

Some time ago I thought about an UBI DT binding too, but
I have been told that device tree is only for describing the hardware
and nothing else.
So I fear this will be rejected by DT folks.

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Hauke Mehrtens]

On 06/18/2016 09:30 PM, Richard Weinberger wrote:
> Am 18.06.2016 um 21:17 schrieb Hauke Mehrtens:
>> This makes it possible to open a ubi layer in device tree, this is
>> helpful when the rootfs is on a ubi layer. It loops though all mtd
>> partitions and mounts the partition which is compatible with
>> "ubi,volume". The same was only possible with kernel command line
>> arguments before.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  Documentation/devicetree/bindings/mtd/ubi.txt | 33 ++++++++++++++
>>  drivers/mtd/ubi/block.c                       | 63 +++++++++++++++++++++++++++
>>  2 files changed, 96 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mtd/ubi.txt
> 
> Some time ago I thought about an UBI DT binding too, but
> I have been told that device tree is only for describing the hardware
> and nothing else.
> So I fear this will be rejected by DT folks.

There are some devices on the market that are storing the root file
system in an ubi layer.
To make this work there are currently two options.
1. patch the kernel to specify which partitions to open in the code.
2. provide some kernel command line parameter which specifies what to open.

I would like to specify this in device tree.

Hauke

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Am 18.06.2016 um 21:35 schrieb Hauke Mehrtens:
> There are some devices on the market that are storing the root file
> system in an ubi layer.
> To make this work there are currently two options.
> 1. patch the kernel to specify which partitions to open in the code.
> 2. provide some kernel command line parameter which specifies what to open.
> 
> I would like to specify this in device tree.

What is wrong with command line parameters? If there is some parameter
the UBI module misses I'll happily add it.
You can also put your kernel command line into device tree.

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Hauke Mehrtens]

On 06/18/2016 09:46 PM, Richard Weinberger wrote:
> Am 18.06.2016 um 21:35 schrieb Hauke Mehrtens:
>> There are some devices on the market that are storing the root file
>> system in an ubi layer.
>> To make this work there are currently two options.
>> 1. patch the kernel to specify which partitions to open in the code.
>> 2. provide some kernel command line parameter which specifies what to open.
>>
>> I would like to specify this in device tree.
> 
> What is wrong with command line parameters? If there is some parameter
> the UBI module misses I'll happily add it.
> You can also put your kernel command line into device tree.

I would like to configure this in device tree. When I install a third
party firmware on a router normally the kernel command line is taken
from the boot loader and the device tree is appended to the kernel.

Hauke

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Am 19.06.2016 um 00:54 schrieb Hauke Mehrtens:
> On 06/18/2016 09:46 PM, Richard Weinberger wrote:
>> Am 18.06.2016 um 21:35 schrieb Hauke Mehrtens:
>>> There are some devices on the market that are storing the root file
>>> system in an ubi layer.
>>> To make this work there are currently two options.
>>> 1. patch the kernel to specify which partitions to open in the code.
>>> 2. provide some kernel command line parameter which specifies what to open.
>>>
>>> I would like to specify this in device tree.
>>
>> What is wrong with command line parameters? If there is some parameter
>> the UBI module misses I'll happily add it.
>> You can also put your kernel command line into device tree.
> 
> I would like to configure this in device tree. When I install a third
> party firmware on a router normally the kernel command line is taken
> from the boot loader and the device tree is appended to the kernel.

Let me get this straight, your use case is booting Linux in an environment
where you can't control the bootloader?
You can still set the command line by building it into the kernel.
And, as noted before, via device tree.

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi!

On Sat, Jun 18, 2016 at 09:35:42PM +0200, Hauke Mehrtens wrote:
> On 06/18/2016 09:30 PM, Richard Weinberger wrote:
> > Am 18.06.2016 um 21:17 schrieb Hauke Mehrtens:
> >> This makes it possible to open a ubi layer in device tree, this is
> >> helpful when the rootfs is on a ubi layer. It loops though all mtd
> >> partitions and mounts the partition which is compatible with
> >> "ubi,volume". The same was only possible with kernel command line
> >> arguments before.
> >>
> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> >> ---
> >>  Documentation/devicetree/bindings/mtd/ubi.txt | 33 ++++++++++++++
> >>  drivers/mtd/ubi/block.c                       | 63 +++++++++++++++++++++++++++
> >>  2 files changed, 96 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mtd/ubi.txt
> > 
> > Some time ago I thought about an UBI DT binding too, but
> > I have been told that device tree is only for describing the hardware
> > and nothing else.
> > So I fear this will be rejected by DT folks.
> 
> There are some devices on the market that are storing the root file
> system in an ubi layer.
> To make this work there are currently two options.
> 1. patch the kernel to specify which partitions to open in the code.
> 2. provide some kernel command line parameter which specifies what to open.
> 
> I would like to specify this in device tree.

In MBR there used to be an 'active' flag stored for each partition.
Maybe it'd be nice to introduce something similar to mark UBI volumes
to be the default rootfs.
Currently we solve this issue by convention: If a volume is named
'rootfs' it is automatically mounted during boot. Depending on the
filesystem in use a ubiblock device has to be created as well.
This is mostly just the continuation of the existing naming convention
of mtd partitions, a patch OpenWrt is carrying around for a long
while already.
To support the same on UBI, another set of patches was made.

I agree that there should be a way to pass this through the of_node
of the mtd partition which is defined in the device tree.
Selecting to-be-ubi-attached mtd partitions in device-tree would
replace patch:

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/490-ubi-auto-attach-mtd-device-named-ubi-or-data-on-boot.patch

To auto-select the rootfs, we currently carry another bunch of patches

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/491-ubi-auto-create-ubiblock-device-for-rootfs.patch

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/492-try-auto-mounting-ubi0-rootfs-in-init-do_mounts.c.patch

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch

This is more or less filesystem-agnostic and works fine as long as
there is only one volume called 'rootfs' and this volume is always
used as rootfs.

Dual-boot setups will need some way to pass the active rootfs volume to
the kernel. While I agree that this is possible by appending or
prepending to the cmdline string passed to the kernel, this either
limits the users' ability to manually specify the rootfs using the
cmdline or becomes a more complex task to only append/prepend the
cmdline in case the user-defined string doesn't already contain
relevant parameters...
Thus it'd be nicer to flag the default rootfs volume via the device-
tree.

However, I realize that other than mtd partitions, ubi volumes are
obviously dynamic and thus don't occur in the device-tree...
Modifying the volume on every boot to select whether it is active
also doesn't seem to be the right way. A non-standard cmdline
parameter is the most common solution implemented by many vendors,
that's also far from beautiful.


Just my 2 cents...


Cheers


Daniel



> 
> Hauke
> 

> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Am 19.06.2016 um 01:20 schrieb Daniel Golle:
> In MBR there used to be an 'active' flag stored for each partition.
> Maybe it'd be nice to introduce something similar to mark UBI volumes
> to be the default rootfs.
> Currently we solve this issue by convention: If a volume is named
> 'rootfs' it is automatically mounted during boot. Depending on the
> filesystem in use a ubiblock device has to be created as well.
> This is mostly just the continuation of the existing naming convention
> of mtd partitions, a patch OpenWrt is carrying around for a long
> while already.
> To support the same on UBI, another set of patches was made.

Sorry, I still have troubles to understand your use case.
Both of you seem to hate the kernel command line for reasons
I don't fully understand so far.

> I agree that there should be a way to pass this through the of_node
> of the mtd partition which is defined in the device tree.
> Selecting to-be-ubi-attached mtd partitions in device-tree would
> replace patch:
> 
> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/490-ubi-auto-attach-mtd-device-named-ubi-or-data-on-boot.patch

What is the need of this? Use use the kernel command line to tell UBI from which MTD to attach.

> To auto-select the rootfs, we currently carry another bunch of patches
> 
> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/491-ubi-auto-create-ubiblock-device-for-rootfs.patch

Same question here.

> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/492-try-auto-mounting-ubi0-rootfs-in-init-do_mounts.c.patch

Ditto.

> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch

Ditto.

> This is more or less filesystem-agnostic and works fine as long as
> there is only one volume called 'rootfs' and this volume is always
> used as rootfs.
> 
> Dual-boot setups will need some way to pass the active rootfs volume to
> the kernel. While I agree that this is possible by appending or
> prepending to the cmdline string passed to the kernel, this either
> limits the users' ability to manually specify the rootfs using the
> cmdline or becomes a more complex task to only append/prepend the
> cmdline in case the user-defined string doesn't already contain
> relevant parameters...

Sorry, but this is just a tooling problem and not to be addressed in the kernel.
There is also the possibility to use an initramfs (either as file or embedded in the kernel)
if the mount/attach logic becomes *really* complicated...

> Thus it'd be nicer to flag the default rootfs volume via the device-
> tree.

As I said, as far I'm informed device tree is for configuring Linux, it describes
the hardware. We also don't have LVM, DM or iSCSI bindings in DT. ;)
Maybe device tree folks will tell more...

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Am 19.06.2016 um 10:53 schrieb Richard Weinberger:
>> Thus it'd be nicer to flag the default rootfs volume via the device-
>> tree.
> 
> As I said, as far I'm informed device tree is for configuring Linux, it describes
> the hardware. We also don't have LVM, DM or iSCSI bindings in DT. ;)
> Maybe device tree folks will tell more...

Should be read "... is *not* for configuring Linux ...".

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi Richard,

On Sun, Jun 19, 2016 at 10:53:42AM +0200, Richard Weinberger wrote:
> Am 19.06.2016 um 01:20 schrieb Daniel Golle:
> > In MBR there used to be an 'active' flag stored for each partition.
> > Maybe it'd be nice to introduce something similar to mark UBI volumes
> > to be the default rootfs.
> > Currently we solve this issue by convention: If a volume is named
> > 'rootfs' it is automatically mounted during boot. Depending on the
> > filesystem in use a ubiblock device has to be created as well.
> > This is mostly just the continuation of the existing naming convention
> > of mtd partitions, a patch OpenWrt is carrying around for a long
> > while already.
> > To support the same on UBI, another set of patches was made.
> 
> Sorry, I still have troubles to understand your use case.
> Both of you seem to hate the kernel command line for reasons
> I don't fully understand so far.

I like the kernel command line a lot :)
And *being the end-user* I like to be the one defining and modifying it
according to my current needs. For that reason I do *not* like firmware
bootloaders to prepend/append or even overwrite the cmdline for things
like selecting the rootfs, because usually that restricts the use of
the relevant parameters by device owners.
Imagine: The device runs a bootloader which already sets rootfs= or
overwrites the cmdline of the stock firmware. How will an end-user
who cannot change the bootloader use an alternative OS which uses e.g.
a USB pendrive as it's rootfs? This is why we end up with things like
renaming kernel cmdline parameters in alternative firmware projects,
e.g. rootfs2=...., so end-users can re-gain access to cmdline
parameters augmented by the bootloader.
For that reason I believe that using the cmdline to pass
*non-user-defined* details from the bootloader to the kernel is just
not such a nice thing to do.

> 
> > I agree that there should be a way to pass this through the of_node
> > of the mtd partition which is defined in the device tree.
> > Selecting to-be-ubi-attached mtd partitions in device-tree would
> > replace patch:
> > 
> > https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/490-ubi-auto-attach-mtd-device-named-ubi-or-data-on-boot.patch
> 
> What is the need of this? Use use the kernel command line to tell UBI from which MTD to attach.

The same kernel gets used on many devices having different $vendor
mtd-partition layouts. A way other than the kernel cmdline allows
to specify the default behaviour without restricting the user to
manually use those cmdline options.

> 
> > To auto-select the rootfs, we currently carry another bunch of patches
> > 
> > https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/491-ubi-auto-create-ubiblock-device-for-rootfs.patch
> 
> Same question here.
> 
> > https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/492-try-auto-mounting-ubi0-rootfs-in-init-do_mounts.c.patch
> 
> Ditto.
> 
> > https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch
> 
> Ditto.
> 

Same arguments as above. In addition, we do not want to hard-code the
filesystem type used for the rootfs volume, as it can either be UBIFS
or a read-only filesystem needing a ubiblock device. Thus we would
need the bootloader to know which filesystem *type* is being used and
then decide wether to pass 'rootfs=ubiX:Y' or
'ubiblock=... rootfs=/dev/ubiblock0'.


> > This is more or less filesystem-agnostic and works fine as long as
> > there is only one volume called 'rootfs' and this volume is always
> > used as rootfs.
> > 
> > Dual-boot setups will need some way to pass the active rootfs volume to
> > the kernel. While I agree that this is possible by appending or
> > prepending to the cmdline string passed to the kernel, this either
> > limits the users' ability to manually specify the rootfs using the
> > cmdline or becomes a more complex task to only append/prepend the
> > cmdline in case the user-defined string doesn't already contain
> > relevant parameters...
> 
> Sorry, but this is just a tooling problem and not to be addressed in the kernel.
> There is also the possibility to use an initramfs (either as file or embedded in the kernel)
> if the mount/attach logic becomes *really* complicated...
> 
> > Thus it'd be nicer to flag the default rootfs volume via the device-
> > tree.
> 
> As I said, as far I'm informed device tree is for configuring Linux, it describes
> the hardware. We also don't have LVM, DM or iSCSI bindings in DT. ;)
> Maybe device tree folks will tell more...

But we do have MTD and MTD partitions in DT. To me it'd feel more
consistent if MTD devices, partitioning and which MTD partition(s) to
use as UBI would be defined in the same place.


Cheers


Daniel

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Daniel,

Am 19.06.2016 um 13:25 schrieb Daniel Golle:
> Hi Richard,
> 
> On Sun, Jun 19, 2016 at 10:53:42AM +0200, Richard Weinberger wrote:
>> Am 19.06.2016 um 01:20 schrieb Daniel Golle:
>>> In MBR there used to be an 'active' flag stored for each partition.
>>> Maybe it'd be nice to introduce something similar to mark UBI volumes
>>> to be the default rootfs.
>>> Currently we solve this issue by convention: If a volume is named
>>> 'rootfs' it is automatically mounted during boot. Depending on the
>>> filesystem in use a ubiblock device has to be created as well.
>>> This is mostly just the continuation of the existing naming convention
>>> of mtd partitions, a patch OpenWrt is carrying around for a long
>>> while already.
>>> To support the same on UBI, another set of patches was made.
>>
>> Sorry, I still have troubles to understand your use case.
>> Both of you seem to hate the kernel command line for reasons
>> I don't fully understand so far.
> 
> I like the kernel command line a lot :)
> And *being the end-user* I like to be the one defining and modifying it
> according to my current needs. For that reason I do *not* like firmware
> bootloaders to prepend/append or even overwrite the cmdline for things
> like selecting the rootfs, because usually that restricts the use of
> the relevant parameters by device owners.
> Imagine: The device runs a bootloader which already sets rootfs= or
> overwrites the cmdline of the stock firmware. How will an end-user
> who cannot change the bootloader use an alternative OS which uses e.g.
> a USB pendrive as it's rootfs? This is why we end up with things like
> renaming kernel cmdline parameters in alternative firmware projects,
> e.g. rootfs2=...., so end-users can re-gain access to cmdline
> parameters augmented by the bootloader.

So, the use case is being able to use a custom OS on a "locked"
device where the bootloader hates you?

But you still can override the cmdline supplied by the bootloader
by adding your cmdline into the device tree, right?

> For that reason I believe that using the cmdline to pass
> *non-user-defined* details from the bootloader to the kernel is just
> not such a nice thing to do.

Well, this is not how the cmdline works. We don't have such
an separation.
Some distros use hacks such as "showopts" to have such a split.


>>
>>> I agree that there should be a way to pass this through the of_node
>>> of the mtd partition which is defined in the device tree.
>>> Selecting to-be-ubi-attached mtd partitions in device-tree would
>>> replace patch:
>>>
>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/490-ubi-auto-attach-mtd-device-named-ubi-or-data-on-boot.patch
>>
>> What is the need of this? Use use the kernel command line to tell UBI from which MTD to attach.
> 
> The same kernel gets used on many devices having different $vendor
> mtd-partition layouts. A way other than the kernel cmdline allows
> to specify the default behaviour without restricting the user to
> manually use those cmdline options.

You can put the cmdline into the per-device device tree.
This is the concept of a multi-device kernel. One kernel and
many device trees.

>>
>>> To auto-select the rootfs, we currently carry another bunch of patches
>>>
>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/491-ubi-auto-create-ubiblock-device-for-rootfs.patch
>>
>> Same question here.
>>
>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/492-try-auto-mounting-ubi0-rootfs-in-init-do_mounts.c.patch
>>
>> Ditto.
>>
>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch
>>
>> Ditto.
>>
> 
> Same arguments as above. In addition, we do not want to hard-code the
> filesystem type used for the rootfs volume, as it can either be UBIFS
> or a read-only filesystem needing a ubiblock device. Thus we would
> need the bootloader to know which filesystem *type* is being used and
> then decide wether to pass 'rootfs=ubiX:Y' or
> 'ubiblock=... rootfs=/dev/ubiblock0'.

What is wrong with having a very minimal initramfs to do such an
auto discovery logic?

> 
>>> This is more or less filesystem-agnostic and works fine as long as
>>> there is only one volume called 'rootfs' and this volume is always
>>> used as rootfs.
>>>
>>> Dual-boot setups will need some way to pass the active rootfs volume to
>>> the kernel. While I agree that this is possible by appending or
>>> prepending to the cmdline string passed to the kernel, this either
>>> limits the users' ability to manually specify the rootfs using the
>>> cmdline or becomes a more complex task to only append/prepend the
>>> cmdline in case the user-defined string doesn't already contain
>>> relevant parameters...
>>
>> Sorry, but this is just a tooling problem and not to be addressed in the kernel.
>> There is also the possibility to use an initramfs (either as file or embedded in the kernel)
>> if the mount/attach logic becomes *really* complicated...
>>
>>> Thus it'd be nicer to flag the default rootfs volume via the device-
>>> tree.
>>
>> As I said, as far I'm informed device tree is for configuring Linux, it describes
>> the hardware. We also don't have LVM, DM or iSCSI bindings in DT. ;)
>> Maybe device tree folks will tell more...
> 
> But we do have MTD and MTD partitions in DT. To me it'd feel more
> consistent if MTD devices, partitioning and which MTD partition(s) to
> use as UBI would be defined in the same place.

This is something you need to discuss wit DT folks.
I'm not per se against UBI DT bindings it just does not feel right
and contradicts my understanding of it.

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi Richard,

On Sun, Jun 19, 2016 at 02:02:20PM +0200, Richard Weinberger wrote:
> Daniel,
> 
> Am 19.06.2016 um 13:25 schrieb Daniel Golle:
> > Hi Richard,
> > 
> > On Sun, Jun 19, 2016 at 10:53:42AM +0200, Richard Weinberger wrote:
> >> Am 19.06.2016 um 01:20 schrieb Daniel Golle:
> >>> In MBR there used to be an 'active' flag stored for each partition.
> >>> Maybe it'd be nice to introduce something similar to mark UBI volumes
> >>> to be the default rootfs.
> >>> Currently we solve this issue by convention: If a volume is named
> >>> 'rootfs' it is automatically mounted during boot. Depending on the
> >>> filesystem in use a ubiblock device has to be created as well.
> >>> This is mostly just the continuation of the existing naming convention
> >>> of mtd partitions, a patch OpenWrt is carrying around for a long
> >>> while already.
> >>> To support the same on UBI, another set of patches was made.
> >>
> >> Sorry, I still have troubles to understand your use case.
> >> Both of you seem to hate the kernel command line for reasons
> >> I don't fully understand so far.
> > 
> > I like the kernel command line a lot :)
> > And *being the end-user* I like to be the one defining and modifying it
> > according to my current needs. For that reason I do *not* like firmware
> > bootloaders to prepend/append or even overwrite the cmdline for things
> > like selecting the rootfs, because usually that restricts the use of
> > the relevant parameters by device owners.
> > Imagine: The device runs a bootloader which already sets rootfs= or
> > overwrites the cmdline of the stock firmware. How will an end-user
> > who cannot change the bootloader use an alternative OS which uses e.g.
> > a USB pendrive as it's rootfs? This is why we end up with things like
> > renaming kernel cmdline parameters in alternative firmware projects,
> > e.g. rootfs2=...., so end-users can re-gain access to cmdline
> > parameters augmented by the bootloader.
> 
> So, the use case is being able to use a custom OS on a "locked"
> device where the bootloader hates you?

At least that's a valid use-case which should be considered, but not
the only. Some people also just don't want to modify the bootloader
(especially on headless systems) so they don't risk to brick it.
Many devices offer some rescue firmware flashing methods through
TFTP -- however, you cannot change bootloader environment variables
like the cmdline in that way and even if you could, the loader might
still patch the root=... parameter in the way needed for their stock
firmware.

> 
> But you still can override the cmdline supplied by the bootloader
> by adding your cmdline into the device tree, right?
> 
> > For that reason I believe that using the cmdline to pass
> > *non-user-defined* details from the bootloader to the kernel is just
> > not such a nice thing to do.
> 
> Well, this is not how the cmdline works. We don't have such
> an separation.
> Some distros use hacks such as "showopts" to have such a split.

/me researching 'showopts' which I had never heard of before.
Ok, this is for grub running interactively on x86, probably not the
best reference if it comes to small headless embedded systems...
There is no such a thing as a proprietary board-specific version
of grub, so on x86 the whole problem just doesn't exist and showopts
is really just cosmetics. However, on most ARM, MIPS or PPC based
boards users may not have the option or may not want to change the
bootloader. As the whole reason why some distro-kernels then decide
to override the bootloader cmdline is *because* $vendor decided to
make her bootloader pass the root=... and similar basic options and
that screws up the rootfs of the 3rd-party Linux.
Thus I believe we shouldn't encourage the use of the kernel cmdline
in places the user cannot influence it (like in most $vendor variants
of U-Boot where the cmdline is patched somewhere behind the scenes
rather than just using the environment variable).


> 
> 
> >>
> >>> I agree that there should be a way to pass this through the of_node
> >>> of the mtd partition which is defined in the device tree.
> >>> Selecting to-be-ubi-attached mtd partitions in device-tree would
> >>> replace patch:
> >>>
> >>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/490-ubi-auto-attach-mtd-device-named-ubi-or-data-on-boot.patch
> >>
> >> What is the need of this? Use use the kernel command line to tell UBI from which MTD to attach.
> > 
> > The same kernel gets used on many devices having different $vendor
> > mtd-partition layouts. A way other than the kernel cmdline allows
> > to specify the default behaviour without restricting the user to
> > manually use those cmdline options.
> 
> You can put the cmdline into the per-device device tree.
> This is the concept of a multi-device kernel. One kernel and
> many device trees.

Exactly true. However, as the mtd partitions are defined in the tree
itself rather than using 'mtdparts=...' in the cmdline built-into the
device-tree, it'd be nice to also select that a specific partition
should be ubiattached.

> 
> >>
> >>> To auto-select the rootfs, we currently carry another bunch of patches
> >>>
> >>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/491-ubi-auto-create-ubiblock-device-for-rootfs.patch
> >>
> >> Same question here.
> >>
> >>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/492-try-auto-mounting-ubi0-rootfs-in-init-do_mounts.c.patch
> >>
> >> Ditto.
> >>
> >>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch
> >>
> >> Ditto.
> >>
> > 
> > Same arguments as above. In addition, we do not want to hard-code the
> > filesystem type used for the rootfs volume, as it can either be UBIFS
> > or a read-only filesystem needing a ubiblock device. Thus we would
> > need the bootloader to know which filesystem *type* is being used and
> > then decide wether to pass 'rootfs=ubiX:Y' or
> > 'ubiblock=... rootfs=/dev/ubiblock0'.
> 
> What is wrong with having a very minimal initramfs to do such an
> auto discovery logic?

Sorry, but we are not talking about traditional desktop or server
systems. OpenWrt/LEDE runs on devices with as little as 32 MB of RAM
and 4 MB of Flash. Devices with UBI will typically have at least
32 MB of NAND Flash, but still not necessarily a lot of RAM.
Have a look at
https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/dts/BTHOMEHUBV2B.dts
for a good example of the type of hardware we are talking about.

Apart from that we already automatically generate initramfs-based
images for devices to be used in situations where you cannot or don't
want to touch the devices' flash.
As there wasn't a need to embed a initramfs for anything as simple
as mounting the rootfs, I reckon it would be hard to argue that we
should have an initramfs for all systems potentially using UBI just
so we won't need those ~ 100 lines of C code added to the kernel to
auto-select the root device *and* filesystem for both MTD and UBI.


> 
> > 
> >>> This is more or less filesystem-agnostic and works fine as long as
> >>> there is only one volume called 'rootfs' and this volume is always
> >>> used as rootfs.
> >>>
> >>> Dual-boot setups will need some way to pass the active rootfs volume to
> >>> the kernel. While I agree that this is possible by appending or
> >>> prepending to the cmdline string passed to the kernel, this either
> >>> limits the users' ability to manually specify the rootfs using the
> >>> cmdline or becomes a more complex task to only append/prepend the
> >>> cmdline in case the user-defined string doesn't already contain
> >>> relevant parameters...
> >>
> >> Sorry, but this is just a tooling problem and not to be addressed in the kernel.
> >> There is also the possibility to use an initramfs (either as file or embedded in the kernel)
> >> if the mount/attach logic becomes *really* complicated...
> >>
> >>> Thus it'd be nicer to flag the default rootfs volume via the device-
> >>> tree.
> >>
> >> As I said, as far I'm informed device tree is for configuring Linux, it describes
> >> the hardware. We also don't have LVM, DM or iSCSI bindings in DT. ;)
> >> Maybe device tree folks will tell more...
> > 
> > But we do have MTD and MTD partitions in DT. To me it'd feel more
> > consistent if MTD devices, partitioning and which MTD partition(s) to
> > use as UBI would be defined in the same place.
> 
> This is something you need to discuss wit DT folks.
> I'm not per se against UBI DT bindings it just does not feel right
> and contradicts my understanding of it.

I agree with your view when it comes to describing UBI volumes
in the device tree. That's wrong just as it would be wrong to add
DT bindings for LVM2.
As said previously, the situation is different for flash chips having
partitions defined for use with UBI on some boards.



Cheers


Daniel

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Daniel,

Am 19.06.2016 um 15:05 schrieb Daniel Golle:
>>> The same kernel gets used on many devices having different $vendor
>>> mtd-partition layouts. A way other than the kernel cmdline allows
>>> to specify the default behaviour without restricting the user to
>>> manually use those cmdline options.
>>
>> You can put the cmdline into the per-device device tree.
>> This is the concept of a multi-device kernel. One kernel and
>> many device trees.
> 
> Exactly true. However, as the mtd partitions are defined in the tree
> itself rather than using 'mtdparts=...' in the cmdline built-into the
> device-tree, it'd be nice to also select that a specific partition
> should be ubiattached.

DT already offer all you need and nothing hinders you from
using mtdparts=.

>>
>>>>
>>>>> To auto-select the rootfs, we currently carry another bunch of patches
>>>>>
>>>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/491-ubi-auto-create-ubiblock-device-for-rootfs.patch
>>>>
>>>> Same question here.
>>>>
>>>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/492-try-auto-mounting-ubi0-rootfs-in-init-do_mounts.c.patch
>>>>
>>>> Ditto.
>>>>
>>>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch
>>>>
>>>> Ditto.
>>>>
>>>
>>> Same arguments as above. In addition, we do not want to hard-code the
>>> filesystem type used for the rootfs volume, as it can either be UBIFS
>>> or a read-only filesystem needing a ubiblock device. Thus we would
>>> need the bootloader to know which filesystem *type* is being used and
>>> then decide wether to pass 'rootfs=ubiX:Y' or
>>> 'ubiblock=... rootfs=/dev/ubiblock0'.
>>
>> What is wrong with having a very minimal initramfs to do such an
>> auto discovery logic?
> 
> Sorry, but we are not talking about traditional desktop or server
> systems. OpenWrt/LEDE runs on devices with as little as 32 MB of RAM
> and 4 MB of Flash. Devices with UBI will typically have at least
> 32 MB of NAND Flash, but still not necessarily a lot of RAM.
> Have a look at
> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/dts/BTHOMEHUBV2B.dts
> for a good example of the type of hardware we are talking about.

I didn't recommend adding glibc+systemd in the initramfs. It can be very, very small
and trivial.
At least it would be much more clean than your pile of not so fancy MTD/UBI patches.

> Apart from that we already automatically generate initramfs-based
> images for devices to be used in situations where you cannot or don't
> want to touch the devices' flash.

The initramfs can be part of the kernel image. No additional file needed.

>> This is something you need to discuss wit DT folks.
>> I'm not per se against UBI DT bindings it just does not feel right
>> and contradicts my understanding of it.
> 
> I agree with your view when it comes to describing UBI volumes
> in the device tree. That's wrong just as it would be wrong to add
> DT bindings for LVM2.
> As said previously, the situation is different for flash chips having
> partitions defined for use with UBI on some boards.

Everything can be done with the existing framework. DT binding maybe be nice
for your use case but I fear it is not applicable.

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi Richard,


On Sun, Jun 19, 2016 at 03:19:38PM +0200, Richard Weinberger wrote:
> Daniel,
> 
> Am 19.06.2016 um 15:05 schrieb Daniel Golle:
> >>> The same kernel gets used on many devices having different $vendor
> >>> mtd-partition layouts. A way other than the kernel cmdline allows
> >>> to specify the default behaviour without restricting the user to
> >>> manually use those cmdline options.
> >>
> >> You can put the cmdline into the per-device device tree.
> >> This is the concept of a multi-device kernel. One kernel and
> >> many device trees.
> > 
> > Exactly true. However, as the mtd partitions are defined in the tree
> > itself rather than using 'mtdparts=...' in the cmdline built-into the
> > device-tree, it'd be nice to also select that a specific partition
> > should be ubiattached.
> 
> DT already offer all you need and nothing hinders you from
> using mtdparts=.

I know. However, it's much nicer to define partitions for a specific
flash chip inside the device tree (I've sent a link to the DTS of the
BTHOMEHUBV2B to illustrate that, please have a look at it and tell
me if you'd really like that partitioning currently stored as
device-tree nodes to be migrated back to 'mtdparts=...'.)


> 
> >>
> >>>>
> >>>>> To auto-select the rootfs, we currently carry another bunch of patches
> >>>>>
> >>>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/491-ubi-auto-create-ubiblock-device-for-rootfs.patch
> >>>>
> >>>> Same question here.
> >>>>
> >>>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/492-try-auto-mounting-ubi0-rootfs-in-init-do_mounts.c.patch
> >>>>
> >>>> Ditto.
> >>>>
> >>>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch
> >>>>
> >>>> Ditto.
> >>>>
> >>>
> >>> Same arguments as above. In addition, we do not want to hard-code the
> >>> filesystem type used for the rootfs volume, as it can either be UBIFS
> >>> or a read-only filesystem needing a ubiblock device. Thus we would
> >>> need the bootloader to know which filesystem *type* is being used and
> >>> then decide wether to pass 'rootfs=ubiX:Y' or
> >>> 'ubiblock=... rootfs=/dev/ubiblock0'.
> >>
> >> What is wrong with having a very minimal initramfs to do such an
> >> auto discovery logic?
> > 
> > Sorry, but we are not talking about traditional desktop or server
> > systems. OpenWrt/LEDE runs on devices with as little as 32 MB of RAM
> > and 4 MB of Flash. Devices with UBI will typically have at least
> > 32 MB of NAND Flash, but still not necessarily a lot of RAM.
> > Have a look at
> > https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/dts/BTHOMEHUBV2B.dts
> > for a good example of the type of hardware we are talking about.
> 
> I didn't recommend adding glibc+systemd in the initramfs. It can be very, very small
> and trivial.

Again, please understand that the whole production firmware can be as
small as 3.5 megs for kernel and rootfs. Of course we never use glibc
or systemd anywhere.
What you imagine to be the ways to make a initramfs really small is
what we already do for *all* our system, ie. we use busybox on top of
a very tiny libc.

> At least it would be much more clean than your pile of not so fancy MTD/UBI patches.
> 

In the end, do_mounts.c is already running in user-space, just the
source-code happens to be part of the kernel tree. Sure, that could
become a seperate executable binary which mounts the rootfs and then
passes control to /sbin/init. On the other hand that would add quite
some redundant infrastructure, because we already got ROOT_DEV and lots
of automagic mounting going on in the kernel.


> > Apart from that we already automatically generate initramfs-based
> > images for devices to be used in situations where you cannot or don't
> > want to touch the devices' flash.
> 
> The initramfs can be part of the kernel image. No additional file needed.

That's excatctly what we've already been doing for initramfs-only
images, see e.g.
http://downloads.lede-project.org/snapshots/targets/lantiq/xway/lede-lantiq-xway-BTHOMEHUBV2B-uImage-initramfs

So if we need an initramfs to mount the on-flash rootfs and *another*
initfamfs containing the whole system for other cases, things become
even more fishy on our side. But yes, I'm aware that this would be
possible.


> 
> >> This is something you need to discuss wit DT folks.
> >> I'm not per se against UBI DT bindings it just does not feel right
> >> and contradicts my understanding of it.
> > 
> > I agree with your view when it comes to describing UBI volumes
> > in the device tree. That's wrong just as it would be wrong to add
> > DT bindings for LVM2.
> > As said previously, the situation is different for flash chips having
> > partitions defined for use with UBI on some boards.
> 
> Everything can be done with the existing framework. DT binding maybe be nice
> for your use case but I fear it is not applicable.

Yes, it can be done. But you would have to define your flash chips in
device-tree, you may define the mtd partitions either in DT or by using
mtdparts=... in the cmdline. Now to define which MTD device to
ubiattach you will have to use ubi.mtd=X on the cmdline. If you defined
your partitions inside the of_node of the flash chip, changes there
(which do happen) may change the index of the partition you want to
ubi-attach. C'mon. This *is* messy. Just having a compatible-flag to
attach the partition would already greatly improve things.

Hauke: Maybe we should discuss the two patches individually, as I feel
that ubiattaching specific mtd partitions flagged in DT is quite
straight forward, while specifying volumes inside the ubi device might
be out-of-scope.


Cheers


Daniel



> 
> Thanks,
> //richard
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Daniel,

Am 19.06.2016 um 16:09 schrieb Daniel Golle:
>> DT already offer all you need and nothing hinders you from
>> using mtdparts=.
> 
> I know. However, it's much nicer to define partitions for a specific
> flash chip inside the device tree (I've sent a link to the DTS of the
> BTHOMEHUBV2B to illustrate that, please have a look at it and tell
> me if you'd really like that partitioning currently stored as
> device-tree nodes to be migrated back to 'mtdparts=...'.)

I'd be fine with having the partitions in DT and the UBI kernel parameters
as kernel cmdline also in DT.

>> I didn't recommend adding glibc+systemd in the initramfs. It can be very, very small
>> and trivial.
> 
> Again, please understand that the whole production firmware can be as
> small as 3.5 megs for kernel and rootfs. Of course we never use glibc
> or systemd anywhere.

I know these use cases, I design and maintain such stuff for customers.

> Yes, it can be done. But you would have to define your flash chips in
> device-tree, you may define the mtd partitions either in DT or by using
> mtdparts=... in the cmdline. Now to define which MTD device to
> ubiattach you will have to use ubi.mtd=X on the cmdline. If you defined
> your partitions inside the of_node of the flash chip, changes there
> (which do happen) may change the index of the partition you want to
> ubi-attach. C'mon. This *is* messy. Just having a compatible-flag to
> attach the partition would already greatly improve things.

You mean marking a MTD partition in DT and UBI will attach from it?
That makes sense.

To sum up, I asked a lot of questions to understand your use case(s).
Everything you described can be done with existing facilities.
But I agree that at least some UBI DT machinery would be nice to have
although we need to check with DT folks first.
At least marking an MTD partition should be fine, hopefully.

Brian, Boris, what do you think?

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi Richard,

On Sun, Jun 19, 2016 at 04:35:05PM +0200, Richard Weinberger wrote:
> Daniel,
> 
> Am 19.06.2016 um 16:09 schrieb Daniel Golle:
> >> DT already offer all you need and nothing hinders you from
> >> using mtdparts=.
> > 
> > I know. However, it's much nicer to define partitions for a specific
> > flash chip inside the device tree (I've sent a link to the DTS of the
> > BTHOMEHUBV2B to illustrate that, please have a look at it and tell
> > me if you'd really like that partitioning currently stored as
> > device-tree nodes to be migrated back to 'mtdparts=...'.)
> 
> I'd be fine with having the partitions in DT and the UBI kernel parameters
> as kernel cmdline also in DT.

That's the current state of affairs. I'd just want to have it in one
place, so if the MTD partitioning gets updated one would see easily how
that affects UBI. For that, I don't think it's good enough to have
'ubi.mtd=X' in the cmdline (in DT or wherever, it's a single string in
a different place than the MTD partitioning if people use DT for MTD
partitioning).

> 
> >> I didn't recommend adding glibc+systemd in the initramfs. It can be very, very small
> >> and trivial.
> > 
> > Again, please understand that the whole production firmware can be as
> > small as 3.5 megs for kernel and rootfs. Of course we never use glibc
> > or systemd anywhere.
> 
> I know these use cases, I design and maintain such stuff for customers.
> 
> > Yes, it can be done. But you would have to define your flash chips in
> > device-tree, you may define the mtd partitions either in DT or by using
> > mtdparts=... in the cmdline. Now to define which MTD device to
> > ubiattach you will have to use ubi.mtd=X on the cmdline. If you defined
> > your partitions inside the of_node of the flash chip, changes there
> > (which do happen) may change the index of the partition you want to
> > ubi-attach. C'mon. This *is* messy. Just having a compatible-flag to
> > attach the partition would already greatly improve things.
> 
> You mean marking a MTD partition in DT and UBI will attach from it?
> That makes sense.

Yes. Currently we just use a naming convention (the first MTD partition
named 'ubi' will be auto-attached), that's obviously not very clean...

> 
> To sum up, I asked a lot of questions to understand your use case(s).
> Everything you described can be done with existing facilities.
> But I agree that at least some UBI DT machinery would be nice to have
> although we need to check with DT folks first.
> At least marking an MTD partition should be fine, hopefully.

Great. That'd already greatly improve things.

I can see that sooner or later we will need some way to reference UBI
volumes in DT, independently of the whole rootfs selection. Vendors do
use UBI volumes also for things like WiFi EEPROM data. Currentlhy, we
extract that in user-space and save a copy in /lib/firmware/... for
things more crazy than plain offsets inside MTD partitions.


Cheers


Daniel


> 
> Brian, Boris, what do you think?
> 
> Thanks,
> //richard
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Am 19.06.2016 um 17:24 schrieb Daniel Golle:
>> You mean marking a MTD partition in DT and UBI will attach from it?
>> That makes sense.
> 
> Yes. Currently we just use a naming convention (the first MTD partition
> named 'ubi' will be auto-attached), that's obviously not very clean...

I was about to reply to my own mail that you can still attach by name.
Boris reminded me of that, I forgot that feature.^^
Why is it not clean?

>>
>> To sum up, I asked a lot of questions to understand your use case(s).
>> Everything you described can be done with existing facilities.
>> But I agree that at least some UBI DT machinery would be nice to have
>> although we need to check with DT folks first.
>> At least marking an MTD partition should be fine, hopefully.
> 
> Great. That'd already greatly improve things.

How is that better than attach by name? You mark the to be attached
MTD by its name...

> I can see that sooner or later we will need some way to reference UBI
> volumes in DT, independently of the whole rootfs selection. Vendors do
> use UBI volumes also for things like WiFi EEPROM data. Currentlhy, we
> extract that in user-space and save a copy in /lib/firmware/... for
> things more crazy than plain offsets inside MTD partitions.

Really, please append a decent root= kernel parameter.

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi Richard,

On Sun, Jun 19, 2016 at 05:31:04PM +0200, Richard Weinberger wrote:
> Am 19.06.2016 um 17:24 schrieb Daniel Golle:
> >> You mean marking a MTD partition in DT and UBI will attach from it?
> >> That makes sense.
> > 
> > Yes. Currently we just use a naming convention (the first MTD partition
> > named 'ubi' will be auto-attached), that's obviously not very clean...
> 
> I was about to reply to my own mail that you can still attach by name.
> Boris reminded me of that, I forgot that feature.^^
> Why is it not clean?

That's nice and I also didn't know that.
It's still not perfect because I got to add it to the cmdline (in DT),
and should have it only on devices where a partition with that given
name actually exists. A flag for the mtd partition would still be
nicer.

> 
> >>
> >> To sum up, I asked a lot of questions to understand your use case(s).
> >> Everything you described can be done with existing facilities.
> >> But I agree that at least some UBI DT machinery would be nice to have
> >> although we need to check with DT folks first.
> >> At least marking an MTD partition should be fine, hopefully.
> > 
> > Great. That'd already greatly improve things.
> 
> How is that better than attach by name? You mark the to be attached
> MTD by its name...

See above. We'd then still need to have that ubi.mtd=name in the
cmdline for NAND devices using UBI and *not* have it for other devices
within the same family which may use SPI or NOR flash without UBI.
I'd prefer to have one place inside the device-tree for everything
flash-storage related, eg.
&nand {
	status = "okay";
	partition@0 {
		label = "boot";
		reg = <0x00000000 0x00e00000>;
		read-only;
	};

	partition@e00000 {
		label = "data";
		compatible = "ubi,device";
		reg = <0x00e00000 0x07200000>;
	};
};

> 
> > I can see that sooner or later we will need some way to reference UBI
> > volumes in DT, independently of the whole rootfs selection. Vendors do
> > use UBI volumes also for things like WiFi EEPROM data. Currentlhy, we
> > extract that in user-space and save a copy in /lib/firmware/... for
> > things more crazy than plain offsets inside MTD partitions.
> 
> Really, please append a decent root= kernel parameter.

Maybe we just focus to solve one problem at a time :)
Anyway, it'd be great if 'root=' was possible and flexible enough.
However, as we support several filesystem types (ubifs vs. squashfs)
this is not straight forward.

ubi.mtd=mtdname root=ubiX:volname rootfstype=ubifs
vs.
ubi.mtd=mtdname ubiblock=... root=/dev/ubiblockX_Y

And we wouldn't want two seperate DT files for those cases nor can we
rely in bootloader support or any other means to differentiate them.

That was part of the initial motivation for having those
not-so-fancy patches in first place... (And yes, others have initramfs
pre-init systems, things like real_root=... in their cmdline and other
also not-that-fancy solutions to the problem)


Cheers


Daniel

> 
> Thanks,
> //richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Boris Brezillon]

On Sun, 19 Jun 2016 18:13:36 +0200
Daniel Golle <daniel@makrotopia.org> wrote:

> Hi Richard,
> 
> On Sun, Jun 19, 2016 at 05:31:04PM +0200, Richard Weinberger wrote:
> > Am 19.06.2016 um 17:24 schrieb Daniel Golle:  
> > >> You mean marking a MTD partition in DT and UBI will attach from it?
> > >> That makes sense.  
> > > 
> > > Yes. Currently we just use a naming convention (the first MTD partition
> > > named 'ubi' will be auto-attached), that's obviously not very clean...  
> > 
> > I was about to reply to my own mail that you can still attach by name.
> > Boris reminded me of that, I forgot that feature.^^
> > Why is it not clean?  
> 
> That's nice and I also didn't know that.
> It's still not perfect because I got to add it to the cmdline (in DT),
> and should have it only on devices where a partition with that given
> name actually exists. A flag for the mtd partition would still be
> nicer.
> 
> >   
> > >>
> > >> To sum up, I asked a lot of questions to understand your use case(s).
> > >> Everything you described can be done with existing facilities.
> > >> But I agree that at least some UBI DT machinery would be nice to have
> > >> although we need to check with DT folks first.
> > >> At least marking an MTD partition should be fine, hopefully.  
> > > 
> > > Great. That'd already greatly improve things.  
> > 
> > How is that better than attach by name? You mark the to be attached
> > MTD by its name...  
> 
> See above. We'd then still need to have that ubi.mtd=name in the
> cmdline for NAND devices using UBI and *not* have it for other devices
> within the same family which may use SPI or NOR flash without UBI.
> I'd prefer to have one place inside the device-tree for everything
> flash-storage related, eg.
> &nand {
> 	status = "okay";
> 	partition@0 {
> 		label = "boot";
> 		reg = <0x00000000 0x00e00000>;
> 		read-only;
> 	};
> 
> 	partition@e00000 {
> 		label = "data";
> 		compatible = "ubi,device";

So, if we follow your logic we should also have

		compatible = "jffs2,file-system";

Because JFFS2 is an MTD user, just as UBI is.

Let's see what Rob and other DT maintainers think about that.

Still, it seems to me that you're trying to solve a problem in the
kernel when it should actually be solved in an upper layer.

Another option would be to try attaching UBI (along with all possible
MTD users) to all the the MTD partitions. That's what's done for block
filesystems when rootfstype is not specified.


> 		reg = <0x00e00000 0x07200000>;
> 	};
> };

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi Boris,

On Sun, Jun 19, 2016 at 06:53:43PM +0200, Boris Brezillon wrote:
> On Sun, 19 Jun 2016 18:13:36 +0200
> Daniel Golle <daniel@makrotopia.org> wrote:
> 
> > Hi Richard,
> > 
> > On Sun, Jun 19, 2016 at 05:31:04PM +0200, Richard Weinberger wrote:
> > > Am 19.06.2016 um 17:24 schrieb Daniel Golle:  
> > > >> You mean marking a MTD partition in DT and UBI will attach from it?
> > > >> That makes sense.  
> > > > 
> > > > Yes. Currently we just use a naming convention (the first MTD partition
> > > > named 'ubi' will be auto-attached), that's obviously not very clean...  
> > > 
> > > I was about to reply to my own mail that you can still attach by name.
> > > Boris reminded me of that, I forgot that feature.^^
> > > Why is it not clean?  
> > 
> > That's nice and I also didn't know that.
> > It's still not perfect because I got to add it to the cmdline (in DT),
> > and should have it only on devices where a partition with that given
> > name actually exists. A flag for the mtd partition would still be
> > nicer.
> > 
> > >   
> > > >>
> > > >> To sum up, I asked a lot of questions to understand your use case(s).
> > > >> Everything you described can be done with existing facilities.
> > > >> But I agree that at least some UBI DT machinery would be nice to have
> > > >> although we need to check with DT folks first.
> > > >> At least marking an MTD partition should be fine, hopefully.  
> > > > 
> > > > Great. That'd already greatly improve things.  
> > > 
> > > How is that better than attach by name? You mark the to be attached
> > > MTD by its name...  
> > 
> > See above. We'd then still need to have that ubi.mtd=name in the
> > cmdline for NAND devices using UBI and *not* have it for other devices
> > within the same family which may use SPI or NOR flash without UBI.
> > I'd prefer to have one place inside the device-tree for everything
> > flash-storage related, eg.
> > &nand {
> > 	status = "okay";
> > 	partition@0 {
> > 		label = "boot";
> > 		reg = <0x00000000 0x00e00000>;
> > 		read-only;
> > 	};
> > 
> > 	partition@e00000 {
> > 		label = "data";
> > 		compatible = "ubi,device";
> 
> So, if we follow your logic we should also have
> 
> 		compatible = "jffs2,file-system";
> 
> Because JFFS2 is an MTD user, just as UBI is.

Well, if there was any use for that, then yes, we should have that.
However, if root=/dev/mtdblockX is set, the filesystem type simply
doesn't matter as it is being probed and thus there simply is no
real need for that.

In some rare cases the bootloader may make use of JFFS2 and a certain
(/boot) partition is thus required to be JFFS2 no-matter-what. In that
case I'd agree, there should be such a thing as "jffs2,file-system" set
in DT (but that's a very rare corner case and abuse would probably
prevail over ligitimate use-cases).

In the end, the classication as "is an MTD user" isn't relevant for
this debate imho.

> 
> Let's see what Rob and other DT maintainers think about that.
> 
> Still, it seems to me that you're trying to solve a problem in the
> kernel when it should actually be solved in an upper layer.

It's definitely inside the grey-zone of early-userland... Doing things
which need to be done to mount the root filesystem are traditionally
the exceptions of things which are user-land and yet happen inside
the kernel before executing init.

> 
> Another option would be to try attaching UBI (along with all possible
> MTD users) to all the the MTD partitions. That's what's done for block
> filesystems when rootfstype is not specified.

Correct, but in case of filesystems, it is only done for the specified
partition/device. Probing all partitions sounds nice, however, please
keep in mind that MTD partitions may overlap and there may be
left-overs from previous UBI devices. Simply trying to ubiattach *all*
available MTD devices will need quite some extra work to handle all
potential outcomes of trying to attach non-UBI or
broken/corrupted/wrongly-sized UBI partitions.
Generally, I believe that's actually the only good alternative, but I
doubt it can easily made as robust and reliable compared to hinting
which mtd device to ubiattach via the devicetree.


Cheers


Daniel


> 
> 
> > 		reg = <0x00e00000 0x07200000>;
> > 	};
> > };
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Boris Brezillon]

On Sun, 19 Jun 2016 21:42:39 +0200
Daniel Golle <daniel@makrotopia.org> wrote:

> Hi Boris,
> 
> On Sun, Jun 19, 2016 at 06:53:43PM +0200, Boris Brezillon wrote:
> > On Sun, 19 Jun 2016 18:13:36 +0200
> > Daniel Golle <daniel@makrotopia.org> wrote:
> >   
> > > Hi Richard,
> > > 
> > > On Sun, Jun 19, 2016 at 05:31:04PM +0200, Richard Weinberger wrote:  
> > > > Am 19.06.2016 um 17:24 schrieb Daniel Golle:    
> > > > >> You mean marking a MTD partition in DT and UBI will attach from it?
> > > > >> That makes sense.    
> > > > > 
> > > > > Yes. Currently we just use a naming convention (the first MTD partition
> > > > > named 'ubi' will be auto-attached), that's obviously not very clean...    
> > > > 
> > > > I was about to reply to my own mail that you can still attach by name.
> > > > Boris reminded me of that, I forgot that feature.^^
> > > > Why is it not clean?    
> > > 
> > > That's nice and I also didn't know that.
> > > It's still not perfect because I got to add it to the cmdline (in DT),
> > > and should have it only on devices where a partition with that given
> > > name actually exists. A flag for the mtd partition would still be
> > > nicer.
> > >   
> > > >     
> > > > >>
> > > > >> To sum up, I asked a lot of questions to understand your use case(s).
> > > > >> Everything you described can be done with existing facilities.
> > > > >> But I agree that at least some UBI DT machinery would be nice to have
> > > > >> although we need to check with DT folks first.
> > > > >> At least marking an MTD partition should be fine, hopefully.    
> > > > > 
> > > > > Great. That'd already greatly improve things.    
> > > > 
> > > > How is that better than attach by name? You mark the to be attached
> > > > MTD by its name...    
> > > 
> > > See above. We'd then still need to have that ubi.mtd=name in the
> > > cmdline for NAND devices using UBI and *not* have it for other devices
> > > within the same family which may use SPI or NOR flash without UBI.
> > > I'd prefer to have one place inside the device-tree for everything
> > > flash-storage related, eg.
> > > &nand {
> > > 	status = "okay";
> > > 	partition@0 {
> > > 		label = "boot";
> > > 		reg = <0x00000000 0x00e00000>;
> > > 		read-only;
> > > 	};
> > > 
> > > 	partition@e00000 {
> > > 		label = "data";
> > > 		compatible = "ubi,device";  
> > 
> > So, if we follow your logic we should also have
> > 
> > 		compatible = "jffs2,file-system";
> > 
> > Because JFFS2 is an MTD user, just as UBI is.  
> 
> Well, if there was any use for that, then yes, we should have that.
> However, if root=/dev/mtdblockX is set, the filesystem type simply
> doesn't matter as it is being probed and thus there simply is no
> real need for that.
> 
> In some rare cases the bootloader may make use of JFFS2 and a certain
> (/boot) partition is thus required to be JFFS2 no-matter-what. In that
> case I'd agree, there should be such a thing as "jffs2,file-system" set
> in DT (but that's a very rare corner case and abuse would probably
> prevail over ligitimate use-cases).
> 
> In the end, the classication as "is an MTD user" isn't relevant for
> this debate imho.

It is, because supporting the UBI specific case is just an open door to
supporting any specific cases people might have, and how should we
justify that UBI is more legitimate than others.

> 
> > 
> > Let's see what Rob and other DT maintainers think about that.
> > 
> > Still, it seems to me that you're trying to solve a problem in the
> > kernel when it should actually be solved in an upper layer.  
> 
> It's definitely inside the grey-zone of early-userland... Doing things
> which need to be done to mount the root filesystem are traditionally
> the exceptions of things which are user-land and yet happen inside
> the kernel before executing init.

But I've never seen any DT properties specifying which FS should be
used on a block device...
I know, partitions on block devices are stored in a dedicated partition
table, and MTD partitions are not (or let's say that most of the time
they are not).

But still, I think the software stack you put on top of your storage
device should not appear in the DT.

Now, if all other people think this is sane to do it, I won't block it.

> 
> > 
> > Another option would be to try attaching UBI (along with all possible
> > MTD users) to all the the MTD partitions. That's what's done for block
> > filesystems when rootfstype is not specified.  
> 
> Correct, but in case of filesystems, it is only done for the specified
> partition/device. Probing all partitions sounds nice, however, please
> keep in mind that MTD partitions may overlap and there may be
> left-overs from previous UBI devices.

Overlapping partitions are unsafe in general, no matter what you want to
do with it. The only valid case would be 1 RW partition and several RO
partitions overlapping with the RW one.
The other accepted use case is exposing the master device (the real MTD
device under MTD partitions). But in any case, using 2 overlapping
partitions in // and both in RW mode is unsafe and should be avoided.

> Simply trying to ubiattach *all*
> available MTD devices will need quite some extra work to handle all
> potential outcomes of trying to attach non-UBI or
> broken/corrupted/wrongly-sized UBI partitions.

Attaching broken/corrupted/wrongly-sized partitions will still fail,
except they might be detected as valid UBI partition by the
auto-detection code, which would trigger a full attach step.
The only drawback is that you might try something that has been
corrupted, but this is already the case with the current approach.

BTW, I don't expect to see a lot of systems defining several UBI
devices attached to a single MTD device. So this only leaves the
multi-devices case. And of course this auto-detection logic would have
to be explicitly enabled, so the penalty would only impact those
enabling this feature.

> Generally, I believe that's actually the only good alternative, but I
> doubt it can easily made as robust and reliable compared to hinting
> which mtd device to ubiattach via the devicetree.

As I said, I'm not in favor of putting this kind of information in the
DT, so I'm trying to find other approaches to solve your problem ;).

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi Boris,

On Sun, Jun 19, 2016 at 10:14:34PM +0200, Boris Brezillon wrote:
> On Sun, 19 Jun 2016 21:42:39 +0200
> Daniel Golle <daniel@makrotopia.org> wrote:
> 
> > Hi Boris,
> > 
> > On Sun, Jun 19, 2016 at 06:53:43PM +0200, Boris Brezillon wrote:
> > > On Sun, 19 Jun 2016 18:13:36 +0200
> > > Daniel Golle <daniel@makrotopia.org> wrote:
> > >   
> > > > Hi Richard,
> > > > 
> > > > On Sun, Jun 19, 2016 at 05:31:04PM +0200, Richard Weinberger wrote:  
> > > > > Am 19.06.2016 um 17:24 schrieb Daniel Golle:    
> > > > > >> You mean marking a MTD partition in DT and UBI will attach from it?
> > > > > >> That makes sense.    
> > > > > > 
> > > > > > Yes. Currently we just use a naming convention (the first MTD partition
> > > > > > named 'ubi' will be auto-attached), that's obviously not very clean...    
> > > > > 
> > > > > I was about to reply to my own mail that you can still attach by name.
> > > > > Boris reminded me of that, I forgot that feature.^^
> > > > > Why is it not clean?    
> > > > 
> > > > That's nice and I also didn't know that.
> > > > It's still not perfect because I got to add it to the cmdline (in DT),
> > > > and should have it only on devices where a partition with that given
> > > > name actually exists. A flag for the mtd partition would still be
> > > > nicer.
> > > >   
> > > > >     
> > > > > >>
> > > > > >> To sum up, I asked a lot of questions to understand your use case(s).
> > > > > >> Everything you described can be done with existing facilities.
> > > > > >> But I agree that at least some UBI DT machinery would be nice to have
> > > > > >> although we need to check with DT folks first.
> > > > > >> At least marking an MTD partition should be fine, hopefully.    
> > > > > > 
> > > > > > Great. That'd already greatly improve things.    
> > > > > 
> > > > > How is that better than attach by name? You mark the to be attached
> > > > > MTD by its name...    
> > > > 
> > > > See above. We'd then still need to have that ubi.mtd=name in the
> > > > cmdline for NAND devices using UBI and *not* have it for other devices
> > > > within the same family which may use SPI or NOR flash without UBI.
> > > > I'd prefer to have one place inside the device-tree for everything
> > > > flash-storage related, eg.
> > > > &nand {
> > > > 	status = "okay";
> > > > 	partition@0 {
> > > > 		label = "boot";
> > > > 		reg = <0x00000000 0x00e00000>;
> > > > 		read-only;
> > > > 	};
> > > > 
> > > > 	partition@e00000 {
> > > > 		label = "data";
> > > > 		compatible = "ubi,device";  
> > > 
> > > So, if we follow your logic we should also have
> > > 
> > > 		compatible = "jffs2,file-system";
> > > 
> > > Because JFFS2 is an MTD user, just as UBI is.  
> > 
> > Well, if there was any use for that, then yes, we should have that.
> > However, if root=/dev/mtdblockX is set, the filesystem type simply
> > doesn't matter as it is being probed and thus there simply is no
> > real need for that.
> > 
> > In some rare cases the bootloader may make use of JFFS2 and a certain
> > (/boot) partition is thus required to be JFFS2 no-matter-what. In that
> > case I'd agree, there should be such a thing as "jffs2,file-system" set
> > in DT (but that's a very rare corner case and abuse would probably
> > prevail over ligitimate use-cases).
> > 
> > In the end, the classication as "is an MTD user" isn't relevant for
> > this debate imho.
> 
> It is, because supporting the UBI specific case is just an open door to
> supporting any specific cases people might have, and how should we
> justify that UBI is more legitimate than others.

That could be true, if there actually are other things which require
device-specific information about MTD partitions. In a way something
similar already exists in a reverse-fashion for in-flash WiFi EEPROMs,
eg.
fpi@10000000 {
        ...
        ath9k_eep {
                compatible = "ath9k,eeprom";
                ath,eep-flash = <&ath9k_cal 0x985>;
                ath,eep-endian;
                ath,eep-swap;
        };
        ...
};

&spi {
        pinctrl-names = "default";
        pinctrl-0 = <&pins_spi_default>;

        status = "ok";

        m25p80@4 {
                #address-cells = <1>;
                #size-cells = <1>;
                compatible = "jedec,spi-nor";
                reg = <4 0>;
                spi-max-frequency = <1000000>;

                ath9k_cal: partition@0 {
                        reg = <0x0 0x20000>;
                        label = "urlader";
                        read-only;
                };
        ...
};

(excerpt from FRITZ3370.dts)


> 
> > 
> > > 
> > > Let's see what Rob and other DT maintainers think about that.
> > > 
> > > Still, it seems to me that you're trying to solve a problem in the
> > > kernel when it should actually be solved in an upper layer.  
> > 
> > It's definitely inside the grey-zone of early-userland... Doing things
> > which need to be done to mount the root filesystem are traditionally
> > the exceptions of things which are user-land and yet happen inside
> > the kernel before executing init.
> 
> But I've never seen any DT properties specifying which FS should be
> used on a block device...

No, because that's not needed for filesystems. UBI sitting on top of
a specific MTD partition is kinda part of the general way a device is
supposed to be used, bootloaders may assume UBI to be present in a
certain area and if used by the stock firmware, the partition used
for UBI by a third party firmware should be within the same
boundaries. To me it looks like a device-specific convention, similar
to flash areas used for WiFi EEPROMs or bootloader environments.
(to detect the bootloader environment in user-space we are currently
appending the device-tree by having per-board defaults. That's also
not that nice, but it's not as problematic because usually uneeded
during normal runtime, see
https://git.lede-project.org/?p=source.git;a=blob;f=package/boot/uboot-envtools/files/lantiq
)

> I know, partitions on block devices are stored in a dedicated partition
> table, and MTD partitions are not (or let's say that most of the time
> they are not).
> 
> But still, I think the software stack you put on top of your storage
> device should not appear in the DT.

I agree, with the exception of whatever is the minimum needed for the
device to boot. LVM2, cryptsetup or FUSE are optional luxury and I
agree there shouldn't be any volume-manager or filesystem specifics in
the device tree. However, while I believe that this includes 

> 
> Now, if all other people think this is sane to do it, I won't block it.
> 
> > 
> > > 
> > > Another option would be to try attaching UBI (along with all possible
> > > MTD users) to all the the MTD partitions. That's what's done for block
> > > filesystems when rootfstype is not specified.  
> > 
> > Correct, but in case of filesystems, it is only done for the specified
> > partition/device. Probing all partitions sounds nice, however, please
> > keep in mind that MTD partitions may overlap and there may be
> > left-overs from previous UBI devices.
> 
> Overlapping partitions are unsafe in general, no matter what you want to
> do with it. The only valid case would be 1 RW partition and several RO
> partitions overlapping with the RW one.
> The other accepted use case is exposing the master device (the real MTD
> device under MTD partitions). But in any case, using 2 overlapping
> partitions in // and both in RW mode is unsafe and should be avoided.

That second case is quite common, and also having a MTD partition
defined for the whole firmware-region (ie. usually starting at an
offset above the bootloader and it's environment and ending just
before the WiFi EEPROM which is commonly stored at the high end of
flash chips). Accidentally ubi-attaching that firmware partition may
end up quite unlucky if it starts with a UBI device...

> 
> > Simply trying to ubiattach *all*
> > available MTD devices will need quite some extra work to handle all
> > potential outcomes of trying to attach non-UBI or
> > broken/corrupted/wrongly-sized UBI partitions.
> 
> Attaching broken/corrupted/wrongly-sized partitions will still fail,
> except they might be detected as valid UBI partition by the
> auto-detection code, which would trigger a full attach step.
> The only drawback is that you might try something that has been
> corrupted, but this is already the case with the current approach.

Not quite, because you are only probing the partition selected rather
than all of them.

> 
> BTW, I don't expect to see a lot of systems defining several UBI
> devices attached to a single MTD device. So this only leaves the
> multi-devices case. And of course this auto-detection logic would have
> to be explicitly enabled, so the penalty would only impact those
> enabling this feature.
> 
> > Generally, I believe that's actually the only good alternative, but I
> > doubt it can easily made as robust and reliable compared to hinting
> > which mtd device to ubiattach via the devicetree.
> 
> As I said, I'm not in favor of putting this kind of information in the
> DT, so I'm trying to find other approaches to solve your problem ;).

Making it safe to auto-probe all MTD devices is indeed a sound option,
I just expected it to be more effort and more pitfalls compared to
an optional marker in the device-tree which is just a convenience
replacement for the cmdline syntax.


Cheers


Daniel

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Hauke Mehrtens]

On 06/19/2016 11:48 PM, Daniel Golle wrote:
> Hi Boris,
> 
> On Sun, Jun 19, 2016 at 10:14:34PM +0200, Boris Brezillon wrote:
>> On Sun, 19 Jun 2016 21:42:39 +0200
>> Daniel Golle <daniel@makrotopia.org> wrote:
>>
>>> Hi Boris,
>>>
>>> On Sun, Jun 19, 2016 at 06:53:43PM +0200, Boris Brezillon wrote:
>>>> On Sun, 19 Jun 2016 18:13:36 +0200
>>>> Daniel Golle <daniel@makrotopia.org> wrote:
>>>>   
>>>>> Hi Richard,
>>>>>
>>>>> On Sun, Jun 19, 2016 at 05:31:04PM +0200, Richard Weinberger wrote:  
>>>>>> Am 19.06.2016 um 17:24 schrieb Daniel Golle:    
>>>>>>>> You mean marking a MTD partition in DT and UBI will attach from it?
>>>>>>>> That makes sense.    
>>>>>>>
>>>>>>> Yes. Currently we just use a naming convention (the first MTD partition
>>>>>>> named 'ubi' will be auto-attached), that's obviously not very clean...    
>>>>>>
>>>>>> I was about to reply to my own mail that you can still attach by name.
>>>>>> Boris reminded me of that, I forgot that feature.^^
>>>>>> Why is it not clean?    
>>>>>
>>>>> That's nice and I also didn't know that.
>>>>> It's still not perfect because I got to add it to the cmdline (in DT),
>>>>> and should have it only on devices where a partition with that given
>>>>> name actually exists. A flag for the mtd partition would still be
>>>>> nicer.

Is it possible to addend the kernel command line with device tree? Some
vendor boot loaders are adding the mac address to the kernel command
line and I would like to get it. Like Daniel said replacing the vendor
boot loader is dangerous because sometimes recovery means to desolder
the flash chip.

>>>>>   
>>>>>>     
>>>>>>>>
>>>>>>>> To sum up, I asked a lot of questions to understand your use case(s).
>>>>>>>> Everything you described can be done with existing facilities.
>>>>>>>> But I agree that at least some UBI DT machinery would be nice to have
>>>>>>>> although we need to check with DT folks first.
>>>>>>>> At least marking an MTD partition should be fine, hopefully.    
>>>>>>>
>>>>>>> Great. That'd already greatly improve things.    
>>>>>>
>>>>>> How is that better than attach by name? You mark the to be attached
>>>>>> MTD by its name...    
>>>>>
>>>>> See above. We'd then still need to have that ubi.mtd=name in the
>>>>> cmdline for NAND devices using UBI and *not* have it for other devices
>>>>> within the same family which may use SPI or NOR flash without UBI.
>>>>> I'd prefer to have one place inside the device-tree for everything
>>>>> flash-storage related, eg.
>>>>> &nand {
>>>>> 	status = "okay";
>>>>> 	partition@0 {
>>>>> 		label = "boot";
>>>>> 		reg = <0x00000000 0x00e00000>;
>>>>> 		read-only;
>>>>> 	};
>>>>>
>>>>> 	partition@e00000 {
>>>>> 		label = "data";
>>>>> 		compatible = "ubi,device";  
>>>>
>>>> So, if we follow your logic we should also have
>>>>
>>>> 		compatible = "jffs2,file-system";
>>>>
>>>> Because JFFS2 is an MTD user, just as UBI is.  
>>>
>>> Well, if there was any use for that, then yes, we should have that.
>>> However, if root=/dev/mtdblockX is set, the filesystem type simply
>>> doesn't matter as it is being probed and thus there simply is no
>>> real need for that.
>>>
>>> In some rare cases the bootloader may make use of JFFS2 and a certain
>>> (/boot) partition is thus required to be JFFS2 no-matter-what. In that
>>> case I'd agree, there should be such a thing as "jffs2,file-system" set
>>> in DT (but that's a very rare corner case and abuse would probably
>>> prevail over ligitimate use-cases).
>>>
>>> In the end, the classication as "is an MTD user" isn't relevant for
>>> this debate imho.
>>
>> It is, because supporting the UBI specific case is just an open door to
>> supporting any specific cases people might have, and how should we
>> justify that UBI is more legitimate than others.
> 
> That could be true, if there actually are other things which require
> device-specific information about MTD partitions. In a way something
> similar already exists in a reverse-fashion for in-flash WiFi EEPROMs,
> eg.
> fpi@10000000 {
>         ...
>         ath9k_eep {
>                 compatible = "ath9k,eeprom";
>                 ath,eep-flash = <&ath9k_cal 0x985>;
>                 ath,eep-endian;
>                 ath,eep-swap;
>         };
>         ...
> };
> 
> &spi {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pins_spi_default>;
> 
>         status = "ok";
> 
>         m25p80@4 {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 compatible = "jedec,spi-nor";
>                 reg = <4 0>;
>                 spi-max-frequency = <1000000>;
> 
>                 ath9k_cal: partition@0 {
>                         reg = <0x0 0x20000>;
>                         label = "urlader";
>                         read-only;
>                 };
>         ...
> };
> 
> (excerpt from FRITZ3370.dts)
> 
> 
>>
>>>
>>>>
>>>> Let's see what Rob and other DT maintainers think about that.
>>>>
>>>> Still, it seems to me that you're trying to solve a problem in the
>>>> kernel when it should actually be solved in an upper layer.  
>>>
>>> It's definitely inside the grey-zone of early-userland... Doing things
>>> which need to be done to mount the root filesystem are traditionally
>>> the exceptions of things which are user-land and yet happen inside
>>> the kernel before executing init.
>>
>> But I've never seen any DT properties specifying which FS should be
>> used on a block device...
> 
> No, because that's not needed for filesystems. UBI sitting on top of
> a specific MTD partition is kinda part of the general way a device is
> supposed to be used, bootloaders may assume UBI to be present in a
> certain area and if used by the stock firmware, the partition used
> for UBI by a third party firmware should be within the same
> boundaries. To me it looks like a device-specific convention, similar
> to flash areas used for WiFi EEPROMs or bootloader environments.
> (to detect the bootloader environment in user-space we are currently
> appending the device-tree by having per-board defaults. That's also
> not that nice, but it's not as problematic because usually uneeded
> during normal runtime, see
> https://git.lede-project.org/?p=source.git;a=blob;f=package/boot/uboot-envtools/files/lantiq
> )

In the end I would like to be able to specify from where to mount the
root filesystem in device tree. The partition layout directly on the
flash chip is already defined in device tree.

The U-Boot from the Intel / Lantiq SDK currently loads the kernel from
an UBI partition and then the kernel should also load the rootfs from an
UBI partition. You can also flash the images from the boot loader the
boot loader takes care of placing your image into the correct UBI
partitions. In this case the boot loader is aware of the partition
schema and also the UBI schema.

>> I know, partitions on block devices are stored in a dedicated partition
>> table, and MTD partitions are not (or let's say that most of the time
>> they are not).
>>
>> But still, I think the software stack you put on top of your storage
>> device should not appear in the DT.
> 
> I agree, with the exception of whatever is the minimum needed for the
> device to boot. LVM2, cryptsetup or FUSE are optional luxury and I
> agree there shouldn't be any volume-manager or filesystem specifics in
> the device tree. However, while I believe that this includes 
> 
>>
>> Now, if all other people think this is sane to do it, I won't block it.
>>
>>>
>>>>
>>>> Another option would be to try attaching UBI (along with all possible
>>>> MTD users) to all the the MTD partitions. That's what's done for block
>>>> filesystems when rootfstype is not specified.  
>>>
>>> Correct, but in case of filesystems, it is only done for the specified
>>> partition/device. Probing all partitions sounds nice, however, please
>>> keep in mind that MTD partitions may overlap and there may be
>>> left-overs from previous UBI devices.
>>
>> Overlapping partitions are unsafe in general, no matter what you want to
>> do with it. The only valid case would be 1 RW partition and several RO
>> partitions overlapping with the RW one.
>> The other accepted use case is exposing the master device (the real MTD
>> device under MTD partitions). But in any case, using 2 overlapping
>> partitions in // and both in RW mode is unsafe and should be avoided.
> 
> That second case is quite common, and also having a MTD partition
> defined for the whole firmware-region (ie. usually starting at an
> offset above the bootloader and it's environment and ending just
> before the WiFi EEPROM which is commonly stored at the high end of
> flash chips). Accidentally ubi-attaching that firmware partition may
> end up quite unlucky if it starts with a UBI device...
> 
>>
>>> Simply trying to ubiattach *all*
>>> available MTD devices will need quite some extra work to handle all
>>> potential outcomes of trying to attach non-UBI or
>>> broken/corrupted/wrongly-sized UBI partitions.
>>
>> Attaching broken/corrupted/wrongly-sized partitions will still fail,
>> except they might be detected as valid UBI partition by the
>> auto-detection code, which would trigger a full attach step.
>> The only drawback is that you might try something that has been
>> corrupted, but this is already the case with the current approach.
> 
> Not quite, because you are only probing the partition selected rather
> than all of them.
> 
>>
>> BTW, I don't expect to see a lot of systems defining several UBI
>> devices attached to a single MTD device. So this only leaves the
>> multi-devices case. And of course this auto-detection logic would have
>> to be explicitly enabled, so the penalty would only impact those
>> enabling this feature.
>>
>>> Generally, I believe that's actually the only good alternative, but I
>>> doubt it can easily made as robust and reliable compared to hinting
>>> which mtd device to ubiattach via the devicetree.
>>
>> As I said, I'm not in favor of putting this kind of information in the
>> DT, so I'm trying to find other approaches to solve your problem ;).
> 
> Making it safe to auto-probe all MTD devices is indeed a sound option,
> I just expected it to be more effort and more pitfalls compared to
> an optional marker in the device-tree which is just a convenience
> replacement for the cmdline syntax.
> 
> 
> Cheers
> 
> 
> Daniel
> 

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Arnd Bergmann]

On Monday, June 20, 2016 12:21:07 AM CEST Hauke Mehrtens wrote:
> On 06/19/2016 11:48 PM, Daniel Golle wrote:
> > Hi Boris,
> > 
> > On Sun, Jun 19, 2016 at 10:14:34PM +0200, Boris Brezillon wrote:
> >> On Sun, 19 Jun 2016 21:42:39 +0200
> >> Daniel Golle <daniel@makrotopia.org> wrote:
> >>>>> That's nice and I also didn't know that.
> >>>>> It's still not perfect because I got to add it to the cmdline (in DT),
> >>>>> and should have it only on devices where a partition with that given
> >>>>> name actually exists. A flag for the mtd partition would still be
> >>>>> nicer.
> 
> Is it possible to addend the kernel command line with device tree? Some
> vendor boot loaders are adding the mac address to the kernel command
> line and I would like to get it. Like Daniel said replacing the vendor
> boot loader is dangerous because sometimes recovery means to desolder
> the flash chip.

The bootloader can easily modify the command line in the dt, but
things like device mac address should be modified in the ethernet
device node if possible.

> > 
> > That could be true, if there actually are other things which require
> > device-specific information about MTD partitions. In a way something
> > similar already exists in a reverse-fashion for in-flash WiFi EEPROMs,
> > eg.
> > fpi@10000000 {
> >         ...
> >         ath9k_eep {
> >                 compatible = "ath9k,eeprom";
> >                 ath,eep-flash = <&ath9k_cal 0x985>;
> >                 ath,eep-endian;
> >                 ath,eep-swap;
> >         };
> >         ...
> > };

This is a  bit of an ad-hoc binding, I wouldn't use that as
an example for how to do things elsewhere.

> >>>> Let's see what Rob and other DT maintainers think about that.
> >>>>
> >>>> Still, it seems to me that you're trying to solve a problem in the
> >>>> kernel when it should actually be solved in an upper layer.  
> >>>
> >>> It's definitely inside the grey-zone of early-userland... Doing things
> >>> which need to be done to mount the root filesystem are traditionally
> >>> the exceptions of things which are user-land and yet happen inside
> >>> the kernel before executing init.
> >>
> >> But I've never seen any DT properties specifying which FS should be
> >> used on a block device...

Right, that goes a bit too far. File system type etc should clearly
be detectable from either the partition table or the partition itself.

> > No, because that's not needed for filesystems. UBI sitting on top of
> > a specific MTD partition is kinda part of the general way a device is
> > supposed to be used, bootloaders may assume UBI to be present in a
> > certain area and if used by the stock firmware, the partition used
> > for UBI by a third party firmware should be within the same
> > boundaries. To me it looks like a device-specific convention, similar
> > to flash areas used for WiFi EEPROMs or bootloader environments.
> > (to detect the bootloader environment in user-space we are currently
> > appending the device-tree by having per-board defaults. That's also
> > not that nice, but it's not as problematic because usually uneeded
> > during normal runtime, see
> > https://git.lede-project.org/?p=source.git;a=blob;f=package/boot/uboot-envtools/files/lantiq
> > )
> 
> In the end I would like to be able to specify from where to mount the
> root filesystem in device tree. The partition layout directly on the
> flash chip is already defined in device tree.
> 
> The U-Boot from the Intel / Lantiq SDK currently loads the kernel from
> an UBI partition and then the kernel should also load the rootfs from an
> UBI partition. You can also flash the images from the boot loader the
> boot loader takes care of placing your image into the correct UBI
> partitions. In this case the boot loader is aware of the partition
> schema and also the UBI schema.

A few more thoughts on various things that came up in the discussion
so far (so I don't have to reply to each of them separately)

- The /chosen and /aliases nodes do configure the OS to some degree,
  and historically we have used them to specify the device names that
  show up in the kernel (e.g. on MacOS and AIX) as well as the rootfs
  itself

- The device tree should give us enough information to identify the
  root partition by a GUID from the kernel. This works on all block
  devices (at least with MBR and EFI partition tables, probably many
  of the lesser known ones as well) and it should really also work
  on UBI.

- MTD partitioning unfortunately does not seem to have a widely used
  method for identifying partitions from what's stored on the flash,
  unlike block devices, so whatever we normally have in the partition
  table has to be stored in DT. This really sucks, but I don't know
  what else to do about it.

- The UBI design was originally meant to cover the entire flash
  device and provide the partitioning information inside. I have not
  looked at UBI in a long time (actually not since before embedded
  PPC moved to DT), but I would hope that this case works without
  any DT magic.

	Arnd

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Am 20.06.2016 um 10:09 schrieb Arnd Bergmann:
> A few more thoughts on various things that came up in the discussion
> so far (so I don't have to reply to each of them separately)
> 
> - The /chosen and /aliases nodes do configure the OS to some degree,
>   and historically we have used them to specify the device names that
>   show up in the kernel (e.g. on MacOS and AIX) as well as the rootfs
>   itself
> 
> - The device tree should give us enough information to identify the
>   root partition by a GUID from the kernel. This works on all block
>   devices (at least with MBR and EFI partition tables, probably many
>   of the lesser known ones as well) and it should really also work
>   on UBI.

It does.

> - MTD partitioning unfortunately does not seem to have a widely used
>   method for identifying partitions from what's stored on the flash,
>   unlike block devices, so whatever we normally have in the partition
>   table has to be stored in DT. This really sucks, but I don't know
>   what else to do about it.

Since blocks on a MTD can render bad you'd lose the table sooner or later.
That's why we cannot store it on the MTD directly.
Defining the table in DT is at least less ugly than using the mtdparts=
kernel parameter.

> - The UBI design was originally meant to cover the entire flash
>   device and provide the partitioning information inside. I have not
>   looked at UBI in a long time (actually not since before embedded
>   PPC moved to DT), but I would hope that this case works without
>   any DT magic.

Of course it works without DT magic.

So, I don't see a urge to have UBI DT bindings.
First I thought having an attach flag would be handy but it turned out
that this behavior can easily achieved by naming. i.e. you a kernel
command line such as "ubi.mtd=UBI" and name the to be attached MTD "UBI"
in your DT.

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Arnd Bergmann]

On Monday, June 20, 2016 10:26:11 AM CEST Richard Weinberger wrote:
> Am 20.06.2016 um 10:09 schrieb Arnd Bergmann:
> > A few more thoughts on various things that came up in the discussion
> > so far (so I don't have to reply to each of them separately)
> > 
> > - The /chosen and /aliases nodes do configure the OS to some degree,
> >   and historically we have used them to specify the device names that
> >   show up in the kernel (e.g. on MacOS and AIX) as well as the rootfs
> >   itself
> > 
> > - The device tree should give us enough information to identify the
> >   root partition by a GUID from the kernel. This works on all block
> >   devices (at least with MBR and EFI partition tables, probably many
> >   of the lesser known ones as well) and it should really also work
> >   on UBI.
> 
> It does.
> 
> > - MTD partitioning unfortunately does not seem to have a widely used
> >   method for identifying partitions from what's stored on the flash,
> >   unlike block devices, so whatever we normally have in the partition
> >   table has to be stored in DT. This really sucks, but I don't know
> >   what else to do about it.
> 
> Since blocks on a MTD can render bad you'd lose the table sooner or later.
> That's why we cannot store it on the MTD directly.
> Defining the table in DT is at least less ugly than using the mtdparts=
> kernel parameter.

Right, there would be no benefit in using the kernel command line,
it just moves the information to another place inside of the same DT
(the /chosen property).

I think you can normally rely on the first block being readable
on flash, in particular if you write it very rarely (when updating
the partition information), so it would be technically possible to
have a partition table in there, but in practice that's not how
things are done, so the argument is useless.

> > - The UBI design was originally meant to cover the entire flash
> >   device and provide the partitioning information inside. I have not
> >   looked at UBI in a long time (actually not since before embedded
> >   PPC moved to DT), but I would hope that this case works without
> >   any DT magic.
> 
> Of course it works without DT magic.
> 
> So, I don't see a urge to have UBI DT bindings.
> First I thought having an attach flag would be handy but it turned out
> that this behavior can easily achieved by naming. i.e. you a kernel
> command line such as "ubi.mtd=UBI" and name the to be attached MTD "UBI"
> in your DT.

Right. I think either way can work, and I see advantages in both.
The downside of relying just on the name is that mounting the rootfs
by GUID doesn't work until you specify another command line, which
is a bit annoying but not a huge problem.

	Arnd

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Brian Norris]

On Mon, Jun 20, 2016 at 05:08:51PM +0200, Arnd Bergmann wrote:
> On Monday, June 20, 2016 10:26:11 AM CEST Richard Weinberger wrote:
> > Am 20.06.2016 um 10:09 schrieb Arnd Bergmann:
> > > - MTD partitioning unfortunately does not seem to have a widely used
> > >   method for identifying partitions from what's stored on the flash,
> > >   unlike block devices, so whatever we normally have in the partition
> > >   table has to be stored in DT. This really sucks, but I don't know
> > >   what else to do about it.
> > 
> > Since blocks on a MTD can render bad you'd lose the table sooner or later.
> > That's why we cannot store it on the MTD directly.

That's assuming you're talking about NAND, and that no one has devised
any clever partition scheme that is robust against bad blocks. I've seen
attempts at the latter, but nothing widely used. And eventually, you're
just reimplementing UBI anyway.

About the former: some systems will still use a dual-flash system, where
they get reliable partition information from a NOR flash to bootstrap
getting the NAND up and running. So it's possible to avoid sticking all
partitions in DT.

> > Defining the table in DT is at least less ugly than using the mtdparts=
> > kernel parameter.
> 
> Right, there would be no benefit in using the kernel command line,
> it just moves the information to another place inside of the same DT
> (the /chosen property).
> 
> I think you can normally rely on the first block being readable
> on flash, in particular if you write it very rarely (when updating
> the partition information), so it would be technically possible to
> have a partition table in there, but in practice that's not how
> things are done, so the argument is useless.

It's not completely useless, but it's difficult for real technical
reasons: many SoCs boot straight off address 0 of a flash (hopefully
something reliable like NOR flash, but I've even seen that for NAND...),
so you can't possibly put a partition table there. That doesn't preclude
coming up with a decent non-0-address-based scheme, but nobody's really
done that generically, although there are various vendor-specific
attempts to just progressively scan all blocks on the flash -- see a few
drivers/mtd/*part.c.

Brian

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Am 20.06.2016 um 17:08 schrieb Arnd Bergmann:
>> Since blocks on a MTD can render bad you'd lose the table sooner or later.
>> That's why we cannot store it on the MTD directly.
>> Defining the table in DT is at least less ugly than using the mtdparts=
>> kernel parameter.
> 
> Right, there would be no benefit in using the kernel command line,
> it just moves the information to another place inside of the same DT
> (the /chosen property).
> 
> I think you can normally rely on the first block being readable
> on flash, in particular if you write it very rarely (when updating
> the partition information), so it would be technically possible to
> have a partition table in there, but in practice that's not how
> things are done, so the argument is useless.

Speaking of NAND, only SLC (and also here not all) chips guarantee that the first
block is better than the other ones.

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Hauke Mehrtens]

On 06/20/2016 09:57 PM, Richard Weinberger wrote:
> Am 20.06.2016 um 17:08 schrieb Arnd Bergmann:
>>> Since blocks on a MTD can render bad you'd lose the table sooner or later.
>>> That's why we cannot store it on the MTD directly.
>>> Defining the table in DT is at least less ugly than using the mtdparts=
>>> kernel parameter.
>>
>> Right, there would be no benefit in using the kernel command line,
>> it just moves the information to another place inside of the same DT
>> (the /chosen property).
>>
>> I think you can normally rely on the first block being readable
>> on flash, in particular if you write it very rarely (when updating
>> the partition information), so it would be technically possible to
>> have a partition table in there, but in practice that's not how
>> things are done, so the argument is useless.
> 
> Speaking of NAND, only SLC (and also here not all) chips guarantee that the first
> block is better than the other ones.

Being able to define a rootfs the kernel should load with a /chosen
property would also be nice.

https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt

These UBI patches were just one step to make it possible to define the
root partition in device tree. It would probably be better to first look
on how to do that with normal mtd partitions and then see how to extend
that to UBI.

Currently I have to extend the boot args with these parameters to boot a
unmodified mainline kernel: "ubi.mtd=ubi ubi.block=0,rootfsA
root=/dev/ubiblock0_1"
This is a SquashFS in a UBI layer on a mtd partition. I know that I can
put this into the bootargs in a device tree file.

Hauke

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Brian Norris]

Hi,

I'll take a moment to hijack this and solicit comments on this old
thread:

On Mon, Jun 20, 2016 at 10:09:50AM +0200, Arnd Bergmann wrote:
> - MTD partitioning unfortunately does not seem to have a widely used
>   method for identifying partitions from what's stored on the flash,
>   unlike block devices, so whatever we normally have in the partition
>   table has to be stored in DT. This really sucks, but I don't know
>   what else to do about it.

I got sidetracked, but I have attempted previously to at least diminish
the amount of MTD partitioning info that is stuck into DT:

http://lists.infradead.org/pipermail/linux-mtd/2015-December/064076.html

That series effectively accepts the fact that there isn't a single
standard or widely used method for identifying partitions, but it does
allow for a description that's at least more flexible than a static list
of partitions. I'd appreciate any thoughts on that. The main factors
that stalled that were that (a) there was some vague discomfort over the
use of the "compatible" property (I could probably have argued my case
better there) and (b) I didn't have more time to pursue that. I may pick
this up again when I get some time, especially if there are constructive
suggestions.

Brian

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Boris Brezillon]

Hi Daniel,

On Sun, 19 Jun 2016 17:24:46 +0200
Daniel Golle <daniel@makrotopia.org> wrote:

> Hi Richard,
> 
> On Sun, Jun 19, 2016 at 04:35:05PM +0200, Richard Weinberger wrote:
> > Daniel,
> > 
> > Am 19.06.2016 um 16:09 schrieb Daniel Golle:  
> > >> DT already offer all you need and nothing hinders you from
> > >> using mtdparts=.  
> > > 
> > > I know. However, it's much nicer to define partitions for a specific
> > > flash chip inside the device tree (I've sent a link to the DTS of the
> > > BTHOMEHUBV2B to illustrate that, please have a look at it and tell
> > > me if you'd really like that partitioning currently stored as
> > > device-tree nodes to be migrated back to 'mtdparts=...'.)  
> > 
> > I'd be fine with having the partitions in DT and the UBI kernel parameters
> > as kernel cmdline also in DT.  
> 
> That's the current state of affairs. I'd just want to have it in one
> place, so if the MTD partitioning gets updated one would see easily how
> that affects UBI. For that, I don't think it's good enough to have
> 'ubi.mtd=X' in the cmdline (in DT or wherever, it's a single string in
> a different place than the MTD partitioning if people use DT for MTD
> partitioning).
> 
> >   
> > >> I didn't recommend adding glibc+systemd in the initramfs. It can be very, very small
> > >> and trivial.  
> > > 
> > > Again, please understand that the whole production firmware can be as
> > > small as 3.5 megs for kernel and rootfs. Of course we never use glibc
> > > or systemd anywhere.  
> > 
> > I know these use cases, I design and maintain such stuff for customers.
> >   
> > > Yes, it can be done. But you would have to define your flash chips in
> > > device-tree, you may define the mtd partitions either in DT or by using
> > > mtdparts=... in the cmdline. Now to define which MTD device to
> > > ubiattach you will have to use ubi.mtd=X on the cmdline. If you defined
> > > your partitions inside the of_node of the flash chip, changes there
> > > (which do happen) may change the index of the partition you want to
> > > ubi-attach. C'mon. This *is* messy. Just having a compatible-flag to
> > > attach the partition would already greatly improve things.  
> > 
> > You mean marking a MTD partition in DT and UBI will attach from it?
> > That makes sense.  
> 
> Yes. Currently we just use a naming convention (the first MTD partition
> named 'ubi' will be auto-attached), that's obviously not very clean...
> 
> > 
> > To sum up, I asked a lot of questions to understand your use case(s).
> > Everything you described can be done with existing facilities.
> > But I agree that at least some UBI DT machinery would be nice to have
> > although we need to check with DT folks first.
> > At least marking an MTD partition should be fine, hopefully.  
> 
> Great. That'd already greatly improve things.
> 
> I can see that sooner or later we will need some way to reference UBI
> volumes in DT, independently of the whole rootfs selection. Vendors do
> use UBI volumes also for things like WiFi EEPROM data. Currentlhy, we
> extract that in user-space and save a copy in /lib/firmware/... for
> things more crazy than plain offsets inside MTD partitions.

Hm, you're clearly abusing the DT role here (and this comment comes from
someone who tend to think that some of the config information should be
described in the DT ;)).

You want to link your partition (which is already a specific usage of
your flash device, and as such might not even be described in the DT)
to a Linux specific software layer (UBI).
Remember that the DT is supposed to be OS agnostic, you clearly don't
need to describe that in the DT, since everything you need is already
there (a way to attach the UBI layer to an MTD device by its name).

Now, if that's something you want to make generic for all the boards
supporting OpenWrt, you just have to enforce the partition name, and
make sure the ubi.mtd=<default-ubi-name> parameter is passed on the
cmdline. IIRC, there's a way to define a default cmdline in case you
don't have access to the bootloaders.

This sounds like something that could be done at the build-system level.

Am I missing something?

For the EEPROM emulation using UBI volumes part, I'm not sure what you
want to do exactly? Are you planning to use the nvmem abstraction and
provide an nvmem wrapper around UBI volumes?

Regards,

Boris

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi Boris,

On Sun, Jun 19, 2016 at 05:43:51PM +0200, Boris Brezillon wrote:
> Hi Daniel,
> 
> On Sun, 19 Jun 2016 17:24:46 +0200
> Daniel Golle <daniel@makrotopia.org> wrote:
> 
> > Hi Richard,
> > 
> > On Sun, Jun 19, 2016 at 04:35:05PM +0200, Richard Weinberger wrote:
> > > Daniel,
> > > 
> > > Am 19.06.2016 um 16:09 schrieb Daniel Golle:  
> > > >> DT already offer all you need and nothing hinders you from
> > > >> using mtdparts=.  
> > > > 
> > > > I know. However, it's much nicer to define partitions for a specific
> > > > flash chip inside the device tree (I've sent a link to the DTS of the
> > > > BTHOMEHUBV2B to illustrate that, please have a look at it and tell
> > > > me if you'd really like that partitioning currently stored as
> > > > device-tree nodes to be migrated back to 'mtdparts=...'.)  
> > > 
> > > I'd be fine with having the partitions in DT and the UBI kernel parameters
> > > as kernel cmdline also in DT.  
> > 
> > That's the current state of affairs. I'd just want to have it in one
> > place, so if the MTD partitioning gets updated one would see easily how
> > that affects UBI. For that, I don't think it's good enough to have
> > 'ubi.mtd=X' in the cmdline (in DT or wherever, it's a single string in
> > a different place than the MTD partitioning if people use DT for MTD
> > partitioning).
> > 
> > >   
> > > >> I didn't recommend adding glibc+systemd in the initramfs. It can be very, very small
> > > >> and trivial.  
> > > > 
> > > > Again, please understand that the whole production firmware can be as
> > > > small as 3.5 megs for kernel and rootfs. Of course we never use glibc
> > > > or systemd anywhere.  
> > > 
> > > I know these use cases, I design and maintain such stuff for customers.
> > >   
> > > > Yes, it can be done. But you would have to define your flash chips in
> > > > device-tree, you may define the mtd partitions either in DT or by using
> > > > mtdparts=... in the cmdline. Now to define which MTD device to
> > > > ubiattach you will have to use ubi.mtd=X on the cmdline. If you defined
> > > > your partitions inside the of_node of the flash chip, changes there
> > > > (which do happen) may change the index of the partition you want to
> > > > ubi-attach. C'mon. This *is* messy. Just having a compatible-flag to
> > > > attach the partition would already greatly improve things.  
> > > 
> > > You mean marking a MTD partition in DT and UBI will attach from it?
> > > That makes sense.  
> > 
> > Yes. Currently we just use a naming convention (the first MTD partition
> > named 'ubi' will be auto-attached), that's obviously not very clean...
> > 
> > > 
> > > To sum up, I asked a lot of questions to understand your use case(s).
> > > Everything you described can be done with existing facilities.
> > > But I agree that at least some UBI DT machinery would be nice to have
> > > although we need to check with DT folks first.
> > > At least marking an MTD partition should be fine, hopefully.  
> > 
> > Great. That'd already greatly improve things.
> > 
> > I can see that sooner or later we will need some way to reference UBI
> > volumes in DT, independently of the whole rootfs selection. Vendors do
> > use UBI volumes also for things like WiFi EEPROM data. Currentlhy, we
> > extract that in user-space and save a copy in /lib/firmware/... for
> > things more crazy than plain offsets inside MTD partitions.
> 
> Hm, you're clearly abusing the DT role here (and this comment comes from
> someone who tend to think that some of the config information should be
> described in the DT ;)).
> 
> You want to link your partition (which is already a specific usage of
> your flash device, and as such might not even be described in the DT)
> to a Linux specific software layer (UBI).
> Remember that the DT is supposed to be OS agnostic, you clearly don't
> need to describe that in the DT, since everything you need is already
> there (a way to attach the UBI layer to an MTD device by its name).

UBI is supported by U-Boot as well and U-Boot supports storing its
environment inside an UBI volume, loading things from volumes or even
mounting UBIFS, so information about which partition to attach might
already be useful outside of Linux as well.
Apart from that, DT already stores a whole lot of Linux-specific
properties, such as linux,keycodes or for gpio-keys-polled or
linux,default-trigger for LEDs. Not having that information in the
devicetree would definitely make things worse imho.


> 
> Now, if that's something you want to make generic for all the boards
> supporting OpenWrt, you just have to enforce the partition name, and
> make sure the ubi.mtd=<default-ubi-name> parameter is passed on the
> cmdline. IIRC, there's a way to define a default cmdline in case you
> don't have access to the bootloaders.
> 
> This sounds like something that could be done at the build-system level.

True, but devices which do not have UBI will throw some errors to the
log about not being able to attach the device.


> 
> Am I missing something?
> 
> For the EEPROM emulation using UBI volumes part, I'm not sure what you
> want to do exactly? Are you planning to use the nvmem abstraction and
> provide an nvmem wrapper around UBI volumes?

Definitely not something I'd hope to get into at all, however, I can
smell it coming ;)


Cheers


Daniel


> 
> Regards,
> 
> Boris

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Brian Norris]

Hi,

On Sun, Jun 19, 2016 at 04:09:53PM +0200, Daniel Golle wrote:
> On Sun, Jun 19, 2016 at 03:19:38PM +0200, Richard Weinberger wrote:
> > Am 19.06.2016 um 15:05 schrieb Daniel Golle:
> > >>> The same kernel gets used on many devices having different $vendor
> > >>> mtd-partition layouts. A way other than the kernel cmdline allows
> > >>> to specify the default behaviour without restricting the user to
> > >>> manually use those cmdline options.
> > >>
> > >> You can put the cmdline into the per-device device tree.
> > >> This is the concept of a multi-device kernel. One kernel and
> > >> many device trees.
> > > 
> > > Exactly true. However, as the mtd partitions are defined in the tree
> > > itself rather than using 'mtdparts=...' in the cmdline built-into the
> > > device-tree, it'd be nice to also select that a specific partition
> > > should be ubiattached.
> > 
> > DT already offer all you need and nothing hinders you from
> > using mtdparts=.
> 
> I know. However, it's much nicer to define partitions for a specific
> flash chip inside the device tree (I've sent a link to the DTS of the
> BTHOMEHUBV2B to illustrate that, please have a look at it and tell
> me if you'd really like that partitioning currently stored as
> device-tree nodes to be migrated back to 'mtdparts=...'.)

Just because it's currently nicer to put partitions in device tree
doesn't really mean it's good to extend those bindings to do even more.
I believe there are alternatives that could work just fine. See below.

> > >>>>
> > >>>>> To auto-select the rootfs, we currently carry another bunch of patches
> > >>>>>
> > >>>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/491-ubi-auto-create-ubiblock-device-for-rootfs.patch
> > >>>>
> > >>>> Same question here.
> > >>>>
> > >>>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/492-try-auto-mounting-ubi0-rootfs-in-init-do_mounts.c.patch
> > >>>>
> > >>>> Ditto.
> > >>>>
> > >>>>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch
> > >>>>
> > >>>> Ditto.
> > >>>>
> > >>>
> > >>> Same arguments as above. In addition, we do not want to hard-code the
> > >>> filesystem type used for the rootfs volume, as it can either be UBIFS
> > >>> or a read-only filesystem needing a ubiblock device. Thus we would
> > >>> need the bootloader to know which filesystem *type* is being used and
> > >>> then decide wether to pass 'rootfs=ubiX:Y' or
> > >>> 'ubiblock=... rootfs=/dev/ubiblock0'.
> > >>
> > >> What is wrong with having a very minimal initramfs to do such an
> > >> auto discovery logic?
> > > 
> > > Sorry, but we are not talking about traditional desktop or server
> > > systems. OpenWrt/LEDE runs on devices with as little as 32 MB of RAM
> > > and 4 MB of Flash. Devices with UBI will typically have at least
> > > 32 MB of NAND Flash, but still not necessarily a lot of RAM.
> > > Have a look at
> > > https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/dts/BTHOMEHUBV2B.dts
> > > for a good example of the type of hardware we are talking about.
[...]
> > > Apart from that we already automatically generate initramfs-based
> > > images for devices to be used in situations where you cannot or don't
> > > want to touch the devices' flash.
[...]
> > >> This is something you need to discuss wit DT folks.
> > >> I'm not per se against UBI DT bindings it just does not feel right
> > >> and contradicts my understanding of it.

I haven't followed the entire thread, though I've skimmed most of it,
and I agree I don't see a good reason for UBI DT bindings.

> > > I agree with your view when it comes to describing UBI volumes
> > > in the device tree. That's wrong just as it would be wrong to add
> > > DT bindings for LVM2.
> > > As said previously, the situation is different for flash chips having
> > > partitions defined for use with UBI on some boards.

I disagree. How is the situation different?

> > Everything can be done with the existing framework. DT binding maybe be nice
> > for your use case but I fear it is not applicable.
> 
> Yes, it can be done. But you would have to define your flash chips in
> device-tree, you may define the mtd partitions either in DT or by using
> mtdparts=... in the cmdline. Now to define which MTD device to
> ubiattach you will have to use ubi.mtd=X on the cmdline. If you defined
> your partitions inside the of_node of the flash chip, changes there
> (which do happen) may change the index of the partition you want to
> ubi-attach. C'mon. This *is* messy. Just having a compatible-flag to
> attach the partition would already greatly improve things.

To be clear, I'd really like to avoid relying on and extending fixed
partitioning like defined in Documentation/.../mtd/partitions.txt. So
anything we can do to help you plan for not relying on that case would
be awesome. For instance, with this series, there will hopefully be more
cases where you don't need your whole partition layout in DT:

http://lists.infradead.org/pipermail/linux-mtd/2015-December/064076.html

So what will you do then?

Also, why are you relying on the index form of ubi.mtd=X (e.g.,
ubi.mtd=0)? Why can't you use ubi.mtd=rootfs (and define 'label =
"rootfs"' with the existing fixed-partition DT bindings)?

http://linux-mtd.infradead.org/faq/ubi.html#L_attachmtd

That seems less likely to break, and it could probably be adapted/reused
on systems where you might not put the entire partition layout in device
tree. (i.e., you don't have to reinvent your wheel if you start using
non-"fixed-partitions" device trees.)

> Hauke: Maybe we should discuss the two patches individually, as I feel
> that ubiattaching specific mtd partitions flagged in DT is quite
> straight forward, while specifying volumes inside the ubi device might
> be out-of-scope.

I agree that those two concepts have a quite different likelihood of
being accepted (the former is possible; the latter seems highly
suspect).

Brian

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Ezequiel Garcia]

On 18 June 2016 at 16:35, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> On 06/18/2016 09:30 PM, Richard Weinberger wrote:
>> Am 18.06.2016 um 21:17 schrieb Hauke Mehrtens:
>>> This makes it possible to open a ubi layer in device tree, this is
>>> helpful when the rootfs is on a ubi layer. It loops though all mtd
>>> partitions and mounts the partition which is compatible with
>>> "ubi,volume". The same was only possible with kernel command line
>>> arguments before.
>>>
>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>> ---
>>>  Documentation/devicetree/bindings/mtd/ubi.txt | 33 ++++++++++++++
>>>  drivers/mtd/ubi/block.c                       | 63 +++++++++++++++++++++++++++
>>>  2 files changed, 96 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mtd/ubi.txt
>>
>> Some time ago I thought about an UBI DT binding too, but
>> I have been told that device tree is only for describing the hardware
>> and nothing else.
>> So I fear this will be rejected by DT folks.
>
> There are some devices on the market that are storing the root file
> system in an ubi layer.
> To make this work there are currently two options.
> 1. patch the kernel to specify which partitions to open in the code.

I wouldn't go as far as to claim this an option.

> 2. provide some kernel command line parameter which specifies what to open.
>

There's another option: use an initramfs, where you can do whatever you want
to find your UBI and mount it.

Frankly, I don't see a real need for this in the devicetree, other than solving
someone's particular workflow/use case.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Hauke Mehrtens]

On 06/24/2016 08:28 PM, Ezequiel Garcia wrote:
> On 18 June 2016 at 16:35, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>> On 06/18/2016 09:30 PM, Richard Weinberger wrote:
>>> Am 18.06.2016 um 21:17 schrieb Hauke Mehrtens:
>>>> This makes it possible to open a ubi layer in device tree, this is
>>>> helpful when the rootfs is on a ubi layer. It loops though all mtd
>>>> partitions and mounts the partition which is compatible with
>>>> "ubi,volume". The same was only possible with kernel command line
>>>> arguments before.
>>>>
>>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>>> ---
>>>>  Documentation/devicetree/bindings/mtd/ubi.txt | 33 ++++++++++++++
>>>>  drivers/mtd/ubi/block.c                       | 63 +++++++++++++++++++++++++++
>>>>  2 files changed, 96 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/mtd/ubi.txt
>>>
>>> Some time ago I thought about an UBI DT binding too, but
>>> I have been told that device tree is only for describing the hardware
>>> and nothing else.
>>> So I fear this will be rejected by DT folks.
>>
>> There are some devices on the market that are storing the root file
>> system in an ubi layer.
>> To make this work there are currently two options.
>> 1. patch the kernel to specify which partitions to open in the code.
> 
> I wouldn't go as far as to claim this an option.

I would also not do this, but when you look into the Linux patches of
"professional" embedded devices you see many strange things.

> 
>> 2. provide some kernel command line parameter which specifies what to open.
>>
> 
> There's another option: use an initramfs, where you can do whatever you want
> to find your UBI and mount it.

An initramfs probably needs some 10 to 100 KBytes of additional memory
on the flash chip and we only have 4 MBytes on some devices for the
bootloader, kernel and rootfs, adding 100 KBytes there is a significant
amount of memory. Instead of adding a initramfs I would like to be able
to define the rootfs somewhere in device tree and not by adding a
bootargs line into device tree. These particular patches are probably
the wrong way, but nobody told me how to do this in a better way.

> Frankly, I don't see a real need for this in the devicetree, other than solving
> someone's particular workflow/use case.

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Richard Weinberger]

Am 25.06.2016 um 22:20 schrieb Hauke Mehrtens:
>> There's another option: use an initramfs, where you can do whatever you want
>> to find your UBI and mount it.
> 
> An initramfs probably needs some 10 to 100 KBytes of additional memory
> on the flash chip and we only have 4 MBytes on some devices for the
> bootloader, kernel and rootfs, adding 100 KBytes there is a significant
> amount of memory. Instead of adding a initramfs I would like to be able
> to define the rootfs somewhere in device tree and not by adding a
> bootargs line into device tree. These particular patches are probably
> the wrong way, but nobody told me how to do this in a better way.

I think it is time to bring this discussion to a end.
It has been stated multiple times why DT is not the right thing for the
job and how all you need can be solved without an UBI DT binding.

Consider both patches as rejected.

Thanks,
//richard

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

Hi!

I got some remarks here:

On Sat, Jun 18, 2016 at 09:17:55PM +0200, Hauke Mehrtens wrote:
> This makes it possible to open a ubi layer in device tree, this is
> helpful when the rootfs is on a ubi layer. It loops though all mtd
> partitions and mounts the partition which is compatible with
> "ubi,volume". The same was only possible with kernel command line
> arguments before.

Strictly speaking this doesn't describe what this change does.
Rather than mounting anything you are creating ubiblock devices...

More comments in-line.

> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  Documentation/devicetree/bindings/mtd/ubi.txt | 33 ++++++++++++++
>  drivers/mtd/ubi/block.c                       | 63 +++++++++++++++++++++++++++
>  2 files changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/ubi.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/ubi.txt b/Documentation/devicetree/bindings/mtd/ubi.txt
> new file mode 100644
> index 0000000..5fcd47e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/ubi.txt
> @@ -0,0 +1,33 @@
> +UBI - Unsorted block images
> +
> +Describe of a UBI layer in device tree.
> +
> + - compatible:		"ubi,device", This marks a partition that contains
> + 			a ubi layer.
> + - vid_hdr_offs:	Optional parameter specifies UBI VID header position
> + 			to be used by UBI. (default value if 0)
> + - max_beb_per1024:	Optional parameter specifies the maximum expected bad
> + 			eraseblock per 1024 eraseblocks.
> + 			(default value CONFIG_MTD_UBI_BEB_LIMIT)
> + -ubi_num:		Optional parameter specifies UBI device number
> + 			which have to be assigned to the newly created UBI
> + 			device (assigned automatically by default)
> +
> +Example:
> +
> +partitions {
> +	compatible = "fixed-partitions";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	partition@0 {
> +		label = "uboot";
> +		reg = <0x00000 0x100000>;
> +	};
> +
> +	partition@1c0000 {
> +		label = "system_sw";
> +		reg = <0x1c0000 0xc800000>;
> +		compatible = "ubi,device";
> +	};
> +};

Similar to the other patch, the example in the documentation is
swapped and doesn't match with the code added by the patch.



> diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
> index ebf46ad..5ed390d 100644
> --- a/drivers/mtd/ubi/block.c
> +++ b/drivers/mtd/ubi/block.c

Wait a moment: What about ubifs being the root filesystem? That
doesn't need a ubiblock device to be created...


> @@ -1,6 +1,7 @@
>  /*
>   * Copyright (c) 2014 Ezequiel Garcia
>   * Copyright (c) 2011 Free Electrons
> + * Copyright (c) 2016 Hauke Mehrtens <hauke@hauke-m.de>
>   *
>   * Driver parameter handling strongly based on drivers/mtd/ubi/build.c
>   *   Copyright (c) International Business Machines Corp., 2006
> @@ -41,6 +42,7 @@
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  #include <linux/slab.h>
>  #include <linux/mtd/ubi.h>
>  #include <linux/workqueue.h>
> @@ -628,6 +630,64 @@ static void __init ubiblock_create_from_param(void)
>  	}
>  }
>  
> +static void __init ubiblock_create_from_device_tree(void)
> +{
> +	int ubi_num;
> +	const char *name;
> +	u32 mode;
> +	struct ubi_device *ubi;
> +	struct ubi_volume_desc *desc;
> +	struct ubi_volume_info vi;
> +	struct mtd_info *mtd;
> +	struct device_node *volume;
> +	int ret;
> +
> +	for (ubi_num = 0; ubi_num < UBI_MAX_DEVICES; ubi_num++) {
> +		ubi = ubi_get_device(ubi_num);
> +		if (!ubi)
> +			continue;
> +		mtd = ubi->mtd;
> +		if (!mtd || !of_device_is_compatible(mtd->dev.of_node,
> +						     "ubi,device")) {
> +			ubi_put_device(ubi);
> +			continue;
> +		}
> +
> +		for_each_child_of_node(mtd->dev.of_node, volume) {
> +			if (!of_device_is_compatible(volume, "ubi,volume"))
> +				continue;
> +
> +			ret = of_property_read_string(volume, "name", &name);
> +			if (ret)
> +				continue;
> +
> +			ret = of_property_read_u32(volume, "ubi-mode", &mode);
> +			if (ret)
> +				continue;
> +
> +			desc = ubi_open_volume_nm(ubi_num, name, mode);
> +			if (IS_ERR(desc)) {
> +				pr_err(
> +				       "UBI: block: can't open volume %s on ubi%d, err=%ld",
> +				       name, ubi_num, PTR_ERR(desc));
> +				continue;
> +			}
> +
> +			ubi_get_volume_info(desc, &vi);
> +			ubi_close_volume(desc);
> +
> +			ret = ubiblock_create(&vi);
> +			if (ret) {
> +				pr_err(
> +				       "UBI: block: can't add '%s' volume on ubi%d, err=%d",
> +				       vi.name, ubi_num, ret);
> +				continue;
> +			}
> +		}
> +		ubi_put_device(ubi);
> +	}
> +}
> +
>  static void ubiblock_remove_all(void)
>  {
>  	struct ubiblock *next;
> @@ -658,6 +718,9 @@ int __init ubiblock_init(void)
>  	 */
>  	ubiblock_create_from_param();
>  
> +	/* Attach block devices from device tree */
> +	ubiblock_create_from_device_tree();
> +

Probably you also want to set ROOT_DEV similar to what we currently
do in

https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch

>  	/*
>  	 * Block devices are only created upon user requests, so we ignore
>  	 * existing volumes.
> -- 
> 2.8.1
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Hauke Mehrtens]

On 06/19/2016 01:56 AM, Daniel Golle wrote:
> Hi!
> 
> I got some remarks here:
> 
> On Sat, Jun 18, 2016 at 09:17:55PM +0200, Hauke Mehrtens wrote:
>> This makes it possible to open a ubi layer in device tree, this is
>> helpful when the rootfs is on a ubi layer. It loops though all mtd
>> partitions and mounts the partition which is compatible with
>> "ubi,volume". The same was only possible with kernel command line
>> arguments before.
> 
> Strictly speaking this doesn't describe what this change does.
> Rather than mounting anything you are creating ubiblock devices...
> 
> More comments in-line.
> 
>>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  Documentation/devicetree/bindings/mtd/ubi.txt | 33 ++++++++++++++
>>  drivers/mtd/ubi/block.c                       | 63 +++++++++++++++++++++++++++
>>  2 files changed, 96 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mtd/ubi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/ubi.txt b/Documentation/devicetree/bindings/mtd/ubi.txt
>> new file mode 100644
>> index 0000000..5fcd47e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/ubi.txt
>> @@ -0,0 +1,33 @@
>> +UBI - Unsorted block images
>> +
>> +Describe of a UBI layer in device tree.
>> +
>> + - compatible:		"ubi,device", This marks a partition that contains
>> + 			a ubi layer.
>> + - vid_hdr_offs:	Optional parameter specifies UBI VID header position
>> + 			to be used by UBI. (default value if 0)
>> + - max_beb_per1024:	Optional parameter specifies the maximum expected bad
>> + 			eraseblock per 1024 eraseblocks.
>> + 			(default value CONFIG_MTD_UBI_BEB_LIMIT)
>> + -ubi_num:		Optional parameter specifies UBI device number
>> + 			which have to be assigned to the newly created UBI
>> + 			device (assigned automatically by default)
>> +
>> +Example:
>> +
>> +partitions {
>> +	compatible = "fixed-partitions";
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +
>> +	partition@0 {
>> +		label = "uboot";
>> +		reg = <0x00000 0x100000>;
>> +	};
>> +
>> +	partition@1c0000 {
>> +		label = "system_sw";
>> +		reg = <0x1c0000 0xc800000>;
>> +		compatible = "ubi,device";
>> +	};
>> +};
> 
> Similar to the other patch, the example in the documentation is
> swapped and doesn't match with the code added by the patch.
> 
> 
> 
>> diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
>> index ebf46ad..5ed390d 100644
>> --- a/drivers/mtd/ubi/block.c
>> +++ b/drivers/mtd/ubi/block.c
> 
> Wait a moment: What about ubifs being the root filesystem? That
> doesn't need a ubiblock device to be created...

Should I write an error message and do nothing when this is a ubifs
partition?

>> @@ -1,6 +1,7 @@
>>  /*
>>   * Copyright (c) 2014 Ezequiel Garcia
>>   * Copyright (c) 2011 Free Electrons
>> + * Copyright (c) 2016 Hauke Mehrtens <hauke@hauke-m.de>
>>   *
>>   * Driver parameter handling strongly based on drivers/mtd/ubi/build.c
>>   *   Copyright (c) International Business Machines Corp., 2006
>> @@ -41,6 +42,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/list.h>
>>  #include <linux/mutex.h>
>> +#include <linux/of.h>
>>  #include <linux/slab.h>
>>  #include <linux/mtd/ubi.h>
>>  #include <linux/workqueue.h>
>> @@ -628,6 +630,64 @@ static void __init ubiblock_create_from_param(void)
>>  	}
>>  }
>>  
>> +static void __init ubiblock_create_from_device_tree(void)
>> +{
>> +	int ubi_num;
>> +	const char *name;
>> +	u32 mode;
>> +	struct ubi_device *ubi;
>> +	struct ubi_volume_desc *desc;
>> +	struct ubi_volume_info vi;
>> +	struct mtd_info *mtd;
>> +	struct device_node *volume;
>> +	int ret;
>> +
>> +	for (ubi_num = 0; ubi_num < UBI_MAX_DEVICES; ubi_num++) {
>> +		ubi = ubi_get_device(ubi_num);
>> +		if (!ubi)
>> +			continue;
>> +		mtd = ubi->mtd;
>> +		if (!mtd || !of_device_is_compatible(mtd->dev.of_node,
>> +						     "ubi,device")) {
>> +			ubi_put_device(ubi);
>> +			continue;
>> +		}
>> +
>> +		for_each_child_of_node(mtd->dev.of_node, volume) {
>> +			if (!of_device_is_compatible(volume, "ubi,volume"))
>> +				continue;
>> +
>> +			ret = of_property_read_string(volume, "name", &name);
>> +			if (ret)
>> +				continue;
>> +
>> +			ret = of_property_read_u32(volume, "ubi-mode", &mode);
>> +			if (ret)
>> +				continue;
>> +
>> +			desc = ubi_open_volume_nm(ubi_num, name, mode);
>> +			if (IS_ERR(desc)) {
>> +				pr_err(
>> +				       "UBI: block: can't open volume %s on ubi%d, err=%ld",
>> +				       name, ubi_num, PTR_ERR(desc));
>> +				continue;
>> +			}
>> +
>> +			ubi_get_volume_info(desc, &vi);
>> +			ubi_close_volume(desc);
>> +
>> +			ret = ubiblock_create(&vi);
>> +			if (ret) {
>> +				pr_err(
>> +				       "UBI: block: can't add '%s' volume on ubi%d, err=%d",
>> +				       vi.name, ubi_num, ret);
>> +				continue;
>> +			}
>> +		}
>> +		ubi_put_device(ubi);
>> +	}
>> +}
>> +
>>  static void ubiblock_remove_all(void)
>>  {
>>  	struct ubiblock *next;
>> @@ -658,6 +718,9 @@ int __init ubiblock_init(void)
>>  	 */
>>  	ubiblock_create_from_param();
>>  
>> +	/* Attach block devices from device tree */
>> +	ubiblock_create_from_device_tree();
>> +
> 
> Probably you also want to set ROOT_DEV similar to what we currently
> do in
> 
> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch
> 
>>  	/*
>>  	 * Block devices are only created upon user requests, so we ignore
>>  	 * existing volumes.
>> -- 
>> 2.8.1
>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] ubi: mount partitions specified in device tree

[Daniel Golle]

On Sun, Jun 19, 2016 at 11:36:19PM +0200, Hauke Mehrtens wrote:
> On 06/19/2016 01:56 AM, Daniel Golle wrote:
> > Hi!
> > 
> > I got some remarks here:
> > 
> > On Sat, Jun 18, 2016 at 09:17:55PM +0200, Hauke Mehrtens wrote:
> >> This makes it possible to open a ubi layer in device tree, this is
> >> helpful when the rootfs is on a ubi layer. It loops though all mtd
> >> partitions and mounts the partition which is compatible with
> >> "ubi,volume". The same was only possible with kernel command line
> >> arguments before.
> > 
> > Strictly speaking this doesn't describe what this change does.
> > Rather than mounting anything you are creating ubiblock devices...
> > 
> > More comments in-line.
> > 
> >>
> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> >> ---
> >>  Documentation/devicetree/bindings/mtd/ubi.txt | 33 ++++++++++++++
> >>  drivers/mtd/ubi/block.c                       | 63 +++++++++++++++++++++++++++
> >>  2 files changed, 96 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mtd/ubi.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mtd/ubi.txt b/Documentation/devicetree/bindings/mtd/ubi.txt
> >> new file mode 100644
> >> index 0000000..5fcd47e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mtd/ubi.txt
> >> @@ -0,0 +1,33 @@
> >> +UBI - Unsorted block images
> >> +
> >> +Describe of a UBI layer in device tree.
> >> +
> >> + - compatible:		"ubi,device", This marks a partition that contains
> >> + 			a ubi layer.
> >> + - vid_hdr_offs:	Optional parameter specifies UBI VID header position
> >> + 			to be used by UBI. (default value if 0)
> >> + - max_beb_per1024:	Optional parameter specifies the maximum expected bad
> >> + 			eraseblock per 1024 eraseblocks.
> >> + 			(default value CONFIG_MTD_UBI_BEB_LIMIT)
> >> + -ubi_num:		Optional parameter specifies UBI device number
> >> + 			which have to be assigned to the newly created UBI
> >> + 			device (assigned automatically by default)
> >> +
> >> +Example:
> >> +
> >> +partitions {
> >> +	compatible = "fixed-partitions";
> >> +	#address-cells = <1>;
> >> +	#size-cells = <1>;
> >> +
> >> +	partition@0 {
> >> +		label = "uboot";
> >> +		reg = <0x00000 0x100000>;
> >> +	};
> >> +
> >> +	partition@1c0000 {
> >> +		label = "system_sw";
> >> +		reg = <0x1c0000 0xc800000>;
> >> +		compatible = "ubi,device";
> >> +	};
> >> +};
> > 
> > Similar to the other patch, the example in the documentation is
> > swapped and doesn't match with the code added by the patch.
> > 
> > 
> > 
> >> diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
> >> index ebf46ad..5ed390d 100644
> >> --- a/drivers/mtd/ubi/block.c
> >> +++ b/drivers/mtd/ubi/block.c
> > 
> > Wait a moment: What about ubifs being the root filesystem? That
> > doesn't need a ubiblock device to be created...
> 
> Should I write an error message and do nothing when this is a ubifs
> partition?

Just do nothing if you find a UBIFS magic. I reckon there shouldn't
be any error message because it's not an error but rather just another
valid case. Maybe an info message could be useful for starters...


> 
> >> @@ -1,6 +1,7 @@
> >>  /*
> >>   * Copyright (c) 2014 Ezequiel Garcia
> >>   * Copyright (c) 2011 Free Electrons
> >> + * Copyright (c) 2016 Hauke Mehrtens <hauke@hauke-m.de>
> >>   *
> >>   * Driver parameter handling strongly based on drivers/mtd/ubi/build.c
> >>   *   Copyright (c) International Business Machines Corp., 2006
> >> @@ -41,6 +42,7 @@
> >>  #include <linux/kernel.h>
> >>  #include <linux/list.h>
> >>  #include <linux/mutex.h>
> >> +#include <linux/of.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/mtd/ubi.h>
> >>  #include <linux/workqueue.h>
> >> @@ -628,6 +630,64 @@ static void __init ubiblock_create_from_param(void)
> >>  	}
> >>  }
> >>  
> >> +static void __init ubiblock_create_from_device_tree(void)
> >> +{
> >> +	int ubi_num;
> >> +	const char *name;
> >> +	u32 mode;
> >> +	struct ubi_device *ubi;
> >> +	struct ubi_volume_desc *desc;
> >> +	struct ubi_volume_info vi;
> >> +	struct mtd_info *mtd;
> >> +	struct device_node *volume;
> >> +	int ret;
> >> +
> >> +	for (ubi_num = 0; ubi_num < UBI_MAX_DEVICES; ubi_num++) {
> >> +		ubi = ubi_get_device(ubi_num);
> >> +		if (!ubi)
> >> +			continue;
> >> +		mtd = ubi->mtd;
> >> +		if (!mtd || !of_device_is_compatible(mtd->dev.of_node,
> >> +						     "ubi,device")) {
> >> +			ubi_put_device(ubi);
> >> +			continue;
> >> +		}
> >> +
> >> +		for_each_child_of_node(mtd->dev.of_node, volume) {
> >> +			if (!of_device_is_compatible(volume, "ubi,volume"))
> >> +				continue;
> >> +
> >> +			ret = of_property_read_string(volume, "name", &name);
> >> +			if (ret)
> >> +				continue;
> >> +
> >> +			ret = of_property_read_u32(volume, "ubi-mode", &mode);
> >> +			if (ret)
> >> +				continue;
> >> +
> >> +			desc = ubi_open_volume_nm(ubi_num, name, mode);
> >> +			if (IS_ERR(desc)) {
> >> +				pr_err(
> >> +				       "UBI: block: can't open volume %s on ubi%d, err=%ld",
> >> +				       name, ubi_num, PTR_ERR(desc));
> >> +				continue;
> >> +			}
> >> +
> >> +			ubi_get_volume_info(desc, &vi);
> >> +			ubi_close_volume(desc);
> >> +
> >> +			ret = ubiblock_create(&vi);
> >> +			if (ret) {
> >> +				pr_err(
> >> +				       "UBI: block: can't add '%s' volume on ubi%d, err=%d",
> >> +				       vi.name, ubi_num, ret);
> >> +				continue;
> >> +			}
> >> +		}
> >> +		ubi_put_device(ubi);
> >> +	}
> >> +}
> >> +
> >>  static void ubiblock_remove_all(void)
> >>  {
> >>  	struct ubiblock *next;
> >> @@ -658,6 +718,9 @@ int __init ubiblock_init(void)
> >>  	 */
> >>  	ubiblock_create_from_param();
> >>  
> >> +	/* Attach block devices from device tree */
> >> +	ubiblock_create_from_device_tree();
> >> +
> > 
> > Probably you also want to set ROOT_DEV similar to what we currently
> > do in
> > 
> > https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/patches-4.4/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch
> > 
> >>  	/*
> >>  	 * Block devices are only created upon user requests, so we ignore
> >>  	 * existing volumes.
> >> -- 
> >> 2.8.1
> >>
> >>
> >> ______________________________________________________
> >> Linux MTD discussion mailing list
> >> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/