Re: [PATCH 4/4] UBI: Implement bitrot checking

From: Boris Brezillon
Date: Sun Apr 12 2015 - 12:31:15 EST


On Sun, 12 Apr 2015 18:14:40 +0200
Richard Weinberger <richard@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/