Hi Tudor and all,
Am 2020-02-07 13:17, schrieb 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.
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/
