Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230606065918.460866-1-martin@geanix.com>
In-Reply-To: <20230606065918.460866-1-martin@geanix.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Tue, 6 Jun 2023 21:22:18 +0200
Message-ID: <CAFBinCB4sv4Fk_VNEPFaPz384XF9CfXGemSqaH9YJnEh7QNnPg@mail.gmail.com>
Subject: Re: [PATCH] mmc: meson: move mmc_request_done() call to irq thread
To:     =?UTF-8?Q?Martin_Hundeb=C3=B8ll?= <martin@geanix.com>
Cc:     Ulf Hansson <ulf.hansson@linaro.org>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        =?UTF-8?Q?Martin_Hundeb=C3=B8ll?= <mhu@rtx.dk>,
        stable@vger.kernel.org, linux-mmc@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Martin,

On Tue, Jun 6, 2023 at 8:59=E2=80=AFAM Martin Hundeb=C3=B8ll <martin@geanix=
.com> wrote:
[...]
> @@ -1040,6 +1034,13 @@ static irqreturn_t meson_mmc_irq_thread(int irq, v=
oid *dev_id)
>         if (WARN_ON(!cmd))
>                 return IRQ_NONE;
>
> +       if (!meson_mmc_bounce_buf_read(cmd->data) &&
> +           !meson_mmc_get_next_command(cmd)) {
> +               meson_mmc_request_done(host->mmc, cmd->mrq);
> +
> +               return IRQ_HANDLED;
> +       }
> +
I'm wondering if this is changing the behavior when cmd->error is non-zero.

Let's take the "status & IRQ_CRC_ERR" case from meson_mmc_irq() - previousl=
y:
- cmd->error is set
- regmap_update_bits(host->regmap, SD_EMMC_START, START_DESC_BUSY, 0);
- IRQ_WAKE_THREAD
- then inside meson_mmc_irq_thread():
-- meson_mmc_wait_desc_stop(host);
-- meson_mmc_request_done(host->mmc, cmd->mrq);

With this patch:
- cmd->error is set
- regmap_update_bits(host->regmap, SD_EMMC_START, START_DESC_BUSY, 0);
- IRQ_WAKE_THREAD
- then inside meson_mmc_irq_thread():
-- !meson_mmc_bounce_buf_read(cmd->data) &&
!meson_mmc_get_next_command(cmd) can be true, which means we only
call:
--- meson_mmc_request_done(host->mmc, cmd->mrq);
--- (note the missing meson_mmc_wait_desc_stop() call before
meson_mmc_request_done())

What do you think?
Maybe we just need to remove the code from meson_mmc_irq() and just
omit any modifications to meson_mmc_irq_thread() (as it seems all
checks that you're adding are already present in that function below
the cmd->error case).


Best regards,
Martin