lkml.org 
[lkml]   [2014]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] mtd: m25p80: Flash protection support for STmicro chips

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.


\
 
 \ /
  Last update: 2014-03-13 14:41    [W:0.074 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site