| Message ID | 20210526175924.4f9ab124@xps13 |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [GIT,PULL] mtd: Changes for v5.13-rc4 | expand |
On Wed, May 26, 2021 at 5:59 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Raw NAND: > * txx9ndfmc, tmio, sharpsl, ndfc, lpc32xx_slc, fsmc, cs553x: > - Fix external use of SW Hamming ECC helper Why are these guys all pointlessly duplicating the ecc wrapper functions for their ecc 'correct' functions? The whole "the Hamming software ECC engine has been updated to become a proper and independent ECC engine" excuse makes no sense. If multiple chips just want a basic sw hamming helper, then they should have one. Not have to be forced to each write their own pointless wrapper like this. These chip drivers just want 'ecc_sw_hamming_correct()' with the proper arguments, and it seems entirely wrong to duplicate the helper five times or whatever. There should just be a generic helper - the way there used to be. In fact, I would generally strongly recommend that if there used to be a generic helper that different chip drivers used (ie the old rawnand_sw_hamming_correct()), then such a helper should be left alone and not change the semantics of it. The new "proper independent ECC engine" that had new semantics should have been the one that got a new name, rather than breaking an old and existing helper function and then making the chip drivers pointlessly write their own new helper functions. I've pulled this, but under protest. The patch honestly just looks like mindless duplication. Linus
The pull request you sent on Wed, 26 May 2021 17:59:24 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git tags/mtd/fixes-for-5.13-rc4
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/7ac3a1c1ae5152e1d18cf6df5b6c3c9847535e78
Thank you!
Hi Linus, Linus Torvalds <torvalds@linux-foundation.org> wrote on Wed, 26 May 2021 06:20:35 -1000: > On Wed, May 26, 2021 at 5:59 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Raw NAND: > > * txx9ndfmc, tmio, sharpsl, ndfc, lpc32xx_slc, fsmc, cs553x: > > - Fix external use of SW Hamming ECC helper > > Why are these guys all pointlessly duplicating the ecc wrapper > functions for their ecc 'correct' functions? > > The whole "the Hamming software ECC engine has been updated to become > a proper and independent ECC engine" excuse makes no sense. If > multiple chips just want a basic sw hamming helper, then they should > have one. Not have to be forced to each write their own pointless > wrapper like this. > > These chip drivers just want 'ecc_sw_hamming_correct()' with the > proper arguments, and it seems entirely wrong to duplicate the helper > five times or whatever. There should just be a generic helper - the > way there used to be. > > In fact, I would generally strongly recommend that if there used to be > a generic helper that different chip drivers used (ie the old > rawnand_sw_hamming_correct()), then such a helper should be left alone > and not change the semantics of it. I am not happy neither with the fix (which I wrote myself) as my first goal was to uniformize the way the Hamming helpers are being called (as part of a much bigger work). I assumed that all drivers either used the Hamming software engine or simply didn't, without thinking about the "intermediate" situations where a particular driver would just want to call a particular Hamming helper to workaround its "missing" hardware capabilities. Unfortunately when I spotted that many drivers were broken by my rework I decided to provide per-driver fixes, while, as you suggest, I should probably have declared a generic 'hamming correct' core helper and use that directly instead of duplicating the logic in each broken driver. > The new "proper independent ECC engine" that had new semantics should > have been the one that got a new name, rather than breaking an old and > existing helper function and then making the chip drivers pointlessly > write their own new helper functions. > > I've pulled this, but under protest. The patch honestly just looks > like mindless duplication. Thanks. Miquèl
Hi Linus, miquel.raynal@bootlin.com wrote on Wed, 26 May 2021 18:46:12 +0200: > Hi Linus, > > Linus Torvalds <torvalds@linux-foundation.org> wrote on Wed, 26 May > 2021 06:20:35 -1000: > > > On Wed, May 26, 2021 at 5:59 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Raw NAND: > > > * txx9ndfmc, tmio, sharpsl, ndfc, lpc32xx_slc, fsmc, cs553x: > > > - Fix external use of SW Hamming ECC helper > > > > Why are these guys all pointlessly duplicating the ecc wrapper > > functions for their ecc 'correct' functions? > > > > The whole "the Hamming software ECC engine has been updated to become > > a proper and independent ECC engine" excuse makes no sense. If > > multiple chips just want a basic sw hamming helper, then they should > > have one. Not have to be forced to each write their own pointless > > wrapper like this. > > > > These chip drivers just want 'ecc_sw_hamming_correct()' with the > > proper arguments, and it seems entirely wrong to duplicate the helper > > five times or whatever. There should just be a generic helper - the > > way there used to be. > > > > In fact, I would generally strongly recommend that if there used to be > > a generic helper that different chip drivers used (ie the old > > rawnand_sw_hamming_correct()), then such a helper should be left alone > > and not change the semantics of it. > > I am not happy neither with the fix (which I wrote myself) as my first > goal was to uniformize the way the Hamming helpers are being called (as > part of a much bigger work). I assumed that all drivers either used the > Hamming software engine or simply didn't, without thinking about the > "intermediate" situations where a particular driver would just want to > call a particular Hamming helper to workaround its "missing" hardware > capabilities. > > Unfortunately when I spotted that many drivers were broken by my rework > I decided to provide per-driver fixes, while, as you suggest, I should > probably have declared a generic 'hamming correct' core helper and use > that directly instead of duplicating the logic in each broken driver. > > > The new "proper independent ECC engine" that had new semantics should > > have been the one that got a new name, rather than breaking an old and > > existing helper function and then making the chip drivers pointlessly > > write their own new helper functions. > > > > I've pulled this, but under protest. The patch honestly just looks > > like mindless duplication. Just to let you know that I proposed there [1] a series to clean this up. [1] https://lore.kernel.org/linux-mtd/20210928221507.199198-1-miquel.raynal@bootlin.com/T/#t Thanks, Miquèl
Hello Linus, This is an MTD fixes PR for the next release cycle. Thanks, Miquèl The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5: Linux 5.13-rc1 (2021-05-09 14:17:44 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git tags/mtd/fixes-for-5.13-rc4 for you to fetch changes up to 562b4e91d3b221f737f84ff78ee7d348c8a6891f: mtd: parsers: ofpart: fix parsing subpartitions (2021-05-10 18:34:30 +0200) ---------------------------------------------------------------- MTD parsers: * ofpart: - Fix subpartitions parsing Raw NAND: * txx9ndfmc, tmio, sharpsl, ndfc, lpc32xx_slc, fsmc, cs553x: - Fix external use of SW Hamming ECC helper ---------------------------------------------------------------- Miquel Raynal (7): mtd: rawnand: cs553x: Fix external use of SW Hamming ECC helper mtd: rawnand: fsmc: Fix external use of SW Hamming ECC helper mtd: rawnand: lpc32xx_slc: Fix external use of SW Hamming ECC helper mtd: rawnand: ndfc: Fix external use of SW Hamming ECC helper mtd: rawnand: sharpsl: Fix external use of SW Hamming ECC helper mtd: rawnand: tmio: Fix external use of SW Hamming ECC helper mtd: rawnand: txx9ndfmc: Fix external use of SW Hamming ECC helper Rafał Miłecki (1): mtd: parsers: ofpart: fix parsing subpartitions drivers/mtd/nand/raw/cs553x_nand.c | 12 +++++++++++- drivers/mtd/nand/raw/fsmc_nand.c | 12 +++++++++++- drivers/mtd/nand/raw/lpc32xx_slc.c | 15 ++++++++++++++- drivers/mtd/nand/raw/ndfc.c | 12 +++++++++++- drivers/mtd/nand/raw/sharpsl.c | 12 +++++++++++- drivers/mtd/nand/raw/tmio_nand.c | 8 +++++--- drivers/mtd/nand/raw/txx9ndfmc.c | 5 +++-- drivers/mtd/parsers/ofpart_core.c | 26 ++++++++++++++------------ 8 files changed, 80 insertions(+), 22 deletions(-)