| Message ID | 20210714235109.25228-1-marek.behun@nic.cz |
|---|---|
| Headers | show |
| Series | Fix `mtd erase` when used with mtdpart | expand |
Hi Marek, On Wed, 14 Jul 2021 at 17:51, Marek Behún <marek.behun@nic.cz> wrote: > > Hello, > > I accidentally forgot to send this series to U-Boot's mailing list last > time, meaning it did not end up in patchwork, so now I am resending it. > Sorry for this mess. > > The original cover letter said: > > this patch series fixes the `mtd erase` command when used with mtdpart > with a partition of non-zero offset. > > Currently when the `mtd erase` command is used for such a partition, > it does not erase all blocks. Instead after a block is erased, the next > block address not current block address + block size, but current block > address + block size + partition offset, due to spi_nor_erase() not > calling mtd_erase_callback(): > => mtd erase "Rescue system" > Erasing 0x00000000 ... 0x006fffff (1792 eraseblock(s)) > jedec_spi_nor spi-nor@0: at 0x100000, len 4096 > jedec_spi_nor spi-nor@0: at 0x201000, len 4096 > jedec_spi_nor spi-nor@0: at 0x302000, len 4096 > jedec_spi_nor spi-nor@0: at 0x403000, len 4096 > jedec_spi_nor spi-nor@0: at 0x504000, len 4096 > jedec_spi_nor spi-nor@0: at 0x605000, len 4096 > jedec_spi_nor spi-nor@0: at 0x706000, len 4096 > > This series adds some fixes to spi_nor_erase() function, then adds > calling of mtd_erase_callback() to fix this bug. > > The series also contains an improvement - adding the posibility to > interrupt spi_nor_erase() with Ctrl+C; and another one - making mtdpart's > _erase() method more sane so that the above mentioned bug will not occur > even if underlying driver does not call mtd_erase_callback(). > > Moreover I would also like to start a discussion regarding the MTD > subsystem: > - U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ > macros > - this was done to make it easier to port Linux's patches to U-Boot > - the problem is that it seems nobody did port Linux's MTD patches to > U-Boot for a long time, the code is many times different from Linux', > and it would be very hard to align it > - therefore I propose to get rid of all the #ifdefs, remove the Linux > specific code, and continue developing the code independently from > Linux. This would make it impossible to apply Linux patches in some > kind of automatic way, but this is currently already impossible > anyway > What do you guys think? That sounds good to me, but I wonder what is the gap today? Are you and/or Jagen going to take on this effort? Regards, Simon
Hi Marek, On Thu, Jul 15, 2021 at 5:21 AM Marek Behún <marek.behun@nic.cz> wrote: > > Hello, > > I accidentally forgot to send this series to U-Boot's mailing list last > time, meaning it did not end up in patchwork, so now I am resending it. > Sorry for this mess. > > The original cover letter said: > > this patch series fixes the `mtd erase` command when used with mtdpart > with a partition of non-zero offset. > > Currently when the `mtd erase` command is used for such a partition, > it does not erase all blocks. Instead after a block is erased, the next > block address not current block address + block size, but current block > address + block size + partition offset, due to spi_nor_erase() not > calling mtd_erase_callback(): > => mtd erase "Rescue system" > Erasing 0x00000000 ... 0x006fffff (1792 eraseblock(s)) > jedec_spi_nor spi-nor@0: at 0x100000, len 4096 > jedec_spi_nor spi-nor@0: at 0x201000, len 4096 > jedec_spi_nor spi-nor@0: at 0x302000, len 4096 > jedec_spi_nor spi-nor@0: at 0x403000, len 4096 > jedec_spi_nor spi-nor@0: at 0x504000, len 4096 > jedec_spi_nor spi-nor@0: at 0x605000, len 4096 > jedec_spi_nor spi-nor@0: at 0x706000, len 4096 > > This series adds some fixes to spi_nor_erase() function, then adds > calling of mtd_erase_callback() to fix this bug. > > The series also contains an improvement - adding the posibility to > interrupt spi_nor_erase() with Ctrl+C; and another one - making mtdpart's > _erase() method more sane so that the above mentioned bug will not occur > even if underlying driver does not call mtd_erase_callback(). > > Moreover I would also like to start a discussion regarding the MTD > subsystem: > - U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ > macros > - this was done to make it easier to port Linux's patches to U-Boot > - the problem is that it seems nobody did port Linux's MTD patches to > U-Boot for a long time, the code is many times different from Linux', > and it would be very hard to align it > - therefore I propose to get rid of all the #ifdefs, remove the Linux > specific code, and continue developing the code independently from > Linux. This would make it impossible to apply Linux patches in some > kind of automatic way, but this is currently already impossible > anyway > What do you guys think? > > Marek > > Marek Behún (8): > mtd: spi-nor-core: Try cleaning up in case writing BAR failed > mtd: spi-nor-core: Check return value of write_enable() in > spi_nor_erase() > mtd: spi-nor-core: Don't overwrite return value if it is non-zero > mtd: spi-nor-core: Check return value of write_disable() in > spi_nor_erase() > mtd: spi-nor-core: Don't check for zero length in spi_nor_erase() > mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase() > mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase() > mtd: mtdpart: Make mtdpart's _erase method sane Found the build error with CI [1], would you please check? [1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345 Jagan.
On Wed, 21 Jul 2021 21:46:56 +0530 Jagan Teki <jagan@amarulasolutions.com> wrote: > Hi Marek, > > On Thu, Jul 15, 2021 at 5:21 AM Marek Behún <marek.behun@nic.cz> wrote: > > > > Hello, > > > > I accidentally forgot to send this series to U-Boot's mailing list last > > time, meaning it did not end up in patchwork, so now I am resending it. > > Sorry for this mess. > > > > The original cover letter said: > > > > this patch series fixes the `mtd erase` command when used with mtdpart > > with a partition of non-zero offset. > > > > Currently when the `mtd erase` command is used for such a partition, > > it does not erase all blocks. Instead after a block is erased, the next > > block address not current block address + block size, but current block > > address + block size + partition offset, due to spi_nor_erase() not > > calling mtd_erase_callback(): > > => mtd erase "Rescue system" > > Erasing 0x00000000 ... 0x006fffff (1792 eraseblock(s)) > > jedec_spi_nor spi-nor@0: at 0x100000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x201000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x302000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x403000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x504000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x605000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x706000, len 4096 > > > > This series adds some fixes to spi_nor_erase() function, then adds > > calling of mtd_erase_callback() to fix this bug. > > > > The series also contains an improvement - adding the posibility to > > interrupt spi_nor_erase() with Ctrl+C; and another one - making mtdpart's > > _erase() method more sane so that the above mentioned bug will not occur > > even if underlying driver does not call mtd_erase_callback(). > > > > Moreover I would also like to start a discussion regarding the MTD > > subsystem: > > - U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ > > macros > > - this was done to make it easier to port Linux's patches to U-Boot > > - the problem is that it seems nobody did port Linux's MTD patches to > > U-Boot for a long time, the code is many times different from Linux', > > and it would be very hard to align it > > - therefore I propose to get rid of all the #ifdefs, remove the Linux > > specific code, and continue developing the code independently from > > Linux. This would make it impossible to apply Linux patches in some > > kind of automatic way, but this is currently already impossible > > anyway > > What do you guys think? > > > > Marek > > > > Marek Behún (8): > > mtd: spi-nor-core: Try cleaning up in case writing BAR failed > > mtd: spi-nor-core: Check return value of write_enable() in > > spi_nor_erase() > > mtd: spi-nor-core: Don't overwrite return value if it is non-zero > > mtd: spi-nor-core: Check return value of write_disable() in > > spi_nor_erase() > > mtd: spi-nor-core: Don't check for zero length in spi_nor_erase() > > mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase() > > mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase() > > mtd: mtdpart: Make mtdpart's _erase method sane > > Found the build error with CI [1], would you please check? > > [1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345 > > Jagan. Jagan, I am unable to get the output of the failed CI tests. Probably because I do not have an account at source.denx.de. I tried to run "sandbox with clang" CI scenario on my local machine and it is successful. Can you send me outputs of the failed scenarios? Marek
On Thu, Jul 22, 2021 at 10:44:59PM +0200, Marek Behun wrote: > On Wed, 21 Jul 2021 21:46:56 +0530 > Jagan Teki <jagan@amarulasolutions.com> wrote: > > > Hi Marek, > > > > On Thu, Jul 15, 2021 at 5:21 AM Marek Behún <marek.behun@nic.cz> wrote: > > > > > > Hello, > > > > > > I accidentally forgot to send this series to U-Boot's mailing list last > > > time, meaning it did not end up in patchwork, so now I am resending it. > > > Sorry for this mess. > > > > > > The original cover letter said: > > > > > > this patch series fixes the `mtd erase` command when used with mtdpart > > > with a partition of non-zero offset. > > > > > > Currently when the `mtd erase` command is used for such a partition, > > > it does not erase all blocks. Instead after a block is erased, the next > > > block address not current block address + block size, but current block > > > address + block size + partition offset, due to spi_nor_erase() not > > > calling mtd_erase_callback(): > > > => mtd erase "Rescue system" > > > Erasing 0x00000000 ... 0x006fffff (1792 eraseblock(s)) > > > jedec_spi_nor spi-nor@0: at 0x100000, len 4096 > > > jedec_spi_nor spi-nor@0: at 0x201000, len 4096 > > > jedec_spi_nor spi-nor@0: at 0x302000, len 4096 > > > jedec_spi_nor spi-nor@0: at 0x403000, len 4096 > > > jedec_spi_nor spi-nor@0: at 0x504000, len 4096 > > > jedec_spi_nor spi-nor@0: at 0x605000, len 4096 > > > jedec_spi_nor spi-nor@0: at 0x706000, len 4096 > > > > > > This series adds some fixes to spi_nor_erase() function, then adds > > > calling of mtd_erase_callback() to fix this bug. > > > > > > The series also contains an improvement - adding the posibility to > > > interrupt spi_nor_erase() with Ctrl+C; and another one - making mtdpart's > > > _erase() method more sane so that the above mentioned bug will not occur > > > even if underlying driver does not call mtd_erase_callback(). > > > > > > Moreover I would also like to start a discussion regarding the MTD > > > subsystem: > > > - U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ > > > macros > > > - this was done to make it easier to port Linux's patches to U-Boot > > > - the problem is that it seems nobody did port Linux's MTD patches to > > > U-Boot for a long time, the code is many times different from Linux', > > > and it would be very hard to align it > > > - therefore I propose to get rid of all the #ifdefs, remove the Linux > > > specific code, and continue developing the code independently from > > > Linux. This would make it impossible to apply Linux patches in some > > > kind of automatic way, but this is currently already impossible > > > anyway > > > What do you guys think? > > > > > > Marek > > > > > > Marek Behún (8): > > > mtd: spi-nor-core: Try cleaning up in case writing BAR failed > > > mtd: spi-nor-core: Check return value of write_enable() in > > > spi_nor_erase() > > > mtd: spi-nor-core: Don't overwrite return value if it is non-zero > > > mtd: spi-nor-core: Check return value of write_disable() in > > > spi_nor_erase() > > > mtd: spi-nor-core: Don't check for zero length in spi_nor_erase() > > > mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase() > > > mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase() > > > mtd: mtdpart: Make mtdpart's _erase method sane > > > > Found the build error with CI [1], would you please check? > > > > [1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345 > > > > Jagan. > > Jagan, I am unable to get the output of the failed CI tests. Probably > because I do not have an account at source.denx.de. Jagan, please check the permissions on your tree, the results should be set to public.
Hi Jagan, On Wed, 21 Jul 2021 21:46:56 +0530 Jagan Teki <jagan@amarulasolutions.com> wrote: > Found the build error with CI [1], would you please check? > > [1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345 > > Jagan. OK I think I've found out what is the problem. I've pushed new version into github CI to check if it builds correctly. The problem seems to be that after this series the function spi_nor_erase() calls mtd_erase_callback(), which is declared in the header file include/linux/mtd/mtd.h, if CONFIG_MTD_PARTITIONS is enabled, and defined as a static inline function otherwise. The problem is that for some boards we have CONFIG_MTD_PARTITIONS together with CONFIG_SPL_SPI_FLASH_SUPPORT. But in SPL, mtdpart.c (where mtd_erase_callback() is defined) is not compiled at all. Thus this leads to undefined reference to mtd_erase_callback(). This is another proof that the whole mtd subsystem has become a gross spaghetti code where hacks upon hacks were introduced by different people to solve different purposes, and the result makes me angry. :-D We really need to rewrite this. Anyway, for now I will just send v2 of this series with another patch fixing this issue, once CI ends smoothly. Marek
Hello, I accidentally forgot to send this series to U-Boot's mailing list last time, meaning it did not end up in patchwork, so now I am resending it. Sorry for this mess. The original cover letter said: this patch series fixes the `mtd erase` command when used with mtdpart with a partition of non-zero offset. Currently when the `mtd erase` command is used for such a partition, it does not erase all blocks. Instead after a block is erased, the next block address not current block address + block size, but current block address + block size + partition offset, due to spi_nor_erase() not calling mtd_erase_callback(): => mtd erase "Rescue system" Erasing 0x00000000 ... 0x006fffff (1792 eraseblock(s)) jedec_spi_nor spi-nor@0: at 0x100000, len 4096 jedec_spi_nor spi-nor@0: at 0x201000, len 4096 jedec_spi_nor spi-nor@0: at 0x302000, len 4096 jedec_spi_nor spi-nor@0: at 0x403000, len 4096 jedec_spi_nor spi-nor@0: at 0x504000, len 4096 jedec_spi_nor spi-nor@0: at 0x605000, len 4096 jedec_spi_nor spi-nor@0: at 0x706000, len 4096 This series adds some fixes to spi_nor_erase() function, then adds calling of mtd_erase_callback() to fix this bug. The series also contains an improvement - adding the posibility to interrupt spi_nor_erase() with Ctrl+C; and another one - making mtdpart's _erase() method more sane so that the above mentioned bug will not occur even if underlying driver does not call mtd_erase_callback(). Moreover I would also like to start a discussion regarding the MTD subsystem: - U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ macros - this was done to make it easier to port Linux's patches to U-Boot - the problem is that it seems nobody did port Linux's MTD patches to U-Boot for a long time, the code is many times different from Linux', and it would be very hard to align it - therefore I propose to get rid of all the #ifdefs, remove the Linux specific code, and continue developing the code independently from Linux. This would make it impossible to apply Linux patches in some kind of automatic way, but this is currently already impossible anyway What do you guys think? Marek Marek Behún (8): mtd: spi-nor-core: Try cleaning up in case writing BAR failed mtd: spi-nor-core: Check return value of write_enable() in spi_nor_erase() mtd: spi-nor-core: Don't overwrite return value if it is non-zero mtd: spi-nor-core: Check return value of write_disable() in spi_nor_erase() mtd: spi-nor-core: Don't check for zero length in spi_nor_erase() mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase() mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase() mtd: mtdpart: Make mtdpart's _erase method sane drivers/mtd/mtdpart.c | 26 +++++++++++++------- drivers/mtd/spi/spi-nor-core.c | 44 ++++++++++++++++++++++++++-------- 2 files changed, 52 insertions(+), 18 deletions(-)