linux-kernel - Re: [PATCH V9 06/23] mmc: core: Support UHS-II card control and access

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPDyKFp=m98EW6+U-1yQaNxpnCqK3eKX8rvkLKYBLZ67EB=h4w@mail.gmail.com>
Date:   Thu, 10 Aug 2023 12:06:39 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Victor Shih <victorshihgli@...il.com>
Cc:     adrian.hunter@...el.com, linux-mmc@...r.kernel.org,
        linux-kernel@...r.kernel.org, benchuanggli@...il.com,
        HL.Liu@...esyslogic.com.tw, Greg.tu@...esyslogic.com.tw,
        takahiro.akashi@...aro.org, dlunev@...omium.org,
        Jason Lai <jason.lai@...esyslogic.com.tw>,
        Victor Shih <victor.shih@...esyslogic.com.tw>
Subject: Re: [PATCH V9 06/23] mmc: core: Support UHS-II card control and access

On Wed, 9 Aug 2023 at 17:09, Victor Shih <victorshihgli@...il.com> wrote:
>
> On Tue, Aug 8, 2023 at 9:48 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
> >
> > On Fri, 21 Jul 2023 at 12:14, Victor Shih <victorshihgli@...il.com> wrote:
> > >
> > > From: Victor Shih <victor.shih@...esyslogic.com.tw>
> > >
> > > Embed UHS-II access/control functionality into the MMC request
> > > processing flow.
> >
> > This deserves to be extended a bit. There is quite some code being
> > added in the $subject patch.
> >
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
> > > Signed-off-by: Jason Lai <jason.lai@...esyslogic.com.tw>
> > > Signed-off-by: Victor Shih <victor.shih@...esyslogic.com.tw>
> > > ---
> > >
> > > Updates in V8:
> > >  - Add MMC_UHS2_SUPPORT to be cleared in sd_uhs2_detect().
> > >  - Modify return value in sd_uhs2_attach().
> > >
> > > Updates in V7:
> > >  - Add mmc_uhs2_card_prepare_cmd helper function in sd_ops.h.
> > >  - Drop uhs2_state in favor of ios->timing.
> > >  - Remove unnecessary functions.
> > >
> > > ---
> > >
> > >  drivers/mmc/core/block.c   |   18 +-
> > >  drivers/mmc/core/core.c    |    8 +
> > >  drivers/mmc/core/mmc_ops.c |   25 +-
> > >  drivers/mmc/core/mmc_ops.h |    1 +
> > >  drivers/mmc/core/sd.c      |   13 +-
> > >  drivers/mmc/core/sd.h      |    4 +
> > >  drivers/mmc/core/sd_ops.c  |   11 +
> > >  drivers/mmc/core/sd_ops.h  |   18 +
> > >  drivers/mmc/core/sd_uhs2.c | 1137 +++++++++++++++++++++++++++++++++++-
> > >  9 files changed, 1176 insertions(+), 59 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > > index f701efb1fa78..6617ae9fc840 100644
> > > --- a/drivers/mmc/core/block.c
> > > +++ b/drivers/mmc/core/block.c
> > > @@ -918,15 +918,9 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks)
> > >
> > >         struct scatterlist sg;
> > >
> > > -       cmd.opcode = MMC_APP_CMD;
> > > -       cmd.arg = card->rca << 16;
> > > -       cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> > > -
> > > -       err = mmc_wait_for_cmd(card->host, &cmd, 0);
> > > -       if (err)
> > > -               return err;
> > > -       if (!mmc_host_is_spi(card->host) && !(cmd.resp[0] & R1_APP_CMD))
> > > -               return -EIO;
> > > +       err = mmc_app_cmd(card->host, card);
> > > +               if (err)
> > > +                       return err;
> > >
> > >         memset(&cmd, 0, sizeof(struct mmc_command));
> >
> > The entire chunk of change above deserves its own separate
> > cleanup-patch. If you want to send it separately I can apply
> > immediately - or if you decide to make it part of the series then it
> > should precede the $subject patch.
> >
> > Note that, after the cleanup above, the call to memset() can be dropped too.
> >
>
> Hi, Ulf
>
>      Which patch do you think would be the best if I decided to make
> it part of the series?

Probably easier to send it separate, before a new respin of the
series. I can apply it immediately and you rebase your series on top.

If there is anything similar part of the series that also can be
considered as a cleanup, feel free to send that separate too, to get
that applied first.

Kind regards
Uffe

>
> Thanks, Victor Shih
>
> > >
> > > @@ -1612,6 +1606,9 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
> >
> > I commented on the changes in mmc_blk_rw_rq_prep() already in version
> > 6 [1] - but it seems like you haven't addressed my comments yet.
> >
> > I have therefore copied the similar comment again, see below.
> >
> > >         struct request *req = mmc_queue_req_to_req(mqrq);
> > >         struct mmc_blk_data *md = mq->blkdata;
> > >         bool do_rel_wr, do_data_tag;
> > > +       bool do_multi;
> > > +
> > > +       do_multi = (card->host->flags & MMC_UHS2_SD_TRAN) ? true : false;
> > >
> > >         mmc_blk_data_prep(mq, mqrq, recovery_mode, &do_rel_wr, &do_data_tag);
> > >
> > > @@ -1622,7 +1619,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
> > >                 brq->cmd.arg <<= 9;
> > >         brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> > >
> > > -       if (brq->data.blocks > 1 || do_rel_wr) {
> > > +       if (brq->data.blocks > 1 || do_rel_wr || do_multi) {
> >
> > This looks wrong to me. UHS2 can use single block read/writes too. Right?
> >
> > >                 /* SPI multiblock writes terminate using a special
> > >                  * token, not a STOP_TRANSMISSION request.
> > >                  */
> > > @@ -1635,6 +1632,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
> > >                 brq->mrq.stop = NULL;
> > >                 readcmd = MMC_READ_SINGLE_BLOCK;
> > >                 writecmd = MMC_WRITE_BLOCK;
> > > +               brq->cmd.uhs2_tmode0_flag = 1;
> > >         }
> >
> > As "do_multi" is always set for UHS2, setting this flag here seems to
> > be wrong/redundant.
> >
> > Anyway, if I understand correctly, the flag is intended to be used to
> > inform the host driver whether the so-called 2L_HD_mode (half-duplex
> > or full-duplex) should be used for the I/O request or not. Did I
> > understand this correctly?
> >
> > To fix the above behaviour, I suggest we try to move the entire
> > control of the flag into mmc_uhs2_prepare_cmd(). It seems like we need
> > the flag to be set for multi block read/writes (CMD18 and CMD25), but
> > only if the host and card supports the 2L_HD_mode too, right?
> >
> > According to my earlier suggestions, we should also be able to check
> > the 2L_HD_mode via the bits we have set in the ios->timing, no?
> >
> > Moreover, by making mmc_uhs2_prepare_cmd() responsible for setting the
> > flag, we can move the definition of the flag into the struct
> > uhs2_command instead. While at it, I suggest we also rename the flag
> > into "tmode_half_duplex", to better describe its purpose. Note that,
> > this also means the interpretation of the flag becomes inverted.
> >
> > >         brq->cmd.opcode = rq_data_dir(req) == READ ? readcmd : writecmd;
> > >
> >
> > Until we have agreed on how to move forward with the above, I am
> > temporarily pausing further review.
> >
> > [...]
> >
> > Kind regards
> > Uffe
> >
> > [1]
> > https://lore.kernel.org/linux-mmc/CAPDyKFoV3Ch-xzXxiT2RnDeLvsO454Pwq1vQL_bdNLptM+amAg@mail.gmail.com/