| 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
| ||
|
Message-ID: <CAPDyKFp=fvyUhkeiw5TmYbELM+MiC8Do20afrainOyq_pLvSHw@mail.gmail.com>
Date: Wed, 9 Jul 2025 16:46:45 +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 v2 1/4] mmc: core: add mmc_read_blocks to mmc_ops
On Mon, 7 Jul 2025 at 17:24, Benoît Monin <benoit.monin@...tlin.com> wrote:
>
> Add a generic function to read some blocks of data from the MMC, to be
> used by drivers as part of their tuning.
>
> Signed-off-by: Benoît Monin <benoit.monin@...tlin.com>
> ---
> drivers/mmc/core/card.h | 10 ++++++
> drivers/mmc/core/mmc_ops.c | 69 ++++++++++++++++++++++++++++++++++++++
> include/linux/mmc/host.h | 3 ++
> 3 files changed, 82 insertions(+)
>
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index 9cbdd240c3a7d..93fd502c1f5fc 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -11,6 +11,7 @@
> #define _MMC_CORE_CARD_H
>
> #include <linux/mmc/card.h>
> +#include <linux/mmc/mmc.h>
>
> #define mmc_card_name(c) ((c)->cid.prod_name)
> #define mmc_card_id(c) (dev_name(&(c)->dev))
> @@ -300,4 +301,13 @@ static inline int mmc_card_no_uhs_ddr50_tuning(const struct mmc_card *c)
> return c->quirks & MMC_QUIRK_NO_UHS_DDR50_TUNING;
> }
>
> +static inline bool mmc_card_can_cmd23(struct mmc_card *card)
> +{
> + return ((mmc_card_mmc(card) &&
> + card->csd.mmca_vsn >= CSD_SPEC_VER_3) ||
> + (mmc_card_sd(card) && !mmc_card_ult_capacity(card) &&
> + card->scr.cmds & SD_SCR_CMD23_SUPPORT)) &&
> + !(card->quirks & MMC_QUIRK_BLK_NO_CMD23);
First, please make the above part a separate patch. It makes sense to
add a helper for this, as you show in patch3 and patch4. I also
recommend that these patches should also be re-ordered so they come
first in the series.
Second, I don't think we should mix mmc_card_can* functions with the
card-quirks. Better to have two separate helpers, especially since
CMD23 is used for other things too, like RPMB and reliable writes, for
example. Thus I suggest we add:
mmc_card_can_cmd23() - which looks at what the card supports, similar
to above without MMC_QUIRK_BLK_NO_CMD23. Put the definition in
drivers/mmc/core/core.h and export the symbols, similar to what we do
for mmc_card_can_erase() and friends.
mmc_card_broken_blk_cmd23() - which should only check
MMC_QUIRK_BLK_NO_CMD23. This belongs in drivers/mmc/core/card.h.
> +}
> +
> #endif
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 66283825513cb..848d8aa3ff2b5 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -1077,3 +1077,72 @@ int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms)
> return err;
> }
> EXPORT_SYMBOL_GPL(mmc_sanitize);
> +
> +/**
> + * mmc_read_blocks() - 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
> + * @blk_addr: first block address
> + * @buf: output buffer
> + * @len: size of the buffer
> + *
> + * Read one or more blocks of data from 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.
> + *
> + * Return: 0 in case of success, otherwise -EIO
> + */
> +int mmc_read_blocks(struct mmc_card *card, struct mmc_host *host,
> + unsigned int blksz, unsigned int blocks,
> + unsigned int blk_addr, void *buf, unsigned int len)
> +{
> + struct mmc_request mrq = {};
> + struct mmc_command sbc = {};
> + struct mmc_command cmd = {};
> + struct mmc_command stop = {};
> + struct mmc_data data = {};
> + struct scatterlist sg;
> +
> + if (blocks > 1) {
> + if (mmc_host_can_cmd23(host) &&
> + (!card || mmc_card_can_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 = blk_addr;
> + data.sg = &sg;
> + data.sg_len = 1;
> + if (card)
> + mmc_set_data_timeout(&data, card);
> + else
> + data.timeout_ns = 1000000000;
> +
> + sg_init_one(&sg, buf, len);
> +
> + mmc_wait_for_req(host, &mrq);
> +
> + if (sbc.error || cmd.error || data.error)
> + return -EIO;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mmc_read_blocks);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 68f09a955a902..72196817a6f0f 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -743,5 +743,8 @@ 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_blocks(struct mmc_card *card, struct mmc_host *host,
> + unsigned int blksz, unsigned int blocks,
> + unsigned int blk_addr, void *buf, unsigned int len);
I really think we must avoid exporting such a generic function. This
becomes visible outside the mmc subsystem and I am worried that it
will be abused.
Can we perhaps make it harder to integrate with the tuning support on
the core, somehow? I haven't thought much about it, but maybe you can
propose something along those lines - otherwise I will try to think of
another way to do it.
>
> #endif /* LINUX_MMC_HOST_H */
Kind regards
Uffe
Powered by blists - more mailing lists