Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 2 Mar 2021 05:50:07 +0100
From:   Ansuel Smith <ansuelsmth@gmail.com>
To:     =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>
Cc:     Richard Weinberger <richard@nod.at>, devicetree@vger.kernel.org,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Boris Brezillon <bbrezillon@kernel.org>,
        linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        linux-mtd@lists.infradead.org,
        Miquel Raynal <miquel.raynal@bootlin.com>
Subject: Re: [PATCH v2 1/3] mtd: partitions: ofpart: skip subnodes parse with
 compatible
Message-ID: <YD3D/zwfLlRITdHj@Ansuel-xps.localdomain>
References: <20210216212638.28382-1-ansuelsmth@gmail.com>
 <20210216212638.28382-2-ansuelsmth@gmail.com>
 <d504a4df-b1f7-b7c9-e62a-599d114d8a18@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d504a4df-b1f7-b7c9-e62a-599d114d8a18@gmail.com>
Precedence: bulk

On Tue, Mar 02, 2021 at 05:53:54PM +0100, Rafał Miłecki wrote:
> On 16.02.2021 22:26, Ansuel Smith wrote:
> > If a partitions structure is not used, parse direct subnodes as
> > fixed-partitions only if a compatible is not found or is of type
> > fixed-partition. A parser can be used directly on the subnode and
> > subnodes should not be parsed as fixed-partitions by default.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >   drivers/mtd/parsers/ofpart.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/mtd/parsers/ofpart.c b/drivers/mtd/parsers/ofpart.c
> > index daf507c123e6..4b363dd0311c 100644
> > --- a/drivers/mtd/parsers/ofpart.c
> > +++ b/drivers/mtd/parsers/ofpart.c
> > @@ -50,6 +50,11 @@ static int parse_fixed_partitions(struct mtd_info *master,
> >   			 master->name, mtd_node);
> >   		ofpart_node = mtd_node;
> >   		dedicated = false;
> > +
> > +		/* Skip parsing direct subnodes if a compatible is found and is not fixed-partitions */
> > +		if (node_has_compatible(ofpart_node) &&
> > +		    !of_device_is_compatible(ofpart_node, "fixed-partitions"))
> > +			return 0;
> >   	} else if (!of_device_is_compatible(ofpart_node, "fixed-partitions")) {
> >   		/* The 'partitions' subnode might be used by another parser */
> >   		return 0;
> 
> I admit I'm not familiar with the old binding, so let me know if my
> understanding is incorrect.
> 
> It seems to me however that your change will break parsing in cases
> like:
> 
> spi-flash@0 {
> 	compatible = "jedec,spi-nor";
> 	reg = <0x0>;
> 
> 	partition@0 {
> 		label = "bootloader";
> 		reg = <0x0 0x100000>;
> 	};
> };
> 
> nandcs@0 {
> 	compatible = "brcm,nandcs";
> 	reg = <0>;
> 
> 	partition@0 {
> 		label = "bootloader";
> 		reg = <0x0000000 0x10000>;
> 	};
> };
> 
> Did we ever use "fixed-partitions" without partitions { } subnode?

Hi, very good point. You are right and I didin't think about this case.
I don't want to assume false statement, but since the ofpart parser and
the partitions structure should have been pushed at the same time, there
shouldn't be any use of "fixed-partitions" without partitions { }
subnodes. With this assumtion, the current implementation looks to be the 
cleanest way to skip parsing. (if the parsing is dubious, don't parse at
all... The idea was that)
The hacky and IMHO dirty way to solve this is add a bool to directly
skip the subnode parsing and check for that. Something like
"no-fixed-partition" that would disable the ofnode parser with no
partitions { } subnode would accomplish the same result of this patch
and keep compatibility with nodes scheme you pointed out.