lkml.org 
[lkml]   [2021]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 0/5] mtdchar: add MEMREAD ioctl
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(-)
>

\
 
 \ /
  Last update: 2021-12-16 11:25    [W:0.148 / U:0.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site