Hi Gerlando,
On Mon, 10 Mar 2014, Gerlando Falauto wrote:
What do you mean exactly by "it is optional"?
I agree with you that an explicit ioctl(MEMLOCK) is order for locking to take
place. However, this seems to be the default action for the u-boot
environment userspace tools. They will issue a MEMUNLOCK/MEMLOCK pair when
trying to write some changes to the environment, without even checking the
return value. This would of course fail silently when the feature was not
implemented (as it was the case before the original patch was applied) and
everything was working as expected.
Now linux supports this feature, and u-boot doesn't, so as soon as you write
something to the flash from userspace, it will be locked and u-boot won't
ever be able to write to it again.
OK I understand your point after checking the code for the u-boot tool.
In my opinion, we're breaking something here (call it userspace API or
otherwise). My suggestion would then be to make it an optional feature to be
explicitly enabled on the device tree, like Heicho did for CFI flashes:
http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html
Or I guess another way would be to implement the _is_locked() function, so to
have the userspace tools check the locking status before unlocking, and only
lock it again if was locked in the first place.
It wouldn't fix my issue right away (as the userspace tools don't currenctly
perform this check), but at least it would provide some way out here without
breaking compatibility with the existing u-boot.
I prefer this suggestion of implementing _is_locked(). It is a simple
change so may as well be done while fixing the lock/unlock code anyway.
One could say it is counterintuitive for ioctl(MEMLOCK) not to work
without previous configuration, as in the first suggestion. If people are
passionate about making the feature configurable it could be done
separately once we've fixed the logic with _lock/_unlock and
adding _is_locked.
I will submit a change to the u-boot env tool to check the locking state
if supported and restore the same state after - obviously more
correct, regardless of the decision made here.
Not ideal but in the meantime if someone needs to ensure the device is
unlocked they can run the flash_unlock from mtd-utils after writing u-boot
environment variables from the userspace tool. (Call it a user space
work around for a user space bug?)
Uhm, I believe it should read like this (unprotected portion is of course "n
- protected portion"):
SR BPs | Protected portion
---------------------------
0 | 0/n
1 | 1/n
2 | 2/n
3 | min(4,n)/n
4 | min(8,n)/n
5 | min(16,n)/n
6 | min(32,n)/n
7 | min(64,n)/n
Yes sorry that was a typo in the previous email, should have been powers
of 2 as above and the patch. Also technically correct about the min macro,
as you saw I was just terminating the loop before that condition was met.
A patch with this implementation follows. Let me know what you think. I
have a spreadsheet summarising the block protect bits for the STmicro
devices I can share if it will help.
Could you please share this?
It sounds like you have already done your own research but here is what I
have:
https://docs.google.com/spreadsheet/ccc?key=0AhKBO-EQCLLkdGR1Q05qLUs2RURsMFA4V2s2X1llY3c&usp=sharing
Thanks for your comments on this issue and patch. I will resubmit
another version addressing your points soon.
Cheers,
Austin.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
