Re: [PATCH V4 1/2] mtd: add support for partition parsers

[Brian Norris <computersforpeace@gmail.com>]

On Tue, May 23, 2017 at 11:06:37AM +0200, Rafał Miłecki wrote:
> On 05/23/2017 10:14 AM, Rafał Miłecki wrote:
> >On 05/11/2017 08:21 PM, Brian Norris wrote:
> >>Maybe a better way to handle that offset modification is to either pass
> >>in the offset to add_mtd_partitions() (e.g., as a simple parameter), or
> >>else adapt add_mtd_partitions() to handle receiving a non-master as the
> >>first parameter. (Then you just pass "slave", and add_mtd_partitions()
> >>handle it with something like "if (mtd_is_partition(...))".)
> >>
> >>Then I wonder how we want the parenting structure to work; should the
> >>sub-partition have the "master" as its parent, or the original
> >>partition? I thought Richard had mentioned some problems with the
> >>existing parenting structure (with CONFIG_MTD_PARTITIONED_MASTER)
> >>already, but I don't remember what those were.
> >>
> >>Also, if you're "parsing" the slave, but "adding" to the master, then
> >>the bounds-checking logic in add_mtd_partitions() won't properly apply,
> >>and you'll be able to create sub-partitions that extend beyond the
> >>slave, right? If so, then I think (after auditing add_mtd_partitions() a
> >>little closer, and possibly adjusting some of its comments) you might
> >>really just want to pass 'slave', not 'slave->master'.
> >
> >I like this idea!
> 
> Oh, it's not that simple. Passing struct mtd_part to the add_mtd_partitions
> is simple, but it's getting complex afterwards.
> 
> 1) We can't simply adjust offset in add_mtd_partitions as we are still
>    dealing with const struct mtd_partition (note: const).
> 2) We can't adjust offset after calling allocate_partition as this would
>    bypass some validation happening in the allocate_partition.

I think I was assuming you'd use a tree structure, in which case you
don't have to modify the offsets at all. The offsets would get
translated by, e.g., a recursive call to part_read() (the subpartition's
part_read() would call the parent partition's part_read(), which would
adjust the offset and call the master's ->read() callback).

Or maybe the recursion (and tree structure) is not a great idea. You
came up with a mostly-OK solution that keeps a flat tree structure and
no recursion in your next version.

> 3) Passing an extra argume to the allocate_partition is a bad idea as it
>    already receives uint64_t cur_offset - this would be confusing.

Yeah, might be. But I think you came up with a different solution
anyway.

Brian