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

[chaochao]

On 2021/11/15 22:02, Jagan Teki wrote:
> On Mon, Nov 15, 2021 at 6:51 PM chaochao2021666 at 163.com
> <chaochao2021666 at 163.com> wrote:
>>
>> On 2021/11/15 13:57, Tudor.Ambarus at microchip.com wrote:
>>
>> Hi,
>>
>> + Michael
>>
>> On 11/15/21 4:37 AM, chaochao2021666 at 163.com wrote:
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> 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
>> ---
>>   drivers/mtd/spi/sf_probe.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
>> index f461082e03..995801817d 100644
>> --- a/drivers/mtd/spi/sf_probe.c
>> +++ b/drivers/mtd/spi/sf_probe.c
>> @@ -109,6 +109,11 @@ 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: Flash is locked\n");
>>
>> I would use a debug message, it's a flash specific thing. Also, I would update
>> a bit the message, something like
>> "SF: Flash has protected areas in the requested length. Writes will be ignored on those."
>>
>> +               return 0;
>>
>> Michael has suggested to drop this line. I agree with him, check the conversation
>> on the previous email thread.
>>
>> Cheers,
>> ta
>>
>> +       }
>> +
>>          return mtd->_write(mtd, offset, len, &retlen, buf);
>>   }
>>
>> @@ -127,6 +132,11 @@ 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: Flash is locked\n");
>> +               return 0;
>> +       }
>> +
>>          return mtd->_erase(mtd, &instr);
>>   }
>>
>> --
>> 2.33.1
>>
>>
>>
>> the background is we like to use sf command to operate the flash under uboot shell,
>>
>> "sf erase" command still would show the prompt  "erase ok" even though  write-enable has enabled.
>>
>>
>> So at the beginning  I'd like to return an error ,so the sf operation would show "erase failed" when operating the write-enabled devices.
>>
>>
>> I'm agree with only output information to prompt the user the operation unsuccessful.
>>
>> But It should explicitly give clear hints,so I suggest at here using printf not debug.
> 
> We cannot encourage sf to show non operational prints like locked or
> unlocked on command line. Just check the contents via read and compare
> and understand whether flash is written properly, if not written
> properly user has to debug on his own.
> 
> Jagan.
> 

I think it’s not user friendly at all. Using debug is not a problem for 
developers, but it is not so good for users who use the sf command.


BRs
Chao