Message-ID: <9903989.eNJFYEL58v@benoit.monin>
Date: Thu, 10 Jul 2025 15:36:06 +0200
From: Benoît Monin <benoit.monin@...tlin.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
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

Hi Ulf,

Thanks for the review.

On Wednesday, 9 July 2025 at 16:46:45 CEST, Ulf Hansson wrote:
> 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.
> 
Ok, I will do that.

> > +}
> > +
> >  #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.
> 
I agree that the function might be too generic now. Here are some of
the ideas I have to make less appealing for abuse:

* Rename it to mention tuning (mmc_tuning_read?)
* Drop some parameters:
  * blk_addr: Reading from 0 should be all that is needed for tuning
  * other?
* Move its declaration to a header private to drivers/mmc (where?)

Let me know what you think.

> >
> >  #endif /* LINUX_MMC_HOST_H */
> 
> Kind regards
> Uffe
> 

Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists