linux-kernel - Re: [PATCH v3 5/6] mmc: core: add mmc_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPDyKFr4urmw75MB48nCOYq+8n1DEufqY9b7EN8mooL2xTY0pw@mail.gmail.com>
Date: Fri, 15 Aug 2025 14:23:37 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Benoît Monin <benoit.monin@...tlin.com>
Cc: Adrian Hunter <adrian.hunter@...el.com>, linux-mmc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>, 
	Tawfik Bayouk <tawfik.bayouk@...ileye.com>, Gregory CLEMENT <gregory.clement@...tlin.com>, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v3 5/6] mmc: core: add mmc_read_tuning

On Wed, 16 Jul 2025 at 17:47, Benoît Monin <benoit.monin@...tlin.com> wrote:
>
> Provide a function to the MMC hosts to read some blocks of data as part
> of their tuning. The card parameter is optional since it is not
> available from the execute_tuning() operation, but present in
> execute_hs400_tuning() and prepare_sd_hs_tuning().
>
> This function only returns the status of the read operation, not the
> data read.
>
> Signed-off-by: Benoît Monin <benoit.monin@...tlin.com>
> ---
>  drivers/mmc/core/mmc_ops.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/host.h   |  2 ++
>  2 files changed, 81 insertions(+)
>
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 66283825513cb4ff993a1b2ec1f0b0cac4e74487..d29e5daf3e326ab37e61c99456421b1f66bcb0de 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -1077,3 +1077,82 @@ int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms)
>         return err;
>  }
>  EXPORT_SYMBOL_GPL(mmc_sanitize);
> +
> +/**
> + * mmc_read_tuning() - read data blocks from the mmc
> + * @card: mmc card to read from, can be NULL
> + * @host: mmc host doing the read
> + * @blksz: data block size
> + * @blocks: number of blocks to read
> + *
> + * Read one or more blocks of data from the beginning of the mmc. This is a
> + * low-level helper for tuning operation. If card is NULL, it is assumed that
> + * CMD23 can be used for multi-block read.

It makes sense to have a comment for this, but I would just just drop
the "struct mmc_card *card" as an in-parameter. If it turns out to be
needed later on, we would need additional changes to enable the caller
of mmc_read_tuning() to pass the "card" along.

> + *
> + * Note: Allocate and free a temporary buffer to store the data read. The data
> + * is not available outside of the function, only the status of the read
> + * operation.
> + *
> + * Return: 0 in case of success, otherwise -EIO / -ENOMEM / -E2BIG
> + */
> +int mmc_read_tuning(struct mmc_card *card, struct mmc_host *host,
> +                   unsigned int blksz, unsigned int blocks)
> +{
> +       struct mmc_request mrq = {};
> +       struct mmc_command sbc = {};
> +       struct mmc_command cmd = {};
> +       struct mmc_command stop = {};
> +       struct mmc_data data = {};
> +       struct scatterlist sg;
> +       void *buf;
> +       unsigned int len;
> +
> +       if (blocks > 1) {
> +               if (mmc_host_can_cmd23(host) &&
> +                   (!card || (mmc_card_can_cmd23(card) &&
> +                    !mmc_card_blk_no_cmd23(card)))) {
> +                       mrq.sbc = &sbc;
> +                       sbc.opcode = MMC_SET_BLOCK_COUNT;
> +                       sbc.arg = blocks;
> +                       sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +               }
> +               cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
> +               mrq.stop = &stop;
> +               stop.opcode = MMC_STOP_TRANSMISSION;
> +               stop.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> +       } else {
> +               cmd.opcode = MMC_READ_SINGLE_BLOCK;
> +       }
> +
> +       mrq.cmd = &cmd;
> +       cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +       mrq.data = &data;
> +       data.flags = MMC_DATA_READ;
> +       data.blksz = blksz;
> +       data.blocks = blocks;
> +       data.blk_addr = 0;
> +       data.sg = &sg;
> +       data.sg_len = 1;
> +       if (card)
> +               mmc_set_data_timeout(&data, card);
> +       else
> +               data.timeout_ns = 1000000000;
> +
> +       if (check_mul_overflow(blksz, blocks, &len))
> +               return -E2BIG;
> +       buf = kmalloc(len, GFP_KERNEL);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       sg_init_one(&sg, buf, len);
> +
> +       mmc_wait_for_req(host, &mrq);
> +       kfree(buf);
> +
> +       if (sbc.error || cmd.error || data.error)
> +               return -EIO;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(mmc_read_tuning);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 68f09a955a902047ac517441b6820fa6e4166a13..5a6471a6219222b199a16afd9e6bd5ab74b05c86 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -743,5 +743,7 @@ int mmc_send_status(struct mmc_card *card, u32 *status);
>  int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
>  int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
>  int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
> +int mmc_read_tuning(struct mmc_card *card, struct mmc_host *host,
> +                   unsigned int blksz, unsigned int blocks);
>
>  #endif /* LINUX_MMC_HOST_H */
>
> --
> 2.50.1
>

Otherwise this looks good to me!

Kind regards
Uffe