[PATCH v2 3/5] mtd: ofpart: update devicetree binding specification

[PATCH v2 3/5] mtd: ofpart: update devicetree binding specification

[Michal Suchanek]

To avoid conflict with other drivers using subnodes of the mtd device
create only one ofpart-specific node rather than any number of
arbitrary partition subnodes.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 .../devicetree/bindings/mtd/partition.txt          | 68 +++++++++++++---------
 1 file changed, 40 insertions(+), 28 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
index 8e5557d..367ce4e 100644
--- a/Documentation/devicetree/bindings/mtd/partition.txt
+++ b/Documentation/devicetree/bindings/mtd/partition.txt
@@ -4,10 +4,16 @@ Partitions can be represented by sub-nodes of an mtd device. This can be used
 on platforms which have strong conventions about which portions of a flash are
 used for what purposes, but which don't use an on-flash partition table such
 as RedBoot.
+
+The partition table should be in ofpart subnode of the mtd node. Partitions are
+defined as subnodes of the ofpart node.
+
+For backwards compatibility partitions as direct subnodes of the mtd device are
+supported. This use is discouraged.
 NOTE: if the sub-node has a compatible string, then it is not a partition.
 
-#address-cells & #size-cells must both be present in the mtd device. There are
-two valid values for both:
+#address-cells & #size-cells must both be present in the ofpart subnode of the
+mtd device. There are two valid values for both:
 <1>: for partitions that require a single 32-bit cell to represent their
      size/address (aka the value is below 4 GiB)
 <2>: for partitions that require two 32-bit cells to represent their
