Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210824170743.710957-1-sashal@kernel.org> <20210824170743.710957-26-sashal@kernel.org>
In-Reply-To: <20210824170743.710957-26-sashal@kernel.org>
From:   Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
Date:   Thu, 26 Aug 2021 20:59:53 +0900
Message-ID: <CABMQnVJrxqB8koLO9-mBCZgRyQydU7x7B8aHgRPjpxw92hBWjQ@mail.gmail.com>
Subject: Re: [PATCH 4.4 25/31] mmc: dw_mmc: Wait for data transfer after
 response errors.
To:     Sasha Levin <sashal@kernel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        stable@vger.kernel.org, Doug Anderson <dianders@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Alim Akhtar <alim.akhtar@gmail.com>,
        Jaehoon Chung <jh80.chung@samsung.com>,
        Ulf Hansson <ulf.hansson@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,


2021=E5=B9=B48=E6=9C=8825=E6=97=A5(=E6=B0=B4) 2:39 Sasha Levin <sashal@kern=
el.org>:
>
> From: Doug Anderson <dianders@chromium.org>
>
> [ Upstream commit 46d179525a1f6d16957dcb4624517bc04142b3e7 ]
>
> According to the DesignWare state machine description, after we get a
> "response error" or "response CRC error" we move into data transfer
> mode. That means that we don't necessarily need to special case
> trying to deal with the failure right away. We can wait until we are
> notified that the data transfer is complete (with or without errors)
> and then we can deal with the failure.
>
> It may sound strange to defer dealing with a command that we know will
> fail anyway, but this appears to fix a bug. During tuning (CMD19) on
> a specific card on an rk3288-based system, we found that we could get
> a "response CRC error". Sending the stop command after the "response
> CRC error" would then throw the system into a confused state causing
> all future tuning phases to report failure.
>
> When in the confused state, the controller would show these (hex codes
> are interrupt status register):
>  CMD ERR: 0x00000046 (cmd=3D19)
>  CMD ERR: 0x0000004e (cmd=3D12)
>  DATA ERR: 0x00000208
>  DATA ERR: 0x0000020c
>  CMD ERR: 0x00000104 (cmd=3D19)
>  CMD ERR: 0x00000104 (cmd=3D12)
>  DATA ERR: 0x00000208
>  DATA ERR: 0x0000020c
>  ...
>  ...
>
> It is inherently difficult to deal with the complexity of trying to
> correctly send a stop command while a data transfer is taking place
> since you need to deal with different corner cases caused by the fact
> that the data transfer could complete (with errors or without errors)
> during various places in sending the stop command (dw_mci_stop_dma,
> send_stop_abort, etc)
>
> Instead of adding a bunch of extra complexity to deal with this, it
> seems much simpler to just use the more straightforward (and less
> error-prone) path of letting the data transfer finish. There
> shouldn't be any huge benefit to sending the stop command slightly
> earlier, anyway.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Cc: Alim Akhtar <alim.akhtar@gmail.com>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>

This commit also requires the following modifications:
  ba2d139b02ba68: mmc: dw_mmc: Fix occasional hang after tuning on eMMC

Please apply this commit too.

Best regards,
  Nobuhiro


> ---
>  drivers/mmc/host/dw_mmc.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 581f5d0271f4..afdf539e06e9 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1744,6 +1744,33 @@ static void dw_mci_tasklet_func(unsigned long priv=
)
>                         }
>
>                         if (cmd->data && err) {
> +                               /*
> +                                * During UHS tuning sequence, sending th=
e stop
> +                                * command after the response CRC error w=
ould
> +                                * throw the system into a confused state
> +                                * causing all future tuning phases to re=
port
> +                                * failure.
> +                                *
> +                                * In such case controller will move into=
 a data
> +                                * transfer state after a response error =
or
> +                                * response CRC error. Let's let that fin=
ish
> +                                * before trying to send a stop, so we'll=
 go to
> +                                * STATE_SENDING_DATA.
> +                                *
> +                                * Although letting the data transfer tak=
e place
> +                                * will waste a bit of time (we already k=
now
> +                                * the command was bad), it can't cause a=
ny
> +                                * errors since it's possible it would ha=
ve
> +                                * taken place anyway if this tasklet got
> +                                * delayed. Allowing the transfer to take=
 place
> +                                * avoids races and keeps things simple.
> +                                */
> +                               if ((err !=3D -ETIMEDOUT) &&
> +                                   (cmd->opcode =3D=3D MMC_SEND_TUNING_B=
LOCK)) {
> +                                       state =3D STATE_SENDING_DATA;
> +                                       continue;
> +                               }
> +
>                                 dw_mci_stop_dma(host);
>                                 send_stop_abort(host, data);
>                                 state =3D STATE_SENDING_STOP;
> --
> 2.30.2
>


--=20
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org}
   GPG ID: 40AD1FA6