lkml.org 
[lkml]   [2015]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/4] UBI: Implement bitrot checking
Am 12.04.2015 um 18:31 schrieb Boris Brezillon:
> 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.

Makes sense!

>>
>>>> + 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 ?

;-)

Thanks,
//richard


\
 
 \ /
  Last update: 2015-04-12 19:01    [W:0.115 / U:3.992 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site