linux-kernel - Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 26 May 2017 00:21:17 +0200
From:   Andrea Adami <andrea.adami@...il.com>
To:     Brian Norris <computersforpeace@...il.com>
Cc:     linux-mtd@...ts.infradead.org,
        David Woodhouse <dwmw2@...radead.org>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>,
        Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
        Robert Jarzmik <robert.jarzmik@...e.fr>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-kernel@...r.kernel.org,
        Rafał Miłecki <zajec5@...il.com>
Subject: Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD
 partition parser

On Thu, May 25, 2017 at 11:10 PM, Brian Norris
<computersforpeace@...il.com> wrote:
> On Thu, May 25, 2017 at 10:47:37PM +0200, Andrea Adami wrote:
>> On Thu, May 25, 2017 at 9:25 PM, Brian Norris
>> <computersforpeace@...il.com> wrote:
>> > On Sat, Apr 22, 2017 at 01:20:13PM +0200, Andrea Adami wrote:
>> >> This is the specific parser for Sharp SL Series (Zaurus)
>> >>
>> >> Signed-off-by: Andrea Adami <andrea.adami@...il.com>
>> >> ---
>> >>  drivers/mtd/nand/tmio_nand.c | 4 +++-
>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/mtd/nand/tmio_nand.c b/drivers/mtd/nand/tmio_nand.c
>> >> index fc5e773..f3612ac 100644
>> >> --- a/drivers/mtd/nand/tmio_nand.c
>> >> +++ b/drivers/mtd/nand/tmio_nand.c
>> >> @@ -357,6 +357,8 @@ static void tmio_hw_stop(struct platform_device *dev, struct tmio_nand *tmio)
>> >>               cell->disable(dev);
>> >>  }
>> >>
>> >> +static const char * const probes[] = { "sharpslpart", NULL };
>> >
>> > This breaks anyone who might have used (or might want to use) the ofpart
>> > or cmdlinepart parsers. At a minimum, you need to include those in your
>> > array here.
>>
>> I have been under the wrong assumption there is cmdlinepart as last
>> option (if compiled) so I have taken a wrong example.
>> Grepping in /mt for probes gives many examples: what if I change it with
>>
>> static const char * const probes[] = { "sharpslpart", "cmdlinepart", NULL };
>>
>> ofpart is utopic at the moment: these machines are not yet converted
>> to devicetree and it will take a while.
>>
>> With this patchset we can move a step forward DT, removing all the
>> static partition definition from spitz.c, tosa.c, corgi.c and poodle.c
>>
>> I don't dare adding ofpart here: this will be done once Zaurus pxa
>> platform is moved to devicetree.
>
> What's the harm in including ofpart? It will be silently skipped if you
> don't have a conforming device tree.
>
>> > But really, I'd rather not add any more parser listings like this in
>> > drivers. Parser selection should be determined by the platform, not by
>> > the driver. See my last response to Rafal, who is trying to extend
>> > support for device-tree based listing of parsers:
>> >
>> > http://lists.infradead.org/pipermail/linux-mtd/2017-April/073729.html
>>
>> Ok then but remember these are obsolete devices and as far as I know
>> these nand drivers are only used on Zaurus devices. No future use I
>> guess.
>
> Yes, but the point is I don't want new examples of a bad pattern. And if
> you ever do gain device tree support, I would then be "breaking" your
> device tree if I dropped "sharpslpart" from your probe list.
>
>> > He has some more work posted to the mailing list since then; search the
>> > archives.
>> >
>> > I'll take a look at the parser itself, and maybe we can merge that. But
>> > I'm not likely to merge this patch, in any form.
>> >
>> The little parser itself is universal for all Zaurus pxa variants.
>> As said above, please consider we can remove many lines of board code.
>
> Speaking of board code: since this is all initialized by board files,
> why can't you put the "platform information" (i.e., the partition parser
> type(s)) in the platform data? e.g, struct sharpsl_nand_platform_data or
> struct tmio_nand_data. That'd resolve my concern about hardcoding lists
> in the driver.
>
> Brian

Brian,

I now understand your objections about hardcoding parsers in the drivers.
I'll ask the maintainers in case there were any objections: the patch
will cross mach-pxa and drivers/mtd.

Thanks again

Andrea