[PATCH V3] sf: Querying write-protect status before operating the flash

[Jan Kiszka]

On 17.11.21 12:59, Tom Rini wrote:
> On Wed, Nov 17, 2021 at 01:43:28PM +0530, Jagan Teki wrote:
>> On Wed, Nov 17, 2021 at 1:33 PM Michael Walle <michael at walle.cc> wrote:
>>>
>>> Hi,
>>>
>>> Am 2021-11-17 03:48, schrieb chaochao2021666 at 163.com:
>>>> From: chao zeng <chao.zeng at siemens.com>
>>>>
>>>> When operating the write-protection flash,spi_flash_std_write() and
>>>> spi_flash_std_erase() would return wrong result.The flash is protected,
>>>> but write or erase the flash would show "OK".
>>>>
>>>> Check the flash write protection state before operating the flash
>>>> and give a prompt to show it has been locked if the write-protection
>>>> has enbale
>>>>
>>>> Signed-off-by: chao zeng <chao.zeng at siemens.com>
>>>>
>>>> ---
>>>>
>>>> Changes for V2:
>>>>       - Return 0 not ENOPROTOOPT to refelect the flash feature
>>>>       - Output prompt information
>>>> Changes for V3:
>>>>       - Modify output information
>>>>       - Delete return statement
>>>> ---
>>>>   drivers/mtd/spi/sf_probe.c | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
>>>> index f461082e03..f9e879aec5 100644
>>>> --- a/drivers/mtd/spi/sf_probe.c
>>>> +++ b/drivers/mtd/spi/sf_probe.c
>>>> @@ -109,6 +109,9 @@ static int spi_flash_std_write(struct udevice
>>>> *dev, u32 offset, size_t len,
>>>>        struct mtd_info *mtd = &flash->mtd;
>>>>        size_t retlen;
>>>>
>>>> +     if (flash->flash_is_locked && flash->flash_is_locked(flash, offset,
>>>> len))
>>>> +             printf("SF: Operate on the protected area.Writes will be
>>>> ignored\n");
>>>
>>> I don't think this is the correct place for this output. This could
>>> also be called from a board file programmatically and then it might
>>> display this error, which is annoying.
>>>
>>> Also, this is issuing an additional command "read SR" for every write.
>>>
>>> What is your intention here? To make the user aware that he is going
>>> to write to a write-protected region when he is using the "sf" command?
>>> If that is the case, this should be added to that command instead.
>>>
>>>> +
>>>>        return mtd->_write(mtd, offset, len, &retlen, buf);
>>>>   }
>>>>
>>>> @@ -127,6 +130,9 @@ static int spi_flash_std_erase(struct udevice
>>>> *dev, u32 offset, size_t len)
>>>>        instr.addr = offset;
>>>>        instr.len = len;
>>>>
>>>> +     if (flash->flash_is_locked && flash->flash_is_locked(flash, offset,
>>>> len))
>>>> +             printf("SF: Operate on the protected area.Erase will be ignored\n");
>>
>> My fundamental question, why cannot we use 'sf protect' then 'sf write'?
> 
> Where do we tell people to always run "sf protect" before "sf write" and
> why is that at all user friendly?  No, we shouldn't run this test more
> than once per time we're told to write an image.  But silently failing
> in cases we can detect a problem is also not correct.  If it's possible
> to spot this easily with "sf protect" why not just do that as part of
> "sf write" and add a flag to skip the check if you know it's not needed?
> I assume it's a fairly cheap/quick operation to do this check.
> 

What's the status here? Who should propose/implement what now?

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux