Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support

[Michael Walle <michael@walle.cc>]

Hi Tudor and all,

Am 2020-02-07 13:17, schrieb Tudor.Ambarus@microchip.com:
> Hi,
> 
> On Monday, February 3, 2020 3:56:58 PM EET Vignesh Raghavendra wrote:
>> >>>>>>>>> /*
>> >>>>>>>>> * Need smallest pow such that:
>> >>>>>>>>> *
>> >>>>>>>>> @@ -1908,7 +1972,17 @@ static int stm_lock(struct
>> >>>>>>>>> spi_nor
>> >>>>>>>>> *nor,
>> >>>>>>>>> loff_t ofs, uint64_t len)
>> >>>>>>>>> *   pow = ceil(log2(size / len)) = log2(size)
>> >>>>>>>>> -
>> >>>>>>>>> floor(log2(len))
>> >>>>>>>>> */
>> >>>>>>>>> pow = ilog2(mtd->size) - ilog2(lock_len);
>> >>>>>>>>> -     val = mask - (pow << SR_BP_SHIFT);
>> >>>>>>>>> +
>> >>>>>>>>> +     if (nor->flags & SNOR_F_HAS_SR_BP3) {
>> >>>>>>>>> +             val = ilog2(nor->n_sectors) + 1 - pow;
>> >>>>>>>>
>> >>>>>>>> Why do you use a new calculation here? As far as I can
>> >>>>>>>> see,
>> >>>>>>>> the
>> >>>>>>>> method is
>> >>>>>>>> the same except that is has one bit more. That also
>> >>>>>>>> raises
>> >>>>>>>> the
>> >>>>>>>> question why
>> >>>>>>>> n_sectors is now needed?
>> 
>> Flash devices have variable sector size, 64KB, 128KB or 256KB... While
>> mapping of number of sectors locked to BP bits is dependent on rules 1
>> to 3 you mentioned below, the size or area of flash protected depends 
>> on
>> sector size.
>> 
>> So, the current formula in spi-nor.c (ignoring TB and other 
>> boilerplate):
>> 
>> pow = ilog2(mtd->size) - ilog2(lock_len);
>> val = mask - (pow << shift);
>> 
>> This works only for devices with 64KB sector size as 8MB flash with 
>> 64KB
>> sector size would have 128 sectors (BP0-2 => 0b111 => 2^7).
>> 
>> A more generic formula would be:
>> 
>> Find n where 2^(n - 1) = len/sector-size
>> OR 2^ (n - 1) = len * n_sectors / mtd->size
>> 
>> Which solves to:
>> 
>> pow = ilog2(mtd->size) - ilog2(lock_len);
>> val = ilog2(nor->n_sectors) + 1 - pow;
> 
> The current mainline locking support is limited. Michael spotted a good
> improvement, but I think there are still others that we should 
> consider.

Sure, as I said my patch was just to show, that there is an underlying 
problem
and that we should not take the 4th BP bit to differentiate between the 
two
different formulas.

> We should use a single formula, for all the BP cases. How about the 
> following:
> 
> bp_slots_available = (bp_mask >> shift) + 1 - 2;
> bp_slots_needed = ilog2(nor->info->n_sectors);
> 
> if (bp_slots_needed > bp_slots_available) {
> 	bp_slot_count = bp_slots_available;
> 	bp_min_slot_size = nor->info->n_sectors <<
> 		(bp_slots_needed - bp_slots_available);

mhh, what is the unit of bp_min_slot_size? bytes or sectors? I guess it 
should
be bytes, eg for a 8MiB flash it would be 128kiB and for a 16MiB flash 
it would
be 256kiB (if there are 3 BP bits).


> } else {
> 	bp_slot_count = bp_slots_needed;
> 	bp_min_slot_size = mtd->size >> bp_block_count;

this is a complicated way of saying its the size of one sector, isn't 
it?
can't we use nor->info->sector_size here? Eg.

if (bp_slots_needed > bp_slots_available) {
	bp_slot_count = bp_slots_available;
	bp_min_slot_size = nor->info->sector_size <<
		(bp_slots_needed - bp_slots_available);
} else {
	bp_slot_count = bp_slots_needed;
  	bp_min_slot_size = nor->info->sector_size;
}



> }
> 
> When both can_be_bottom and can_be_top are true, we prefer the top 
> protection,
> which is incorrect/buggy/sub-optimal. If the received offset is not 
> aligned to
> one of the start addresses of the bp slots, then we should up/down 
> align the
> offset to the closest bp slot, depending on TB and which (top or 
> bottom) fits
> better. Based on the updated offset and length we can compute the lock 
> range,
> and after that:
> 
> n = ilog2(bp_lock_range/bp_min_slot_size) + 1;
> val = mask - (n << shift);

btw. we should catch the two special cases:
  - lock none -> 0 (that was already the case)
  - lock all -> all BP bits

The latter is important if "bp_slots_needed < bp_slots_available" 
because there
are multiple settings for protect all. Most flashes will define any 
remaining
setting for "protect all", but I've also seen flashes where the 
in-between ones
were undefined (not mentioned) and only the "all bit set" was protect 
all.


-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/