@@ -28,44 +34,50 @@ Examples:
 
 
 flash@0 {
-	#address-cells = <1>;
-	#size-cells = <1>;
+	ofpart {
+		#address-cells = <1>;
+		#size-cells = <1>;
 
-	partition@0 {
-		label = "u-boot";
-		reg = <0x0000000 0x100000>;
-		read-only;
-	};
+		partition@0 {
+			label = "u-boot";
+			reg = <0x0000000 0x100000>;
+			read-only;
+		};
 
-	uimage@100000 {
-		reg = <0x0100000 0x200000>;
+		uimage@100000 {
+			reg = <0x0100000 0x200000>;
+		};
 	};
 };
 
 flash@1 {
-	#address-cells = <1>;
-	#size-cells = <2>;
+	ofpart {
+		#address-cells = <1>;
+		#size-cells = <2>;
 
-	/* a 4 GiB partition */
-	partition@0 {
-		label = "filesystem";
-		reg = <0x00000000 0x1 0x00000000>;
+		/* a 4 GiB partition */
+		partition@0 {
+			label = "filesystem";
+			reg = <0x00000000 0x1 0x00000000>;
+		};
 	};
 };
 
 flash@2 {
-	#address-cells = <2>;
-	#size-cells = <2>;
+	ofpart {
+		#address-cells = <2>;
+		#size-cells = <2>;
 
-	/* an 8 GiB partition */
-	partition@0 {
-		label = "filesystem #1";
-		reg = <0x0 0x00000000 0x2 0x00000000>;
-	};
+		/* an 8 GiB partition */
+		partition@0 {
+			label = "filesystem #1";
+			reg = <0x0 0x00000000 0x2 0x00000000>;
+		};
 
-	/* a 4 GiB partition */
-	partition@200000000 {
-		label = "filesystem #2";
-		reg = <0x2 0x00000000 0x1 0x00000000>;
+		/* a 4 GiB partition */
+		partition@200000000 {
+			label = "filesystem #2";
+			reg = <0x2 0x00000000 0x1 0x00000000>;
+		};
 	};
 };
-- 
2.1.4


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

[PATCH v2 5/5] mtd: ofpart: move ofpart partitions to a dedicated dt node

[Michal Suchanek]

Parsing direct subnodes of a mtd device as partitions is unreliable
since the mtd device is also part of its bus subsystem and can contain
bus data in subnodes.

Move ofpart data to a subnode of its own so it is clear which data is
part of the partition layout.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/mtd/ofpart.c | 56 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index aa26c32..2c28aaa 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -29,23 +29,33 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 				   struct mtd_partition **pparts,
 				   struct mtd_part_parser_data *data)
 {
-	struct device_node *node;
+	struct device_node *mtd_node;
+	struct device_node *ofpart_node;
 	const char *partname;
 	struct device_node *pp;
 	int nr_parts, i;
+	bool dedicated = true;
 
 
 	if (!data)
 		return 0;
 
-	node = data->of_node;
-	if (!node)
+	mtd_node = data->of_node;
+	if (!mtd_node)
 		return 0;
 
+	ofpart_node = of_get_child_by_name(mtd_node, "ofpart");
+	if (!ofpart_node) {
+		pr_warn("%s: 'ofpart' subnode not found on %s. Trying to parse direct subnodes as partitions.\n",
+			master->name, mtd_node->full_name);
+		ofpart_node = mtd_node;
+		dedicated = false;
+	}
+
 	/* First count the subnodes */
 	nr_parts = 0;
-	for_each_child_of_node(node,  pp) {
-		if (node_has_compatible(pp))
+	for_each_child_of_node(ofpart_node,  pp) {
+		if (!dedicated && node_has_compatible(pp))
 			continue;
 
 		nr_parts++;
@@ -59,22 +69,36 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 		return -ENOMEM;
 
 	i = 0;
-	for_each_child_of_node(node,  pp) {
+	for_each_child_of_node(ofpart_node,  pp) {
 		const __be32 *reg;
 		int len;
 		int a_cells, s_cells;
 
-		if (node_has_compatible(pp))
-			continue;
+		if (!dedicated && node_has_compatible(pp))
+				continue;
 
 		reg = of_get_property(pp, "reg", &len);
 		if (!reg) {
+			if (dedicated) {
+				pr_debug("%s: ofpart partition %s (%s) missing reg property.\n",
+					 master->name, pp->full_name,
+					 mtd_node->full_name);
+				goto ofpart_fail;
+			} else {
 			nr_parts--;
 			continue;
+			}
 		}
 
 		a_cells = of_n_addr_cells(pp);
 		s_cells = of_n_size_cells(pp);
+		if (len / 4 != a_cells + s_cells) {
+			pr_debug("%s: ofpart partition %s (%s) error parsing reg property.\n",
+				 master->name, pp->full_name,
+				 mtd_node->full_name);
+			goto ofpart_fail;
+		}
+
 		(*pparts)[i].offset = of_read_number(reg, a_cells);
 		(*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
 
@@ -92,15 +116,15 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 		i++;
 	}
 
-	if (!i) {
-		of_node_put(pp);
-		pr_err("No valid partition found on %s\n", node->full_name);
-		kfree(*pparts);
-		*pparts = NULL;
-		return -EINVAL;
-	}
-
 	return nr_parts;
+
+ofpart_fail:
+	pr_err("%s: error parsing ofpart partition %s (%s)\n",
+	       master->name, pp->full_name, mtd_node->full_name);
+	of_node_put(pp);
+	kfree(*pparts);
+	*pparts = NULL;
+	return -EINVAL;
 }
 
 static struct mtd_part_parser ofpart_parser = {
-- 
2.1.4


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

[PATCH v2 4/5] mtd: ofpart: document the lock flag.

[Michal Suchanek]

The lock flag of ofpart is undocumented. Add to binding doc.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 Documentation/devicetree/bindings/mtd/partition.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
index 367ce4e..19a1203 100644
--- a/Documentation/devicetree/bindings/mtd/partition.txt
+++ b/Documentation/devicetree/bindings/mtd/partition.txt
@@ -29,6 +29,7 @@ Optional properties:
   partition should only be mounted read-only. This is usually used for flash
   partitions containing early-boot firmware images or data which should not be
   clobbered.
+- lock : Clear always locked after reset flag
 
 Examples:
 
-- 
2.1.4


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

[PATCH v2 0/5] improve mtdpart robustness

[Michal Suchanek]

Hello,

this is v2 series following the discussion about conflict between s3c64xx and
ofpart.

I tried loading some fabricated ofpart partitions and it appears to work
reasonably.

ofpart assigns partition numbers backwards from bottom to top but that's
probably not something to be concerned about. Sorting partitions by offset
would be nice but that's way beyond just making the two drivers not conflict.

Thanks

Michal

Michal Suchanek (5):
  mtd: mtdpart: add debug prints to partition parser.
  mtd: mtdpart: Do not fail mtd probe when parsing partitions fails.
  mtd: ofpart: update devicetree binding specification
  mtd: ofpart: document the lock flag.
  mtd: ofpart: move ofpart partitions to a dedicated dt node

 .../devicetree/bindings/mtd/partition.txt          | 69 +++++++++++++---------
 drivers/mtd/mtdpart.c                              | 14 ++++-
 drivers/mtd/ofpart.c                               | 56 +++++++++++++-----
 3 files changed, 93 insertions(+), 46 deletions(-)

-- 
2.1.4


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

[PATCH v2 2/5] mtd: mtdpart: Do not fail mtd probe when parsing partitions fails.

[Michal Suchanek]

Due to wrong assumption in ofpart ofpart fails on Exynos on SPI chips
with no partitions because the subnode containing controller data
confuses the ofpart parser.

Thus compiling in ofpart support automatically fails probing any SPI NOR
flash without partitions on Exynos.

Compiling in a partitioning scheme should not cause probe of otherwise
valid device to fail.

Remove that failure possibility when MTD_PARTITIONED_MASTER is set.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>

---
v2:

 - only allow partition parsing failure when MTD_PARTITIONED_MASTER is
   set
---
 drivers/mtd/mtdpart.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 31888c2..6eafbe9 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -774,10 +774,15 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		if (ret > 0) {
 			printk(KERN_NOTICE "%d %s partitions found on MTD device %s\n",
 			       ret, parser->name, master->name);
-			break;
+			return ret;
+		}
+		if (!IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER) && (ret < 0)) {
+			pr_err("Error parsing %s partitions on %s\n",
+			       parser->name, master->name);
+			return ret;
 		}
 	}
-	return ret;
+	return 0;
 }
 
 int mtd_is_partition(const struct mtd_info *mtd)
-- 
2.1.4


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

[PATCH v2 1/5] mtd: mtdpart: add debug prints to partition parser.

[Michal Suchanek]

The probe of a mtd device can fail when a partition parser returns
error. The failure due to partition parsing can be quite mysterious when
multiple partitioning schemes are comiled in and any of them can fail
the probe.

Add debug prints which show what parsers were tried and what they
returned.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
v2:

 - reformat debug messages
---
 drivers/mtd/mtdpart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index cafdb88..31888c2 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -759,12 +759,17 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		types = default_mtd_part_types;
 
 	for ( ; ret <= 0 && *types; types++) {
+		pr_debug("%s: parsing partitions %s\n", master->name, *types);
 		parser = get_partition_parser(*types);
 		if (!parser && !request_module("%s", *types))
 			parser = get_partition_parser(*types);
+		pr_debug("%s: got parser %s\n", master->name,
+			 parser ? parser->name : NULL);
 		if (!parser)
 			continue;
 		ret = (*parser->parse_fn)(master, pparts, data);
+		pr_debug("%s: parser %s: %i\n",
+			 master->name, parser->name, ret);
 		put_partition_parser(parser);
 		if (ret > 0) {
 			printk(KERN_NOTICE "%d %s partitions found on MTD device %s\n",
-- 
2.1.4


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

Re: [PATCH v2 5/5] mtd: ofpart: move ofpart partitions to a dedicated dt node

[Boris Brezillon]

Hi Michal,

On Thu, 30 Jul 2015 12:10:42 +0200
Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Parsing direct subnodes of a mtd device as partitions is unreliable
> since the mtd device is also part of its bus subsystem and can contain
> bus data in subnodes.
> 
> Move ofpart data to a subnode of its own so it is clear which data is
> part of the partition layout.
> 
> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/mtd/ofpart.c | 56 +++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
> index aa26c32..2c28aaa 100644
> --- a/drivers/mtd/ofpart.c
> +++ b/drivers/mtd/ofpart.c
> @@ -29,23 +29,33 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>  				   struct mtd_partition **pparts,
>  				   struct mtd_part_parser_data *data)
>  {
> -	struct device_node *node;
> +	struct device_node *mtd_node;
> +	struct device_node *ofpart_node;
>  	const char *partname;
>  	struct device_node *pp;
>  	int nr_parts, i;
> +	bool dedicated = true;
>  
>  
>  	if (!data)
>  		return 0;
>  
> -	node = data->of_node;
> -	if (!node)
> +	mtd_node = data->of_node;
> +	if (!mtd_node)
>  		return 0;
>  
> +	ofpart_node = of_get_child_by_name(mtd_node, "ofpart");

Hm, you should use a more generic name, ofpart of the linux MTD
DT partition parser, but another operating system might decide to name
it otherwise. I think "partitions" is more appropriate. 

> +	if (!ofpart_node) {
> +		pr_warn("%s: 'ofpart' subnode not found on %s. Trying to parse direct subnodes as partitions.\n",
> +			master->name, mtd_node->full_name);

Do we really want to complain here. I mean, a lot of users do not need
to define their partition in a different node.

> +		ofpart_node = mtd_node;
> +		dedicated = false;
> +	}
> +
>  	/* First count the subnodes */
>  	nr_parts = 0;
> -	for_each_child_of_node(node,  pp) {
> -		if (node_has_compatible(pp))
> +	for_each_child_of_node(ofpart_node,  pp) {
> +		if (!dedicated && node_has_compatible(pp))
>  			continue;
>  
>  		nr_parts++;
> @@ -59,22 +69,36 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>  		return -ENOMEM;
>  
>  	i = 0;
> -	for_each_child_of_node(node,  pp) {
> +	for_each_child_of_node(ofpart_node,  pp) {
>  		const __be32 *reg;
>  		int len;
>  		int a_cells, s_cells;
>  
> -		if (node_has_compatible(pp))
> -			continue;
> +		if (!dedicated && node_has_compatible(pp))
> +				continue;

Check your indentation (checkpatch should complain here).

>  
>  		reg = of_get_property(pp, "reg", &len);
>  		if (!reg) {
> +			if (dedicated) {
> +				pr_debug("%s: ofpart partition %s (%s) missing reg property.\n",
> +					 master->name, pp->full_name,
> +					 mtd_node->full_name);
> +				goto ofpart_fail;
> +			} else {
>  			nr_parts--;
>  			continue;

Ditto.

> +			}
>  		}
>  
>  		a_cells = of_n_addr_cells(pp);
>  		s_cells = of_n_size_cells(pp);
> +		if (len / 4 != a_cells + s_cells) {
> +			pr_debug("%s: ofpart partition %s (%s) error parsing reg property.\n",
> +				 master->name, pp->full_name,
> +				 mtd_node->full_name);
> +			goto ofpart_fail;
> +		}
> +

The above changes have nothing to do with the description you gave in
your commit message.

>  		(*pparts)[i].offset = of_read_number(reg, a_cells);
>  		(*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
>  
> @@ -92,15 +116,15 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>  		i++;
>  	}
>  
> -	if (!i) {
> -		of_node_put(pp);
> -		pr_err("No valid partition found on %s\n", node->full_name);
> -		kfree(*pparts);
> -		*pparts = NULL;
> -		return -EINVAL;
> -	}
> -

Are you sure you can safely remove this check?


Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] mtd: ofpart: move ofpart partitions to a dedicated dt node

[Michal Suchanek]

On 31 July 2015 at 18:06, Boris Brezillon
<boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Michal,
>
> On Thu, 30 Jul 2015 12:10:42 +0200
> Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Parsing direct subnodes of a mtd device as partitions is unreliable
>> since the mtd device is also part of its bus subsystem and can contain
>> bus data in subnodes.
>>
>> Move ofpart data to a subnode of its own so it is clear which data is
>> part of the partition layout.
>>
>> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/mtd/ofpart.c | 56 +++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
>> index aa26c32..2c28aaa 100644
>> --- a/drivers/mtd/ofpart.c
>> +++ b/drivers/mtd/ofpart.c
>> @@ -29,23 +29,33 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>>                                  struct mtd_partition **pparts,
>>                                  struct mtd_part_parser_data *data)
>>  {
>> -     struct device_node *node;
>> +     struct device_node *mtd_node;
>> +     struct device_node *ofpart_node;
>>       const char *partname;
>>       struct device_node *pp;
>>       int nr_parts, i;
>> +     bool dedicated = true;
>>
>>
>>       if (!data)
>>               return 0;
>>
>> -     node = data->of_node;
>> -     if (!node)
>> +     mtd_node = data->of_node;
>> +     if (!mtd_node)
>>               return 0;
>>
>> +     ofpart_node = of_get_child_by_name(mtd_node, "ofpart");
>
> Hm, you should use a more generic name, ofpart of the linux MTD
> DT partition parser, but another operating system might decide to name
> it otherwise. I think "partitions" is more appropriate.

Whatever. Presumably some dt maintainer will look at this and figure
out a name that perfectly fits current dt policy.

>
>> +     if (!ofpart_node) {
>> +             pr_warn("%s: 'ofpart' subnode not found on %s. Trying to parse direct subnodes as partitions.\n",
>> +                     master->name, mtd_node->full_name);
>
> Do we really want to complain here. I mean, a lot of users do not need
> to define their partition in a different node.

Yes, we do.

The binding without subnode is considered defective and obsolete.

>
>> +             ofpart_node = mtd_node;
>> +             dedicated = false;
>> +     }
>> +
>>       /* First count the subnodes */
>>       nr_parts = 0;
>> -     for_each_child_of_node(node,  pp) {
>> -             if (node_has_compatible(pp))
>> +     for_each_child_of_node(ofpart_node,  pp) {
>> +             if (!dedicated && node_has_compatible(pp))
>>                       continue;
>>
>>               nr_parts++;
>> @@ -59,22 +69,36 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>>               return -ENOMEM;
>>
>>       i = 0;
>> -     for_each_child_of_node(node,  pp) {
>> +     for_each_child_of_node(ofpart_node,  pp) {
>>               const __be32 *reg;
>>               int len;
>>               int a_cells, s_cells;
>>
>> -             if (node_has_compatible(pp))
>> -                     continue;
>> +             if (!dedicated && node_has_compatible(pp))
>> +                             continue;
>
> Check your indentation (checkpatch should complain here).
>
>>
>>               reg = of_get_property(pp, "reg", &len);
>>               if (!reg) {
>> +                     if (dedicated) {
>> +                             pr_debug("%s: ofpart partition %s (%s) missing reg property.\n",
>> +                                      master->name, pp->full_name,
>> +                                      mtd_node->full_name);
>> +                             goto ofpart_fail;
>> +                     } else {
>>                       nr_parts--;
>>                       continue;
>
> Ditto.

Well, it does not complain but the indent is definitely off here.

>
>> +                     }
>>               }
>>
>>               a_cells = of_n_addr_cells(pp);
>>               s_cells = of_n_size_cells(pp);
>> +             if (len / 4 != a_cells + s_cells) {
>> +                     pr_debug("%s: ofpart partition %s (%s) error parsing reg property.\n",
>> +                              master->name, pp->full_name,
>> +                              mtd_node->full_name);
>> +                     goto ofpart_fail;
>> +             }
>> +
>
> The above changes have nothing to do with the description you gave in
> your commit message.
>
>>               (*pparts)[i].offset = of_read_number(reg, a_cells);
>>               (*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
>>
>> @@ -92,15 +116,15 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>>               i++;
>>       }
>>
>> -     if (!i) {
>> -             of_node_put(pp);
>> -             pr_err("No valid partition found on %s\n", node->full_name);
>> -             kfree(*pparts);
>> -             *pparts = NULL;
>> -             return -EINVAL;
>> -     }
>> -
>
> Are you sure you can safely remove this check?

Yes. It was incomplete check to reject some partitioning schemes
considered invalid.

Now there is stricter checking above so this can be removed.

Basically the whole point of this patch is to remove this bogus check.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] mtd: ofpart: move ofpart partitions to a dedicated dt node

[Boris Brezillon]

On Fri, 31 Jul 2015 18:52:01 +0200
Michal Suchanek <hramrach@gmail.com> wrote:


> >
> >>               (*pparts)[i].offset = of_read_number(reg, a_cells);
> >>               (*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
> >>
> >> @@ -92,15 +116,15 @@ static int parse_ofpart_partitions(struct mtd_info *master,
> >>               i++;
> >>       }
> >>
> >> -     if (!i) {
> >> -             of_node_put(pp);
> >> -             pr_err("No valid partition found on %s\n", node->full_name);
> >> -             kfree(*pparts);
> >> -             *pparts = NULL;
> >> -             return -EINVAL;
> >> -     }
> >> -
> >
> > Are you sure you can safely remove this check?
> 
> Yes. It was incomplete check to reject some partitioning schemes
> considered invalid.
> 
> Now there is stricter checking above so this can be removed.

Indeed, I was worried about resources deallocation, but this is handle
by the caller, and if nr_parts is zero the master MTD device will
be exposed.




-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 5/5] mtd: ofpart: move ofpart partitions to a dedicated dt node

[Michal Suchanek]

On 31 July 2015 at 19:24, Boris Brezillon
<boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Fri, 31 Jul 2015 18:52:01 +0200
> Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>
>> >
>> >>               (*pparts)[i].offset = of_read_number(reg, a_cells);
>> >>               (*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
>> >>
>> >> @@ -92,15 +116,15 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>> >>               i++;
>> >>       }
>> >>
>> >> -     if (!i) {
>> >> -             of_node_put(pp);
>> >> -             pr_err("No valid partition found on %s\n", node->full_name);
>> >> -             kfree(*pparts);
>> >> -             *pparts = NULL;
>> >> -             return -EINVAL;
>> >> -     }
>> >> -
>> >
>> > Are you sure you can safely remove this check?
>>
>> Yes. It was incomplete check to reject some partitioning schemes
>> considered invalid.
>>
>> Now there is stricter checking above so this can be removed.
>
> Indeed, I was worried about resources deallocation, but this is handle
> by the caller, and if nr_parts is zero the master MTD device will
> be exposed.

Due to compatibility with the previous scheme there is still
possibility that partitions are allocated, and no partitions are
returned due to the nr_parts--;

So yes, the pparts could possibly leak in this case if there were more
partition parsers following ofpart and should be deallocated.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html