Re: [PATCH] [MTD] UBI: Fix counting of ec value

Artem Bityutskiy
2007-02-12T10:03:12Z

Hi Timo,

thanks for the patches. The comment-fixing one is fine - applied. This
one is fine in general, but i have questions.

On Mon, 2007-02-12 at 10:16 +0100, Timo Lindhorst wrote:
> static int sync_erase(const struct ubi_info *ubi, struct ubi_wl_entry *e,
> int torture)
> {
> - int err;
> + int err, ret;
> struct ubi_ec_hdr *ec_hdr;
> struct ubi_wl_info *wl = ubi->wl;
> - uint64_t ec = e->ec + 1;
> + uint64_t ec = e->ec;
>
> - dbg_wl("erase PEB %d, new EC %lld", e->pnum, (long long)ec);
> + dbg_wl("erase PEB %d, old EC %lld", e->pnum, (long long)ec);
>
> err = paranoid_check_ec(ubi, e->pnum, e->ec);
> if (unlikely(err > 0))
> return -EINVAL;
>
> + ec_hdr = ubi_zalloc_ec_hdr(ubi);
> + if (unlikely(!ec_hdr))
> + return -ENOMEM;

So why have you moved this memory allocation here?

> +
> + ret = err = ubi_io_sync_erase(ubi, e->pnum, torture);
> + if (unlikely(err < 0))
> + goto out_free;
> +
> + ec += ret;
What's the point in the new 'ret' variable? Why ec += err does not work?

> if (unlikely(ec > UBI_MAX_ERASECOUNTER)) {
> /*
> * Erase counter overflow. Upgrade UBI and use 64-bit
> * erase counters internally.
> */
> ubi_err("erase counter overflow at PEB %d, EC %d",
> - e->pnum, e->ec);
> + e->pnum, ec);
> return -EINVAL;
And now you do not free memory. Please do not move the allocation if it
is not really necessary.

--
Best regards,
Artem Bityutskiy (Битюцкий Артём)