Hi Tudor,
Am 2020-02-10 10:47, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
Hi, Michael,
On Monday, February 10, 2020 10:33:41 AM EET Michael Walle wrote:
cut
> 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.
Right, this is the goal.
Let me try to extend the description of the proposal.
> We should use a single formula, for all the BP cases. How about the
> following:
>
> bp_slots_available = (bp_mask >> shift) + 1 - 2;
This formula is derived from Michael's patch.
A slot (to me) is a horizontal line in the Memory protection table.
Maybe we
can find a better/standardized name for this.
So for BP0-2, bp_slots_available = 6, and for BP0-3, bp_slots_available
= 14.
Notice that I stripped the two special cases: lock none and lock all.
> bp_slots_needed = ilog2(nor->info->n_sectors);
With bp_slots_needed I tried to describe how many slots are needed if
the
protected density for the first slot is at minimum (sector size).
>
> 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
It's bytes. Take a look at W25Q128JV. The sector size for this flash is
64KByte. The flash has 256 sectors. For this specific case:
bp_slots_available = 6;
bp_slots_needed = 8;
The if condition is true, so
bp_slot_count = 6;
bp_min_slot_size = 64k << (8 - 6); //256k
But nor->info->n_sectors is not 64k, its 256. Do you mean sector_size
(like in
my example below? Then we are on the same page ;)
which is exactly the protected density for the first slot. The
protected
densities of the other slots can be computed by multiplying with powers
of 2.
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;
typo: s/bp_block_count/bp_slot_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;
}
you're right, we're in the else case, where the assumption that the
minimum
protected density is sector size is true, we can use directly
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.
This case is addressed by using bp_slot_count and bp_slots_available.
We're in
the else case from above. From bp_slot_count up to the
bp_slots_available,
those slots are "protect all".
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
