Am 12.04.2015 um 17:14 schrieb Boris Brezillon:
> Second pass on this patch :-).
>
> On Sun, 29 Mar 2015 14:13:17 +0200
> Richard Weinberger <rich...@nod.at> wrote:
>
>> /**
>> + * bitrot_check_worker - physical eraseblock bitrot check worker function.
>> + * @ubi: UBI device description object
>> + * @wl_wrk: the work object
>> + * @shutdown: non-zero if the worker has to free memory and exit
>> + *
>> + * This function reads a physical eraseblock and schedules scrubbing if
>> + * bit flips are detected.
>> + */
>> +static int bitrot_check_worker(struct ubi_device *ubi, struct ubi_work
>> *wl_wrk,
>> + int shutdown)
>> +{
>> + struct ubi_wl_entry *e = wl_wrk->e;
>> + int err;
>> +
>> + kfree(wl_wrk);
>> + if (shutdown) {
>> + dbg_wl("cancel bitrot check of PEB %d", e->pnum);
>> + wl_entry_destroy(ubi, e);
>> + return 0;
>> + }
>> +
>> + mutex_lock(&ubi->buf_mutex);
>> + err = ubi_io_read(ubi, ubi->peb_buf, e->pnum, 0, ubi->peb_size);
>> + mutex_unlock(&ubi->buf_mutex);
>> + if (err == UBI_IO_BITFLIPS) {
>> + dbg_wl("found bitflips in PEB %d", e->pnum);
>> + spin_lock(&ubi->wl_lock);
>> +
>> + if (in_pq(ubi, e)) {
>> + prot_queue_del(ubi, e->pnum);
>> + wl_tree_add(e, &ubi->scrub);
>> + spin_unlock(&ubi->wl_lock);
>> +
>> + err = ensure_wear_leveling(ubi, 1);
>> + }
>> + else if (in_wl_tree(e, &ubi->used)) {
>> + rb_erase(&e->u.rb, &ubi->used);
>> + wl_tree_add(e, &ubi->scrub);
>> + spin_unlock(&ubi->wl_lock);
>> +
>> + err = ensure_wear_leveling(ubi, 1);
>> + }
>> + else if (in_wl_tree(e, &ubi->free)) {
>> + rb_erase(&e->u.rb, &ubi->free);
>> + spin_unlock(&ubi->wl_lock);
>> +
>
> 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.
>> + 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.
Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
