Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754249AbdHYExU (ORCPT ); Fri, 25 Aug 2017 00:53:20 -0400 Received: from mail-pg0-f67.google.com ([74.125.83.67]:33455 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751424AbdHYExS (ORCPT ); Fri, 25 Aug 2017 00:53:18 -0400 Date: Thu, 24 Aug 2017 21:53:14 -0700 From: Brian Norris To: Boris Brezillon Cc: Andrea Adami , linux-mtd@lists.infradead.org, David Woodhouse , Marek Vasut , Richard Weinberger , Cyrille Pitchen , =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Haojian Zhuang , Dmitry Eremin-Solenikov , Robert Jarzmik , linux-kernel@vger.kernel.org Subject: Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser Message-ID: <20170825045314.GC68252@google.com> References: <1503394972-12541-1-git-send-email-andrea.adami@gmail.com> <20170822145424.54e57b94@bbrezillon> <20170824120428.46e266c7@bbrezillon> <20170824132710.5fdea20c@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170824132710.5fdea20c@bbrezillon> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3823 Lines: 89 On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote: > On Thu, 24 Aug 2017 12:30:02 +0200 > Andrea Adami wrote: > > > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon > > wrote: > > > On Thu, 24 Aug 2017 11:19:56 +0200 > > > Andrea Adami wrote: ... > > >> >> + /* create physical-logical table */ > > >> >> + for (block_num = 0; block_num < phymax; block_num++) { > > >> >> + block_adr = block_num * mtd->erasesize; > > >> >> + > > >> >> + if (mtd_block_isbad(mtd, block_adr)) > > >> >> + continue; > > >> >> + > > >> >> + if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob)) > > >> >> + continue; > > >> >> + > > >> >> + /* get logical block */ > > >> >> + log_num = sharpsl_nand_get_logical_num(oob); > > >> >> + > > >> >> + /* FTL is not used? Exit here if the oob fingerprint is wrong */ > > >> >> + if (log_num == UINT_MAX) { > > >> >> + pr_info("sharpslpart: Sharp SL FTL not found.\n"); > > >> >> + ret = -EINVAL; > > >> >> + goto exit; > > >> >> + } > > > > > > Okay, I overlooked that part. Why do you exit if there's a fingerprint > > > mismatch? Can't you just ignore this physical block and continue > > > scanning the remaining ones? > > > > Norris asked to quit immediately in this case. > > https://patchwork.kernel.org/patch/9758361/ I didn't specifically ask for you to quit in *that* case. Quoting myself here, as you did: > > "I wouldn't expect people to want to use this parser, but if we have a > > quick way to say "this doesn't match, skip me", then that would be > > helpful." > > "We don't want to waste too much time scanning for this partition > > table if possible." That means, is there something (not necessarily writting in the "original code" that you're massaging) that could be used to reliably detect that this is/isn't a valid "Sharp FTL"? I don't think the check you wrote is a good one. Particularly, you *don't* want to just abort completely because there's one corrupt block. This check is a reliability check (so you can possibly ignore old/bad copies and skip onto better blocks), not a validity check. It is counter-productive to abort here, IIUC. > Actually, you don't save much by exiting on "bad OOB fingerprint". If > you look at the code you'll see that the only thing you check is > whether some oob portions are equal or not, and most of the time the > OOB area is left untouched by the upper layer, which means all free > bytes will be left to 0xff, which in turn means the "is fingerprint > good?" test will pass. Agreed. I'd drop this "abort early" check and just admit that it's not possible to do what I asked. > > Now we are quitting ever before checking for parity erors ... > > Honestly, I'd recommend not using this parser for anything but SHARPSL > platforms, so I don't think we care much about the "scan all blocks" > overhead. Sounds about right. > Moreover, the sharpsl parser is the last one in the > part_parsers list, so it should be quite fast if the user specifies a > valid mtdparts= on the cmdline or valid partitions in the DT. Brian P.S. I alluded to it earlier, but I figured I should write it down properly here sometime, as food for thought; you don't actually need any of this parser at all if you're willing to contruct an initramfs that will do the parsing in user space (e.g., some scripting and 'nanddump'; or link to libmtd) and then add partitions yourself (e.g., with 'mtdpart add ...', or calling the BLKPG ioctls directly). This would just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y.