| 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 12:11:22 +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 V14 07/21] mmc: core: Support UHS-II card control and
access
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.
Powered by blists - more mailing lists