| Message ID | 20211216083418.13512-1-kernel@kempniu.pl |
|---|---|
| Headers | show |
| Series | mtdchar: add MEMREAD ioctl | expand |
On Thu, 16 Dec 2021 09:34:13 +0100 Michał Kępień <kernel@kempniu.pl> wrote: > This patch series adds a new mtdchar ioctl, MEMREAD. Its purpose is to > serve as a read counterpart of the MEMWRITE ioctl, exposing a broader > set of capabilities for read operations (e.g. use of MTD_OPS_AUTO_OOB, > access to ECC statistics) to user-space applications making use of MTD > devices via /dev/mtd* character devices. > > Changes from v1: > > - Added patches 2-5 which enable the new MEMREAD ioctl to report ECC > statistics for the read operation back to user space. (There are > obviously different ways these changes can be split up into separate > commits; I was aiming for maximum ease of review.) > > - The 'retlen' and 'oobretlen' fields were not set in the struct > mtd_read_req returned to userspace. This was done properly in > Boris' original draft patch [1], but I missed it in my v1. > > - Invalid IS_ERR() checks were replaced with NULL checks. This was an > artifact of copy-pasting mtdchar_write_ioctl() in v1: unlike > memdup_user() used therein, kmalloc() always returns NULL on error. > > - Minor subject prefix adjustment for patch 1/5 ("mtd" -> "mtdchar"). > > [1] https://www.infradead.org/pipermail/linux-mtd/2016-April/067187.html > > Michał Kępień (5): > mtdchar: add MEMREAD ioctl > mtd: track maximum number of bitflips for each read request > mtd: always initialize 'stats' in struct mtd_oob_ops > mtd: add ECC error accounting for each read request > mtdchar: extend MEMREAD ioctl to return ECC statistics Splitting patch 1 and 5 means you have an incompatible ABI change between those 2 commits, thus breaking bisectability. I'd recommend putting patches 2-4 first and squashing patch 1 and 5 in a single commit placed at the end of the series. The other options would be to add a way to extend ioctls in a backward compatible way (the DRM subsystem does that by filling the unspecified part of the struct with zeros, and relying on the fact that 0 values always implies 'default behavior' when the struct is extended [1]). [1]https://elixir.bootlin.com/linux/v5.16-rc5/source/drivers/gpu/drm/drm_ioctl.c#L882 > > drivers/mtd/devices/docg3.c | 8 ++ > drivers/mtd/inftlcore.c | 6 +- > drivers/mtd/mtdchar.c | 136 ++++++++++++++++++++++++ > drivers/mtd/mtdcore.c | 5 + > drivers/mtd/mtdswap.c | 6 +- > drivers/mtd/nand/onenand/onenand_base.c | 16 ++- > drivers/mtd/nand/onenand/onenand_bbt.c | 2 +- > drivers/mtd/nand/raw/nand_base.c | 10 ++ > drivers/mtd/nand/raw/nand_bbt.c | 8 +- > drivers/mtd/nand/raw/sm_common.c | 2 +- > drivers/mtd/nand/spi/core.c | 10 ++ > drivers/mtd/nftlcore.c | 6 +- > drivers/mtd/sm_ftl.c | 4 +- > drivers/mtd/ssfdc.c | 2 +- > drivers/mtd/tests/nandbiterrs.c | 2 +- > drivers/mtd/tests/oobtest.c | 8 +- > drivers/mtd/tests/readtest.c | 2 +- > fs/jffs2/wbuf.c | 6 +- > include/linux/mtd/mtd.h | 7 ++ > include/uapi/mtd/mtd-abi.h | 64 ++++++++++- > 20 files changed, 276 insertions(+), 34 deletions(-) >
> Splitting patch 1 and 5 means you have an incompatible ABI change > between those 2 commits, thus breaking bisectability. I'd recommend > putting patches 2-4 first and squashing patch 1 and 5 in a single > commit placed at the end of the series. The other options would be to > add a way to extend ioctls in a backward compatible way (the DRM > subsystem does that by filling the unspecified part of the struct with > zeros, and relying on the fact that 0 values always implies 'default > behavior' when the struct is extended [1]). Ack, thanks. I will follow your recommendation in v3 and in the meantime the existing split should make it easier to follow how v2 extends v1 with ECC statistics handling.
This patch series adds a new mtdchar ioctl, MEMREAD. Its purpose is to serve as a read counterpart of the MEMWRITE ioctl, exposing a broader set of capabilities for read operations (e.g. use of MTD_OPS_AUTO_OOB, access to ECC statistics) to user-space applications making use of MTD devices via /dev/mtd* character devices. Changes from v1: - Added patches 2-5 which enable the new MEMREAD ioctl to report ECC statistics for the read operation back to user space. (There are obviously different ways these changes can be split up into separate commits; I was aiming for maximum ease of review.) - The 'retlen' and 'oobretlen' fields were not set in the struct mtd_read_req returned to userspace. This was done properly in Boris' original draft patch [1], but I missed it in my v1. - Invalid IS_ERR() checks were replaced with NULL checks. This was an artifact of copy-pasting mtdchar_write_ioctl() in v1: unlike memdup_user() used therein, kmalloc() always returns NULL on error. - Minor subject prefix adjustment for patch 1/5 ("mtd" -> "mtdchar"). [1] https://www.infradead.org/pipermail/linux-mtd/2016-April/067187.html Michał Kępień (5): mtdchar: add MEMREAD ioctl mtd: track maximum number of bitflips for each read request mtd: always initialize 'stats' in struct mtd_oob_ops mtd: add ECC error accounting for each read request mtdchar: extend MEMREAD ioctl to return ECC statistics drivers/mtd/devices/docg3.c | 8 ++ drivers/mtd/inftlcore.c | 6 +- drivers/mtd/mtdchar.c | 136 ++++++++++++++++++++++++ drivers/mtd/mtdcore.c | 5 + drivers/mtd/mtdswap.c | 6 +- drivers/mtd/nand/onenand/onenand_base.c | 16 ++- drivers/mtd/nand/onenand/onenand_bbt.c | 2 +- drivers/mtd/nand/raw/nand_base.c | 10 ++ drivers/mtd/nand/raw/nand_bbt.c | 8 +- drivers/mtd/nand/raw/sm_common.c | 2 +- drivers/mtd/nand/spi/core.c | 10 ++ drivers/mtd/nftlcore.c | 6 +- drivers/mtd/sm_ftl.c | 4 +- drivers/mtd/ssfdc.c | 2 +- drivers/mtd/tests/nandbiterrs.c | 2 +- drivers/mtd/tests/oobtest.c | 8 +- drivers/mtd/tests/readtest.c | 2 +- fs/jffs2/wbuf.c | 6 +- include/linux/mtd/mtd.h | 7 ++ include/uapi/mtd/mtd-abi.h | 64 ++++++++++- 20 files changed, 276 insertions(+), 34 deletions(-)