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

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

[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;
+	}
+
 	buf = map_physmem(addr, len, MAP_WRBACK);
 	if (!buf && addr) {
 		puts("Failed to map physical memory\n");
@@ -343,6 +349,12 @@ static int do_spi_flash_erase(int argc, char *const argv[])
 		return 1;
 	}
 
+	if (flash->flash_is_locked &&
+	    flash->flash_is_locked(flash, offset, len)) {
+		printf("ERROR: flash area is locked\n");
+		return 1;
+	}
+
 	ret = spi_flash_erase(flash, offset, size);
 	printf("SF: %zu bytes @ %#x Erased: ", (size_t)size, (u32)offset);
 	if (ret)
-- 
2.34.1

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

[Michael Walle]

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.

-michael

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

[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... Back to square #1? Or can/should we split the request into
blocks?

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

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

[Michael Walle]

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

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

[Jan Kiszka]

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@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... 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.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [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@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... 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

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

[Michael Walle]

Am 2022-02-04 13:55, schrieb Jan Kiszka:
> On 02.02.22 10:57, Jan Kiszka wrote:

..

>> 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?

Couldn't wrap my head around that code, but I wouldn't be suprised if it
is.

-michael