[PATCH v4] sf: Query write-protection status before operating the flash

[Jan Kiszka]

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.kiszka at 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.kiszka at 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