| 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: <64729494-6a61-99f9-db08-ab9d5f6af4cf@rock-chips.com>
Date: Fri, 7 May 2021 09:42:18 +0800
From: Shawn Lin <shawn.lin@...k-chips.com>
To: Ulf Hansson <ulf.hansson@...aro.org>, linux-mmc@...r.kernel.org,
Adrian Hunter <adrian.hunter@...el.com>
Cc: shawn.lin@...k-chips.com, Linus Walleij <linus.walleij@...aro.org>,
Wolfram Sang <wsa+renesas@...g-engineering.com>,
Avri Altman <avri.altman@....com>,
Masami Hiramatsu <masami.hiramatsu@...aro.org>,
linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/11] mmc: core: Drop open coding when preparing commands
with busy signaling
On 2021/5/5 0:12, Ulf Hansson wrote:
> Similar code for validating the host->max_busy_timeout towards the current
> command's busy timeout, exists in mmc_do_erase(), mmc_sleep() and
> __mmc_switch(). Let's move the common code into a helper function.
Looks nice.
Reviewed-by: Shawn Lin <shawn.lin@...k-chips.com>
>
> Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
> ---
> drivers/mmc/core/core.c | 20 ++----------------
> drivers/mmc/core/mmc.c | 20 +++---------------
> drivers/mmc/core/mmc_ops.c | 42 +++++++++++++++++++++-----------------
> drivers/mmc/core/mmc_ops.h | 3 +++
> 4 files changed, 31 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index f194940c5974..b00c84ea8441 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1582,7 +1582,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
> {
> struct mmc_command cmd = {};
> unsigned int qty = 0, busy_timeout = 0;
> - bool use_r1b_resp = false;
> + bool use_r1b_resp;
> int err;
>
> mmc_retune_hold(card->host);
> @@ -1650,23 +1650,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
> cmd.opcode = MMC_ERASE;
> cmd.arg = arg;
> busy_timeout = mmc_erase_timeout(card, arg, qty);
> - /*
> - * If the host controller supports busy signalling and the timeout for
> - * the erase operation does not exceed the max_busy_timeout, we should
> - * use R1B response. Or we need to prevent the host from doing hw busy
> - * detection, which is done by converting to a R1 response instead.
> - * Note, some hosts requires R1B, which also means they are on their own
> - * when it comes to deal with the busy timeout.
> - */
> - if (!(card->host->caps & MMC_CAP_NEED_RSP_BUSY) &&
> - card->host->max_busy_timeout &&
> - busy_timeout > card->host->max_busy_timeout) {
> - cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> - } else {
> - cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> - cmd.busy_timeout = busy_timeout;
> - use_r1b_resp = true;
> - }
> + use_r1b_resp = mmc_prepare_busy_cmd(card->host, &cmd, busy_timeout);
>
> err = mmc_wait_for_cmd(card->host, &cmd, 0);
> if (err) {
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 8674c3e0c02c..63a7bd0b239c 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1910,6 +1910,7 @@ static int mmc_sleep(struct mmc_host *host)
> struct mmc_command cmd = {};
> struct mmc_card *card = host->card;
> unsigned int timeout_ms = DIV_ROUND_UP(card->ext_csd.sa_timeout, 10000);
> + bool use_r1b_resp;
> int err;
>
> /* Re-tuning can't be done once the card is deselected */
> @@ -1922,22 +1923,7 @@ static int mmc_sleep(struct mmc_host *host)
> cmd.opcode = MMC_SLEEP_AWAKE;
> cmd.arg = card->rca << 16;
> cmd.arg |= 1 << 15;
> -
> - /*
> - * If the max_busy_timeout of the host is specified, validate it against
> - * the sleep cmd timeout. A failure means we need to prevent the host
> - * from doing hw busy detection, which is done by converting to a R1
> - * response instead of a R1B. Note, some hosts requires R1B, which also
> - * means they are on their own when it comes to deal with the busy
> - * timeout.
> - */
> - if (!(host->caps & MMC_CAP_NEED_RSP_BUSY) && host->max_busy_timeout &&
> - (timeout_ms > host->max_busy_timeout)) {
> - cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> - } else {
> - cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
> - cmd.busy_timeout = timeout_ms;
> - }
> + use_r1b_resp = mmc_prepare_busy_cmd(host, &cmd, timeout_ms);
>
> err = mmc_wait_for_cmd(host, &cmd, 0);
> if (err)
> @@ -1949,7 +1935,7 @@ static int mmc_sleep(struct mmc_host *host)
> * SEND_STATUS command to poll the status because that command (and most
> * others) is invalid while the card sleeps.
> */
> - if (!cmd.busy_timeout || !(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
> + if (!use_r1b_resp || !(host->caps & MMC_CAP_WAIT_WHILE_BUSY))
> mmc_delay(timeout_ms);
>
> out_release:
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 5756781fef37..025a4134d5c7 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -521,6 +521,27 @@ int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
> return __mmc_poll_for_busy(card, timeout_ms, true, false, busy_cmd);
> }
>
> +bool mmc_prepare_busy_cmd(struct mmc_host *host, struct mmc_command *cmd,
> + unsigned int timeout_ms)
> +{
> + /*
> + * If the max_busy_timeout of the host is specified, make sure it's
> + * enough to fit the used timeout_ms. In case it's not, let's instruct
> + * the host to avoid HW busy detection, by converting to a R1 response
> + * instead of a R1B. Note, some hosts requires R1B, which also means
> + * they are on their own when it comes to deal with the busy timeout.
> + */
> + if (!(host->caps & MMC_CAP_NEED_RSP_BUSY) && host->max_busy_timeout &&
> + (timeout_ms > host->max_busy_timeout)) {
> + cmd->flags = MMC_CMD_AC | MMC_RSP_SPI_R1 | MMC_RSP_R1;
> + return false;
> + }
> +
> + cmd->flags = MMC_CMD_AC | MMC_RSP_SPI_R1B | MMC_RSP_R1B;
> + cmd->busy_timeout = timeout_ms;
> + return true;
> +}
> +
> /**
> * __mmc_switch - modify EXT_CSD register
> * @card: the MMC card associated with the data transfer
> @@ -543,7 +564,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
> struct mmc_host *host = card->host;
> int err;
> struct mmc_command cmd = {};
> - bool use_r1b_resp = true;
> + bool use_r1b_resp;
> unsigned char old_timing = host->ios.timing;
>
> mmc_retune_hold(host);
> @@ -554,29 +575,12 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
> timeout_ms = card->ext_csd.generic_cmd6_time;
> }
>
> - /*
> - * If the max_busy_timeout of the host is specified, make sure it's
> - * enough to fit the used timeout_ms. In case it's not, let's instruct
> - * the host to avoid HW busy detection, by converting to a R1 response
> - * instead of a R1B. Note, some hosts requires R1B, which also means
> - * they are on their own when it comes to deal with the busy timeout.
> - */
> - if (!(host->caps & MMC_CAP_NEED_RSP_BUSY) && host->max_busy_timeout &&
> - (timeout_ms > host->max_busy_timeout))
> - use_r1b_resp = false;
> -
> cmd.opcode = MMC_SWITCH;
> cmd.arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
> (index << 16) |
> (value << 8) |
> set;
> - cmd.flags = MMC_CMD_AC;
> - if (use_r1b_resp) {
> - cmd.flags |= MMC_RSP_SPI_R1B | MMC_RSP_R1B;
> - cmd.busy_timeout = timeout_ms;
> - } else {
> - cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
> - }
> + use_r1b_resp = mmc_prepare_busy_cmd(host, &cmd, timeout_ms);
>
> err = mmc_wait_for_cmd(host, &cmd, retries);
> if (err)
> diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
> index 7bc1cfb0654c..ba898c435658 100644
> --- a/drivers/mmc/core/mmc_ops.h
> +++ b/drivers/mmc/core/mmc_ops.h
> @@ -18,6 +18,7 @@ enum mmc_busy_cmd {
>
> struct mmc_host;
> struct mmc_card;
> +struct mmc_command;
>
> int mmc_select_card(struct mmc_card *card);
> int mmc_deselect_cards(struct mmc_host *host);
> @@ -35,6 +36,8 @@ int mmc_bus_test(struct mmc_card *card, u8 bus_width);
> int mmc_can_ext_csd(struct mmc_card *card);
> int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
> int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
> +bool mmc_prepare_busy_cmd(struct mmc_host *host, struct mmc_command *cmd,
> + unsigned int timeout_ms);
> int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
> enum mmc_busy_cmd busy_cmd);
> int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>
Powered by blists - more mailing lists