| 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
| ||
|
Date: Wed, 22 May 2024 18:46:38 +0800
From: Victor Shih <victorshihgli@...il.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: ulf.hansson@...aro.org, 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 V14 07/21] mmc: core: Support UHS-II card control and access
On Wed, May 22, 2024 at 5:11 PM Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> On 22/05/24 12:04, Victor Shih wrote:
> > On Tue, Jan 30, 2024 at 5:36 PM Adrian Hunter <adrian.hunter@...el.com> wrote:
> >>
> >> On 27/01/24 10:28, Victor Shih wrote:
> >>> On Tue, Jan 23, 2024 at 2:28 PM 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.
> >>>>
> >>>> 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 V13:
> >>>> - Separate __mmc_go_idle() into one patch for re-factorring the code.
> >>>> - Move mmc_decode_scr declaration to sd.h.
> >>>> - Ues uhs2_sd_tran to stead MMC_UHS2_SD_TRAN.
> >>>> - Drop unnecessary comment.
> >>>>
> >>>> Updates in V12:
> >>>> - Use mmc_op_multi() to check DCMD which supports multi read/write
> >>>> in mmc_uhs2_prepare_cmd().
> >>>>
> >>>> Updates in V10:
> >>>> - Move some definitions of PatchV9[02/23] to PatchV10[06/23].
> >>>> - Move some definitions of PatchV9[05/23] to PatchV10[06/23].
> >>>> - Drop do_multi in the mmc_blk_rw_rq_prep().
> >>>> - Use tmode_half_duplex to instead of uhs2_tmode0_flag.
> >>>> - Move entire control of the tmode into mmc_uhs2_prepare_cmd().
> >>>>
> >>>> 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/core.c | 10 +-
> >>>> drivers/mmc/core/sd.c | 10 +-
> >>>> drivers/mmc/core/sd.h | 5 +
> >>>> drivers/mmc/core/sd_ops.c | 9 +
> >>>> drivers/mmc/core/sd_ops.h | 17 +
> >>>> drivers/mmc/core/sd_uhs2.c | 1115 ++++++++++++++++++++++++++++++++++--
> >>>> include/linux/mmc/core.h | 13 +
> >>>> include/linux/mmc/host.h | 15 +
> >>>> 8 files changed, 1155 insertions(+), 39 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >>>> index 2edf31492a5d..be77cebe1fb8 100644
> >>>> --- a/drivers/mmc/core/core.c
> >>>> +++ b/drivers/mmc/core/core.c
> >>>> @@ -334,6 +334,8 @@ static int mmc_mrq_prep(struct mmc_host *host, struct mmc_request *mrq)
> >>>>
> >>>> int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> >>>> {
> >>>> + struct uhs2_command uhs2_cmd;
> >>>> + __be32 payload[4]; /* for maximum size */
> >>>> int err;
> >>>>
> >>>> init_completion(&mrq->cmd_completion);
> >>>> @@ -351,6 +353,8 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> >>>> if (err)
> >>>> return err;
> >>>>
> >>>> + mmc_uhs2_card_prepare_cmd(host, mrq, uhs2_cmd, payload);
> >>>> +
> >>>> led_trigger_event(host->led, LED_FULL);
> >>>> __mmc_start_request(host, mrq);
> >>>>
> >>>
> >>> Hi, Adrian
> >>>
> >>> I referenced your comments of the V9:
> >>>
> >>> Refer:
> >>> https://patchwork.kernel.org/project/linux-mmc/patch/20230721101349.12387-7-victorshihgli@gmail.com/
> >>>
> >>> My understanding is as follows, please correct me if there are any mistakes.
> >>> There is already "struct uhs2_command *uhs2_cmd" in struct mmc_command.
> >>> If I also put "__be32 payload[4]" in struct mmc_command.
> >>> The code will become:
> >>> mmc_uhs2_card_prepare_cmd(host, mrq, *mrq->cmd->uhs2_cmd, mrq->cmd->payload);
> >>> But a null pointer problem occurs when sending commands like COM0(mmc_go_idle).
> >>> In this case I just can only plan for the time being as follows:
> >>>
> >>> if (mrq->cmd->uhs2_cmd)
> >>> mmc_uhs2_card_prepare_cmd(host, mrq, *mrq->cmd->uhs2_cmd,
> >>> mrq->cmd->payload);
> >>> else
> >>> mmc_uhs2_card_prepare_cmd(host, mrq, uhs2_cmd, payload);
> >>>
> >>> Would you give me any other advice?
> >>
> >> struct uhs2_command uhs2_cmd should not be on the stack local to
> >> mmc_start_request(). Probably moving it to struct mmc_request
> >> is as good as any other option. So starting like below and
> >> then with whatever other changes are needed to make it work.
> >>
> >>
> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >> index be77cebe1fb8..68496c51a521 100644
> >> --- a/drivers/mmc/core/core.c
> >> +++ b/drivers/mmc/core/core.c
> >> @@ -334,8 +334,6 @@ static int mmc_mrq_prep(struct mmc_host *host, struct mmc_request *mrq)
> >>
> >> int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> >> {
> >> - struct uhs2_command uhs2_cmd;
> >> - __be32 payload[4]; /* for maximum size */
> >> int err;
> >>
> >> init_completion(&mrq->cmd_completion);
> >> @@ -353,7 +351,7 @@ int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
> >> if (err)
> >> return err;
> >>
> >> - mmc_uhs2_card_prepare_cmd(host, mrq, uhs2_cmd, payload);
> >> + mmc_uhs2_card_prepare_cmd(host, mrq);
> >>
> >> led_trigger_event(host->led, LED_FULL);
> >> __mmc_start_request(host, mrq);
> >> @@ -434,8 +432,6 @@ EXPORT_SYMBOL(mmc_wait_for_req_done);
> >> */
> >> int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
> >> {
> >> - struct uhs2_command uhs2_cmd;
> >> - __be32 payload[4]; /* for maximum size */
> >> int err;
> >>
> >> /*
> >> @@ -456,7 +452,7 @@ int mmc_cqe_start_req(struct mmc_host *host, struct mmc_request *mrq)
> >> if (err)
> >> goto out_err;
> >>
> >> - mmc_uhs2_card_prepare_cmd(host, mrq, uhs2_cmd, payload);
> >> + mmc_uhs2_card_prepare_cmd(host, mrq);
> >>
> >> err = host->cqe_ops->cqe_request(host, mrq);
> >> if (err)
> >> diff --git a/drivers/mmc/core/sd_ops.h b/drivers/mmc/core/sd_ops.h
> >> index d3a3465c7669..e3af68a52de8 100644
> >> --- a/drivers/mmc/core/sd_ops.h
> >> +++ b/drivers/mmc/core/sd_ops.h
> >> @@ -24,14 +24,10 @@ int mmc_app_sd_status(struct mmc_card *card, void *ssr);
> >> int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card);
> >> void mmc_uhs2_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq);
> >>
> >> -static inline void mmc_uhs2_card_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq,
> >> - struct uhs2_command uhs2_cmd, __be32 payload[4])
> >> +static inline void mmc_uhs2_card_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq)
> >> {
> >> - if (host->uhs2_sd_tran) {
> >> - uhs2_cmd.payload = payload;
> >> - mrq->cmd->uhs2_cmd = &uhs2_cmd;
> >> + if (host->uhs2_sd_tran)
> >> mmc_uhs2_prepare_cmd(host, mrq);
> >> - }
> >> }
> >>
> >> static inline int mmc_sd_can_poweroff_notify(struct mmc_card *card)
> >> diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> >> index c46729d85644..9cabb6937dc1 100644
> >> --- a/drivers/mmc/core/sd_uhs2.c
> >> +++ b/drivers/mmc/core/sd_uhs2.c
> >> @@ -1194,6 +1194,7 @@ void mmc_uhs2_prepare_cmd(struct mmc_host *host, struct mmc_request *mrq)
> >> u8 plen;
> >>
> >> cmd = mrq->cmd;
> >> + cmd->uhs2_cmd = &mrq->uhs2_cmd;
> >> header = host->card->uhs2_config.node_id;
> >> if ((cmd->flags & MMC_CMD_MASK) == MMC_CMD_ADTC)
> >> header |= UHS2_PACKET_TYPE_DCMD;
> >> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> >> index f30f6be86f66..83c901794c17 100644
> >> --- a/include/linux/mmc/core.h
> >> +++ b/include/linux/mmc/core.h
> >> @@ -23,13 +23,18 @@ enum mmc_blk_status {
> >> MMC_BLK_NEW_REQUEST,
> >> };
> >>
> >> +#define UHS2_MAX_PAYLOAD_LEN 2
> >> +#define UHS2_MAX_RESP_LEN 20
> >> +
> >> struct uhs2_command {
> >> u16 header;
> >> u16 arg;
> >> - __be32 *payload;
> >> - u32 payload_len;
> >> - u32 packet_len;
> >> + __be32 payload[UHS2_MAX_PAYLOAD_LEN];
> >> + u8 payload_len;
> >> + u8 packet_len; // TODO: is this really needed?
> >> u8 tmode_half_duplex;
> >> + u8 uhs2_resp_len; /* UHS2 native cmd resp len */
> >> + u8 uhs2_resp[UHS2_MAX_RESP_LEN]; /* UHS2 native cmd resp */
> >> };
> >>
> >> struct mmc_command {
> >> @@ -119,8 +124,6 @@ struct mmc_command {
> >> struct mmc_request *mrq; /* associated request */
> >>
> >> struct uhs2_command *uhs2_cmd; /* UHS2 command */
> >> - u8 *uhs2_resp; /* UHS2 native cmd resp */
> >> - u8 uhs2_resp_len; /* UHS2 native cmd resp len */
> >> };
> >>
> >> struct mmc_data {
> >> @@ -179,6 +182,7 @@ struct mmc_request {
> >> const struct bio_crypt_ctx *crypto_ctx;
> >> int crypto_key_slot;
> >> #endif
> >> + struct uhs2_command uhs2_cmd;
> >> };
> >>
> >> struct mmc_card;
> >>
> >>
> >>
> >
> > Hi, Adrian
> >
> > I have modified the above into patch#7 and patch#16 in the patch series v15.
> > But I will submit the patch series v16 later.
> > Please help review the changes in patch series v16.
> >
> > Thanks, Victor Shih
>
> If you make a V16, please separate the Error Recovery mechanism added in V15
> into a separate patch.
>
Hi, Adrian
I will separate this part to patch#8 in v16.
Thanks, Victor Shih
Powered by blists - more mailing lists