Messages in this thread | | | Date | Sun, 12 Apr 2015 18:31:01 +0200 | From | Boris Brezillon <> | Subject | Re: [PATCH 4/4] UBI: Implement bitrot checking |
| |
On Sun, 12 Apr 2015 18:14:40 +0200 Richard Weinberger <richard@nod.at> 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
| |