RFT: mmc: block: restore NULL check when retrying

Message ID 20170130132625.18296-1-linus.walleij@linaro.org
State New
Headers show

Commit Message

Linus Walleij Jan. 30, 2017, 1:26 p.m.
When retrying to send a request, we used to have a NULL
check on the request we try to resend.

It was removed in commit
c659b878689c ("mmc: block: inline command abortions")
but maybe that was not such a good idea. Maybe it can
actually be NULL?

This might be causeing regressions in errorpath behaviour
seen in kernelci boot tests on the sunxi.

I'm not sure because this state machine has so many
entrances and exits that I can't wrap my head around it.

Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/mmc/core/block.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ulf Hansson Jan. 31, 2017, 11:28 a.m. | #1
On 30 January 2017 at 14:26, Linus Walleij <linus.walleij@linaro.org> wrote:
> When retrying to send a request, we used to have a NULL

> check on the request we try to resend.

>

> It was removed in commit

> c659b878689c ("mmc: block: inline command abortions")

> but maybe that was not such a good idea. Maybe it can

> actually be NULL?

>

> This might be causeing regressions in errorpath behaviour

> seen in kernelci boot tests on the sunxi.

>

> I'm not sure because this state machine has so many

> entrances and exits that I can't wrap my head around it.

>

> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>

> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


I have applied this for next. Let's see what kernelci reports from the
boot test of the sunxi boards.

If it turns out to be a good fix, I am going to fold this change into
"mmc: block: inline command abortions", but re-writing the changelog
of that a bit.

Please tell me if you don't think this is a good idea.

Kind regards
Uffe

> ---

>  drivers/mmc/core/block.c | 3 +++

>  1 file changed, 3 insertions(+)

>

> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c

> index ede759dda395..6c8be1a80551 100644

> --- a/drivers/mmc/core/block.c

> +++ b/drivers/mmc/core/block.c

> @@ -1606,6 +1606,9 @@ static void mmc_blk_rw_cmd_abort(struct mmc_card *card, struct request *req)

>  static void mmc_blk_rw_start_new(struct mmc_queue *mq, struct mmc_card *card,

>                                  struct request *req)

>  {

> +       if (!req)

> +               return;

> +

>         if (mmc_card_removed(card)) {

>                 req->rq_flags |= RQF_QUIET;

>                 blk_end_request_all(req, -EIO);

> --

> 2.9.3

>

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Feb. 1, 2017, 12:11 p.m. | #2
On 31 January 2017 at 12:28, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 30 January 2017 at 14:26, Linus Walleij <linus.walleij@linaro.org> wrote:

>> When retrying to send a request, we used to have a NULL

>> check on the request we try to resend.

>>

>> It was removed in commit

>> c659b878689c ("mmc: block: inline command abortions")

>> but maybe that was not such a good idea. Maybe it can

>> actually be NULL?

>>

>> This might be causeing regressions in errorpath behaviour

>> seen in kernelci boot tests on the sunxi.

>>

>> I'm not sure because this state machine has so many

>> entrances and exits that I can't wrap my head around it.

>>

>> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>

>> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>

>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>

> I have applied this for next. Let's see what kernelci reports from the

> boot test of the sunxi boards.


Seems like this fixed the problem. Kernelci no longer reports issues
for the sunxi boards [1].

Regarding the existing sunxi mmc driver error messages, those are
still there [2].

>

> If it turns out to be a good fix, I am going to fold this change into

> "mmc: block: inline command abortions", but re-writing the changelog

> of that a bit.

>

> Please tell me if you don't think this is a good idea.

>


Folding in this change into "mmc: block: inline command abortions"....

Kind regards
Uffe

[1]
https://kernelci.org/boot/all/job/ulfh/kernel/v4.10-rc6-120-g3e05469d6088/

[2]
https://storage.kernelci.org/ulfh/v4.10-rc6-120-g3e05469d6088/arm-multi_v7_defconfig/lab-baylibre-seattle/boot-sun7i-a20-bananapi.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 1, 2017, 12:34 p.m. | #3
On Wed, Feb 1, 2017 at 1:11 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 31 January 2017 at 12:28, Ulf Hansson <ulf.hansson@linaro.org> wrote:


>> I have applied this for next. Let's see what kernelci reports from the

>> boot test of the sunxi boards.

>

> Seems like this fixed the problem. Kernelci no longer reports issues

> for the sunxi boards [1].


Phew! At least my patch churn is not causing *new* regressions...

> Regarding the existing sunxi mmc driver error messages, those are

> still there [2].


That still sucks :(

> Folding in this change into "mmc: block: inline command abortions"....


Thanks. I will resend the rest of my cleanup patches, with some new
on top. You can disregard my previous patch series posted on the
26th for now.

Also feel free to push all of it until after v4.11-rc1! I understand that it's
getting hectic due to the imminent merge window.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index ede759dda395..6c8be1a80551 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1606,6 +1606,9 @@  static void mmc_blk_rw_cmd_abort(struct mmc_card *card, struct request *req)
 static void mmc_blk_rw_start_new(struct mmc_queue *mq, struct mmc_card *card,
 				 struct request *req)
 {
+	if (!req)
+		return;
+
 	if (mmc_card_removed(card)) {
 		req->rq_flags |= RQF_QUIET;
 		blk_end_request_all(req, -EIO);