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

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@gmail.com> 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@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");

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

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@free-electrons.com> wrote:
> Hi Michal,
>
> On Thu, 30 Jul 2015 12:10:42 +0200
> Michal Suchanek <hramrach@gmail.com> 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@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");
>
> 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

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@free-electrons.com> wrote:
> 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.

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