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

[Artem Bityutskiy <dedekind@infradead.org>]

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 (Битюцкий Артём)