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

[Timo Lindhorst <lindhors@linux.vnet.ibm.com>]

Hi Artem,

>>+	ec_hdr = ubi_zalloc_ec_hdr(ubi);
>>+	if (unlikely(!ec_hdr))
>>+		return -ENOMEM;
> 
> 
> So why have you moved this memory allocation here?
Actually I have not moved up the allocation, but moved down the
'if (unlikely(ec > UBI_MAX_ERASECOUNTER)) { ... ' block, since we need 
to know the new ec value before we can do this test.

I thought you want the allocation to be done before erasure, to prevent 
the erasure if the ec_hdr cannot be written due to a failure of memory 
allocation.
Otherwise, the allocation can be moved down again, but than the new ec 
is possibly not written to the EB if no memory can be allocated for ec_hdr.

Do I miss something? What do you think?

>>+
>>+	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?
I just thought adding an error code to a counter seems odd. Do you 
prefer this way?

> 
>> 	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.
Yes, sorry! I have changed it to:

		err = -EINVAL;
		goto out_free;
	}


I will resend the patch when I have got your comments to the upper points.

Kind regards,
Timo