From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751839AbbDLQbM (ORCPT ); Sun, 12 Apr 2015 12:31:12 -0400 Received: from down.free-electrons.com ([37.187.137.238]:35656 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751665AbbDLQbK (ORCPT ); Sun, 12 Apr 2015 12:31:10 -0400 Date: Sun, 12 Apr 2015 18:31:01 +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: <20150412183101.2cbcde3f@bbrezillon> In-Reply-To: <552A99F0.9040701@nod.at> References: <1427631197-23610-1-git-send-email-richard@nod.at> <1427631197-23610-5-git-send-email-richard@nod.at> <20150412171450.51170602@bbrezillon> <552A99F0.9040701@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 On Sun, 12 Apr 2015 18:14:40 +0200 Richard Weinberger wrote: > > IMHO the following code chunk, starting here: > > > >> + wl_wrk = prepare_erase_work(e, -1, -1, 1); > >> + if (IS_ERR(wl_wrk)) { > >> + err = PTR_ERR(wl_wrk); > >> + goto out; > >> + } > >> + > >> + __schedule_ubi_work(ubi, wl_wrk); > > > > and ending here ^, could be placed in an helper function > > (re_erase_peb ?) > > As long we have only one user of that pattern I'd keep it as is. > We have in UBI already a gazillion helper functions. Okay, then maybe you should comment what you're doing here: erase an already erased PEB where bitflips have occured. > > >> + err = 0; > >> + } > >> + /* > >> + * e is target of a move operation, all we can do is kicking > >> + * wear leveling such that we can catch it later or wear > >> + * leveling itself scrubbs the PEB. > >> + */ > >> + else if (ubi->move_to == e || ubi->move_from == e) { > >> + spin_unlock(&ubi->wl_lock); > >> + > >> + err = ensure_wear_leveling(ubi, 1); > >> + } > >> + /* > >> + * 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; > >> + } > > > > I'm nitpicking again, but I like to have a single place where spinlocks > > are locked and unlocked, so here is a rework suggestion for the code > > inside the 'if (err == UBI_IO_BITFLIPS)' statement: > > A single lock/unlock place is nice but in this case the whole logic fits > into a single page on screen. "do_this" and "do_that" variables don't make > the code more readable IMHO. > But as with all nitpicks it is a matter of taste and we could waste multiple > days on such things. Isn't that the whole point of code reviews :-P ? -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com