Re: [PATCH v3 1/3] mmc: dw_mmc: Check busy state in dw_mci_request()

From: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
To: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Cc: Jaehoon Chung
	<jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	"linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Douglas Anderson
	<dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Subject: Re: [PATCH v3 1/3] mmc: dw_mmc: Check busy state in dw_mci_request()
Date: Tue, 19 Mar 2019 15:41:11 +0100	[thread overview]
Message-ID: <CAPDyKFqMjEbM1L5=AjDv46xZt5KxOk8GLMGbWsdq2Xcxoi3gTg@mail.gmail.com> (raw)
In-Reply-To: <1552986778-33904-2-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

On Tue, 19 Mar 2019 at 10:14, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>
> Move it from dw_mci_start_command() to dw_mci_request().
> Then dw_mci_wait_while_busy() isn't called with host's
> lock hold.

So, I decided to have a closer look at this.

>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Tested-by: Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/mmc/host/dw_mmc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 80dc2fd..703dedf 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -426,7 +426,6 @@ static void dw_mci_start_command(struct dw_mci *host,
>
>         mci_writel(host, CMDARG, cmd->arg);
>         wmb(); /* drain writebuffer */
> -       dw_mci_wait_while_busy(host, cmd_flags);
>
>         mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);
>
> @@ -1419,6 +1418,10 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>                 return;
>         }
>
> +       if ((mrq->cmd->opcode != MMC_SEND_STATUS && mrq->cmd->data) &&
> +           !(mrq->cmd->opcode == SD_SWITCH_VOLTAGE))
> +               dw_mci_wait_while_busy(host, SDMMC_CMD_PRV_DAT_WAIT);
> +

This looks weird, because according to the change-log it sounds like
you are moving things around to just avoid having the "lock" held.

To me, there seems to be more changes because of the new checks for
the "opcode" above, no?

 Moreover, dw_mci_wait_while_busy() is also called from
mci_send_cmd(), which means it may still be called with the "lock" is
held. That is really confusing and needs more explanation.

>         spin_lock_bh(&host->lock);
>
>         dw_mci_queue_request(host, slot, mrq);
> --
> 1.9.1
>
>
>

Finally, I am not sure I understand why dw_mci_wait_while_busy() is
called before starting a command, at all. That seems wrong to me.
Instead, shouldn't we look at cmd->flags & MMC_RSP_BUSY, for the
request in question and don't call mmc_request_done() for it, before
the card have stop signaled busy. At least that the behavior the mmc
core expects by the driver.

Kind regards
Uffe