linux-kernel - Re: [PATCH 4/4] UBI: Implement bitrot checking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <552A9E20.6010207@nod.at>
Date:	Sun, 12 Apr 2015 18:32:32 +0200
From:	Richard Weinberger <richard@....at>
To:	Boris Brezillon <boris.brezillon@...e-electrons.com>
CC:	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
	dedekind1@...il.com
Subject: Re: [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@....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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/