* [PATCH V3] sf: Querying write-protect status before operating the flash
@ 2021-11-17 2:48 chaochao2021666
2021-11-17 8:03 ` Michael Walle
0 siblings, 1 reply; 6+ messages in thread
From: chaochao2021666 @ 2021-11-17 2:48 UTC (permalink / raw)
To: jagan, Tudor.Ambarus, vigneshr
Cc: michael, jan.kiszka, baocheng.su, le.jin, u-boot, trini, chao zeng
From: chao zeng <chao.zeng@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@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");
+
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");
+
return mtd->_erase(mtd, &instr);
}
--
2.33.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V3] sf: Querying write-protect status before operating the flash
2021-11-17 2:48 [PATCH V3] sf: Querying write-protect status before operating the flash chaochao2021666
@ 2021-11-17 8:03 ` Michael Walle
2021-11-17 8:13 ` Jagan Teki
0 siblings, 1 reply; 6+ messages in thread
From: Michael Walle @ 2021-11-17 8:03 UTC (permalink / raw)
To: chaochao2021666
Cc: jagan, Tudor.Ambarus, vigneshr, jan.kiszka, baocheng.su, le.jin,
u-boot, trini, chao zeng
Hi,
Am 2021-11-17 03:48, schrieb chaochao2021666@163.com:
> From: chao zeng <chao.zeng@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@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");
likewise.
-michael
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V3] sf: Querying write-protect status before operating the flash
2021-11-17 8:03 ` Michael Walle
@ 2021-11-17 8:13 ` Jagan Teki
2021-11-17 11:59 ` Tom Rini
0 siblings, 1 reply; 6+ messages in thread
From: Jagan Teki @ 2021-11-17 8:13 UTC (permalink / raw)
To: Michael Walle, chaochao2021666, trini
Cc: Tudor.Ambarus, vigneshr, jan.kiszka, baocheng.su, le.jin, u-boot,
chao zeng
On Wed, Nov 17, 2021 at 1:33 PM Michael Walle <michael@walle.cc> wrote:
>
> Hi,
>
> Am 2021-11-17 03:48, schrieb chaochao2021666@163.com:
> > From: chao zeng <chao.zeng@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@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'?
Jagan.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V3] sf: Querying write-protect status before operating the flash
2021-11-17 8:13 ` Jagan Teki
@ 2021-11-17 11:59 ` Tom Rini
2022-01-13 7:38 ` Jan Kiszka
0 siblings, 1 reply; 6+ messages in thread
From: Tom Rini @ 2021-11-17 11:59 UTC (permalink / raw)
To: Jagan Teki
Cc: Michael Walle, chaochao2021666, Tudor.Ambarus, vigneshr,
jan.kiszka, baocheng.su, le.jin, u-boot, chao zeng
[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]
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@walle.cc> wrote:
> >
> > Hi,
> >
> > Am 2021-11-17 03:48, schrieb chaochao2021666@163.com:
> > > From: chao zeng <chao.zeng@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@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.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V3] sf: Querying write-protect status before operating the flash
2021-11-17 11:59 ` Tom Rini
@ 2022-01-13 7:38 ` Jan Kiszka
2022-01-17 17:56 ` Tom Rini
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kiszka @ 2022-01-13 7:38 UTC (permalink / raw)
To: Tom Rini, Jagan Teki
Cc: Michael Walle, chaochao2021666, Tudor.Ambarus, vigneshr,
baocheng.su, le.jin, u-boot, chao zeng
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@walle.cc> wrote:
>>>
>>> Hi,
>>>
>>> Am 2021-11-17 03:48, schrieb chaochao2021666@163.com:
>>>> From: chao zeng <chao.zeng@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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V3] sf: Querying write-protect status before operating the flash
2022-01-13 7:38 ` Jan Kiszka
@ 2022-01-17 17:56 ` Tom Rini
0 siblings, 0 replies; 6+ messages in thread
From: Tom Rini @ 2022-01-17 17:56 UTC (permalink / raw)
To: Jan Kiszka
Cc: Jagan Teki, Michael Walle, chaochao2021666, Tudor.Ambarus,
vigneshr, baocheng.su, le.jin, u-boot, chao zeng
[-- Attachment #1: Type: text/plain, Size: 4120 bytes --]
On Thu, Jan 13, 2022 at 08:38:04AM +0100, Jan Kiszka wrote:
> 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@walle.cc> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Am 2021-11-17 03:48, schrieb chaochao2021666@163.com:
> > > > > From: chao zeng <chao.zeng@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@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?
Good question. Re-reading the quoted part here, the (valid!) concern I
see on the one hand is that today you can "sf write", see "OK" and have
had nothing written because the flash was protected, and that's
something we could have known at the start of "sf write". The change as
written is within the write API, rather than the CLI API, so could we
move that check to cmd/sf.c instead?
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-17 17:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 2:48 [PATCH V3] sf: Querying write-protect status before operating the flash chaochao2021666
2021-11-17 8:03 ` Michael Walle
2021-11-17 8:13 ` Jagan Teki
2021-11-17 11:59 ` Tom Rini
2022-01-13 7:38 ` Jan Kiszka
2022-01-17 17:56 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.