| 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: <047c746f-0787-37ec-7989-cd932f288564@intel.com>
Date: Thu, 20 Jul 2023 11:06:57 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Victor Shih <victorshihgli@...il.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 V8 06/23] mmc: core: Support UHS-II card control and
access
On 19/07/23 13:24, Victor Shih wrote:
> On Mon, Jul 10, 2023 at 9:24 PM Adrian Hunter <adrian.hunter@...el.com> wrote:
>>
>> On 21/06/23 13:01, Victor Shih wrote:
>>> From: Victor Shih <victor.shih@...esyslogic.com.tw>
>>>
>>> Embed UHS-II access/control functionality into the MMC request
>>> processing flow.
>>>
>>> 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.
>>>
>>> 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>
>>> ---
>>> 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 2e9d3760c202..013ab071db9b 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));
>>>
>>> @@ -1612,6 +1606,9 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>>> 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) {
>>> /* 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;
>>> }
>>> brq->cmd.opcode = rq_data_dir(req) == READ ? readcmd : writecmd;
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 7b8227e7ed26..04783f37da92 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);
>>
>> __mmc_start_request() is also called by mmc_wait_for_req_done()
>> but uhs2_cmd is local to mmc_start_request(), so if mmc_wait_for_req_done()
>> is ever called in UHS2 case with cmd->retries, it looks like
>> host controller might try to access uhs2_cmd which is no
>> longer valid?
>>
>>
>
> Hi, Adrian
>
> I traced the code between the SD card initialization process and
> found that
> __mmc_start_request() is only called by mmc_start_request in the
> UHS2 case.
> I'm not sure if there's anything I'm missing, could you help me
> with more comments?
Perhaps mmc_send_status() used in block.c ?
> Or may you have any ideas on how to avoid this situation?
>
Need to get Ulf's view, but putting "struct uhs2_command uhs2_cmd"
and "__be32 payload[4]" in struct mmc_command or struct mmc_request
would seem to be options. If size is a concern, some optimization
to reduce the size of struct uhs2_command looks possible, and is
max payload len actually only 2 at the moment.
Powered by blists - more mailing lists