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 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". Cheers, ta ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/