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

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

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.

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);
} else {
	bp_slot_count = bp_slots_needed;
	bp_min_slot_size = mtd->size >> bp_block_count;
}

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);

Cheers,
ta



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