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

[Michael Walle <michael@walle.cc>]

Am 2022-02-02 10:38, schrieb Jan Kiszka:
> On 02.02.22 09:21, Michael Walle wrote:
>> Am 2022-02-02 07:35, schrieb Jan Kiszka:
>>> From: Jan Kiszka <jan.kiszka@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@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...

I don't think that will help either because ultimately, you'd need to
know the exact sizes and offsets of the areas which can be protected.

> Back to square #1? Or can/should we split the request into
> blocks?

Blocks of which size? 4k? Might work but sounds hackish.

You could introduce a new function which checks if *any* area is
protected.

-michael