| Message ID | 20210622052126.49853-1-chaochao2021666@163.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | sf: Querying write-protect status before operating the flash | expand |
On Tue, Jun 22, 2021 at 10:51 AM chao zeng <chaochao2021666@163.com> wrote: > > 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 if the write-protection has enbale > before operating the flash. > > Signed-off-by: Chao Zeng <chao.zeng@siemens.com> > --- Does it broken on master? if yes can you check in u-boot-spi/next? Jagan.
HI Jagan sorry for the delay response. And I have checked the maser. There is still a problem with this feature。 reproduce steps: 1. enable the flash protect function 2. using sf cmd to erase the flash. I can get the erase "OK",not the "error". I think the root cause is that the detection mechanism is missing and to judge the permissions of the action So pull this PR to improve the erase flow another question: how can I visit the u-boot-spi/next? do there any link? BRs Chao At 2021-06-29 21:50:28, "Jagan Teki" <jagan@amarulasolutions.com> wrote: >On Tue, Jun 22, 2021 at 10:51 AM chao zeng <chaochao2021666@163.com> wrote: >> >> 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 if the write-protection has enbale >> before operating the flash. >> >> Signed-off-by: Chao Zeng <chao.zeng@siemens.com> >> --- > >Does it broken on master? if yes can you check in u-boot-spi/next? > >Jagan.
Hi all, Chao, please no top-post on mailing list. Also check your mail client, it seems to inject a lot of bogus newlines. On 08.09.21 06:55, chaochao2021666 wrote: > > > > HI Jagan > > > > sorry for the delay response. > > > And I have checked the maser. There is still a problem with this feature。 > > > reproduce steps: > 1. enable the flash protect function > 2. using sf cmd to erase the flash. I can get the erase "OK",not the "error". > > > > I think the root cause is that the detection mechanism is missing and to judge the permissions of the action > > So pull this PR to improve the erase flow > > > another question: > how can I visit the u-boot-spi/next? do there any link? > See MAINTAINERS: https://source.denx.de/u-boot/custodians/u-boot-spi.git But also that tree contains no usage of the flash_is_locked callback. That was once evaluated by drivers/mtd/spi/spi_flash.c but then forgotten in the new SPI NOR framework it seems. Chao's patch makes sense to me to restore this feature. Jan > > > > > BRs > Chao > > > > At 2021-06-29 21:50:28, "Jagan Teki" <jagan@amarulasolutions.com> wrote: >> On Tue, Jun 22, 2021 at 10:51 AM chao zeng <chaochao2021666@163.com> wrote: >>> >>> 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 if the write-protection has enbale >>> before operating the flash. >>> >>> Signed-off-by: Chao Zeng <chao.zeng@siemens.com> >>> --- >> >> Does it broken on master? if yes can you check in u-boot-spi/next? >> >> Jagan.
On 13.09.21 09:48, Jan Kiszka wrote: > Hi all, > > Chao, please no top-post on mailing list. Also check your mail client, > it seems to inject a lot of bogus newlines. > > On 08.09.21 06:55, chaochao2021666 wrote: >> >> >> >> HI Jagan >> >> >> >> sorry for the delay response. >> >> >> And I have checked the maser. There is still a problem with this feature。 >> >> >> reproduce steps: >> 1. enable the flash protect function >> 2. using sf cmd to erase the flash. I can get the erase "OK",not the "error". >> >> >> >> I think the root cause is that the detection mechanism is missing and to judge the permissions of the action >> >> So pull this PR to improve the erase flow >> >> >> another question: >> how can I visit the u-boot-spi/next? do there any link? >> > > See MAINTAINERS: https://source.denx.de/u-boot/custodians/u-boot-spi.git > > But also that tree contains no usage of the flash_is_locked callback. > That was once evaluated by drivers/mtd/spi/spi_flash.c but then > forgotten in the new SPI NOR framework it seems. > > Chao's patch makes sense to me to restore this feature. > > Jan > >> >> >> >> >> BRs >> Chao >> >> >> >> At 2021-06-29 21:50:28, "Jagan Teki" <jagan@amarulasolutions.com> wrote: >>> On Tue, Jun 22, 2021 at 10:51 AM chao zeng <chaochao2021666@163.com> wrote: >>>> >>>> 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 if the write-protection has enbale >>>> before operating the flash. >>>> >>>> Signed-off-by: Chao Zeng <chao.zeng@siemens.com> >>>> --- >>> >>> Does it broken on master? if yes can you check in u-boot-spi/next? >>> >>> Jagan. > Ping - how do we proceed here? Jan
On 04.10.21 15:36, Jan Kiszka wrote: > On 13.09.21 09:48, Jan Kiszka wrote: >> Hi all, >> >> Chao, please no top-post on mailing list. Also check your mail client, >> it seems to inject a lot of bogus newlines. >> >> On 08.09.21 06:55, chaochao2021666 wrote: >>> >>> >>> >>> HI Jagan >>> >>> >>> >>> sorry for the delay response. >>> >>> >>> And I have checked the maser. There is still a problem with this feature。 >>> >>> >>> reproduce steps: >>> 1. enable the flash protect function >>> 2. using sf cmd to erase the flash. I can get the erase "OK",not the "error". >>> >>> >>> >>> I think the root cause is that the detection mechanism is missing and to judge the permissions of the action >>> >>> So pull this PR to improve the erase flow >>> >>> >>> another question: >>> how can I visit the u-boot-spi/next? do there any link? >>> >> >> See MAINTAINERS: https://source.denx.de/u-boot/custodians/u-boot-spi.git >> >> But also that tree contains no usage of the flash_is_locked callback. >> That was once evaluated by drivers/mtd/spi/spi_flash.c but then >> forgotten in the new SPI NOR framework it seems. >> >> Chao's patch makes sense to me to restore this feature. >> >> Jan >> >>> >>> >>> >>> >>> BRs >>> Chao >>> >>> >>> >>> At 2021-06-29 21:50:28, "Jagan Teki" <jagan@amarulasolutions.com> wrote: >>>> On Tue, Jun 22, 2021 at 10:51 AM chao zeng <chaochao2021666@163.com> wrote: >>>>> >>>>> 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 if the write-protection has enbale >>>>> before operating the flash. >>>>> >>>>> Signed-off-by: Chao Zeng <chao.zeng@siemens.com> >>>>> --- >>>> >>>> Does it broken on master? if yes can you check in u-boot-spi/next? >>>> >>>> Jagan. >> > > Ping - how do we proceed here? > > Jan > Ping² - who can help to move this forward? Thanks, Jan
On Wed, Nov 03, 2021 at 01:18:42PM +0100, Jan Kiszka wrote: > On 04.10.21 15:36, Jan Kiszka wrote: > > On 13.09.21 09:48, Jan Kiszka wrote: > >> Hi all, > >> > >> Chao, please no top-post on mailing list. Also check your mail client, > >> it seems to inject a lot of bogus newlines. > >> > >> On 08.09.21 06:55, chaochao2021666 wrote: > >>> > >>> > >>> > >>> HI Jagan > >>> > >>> > >>> > >>> sorry for the delay response. > >>> > >>> > >>> And I have checked the maser. There is still a problem with this feature。 > >>> > >>> > >>> reproduce steps: > >>> 1. enable the flash protect function > >>> 2. using sf cmd to erase the flash. I can get the erase "OK",not the "error". > >>> > >>> > >>> > >>> I think the root cause is that the detection mechanism is missing and to judge the permissions of the action > >>> > >>> So pull this PR to improve the erase flow > >>> > >>> > >>> another question: > >>> how can I visit the u-boot-spi/next? do there any link? > >>> > >> > >> See MAINTAINERS: https://source.denx.de/u-boot/custodians/u-boot-spi.git > >> > >> But also that tree contains no usage of the flash_is_locked callback. > >> That was once evaluated by drivers/mtd/spi/spi_flash.c but then > >> forgotten in the new SPI NOR framework it seems. > >> > >> Chao's patch makes sense to me to restore this feature. > >> > >> Jan > >> > >>> > >>> > >>> > >>> > >>> BRs > >>> Chao > >>> > >>> > >>> > >>> At 2021-06-29 21:50:28, "Jagan Teki" <jagan@amarulasolutions.com> wrote: > >>>> On Tue, Jun 22, 2021 at 10:51 AM chao zeng <chaochao2021666@163.com> wrote: > >>>>> > >>>>> 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 if the write-protection has enbale > >>>>> before operating the flash. > >>>>> > >>>>> Signed-off-by: Chao Zeng <chao.zeng@siemens.com> > >>>>> --- > >>>> > >>>> Does it broken on master? if yes can you check in u-boot-spi/next? > >>>> > >>>> Jagan. > >> > > > > Ping - how do we proceed here? > > > > Jan > > > > Ping² - who can help to move this forward? I've put this in my to-watch bundle, if Jagan doesn't chime in soon. Thanks!
Hi, On 6/22/21 8:21 AM, chao zeng wrote: > 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 if the write-protection has enbale > before operating the flash. > > Signed-off-by: Chao Zeng <chao.zeng@siemens.com> > --- > > 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 3befbe91ca..f06e6b88bd 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)) { > + debug("SF: Flash is locked\n"); > + return -ENOPROTOOPT; Keep a debug message, but return 0 please. Writes or erases on protected areas are ignored by the flash, we should reflect that in the code. Cheers, ta
On Fri, Nov 5, 2021 at 10:47 PM <Tudor.Ambarus@microchip.com> wrote: > > Hi, > > On 6/22/21 8:21 AM, chao zeng wrote: > > 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 if the write-protection has enbale > > before operating the flash. > > > > Signed-off-by: Chao Zeng <chao.zeng@siemens.com> > > --- > > > > 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 3befbe91ca..f06e6b88bd 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)) { > > + debug("SF: Flash is locked\n"); > > + return -ENOPROTOOPT; > > Keep a debug message, but return 0 please. Writes or erases on protected areas > are ignored by the flash, we should reflect that in the code. Agreed this point, Chao are you fine to do this change while applying it? Jagan.
> On Fri, Nov 5, 2021 at 10:47 PM <Tudor.Ambarus@microchip.com> wrote: > > > > Hi, > > > > On 6/22/21 8:21 AM, chao zeng wrote: > > > 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 if the write-protection has enbale > > > before operating the flash. > > > > > > Signed-off-by: Chao Zeng <chao.zeng@siemens.com> > > > --- > > > > > > 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 3befbe91ca..f06e6b88bd 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)) { > > > + debug("SF: Flash is locked\n"); > > > + return -ENOPROTOOPT; > > > > Keep a debug message, but return 0 please. Writes or erases on protected areas > > are ignored by the flash, we should reflect that in the code. Mh, will this then make the whole write fail? We do rely on the fact, that we can update the whole flash image, but the first sectors will be 'skipped' because the first are write-protected. I guess this patch will then break this. Shouldn't this then be on a per sector basis? -michael
On 11/8/21 11:50 AM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> On Fri, Nov 5, 2021 at 10:47 PM <Tudor.Ambarus@microchip.com> wrote: >>> >>> Hi, >>> >>> On 6/22/21 8:21 AM, chao zeng wrote: >>>> 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 if the write-protection has enbale >>>> before operating the flash. >>>> >>>> Signed-off-by: Chao Zeng <chao.zeng@siemens.com> >>>> --- >>>> >>>> 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 3befbe91ca..f06e6b88bd 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)) { >>>> + debug("SF: Flash is locked\n"); >>>> + return -ENOPROTOOPT; >>> >>> Keep a debug message, but return 0 please. Writes or erases on protected areas >>> are ignored by the flash, we should reflect that in the code. > > Mh, will this then make the whole write fail? We do rely on the fact, > that we can update the whole flash image, but the first sectors will > be 'skipped' because the first are write-protected. I guess this patch > will then break this. > > Shouldn't this then be on a per sector basis? > Writes are already very slow, interrogating the flash for each sector will make the writes even slower. We may extend the API to have a function that return the map of locked sectors, so that we can read only once and decide on a per sector basis. But you are right, it would be better to have the granularity per sector. Other option would be to just check if flash is locked at this level, just print a debug message then fallthrough to the write/erase method and let the flash ignore the opcode on protected areas. Cheers, ta
HI jagan and ta I might have a different view， the caller can not get the correct response even though we can not operate the device sucessfully. I think it is necessary to return a valid value. if return 0, the device cannot actually be operated but the correct results are not possible BRs Chao At 2021-11-06 02:08:04, "Jagan Teki" <jagan@amarulasolutions.com> wrote: >On Fri, Nov 5, 2021 at 10:47 PM <Tudor.Ambarus@microchip.com> wrote: >> >> Hi, >> >> On 6/22/21 8:21 AM, chao zeng wrote: >> > 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 if the write-protection has enbale >> > before operating the flash. >> > >> > Signed-off-by: Chao Zeng <chao.zeng@siemens.com> >> > --- >> > >> > 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 3befbe91ca..f06e6b88bd 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)) { >> > + debug("SF: Flash is locked\n"); >> > + return -ENOPROTOOPT; >> >> Keep a debug message, but return 0 please. Writes or erases on protected areas >> are ignored by the flash, we should reflect that in the code. > >Agreed this point, Chao are you fine to do this change while applying it? > >Jagan.
On 11/9/21 3:52 AM, chaochao2021666 wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > HI jagan and ta Hi, Please don't top post in the future. > > I might have a different view， the caller can not get the correct response even though we can not > operate the device sucessfully. > > I think it is necessary to return a valid value. > if return 0, the device cannot actually be operated but the correct Actually the last recommendation was to just print a debug message and then fallthrough the erase/write methods and let the flash ignore the commands on the protected areas. So no change in functionality, but merely a debug message that informs the user that there are protected areas in the requested length and the flash will ignore erases and writes on those. results are not possible Erases and writes on the remaining unprotected sectors are working fine, aren't they? Cheers, ta > > BRs > Chao > > At 2021-11-06 02:08:04, "Jagan Teki" <jagan@amarulasolutions.com> wrote: >> On Fri, Nov 5, 2021 at 10:47 PM <Tudor.Ambarus@microchip.com> wrote: >>> >>> Hi, >>> >>> On 6/22/21 8:21 AM, chao zeng wrote: >>>> 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 if the write-protection has enbale >>>> before operating the flash. >>>> >>>> Signed-off-by: Chao Zeng <chao.zeng@siemens.com> >>>> --- >>>> >>>> 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 3befbe91ca..f06e6b88bd 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)) { >>>> + debug("SF: Flash is locked\n"); >>>> + return -ENOPROTOOPT; >>> >>> Keep a debug message, but return 0 please. Writes or erases on protected areas >>> are ignored by the flash, we should reflect that in the code. >> >> Agreed this point, Chao are you fine to do this change while applying it? >> >> Jagan. >
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 3befbe91ca..f06e6b88bd 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)) { + debug("SF: Flash is locked\n"); + return -ENOPROTOOPT; + } + 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)) { + debug("SF: Flash is locked\n"); + return -ENOPROTOOPT; + } + return mtd->_erase(mtd, &instr); }