Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751918AbbDLQng (ORCPT ); Sun, 12 Apr 2015 12:43:36 -0400 Received: from down.free-electrons.com ([37.187.137.238]:35962 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751337AbbDLQnf (ORCPT ); Sun, 12 Apr 2015 12:43:35 -0400 Date: Sun, 12 Apr 2015 18:43:30 +0200 From: Boris Brezillon To: Richard Weinberger Cc: linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org, dedekind1@gmail.com Subject: Re: [PATCH 4/4] UBI: Implement bitrot checking Message-ID: <20150412184330.73a62484@bbrezillon> In-Reply-To: <552A98B3.5000202@nod.at> References: <1427631197-23610-1-git-send-email-richard@nod.at> <1427631197-23610-5-git-send-email-richard@nod.at> <20150412161243.4ee7eb59@bbrezillon> <552A98B3.5000202@nod.at> X-Mailer: Claws Mail 3.9.3 (GTK+ 2.24.23; 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: 5217 Lines: 152 On Sun, 12 Apr 2015 18:09:23 +0200 Richard Weinberger wrote: > Am 12.04.2015 um 16:12 schrieb Boris Brezillon: > > Hi Richard, > > > > Sorry for the late reply. > > > > On Sun, 29 Mar 2015 14:13:17 +0200 > > Richard Weinberger wrote: > > > >> This patch implements bitrot checking for UBI. > >> ubi_wl_trigger_bitrot_check() triggers a re-read of every > >> PEB. If a bitflip is detected PEBs in use will get scrubbed > >> and free ones erased. > > > > As you'll see, I didn't have much to say about the 'UBI bitrot > > detection' mechanism, so this review is a collection of > > nitpicks :-). > > > >> > >> Signed-off-by: Richard Weinberger > >> --- > >> drivers/mtd/ubi/build.c | 39 +++++++++++++ > >> drivers/mtd/ubi/ubi.h | 4 ++ > >> drivers/mtd/ubi/wl.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 189 insertions(+) > >> > >> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > >> index 9690cf9..f58330b 100644 > >> --- a/drivers/mtd/ubi/build.c > >> +++ b/drivers/mtd/ubi/build.c > >> @@ -118,6 +118,9 @@ static struct class_attribute ubi_version = > >> > >> static ssize_t dev_attribute_show(struct device *dev, > >> struct device_attribute *attr, char *buf); > >> +static ssize_t trigger_bitrot_check(struct device *dev, > >> + struct device_attribute *mattr, > >> + const char *data, size_t count); > >> > >> /* UBI device attributes (correspond to files in '//class/ubi/ubiX') */ > >> static struct device_attribute dev_eraseblock_size = > >> @@ -142,6 +145,8 @@ static struct device_attribute dev_bgt_enabled = > >> __ATTR(bgt_enabled, S_IRUGO, dev_attribute_show, NULL); > >> static struct device_attribute dev_mtd_num = > >> __ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL); > >> +static struct device_attribute dev_trigger_bitrot_check = > >> + __ATTR(trigger_bitrot_check, S_IWUSR, NULL, trigger_bitrot_check); > > > > How about making this attribute a RW one, so that users could check > > if there's a bitrot check in progress. > > As the check will be initiated only by userspace and writing to the trigger > while a check is running will return anyway a EBUSY I don't really see > a point why userspace would check for it. Sometime you just want to know whether something is running or not (in this case the bitrot check) without risking to trigger a new action... > > >> > >> /** > >> * ubi_volume_notify - send a volume change notification. > >> @@ -334,6 +339,36 @@ int ubi_major2num(int major) > >> return ubi_num; > >> } > >> > >> +/* "Store" method for file '//class/ubi/ubiX/trigger_bitrot_check' */ > >> +static ssize_t trigger_bitrot_check(struct device *dev, > >> + struct device_attribute *mattr, > >> + const char *data, size_t count) > >> +{ > >> + struct ubi_device *ubi; > >> + int ret; > >> + > > > > Maybe that's on purpose, but you do not check the value passed in data > > (in your documention you suggest to do an > > echo 1 > /sys/class/ubi/ubiX/trigger_bitrot_check). > > Yeah, the example using "1", but why should I limit it to it? > The idea was that any write will trigger a check. Okay. [...] > >> + /* > >> + * e is member of a fastmap pool. We are not allowed to > >> + * remove it from that pool as the on-flash fastmap data > >> + * structure refers to it. Let's schedule a new fastmap write > >> + * such that the said PEB can get released. > >> + */ > >> + else { > >> + ubi_schedule_fm_work(ubi); > >> + spin_unlock(&ubi->wl_lock); > >> + > >> + err = 0; > >> + } > > > > Nitpick, but checkpatch complains about 'else' or 'else if' statements > > that are not on the '}' line. > > I like it as is because I can nicely place the comment above the else {. > And checkpatch is not our lawmaker. You could put your comment after the braces. Anyway, you might dislike the coding style imposed by kernel developers/maintainers, but this is what keeps the kernel code consistent across the different subsystems. I agree that some checks done by checkpatch can be a bit restrictive in some cases (like the 80 characters limit), but I really think the braces and else[ if] placements should be enforced. This being said, this is your call to make, so I won't complain about it anymore ;-). > > >> + } > >> + else { > >> + /* > >> + * Ignore read errors as we return only work related errors. > >> + * Read errors will be logged by ubi_io_read(). > >> + */ > >> + err = 0; > >> + } > > > > Nitpicking again, but you can avoid another level of indentation by > > doing the following: > > > > if (err != UBI_IO_BITFLIPS) { > > err = 0; > > goto out; > > } > > > > dbg_wl("found bitflips in PEB %d", e->pnum); > > spin_lock(&ubi->wl_lock); > > /* ... */ You didn't answer to that one. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/