Re: [PATCH v2 0/5] mtdchar: add MEMREAD ioctl

[Boris Brezillon <boris.brezillon@collabora.com>]

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(-)
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/