On 02.02.22 10:57, Jan Kiszka wrote: > On 02.02.22 10:38, Jan Kiszka wrote: >> On 02.02.22 09:21, Michael Walle wrote: >>> Am 2022-02-02 07:35, schrieb Jan Kiszka: >>>> From: Jan Kiszka <jan.kis...@siemens.com> >>>> >>>> Do not suggest successful operation if a flash area to be changed is >>>> actually locked, thus will not execute the request. Rather report an >>>> error and bail out. That's way more user-friendly than asking them to >>>> manually check for this case. >>>> >>>> Derived from original patch by Chao Zeng. >>>> >>>> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com> >>>> --- >>>> >>>> This is the successor of "[PATCH V3] sf: Querying write-protect status >>>> before operating the flash", moving the test into the CLI API, see >>>> https://lore.kernel.org/u-boot/20220117175628.GQ2631111@bill-the-cat/. >>>> >>>> cmd/sf.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/cmd/sf.c b/cmd/sf.c >>>> index 8bdebd9fd8f..a24e04c690b 100644 >>>> --- a/cmd/sf.c >>>> +++ b/cmd/sf.c >>>> @@ -287,6 +287,12 @@ static int do_spi_flash_read_write(int argc, char >>>> *const argv[]) >>>> return 1; >>>> } >>>> >>>> + if (strncmp(argv[0], "read", 4) != 0 && flash->flash_is_locked && >>>> + flash->flash_is_locked(flash, offset, len)) { >>>> + printf("ERROR: flash area is locked\n"); >>>> + return 1; >>>> + } >>> >>> Much better to handle it here. But I'm not sure if this is doing >>> the right thing: >>> >>> Eventually, this function is called: >>> >>> /* >>> * Return 1 if the entire region is locked (if @locked is true) or >>> unlocked (if >>> * @locked is false); 0 otherwise >>> */ >>> static int stm_check_lock_status_sr(struct spi_nor *nor, loff_t ofs, u64 >>> len, >>> u8 sr, bool locked) >>> >>> So I'd guess if you try to write to an area of the flash where only parts >>> are locked, you still see it as unlocked and thus there will be no error. >>> Which IMHO is even more confusing for a user. >> >> I suppose this is why the original patch was placed way more down the >> call chain... Back to square #1? Or can/should we split the request into >> blocks? > > Hmm, no, the other versions should have had the same problem. > > What about also exposing a "is_unlocked" service? Seems that would have > the semantic we need, and there is at least already stm_is_unlocked_sr. > But no sst26_is_unlocked.
>From my reading of sst26_is_locked, it has "is fully unlocked" semantic, rather than "is fully locked". Looks inconsistent today, right? Jan -- Siemens AG, Technology Competence Center Embedded Linux