Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753387AbdDQOo3 (ORCPT ); Mon, 17 Apr 2017 10:44:29 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:52059 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752659AbdDQOo0 (ORCPT ); Mon, 17 Apr 2017 10:44:26 -0400 Date: Mon, 17 Apr 2017 16:44:12 +0200 From: Boris Brezillon To: Marek Vasut , Andrea Adami , linux-mtd@lists.infradead.org, David Woodhouse , Brian Norris , Richard Weinberger , Cyrille Pitchen Cc: Dmitry Eremin-Solenikov , Robert Jarzmik , Linus Walleij , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] mtd: sharpsl: add sharpslpart MTD partition parser Message-ID: <20170417164412.3dc5b457@bbrezillon> In-Reply-To: References: <1492287078-22369-1-git-send-email-andrea.adami@gmail.com> <1492287078-22369-2-git-send-email-andrea.adami@gmail.com> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5613 Lines: 123 Marek, Andrea, Before we even start discussing minor improvements (like coding style), I'd like to discuss the sharp FTL and partition table format, and decide whether we want to have such an old FTL included in the kernel. Actually, that's the very reason I asked Andrea to post his series as soon as possible even if some things were not perfect. I'll try to document myself on the on-flash format of the FTL and partition table before giving a definitive opinion, but I have the feeling this ancient FTL is not 100% safe, and by accepting an old (maybe unreliable?) FTL we are setting a precedent, and refusing other (broken) proprietary/vendor FTLs will be almost impossible after that. Maybe I'm wrong, and this FTL is really safe and can be used on other devices, that's why I'd like to understand how it works before giving my opinion. On Sun, 16 Apr 2017 18:07:47 +0200 Marek Vasut wrote: > On 04/15/2017 10:11 PM, Andrea Adami wrote: > > The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash > > and share the same layout of the first 7M partition, managed by Sharp FTL. > > > > The purpose of this self-contained patch is to add a common parser and > > remove the hardcoded sizes in the board files (these devices are not yet > > converted to devicetree). > > Users will have benefits because the mtdparts= tag will not be necessary > > anymore and they will be free to repartition the little sized flash. > > > > The obsolete bootloader can not pass the partitioning info to modern > > kernels anymore so it has to be read from flash at known logical addresses. > > (see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm ) Okay, IMO that's not really a good argument to support this partition parser. As Richard and I already mentioned several times, if your bootloader is not capable of passing valid mtdparts= you can hardcode it in the kernel using a default CMDLINE. Now, I understand that you want to support multiple devices with the same kernel, and having this partition parser would simplify your job. I also know you are developing a 2nd stage bootloader (based on kexec+a minimal initramfs) to address the limitations of the non-upgradable 1st stage bootloader. According to the rest of the description, you already have user-space tools to manipulate the partition-table and those are aware of the FTL layer, so I think you have all the basic blocks to get rid of this in-kernel implementation. All you need is a way to extract the partitions from the 2nd stage bootloader (using some tools you have in your initramfs) and build the according mtdparts= parameter to pass to kexec when booting the real kernel (the one used by the system). You tried to explain why it was not doable, but I still don't see why. You first said that not all systems had CONFIG_MTD_CMDLINE_PARTS enabled and that some people were booting distro kernels. Honestly, I think you have more chances to have CONFIG_MTD_CMDLINE_PARTS in those distro kernels than having your custom FTL enabled. Then you tried to explain that with the user-space only solution you wouldn't be able to support systems where the user had resized the partitions with the nandlogical tool, and I still don't see why. Maybe you can give more details to explain why this is impossible. Just going through all these details to say that, IMO, we should only consider inclusion of this feature if we think it's safe, because I think all that is done here can be done from user-space. One last thing I was wondering. You said you want to keep existing partitioning unchanged, but I'd recommend to just drop the existing partitioning and have a single 121MB partition with UBI on it. This way you get support for UBI volumes, which is doing exactly what this FTL+partition-table is doing but in a standard/safe way. What is forcing you to keep the existing partitioning exactly? > > > > In kernel, under arch/arm/mach-pxa we have already 8 machines: > > MACH_POODLE, MACH_CORGI, MACH_SHEPERD, MACH_HUSKY, MACH_AKITA, MACH_SPITZ, > > MACH_BORZOI, MACH_TOSA. > > Lost after the 2.4 vendor kernel are MACH_BOXER and MACH_TERRIER. > > > > Almost every model has different factory partitioning: add to this the > > units can be repartitioned by users with userspace tools (nandlogical) > > and installers for popular (back then) linux distributions. > > > > The Parameter Area in the first (boot) partition extends from 0x00040000 to > > 0x0007bfff (176k) and contains two copies of the partition table: > > ... > > 0x00060000: Partition Info1 16k > > 0x00064000: Partition Info2 16k > > 0x00668000: Model 16k > > ... > > > > The first 7M partition is managed by the Sharp FTL reserving 5% + 1 blocks > > for wear-leveling: some blocks are remapped and one layer of translation > > (logical to physical) is necessary. > > > > There isn't much documentation about this FTL in the 2.4 sources, just the > > MTD methods for reading and writing using logical addresses and the block > > management (wear-leveling, use counter). > > For the purpose of the MTD parser only the read part of the code was taken. > > > > The NAND drivers that can use this parser are sharpsl.c and tmio_nand.c. > > > > Signed-off-by: Andrea Adami I'll comment on the FTL and partition-table format later, after I had time to review it properly. In the meantime, I'd like other MTD maintainers to comment on my reply. Maybe I'm the only one to think that supporting old/unmaintainerd FTLs is a bad idea, and in this case I'll have to accept the situation ;-). Regards, Boris