Re: [PATCH] mtd: m25p80: Flash protection support for STmicro chips

[Austin Boyle]

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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/