[PATCH 0/6] mmc: block: command issue cleanups

[PATCH 0/6] mmc: block: command issue cleanups

[Linus Walleij]

The function mmc_blk_issue_rw_rq() is hopelessly convoluted and
need to be refactored to it can be understood by humans.

In the process I found some weird magic return values passed
around for no good reason.

Things are more readable after this.

This work is done towards the goal of breaking the function in
two parts: one just submitting the requests and one checking the
result and possibly resubmitting the command on error, so we
can make the usual path (non-errorpath) smooth and quick, and
be called directly when the driver completes a request.

That in turn is a prerequisite for proper blk-mq integration
with the MMC/SD stack.

All that comes later.

Linus Walleij (6):
  mmc: block: break out mmc_blk_rw_cmd_abort()
  mmc: block: break out mmc_blk_rw_start_new()
  mmc: block: do not assign mq_rq when aborting command
  mmc: block: inline command abortions
  mmc: block: introduce new_areq and old_areq
  mmc: block: stop passing around pointless return values

 drivers/mmc/core/block.c | 108 ++++++++++++++++++++++++++---------------------
 drivers/mmc/core/block.h |   2 +-
 2 files changed, 60 insertions(+), 50 deletions(-)

-- 
2.9.3

[PATCH 1/6] mmc: block: break out mmc_blk_rw_cmd_abort()

[Linus Walleij]

As a first step toward breaking apart the very complex function
mmc_blk_issue_rw_rq() we break out the command abort code.
This code assumes "ret" is != 0 and then repeatedly hammers
blk_end_request() until the request to the block layer to end
the request succeeds.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 7bd03381810d..14efe92a14ef 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1598,6 +1598,17 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
 	return ret;
 }
 
+static void mmc_blk_rw_cmd_abort(struct mmc_card *card, struct request *req)
+{
+	int ret = 1;
+
+	if (mmc_card_removed(card))
+		req->rq_flags |= RQF_QUIET;
+	while (ret)
+		ret = blk_end_request(req, -EIO,
+				      blk_rq_cur_bytes(req));
+}
+
 static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 {
 	struct mmc_blk_data *md = mq->blkdata;
@@ -1737,11 +1748,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 	return 1;
 
  cmd_abort:
-	if (mmc_card_removed(card))
-		req->rq_flags |= RQF_QUIET;
-	while (ret)
-		ret = blk_end_request(req, -EIO,
-				blk_rq_cur_bytes(req));
+	mmc_blk_rw_cmd_abort(card, req);
 
  start_new_req:
 	if (rqc) {
-- 
2.9.3

[PATCH 2/6] mmc: block: break out mmc_blk_rw_start_new()

[Linus Walleij]

As a step toward breaking apart the very complex function
mmc_blk_issue_rw_rq() we break out the code to start a new
request.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 14efe92a14ef..b60d1fb3a07a 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1609,6 +1609,22 @@ static void mmc_blk_rw_cmd_abort(struct mmc_card *card, struct request *req)
 				      blk_rq_cur_bytes(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);
+	} else {
+		mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
+		mmc_start_req(card->host,
+			      &mq->mqrq_cur->mmc_active, NULL);
+	}
+}
+
 static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 {
 	struct mmc_blk_data *md = mq->blkdata;
@@ -1751,16 +1767,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 	mmc_blk_rw_cmd_abort(card, req);
 
  start_new_req:
-	if (rqc) {
-		if (mmc_card_removed(card)) {
-			rqc->rq_flags |= RQF_QUIET;
-			blk_end_request_all(rqc, -EIO);
-		} else {
-			mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
-			mmc_start_req(card->host,
-				      &mq->mqrq_cur->mmc_active, NULL);
-		}
-	}
+	mmc_blk_rw_start_new(mq, card, rqc);
 
 	return 0;
 }
-- 
2.9.3

[PATCH 3/6] mmc: block: do not assign mq_rq when aborting command

[Linus Walleij]

The code in mmc_blk_issue_rq_rq() aborts a command if the request
is not properly aligned on large sectors. As part of the path
jumping out, it assigns the local variable mq_rq reflecting
a MMC queue request to the current MMC queue request, which is
confusing since the variable is not used after this jump.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index b60d1fb3a07a..13e6fe060f26 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1649,7 +1649,6 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 				!IS_ALIGNED(blk_rq_sectors(rqc), 8)) {
 				pr_err("%s: Transfer size is not 4KB sector size aligned\n",
 					rqc->rq_disk->disk_name);
-				mq_rq = mq->mqrq_cur;
 				req = rqc;
 				rqc = NULL;
 				goto cmd_abort;
-- 
2.9.3

[PATCH 4/6] mmc: block: inline command abortions

[Linus Walleij]

Setting rqc to NULL followed by a goto to cmd_abort is just a way
to do unconditional abort without starting any new command.
Inline the calls to mmc_blk_rw_cmd_abort() and return immediately
in those cases.

As a result, mmc_blk_rw_start_new() is not called with NULL
requests, and we can remove the NULL check in the beginning of
this function.

Add some comments to the code flow so it is clear that this is
where the asynchronous requests come back in and the result of
them gets handled.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 13e6fe060f26..4bbb3d16c09b 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1612,9 +1612,6 @@ 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);
@@ -1649,9 +1646,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 				!IS_ALIGNED(blk_rq_sectors(rqc), 8)) {
 				pr_err("%s: Transfer size is not 4KB sector size aligned\n",
 					rqc->rq_disk->disk_name);
-				req = rqc;
-				rqc = NULL;
-				goto cmd_abort;
+				mmc_blk_rw_cmd_abort(card, rqc);
+				return 0;
 			}
 
 			mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
@@ -1660,11 +1656,20 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			areq = NULL;
 		areq = mmc_start_req(card->host, areq, &status);
 		if (!areq) {
+			/*
+			 * We have just put the first request into the pipeline
+			 * and there is nothing more to do until it is
+			 * complete.
+			 */
 			if (status == MMC_BLK_NEW_REQUEST)
 				mq->flags |= MMC_QUEUE_NEW_REQUEST;
 			return 0;
 		}
 
+		/*
+		 * An asynchronous request has been completed and we proceed
+		 * to handle the result of it.
+		 */
 		mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
 		brq = &mq_rq->brq;
 		req = mq_rq->req;
@@ -1691,8 +1696,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 				pr_err("%s BUG rq_tot %d d_xfer %d\n",
 				       __func__, blk_rq_bytes(req),
 				       brq->data.bytes_xfered);
-				rqc = NULL;
-				goto cmd_abort;
+				mmc_blk_rw_cmd_abort(card, req);
+				return 0;
 			}
 			break;
 		case MMC_BLK_CMD_ERR:
-- 
2.9.3

[PATCH 5/6] mmc: block: introduce new_areq and old_areq

[Linus Walleij]

Recycling the same variable in an x=x+1 fashion may seem
clever here but it makes the code terse and hard to follow
for humans. Introduce a new_areq and old_areq variable so
we see what is going on.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 4bbb3d16c09b..f3e0c778cdbd 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1631,7 +1631,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 	enum mmc_blk_status status;
 	struct mmc_queue_req *mq_rq;
 	struct request *req;
-	struct mmc_async_req *areq;
+	struct mmc_async_req *new_areq;
+	struct mmc_async_req *old_areq;
 
 	if (!rqc && !mq->mqrq_prev->req)
 		return 0;
@@ -1651,11 +1652,12 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			}
 
 			mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
-			areq = &mq->mqrq_cur->mmc_active;
+			new_areq = &mq->mqrq_cur->mmc_active;
 		} else
-			areq = NULL;
-		areq = mmc_start_req(card->host, areq, &status);
-		if (!areq) {
+			new_areq = NULL;
+
+		old_areq = mmc_start_req(card->host, new_areq, &status);
+		if (!old_areq) {
 			/*
 			 * We have just put the first request into the pipeline
 			 * and there is nothing more to do until it is
@@ -1670,7 +1672,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 		 * An asynchronous request has been completed and we proceed
 		 * to handle the result of it.
 		 */
-		mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
+		mq_rq =	container_of(old_areq, struct mmc_queue_req, mmc_active);
 		brq = &mq_rq->brq;
 		req = mq_rq->req;
 		type = rq_data_dir(req) == READ ? MMC_BLK_READ : MMC_BLK_WRITE;
-- 
2.9.3

[PATCH 6/6] mmc: block: stop passing around pointless return values

[Linus Walleij]

The mmc_blk_issue_rq() function is called in exactly one place
in queue.c and there the return value is ignored. So the
functions called from that function that also meticulously
return 0/1 do so for no good reason.

Error reporting on the asynchronous requests are done upward to
the block layer when the requests are eventually completed or
fail, which may happen during the flow of the mmc_blk_issue_*
functions directly (for "special commands") or later, when an
asynchronous read/write request is completed.

The issuing functions do not give rise to errors on their own,
and there is nothing to return back to the caller in queue.c.
Drop all return values and make the function return void.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c | 38 ++++++++++++++------------------------
 drivers/mmc/core/block.h |  2 +-
 2 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index f3e0c778cdbd..ede759dda395 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1149,7 +1149,7 @@ int mmc_access_rpmb(struct mmc_queue *mq)
 	return false;
 }
 
-static int mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
+static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
 {
 	struct mmc_blk_data *md = mq->blkdata;
 	struct mmc_card *card = md->queue.card;
@@ -1187,11 +1187,9 @@ static int mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
 		mmc_blk_reset_success(md, type);
 fail:
 	blk_end_request(req, err, blk_rq_bytes(req));
-
-	return err ? 0 : 1;
 }
 
-static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
+static void mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
 				       struct request *req)
 {
 	struct mmc_blk_data *md = mq->blkdata;
@@ -1254,11 +1252,9 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
 		mmc_blk_reset_success(md, type);
 out:
 	blk_end_request(req, err, blk_rq_bytes(req));
-
-	return err ? 0 : 1;
 }
 
-static int mmc_blk_issue_flush(struct mmc_queue *mq, struct request *req)
+static void mmc_blk_issue_flush(struct mmc_queue *mq, struct request *req)
 {
 	struct mmc_blk_data *md = mq->blkdata;
 	struct mmc_card *card = md->queue.card;
@@ -1269,8 +1265,6 @@ static int mmc_blk_issue_flush(struct mmc_queue *mq, struct request *req)
 		ret = -EIO;
 
 	blk_end_request_all(req, ret);
-
-	return ret ? 0 : 1;
 }
 
 /*
@@ -1622,7 +1616,7 @@ static void mmc_blk_rw_start_new(struct mmc_queue *mq, struct mmc_card *card,
 	}
 }
 
-static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
+static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 {
 	struct mmc_blk_data *md = mq->blkdata;
 	struct mmc_card *card = md->queue.card;
@@ -1635,7 +1629,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 	struct mmc_async_req *old_areq;
 
 	if (!rqc && !mq->mqrq_prev->req)
-		return 0;
+		return;
 
 	do {
 		if (rqc) {
@@ -1648,7 +1642,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 				pr_err("%s: Transfer size is not 4KB sector size aligned\n",
 					rqc->rq_disk->disk_name);
 				mmc_blk_rw_cmd_abort(card, rqc);
-				return 0;
+				return;
 			}
 
 			mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
@@ -1665,7 +1659,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			 */
 			if (status == MMC_BLK_NEW_REQUEST)
 				mq->flags |= MMC_QUEUE_NEW_REQUEST;
-			return 0;
+			return;
 		}
 
 		/*
@@ -1699,7 +1693,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 				       __func__, blk_rq_bytes(req),
 				       brq->data.bytes_xfered);
 				mmc_blk_rw_cmd_abort(card, req);
-				return 0;
+				return;
 			}
 			break;
 		case MMC_BLK_CMD_ERR:
@@ -1767,18 +1761,16 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 		}
 	} while (ret);
 
-	return 1;
+	return;
 
  cmd_abort:
 	mmc_blk_rw_cmd_abort(card, req);
 
  start_new_req:
 	mmc_blk_rw_start_new(mq, card, rqc);
-
-	return 0;
 }
 
-int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
+void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 {
 	int ret;
 	struct mmc_blk_data *md = mq->blkdata;
@@ -1794,7 +1786,6 @@ int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 		if (req) {
 			blk_end_request_all(req, -EIO);
 		}
-		ret = 0;
 		goto out;
 	}
 
@@ -1803,19 +1794,19 @@ int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 		/* complete ongoing async transfer before issuing discard */
 		if (card->host->areq)
 			mmc_blk_issue_rw_rq(mq, NULL);
-		ret = mmc_blk_issue_discard_rq(mq, req);
+		mmc_blk_issue_discard_rq(mq, req);
 	} else if (req && req_op(req) == REQ_OP_SECURE_ERASE) {
 		/* complete ongoing async transfer before issuing secure erase*/
 		if (card->host->areq)
 			mmc_blk_issue_rw_rq(mq, NULL);
-		ret = mmc_blk_issue_secdiscard_rq(mq, req);
+		mmc_blk_issue_secdiscard_rq(mq, req);
 	} else if (req && req_op(req) == REQ_OP_FLUSH) {
 		/* complete ongoing async transfer before issuing flush */
 		if (card->host->areq)
 			mmc_blk_issue_rw_rq(mq, NULL);
-		ret = mmc_blk_issue_flush(mq, req);
+		mmc_blk_issue_flush(mq, req);
 	} else {
-		ret = mmc_blk_issue_rw_rq(mq, req);
+		mmc_blk_issue_rw_rq(mq, req);
 	}
 
 out:
@@ -1827,7 +1818,6 @@ int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 		 * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
 		 */
 		mmc_put_card(card);
-	return ret;
 }
 
 static inline int mmc_blk_readonly(struct mmc_card *card)
diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
index 00b7483f2d6e..860ca7c8df86 100644
--- a/drivers/mmc/core/block.h
+++ b/drivers/mmc/core/block.h
@@ -4,6 +4,6 @@
 struct mmc_queue;
 struct request;
 
-int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);
+void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);
 
 #endif
-- 
2.9.3

Re: [PATCH 1/6] mmc: block: break out mmc_blk_rw_cmd_abort()

[Mateusz Nowak]

Hi Linus,

On 1/24/2017 11:17, Linus Walleij wrote:
> As a first step toward breaking apart the very complex function
> mmc_blk_issue_rw_rq() we break out the command abort code.
> This code assumes "ret" is != 0 and then repeatedly hammers
> blk_end_request() until the request to the block layer to end
> the request succeeds.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/core/block.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 7bd03381810d..14efe92a14ef 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1598,6 +1598,17 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
>  	return ret;
>  }
>
> +static void mmc_blk_rw_cmd_abort(struct mmc_card *card, struct request *req)
> +{
> +	int ret = 1;
blk_end_request is returning bool, so maybe this variable should have 
matching type since it is only usage in this scope? And maybe it should 
have more meaningful name for this case?

> +
> +	if (mmc_card_removed(card))
> +		req->rq_flags |= RQF_QUIET;
> +	while (ret)
> +		ret = blk_end_request(req, -EIO,
> +				      blk_rq_cur_bytes(req));
> +}
> +
>  static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>  {
>  	struct mmc_blk_data *md = mq->blkdata;
> @@ -1737,11 +1748,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>  	return 1;
>
>   cmd_abort:
> -	if (mmc_card_removed(card))
> -		req->rq_flags |= RQF_QUIET;
> -	while (ret)
> -		ret = blk_end_request(req, -EIO,
> -				blk_rq_cur_bytes(req));
> +	mmc_blk_rw_cmd_abort(card, req);
>
>   start_new_req:
>  	if (rqc) {
>

Regards,
Mateusz.

Re: [PATCH 0/6] mmc: block: command issue cleanups

[Ulf Hansson]

On 24 January 2017 at 11:17, Linus Walleij <linus.walleij@linaro.org> wrote:
> The function mmc_blk_issue_rw_rq() is hopelessly convoluted and
> need to be refactored to it can be understood by humans.
>
> In the process I found some weird magic return values passed
> around for no good reason.
>
> Things are more readable after this.
>
> This work is done towards the goal of breaking the function in
> two parts: one just submitting the requests and one checking the
> result and possibly resubmitting the command on error, so we
> can make the usual path (non-errorpath) smooth and quick, and
> be called directly when the driver completes a request.
>
> That in turn is a prerequisite for proper blk-mq integration
> with the MMC/SD stack.
>
> All that comes later.
>
> Linus Walleij (6):
>   mmc: block: break out mmc_blk_rw_cmd_abort()
>   mmc: block: break out mmc_blk_rw_start_new()
>   mmc: block: do not assign mq_rq when aborting command
>   mmc: block: inline command abortions
>   mmc: block: introduce new_areq and old_areq
>   mmc: block: stop passing around pointless return values
>
>  drivers/mmc/core/block.c | 108 ++++++++++++++++++++++++++---------------------
>  drivers/mmc/core/block.h |   2 +-
>  2 files changed, 60 insertions(+), 50 deletions(-)
>
> --
> 2.9.3
>

Thanks, applied for next!

Kind regards
Uffe

Re: [PATCH 1/6] mmc: block: break out mmc_blk_rw_cmd_abort()

[Linus Walleij]

On Wed, Jan 25, 2017 at 10:23 AM, Mateusz Nowak
<mateusz.nowak@linux.intel.com> wrote:
> On 1/24/2017 11:17, Linus Walleij wrote:
>>
>> As a first step toward breaking apart the very complex function
>> mmc_blk_issue_rw_rq() we break out the command abort code.
>> This code assumes "ret" is != 0 and then repeatedly hammers
>> blk_end_request() until the request to the block layer to end
>> the request succeeds.
>>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  drivers/mmc/core/block.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>> index 7bd03381810d..14efe92a14ef 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -1598,6 +1598,17 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md,
>> struct mmc_card *card,
>>         return ret;
>>  }
>>
>> +static void mmc_blk_rw_cmd_abort(struct mmc_card *card, struct request
>> *req)
>> +{
>> +       int ret = 1;
>
> blk_end_request is returning bool, so maybe this variable should have
> matching type since it is only usage in this scope? And maybe it should have
> more meaningful name for this case?

I am just moving/refactoring the code syntax without changing any
semantics. It was an int in the old code so it is an int in the new
code.

It is possible to fix this and other problems as separate patches.

As you can see it also spins here for all eternity if the function
just returns != 0 all the time, that doesn't look good either.

Yours,
Linus Walleij

Re: [PATCH 0/6] mmc: block: command issue cleanups

[Ulf Hansson]

+Maxime

On 26 January 2017 at 09:07, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 24 January 2017 at 11:17, Linus Walleij <linus.walleij@linaro.org> wrote:
>> The function mmc_blk_issue_rw_rq() is hopelessly convoluted and
>> need to be refactored to it can be understood by humans.
>>
>> In the process I found some weird magic return values passed
>> around for no good reason.
>>
>> Things are more readable after this.
>>
>> This work is done towards the goal of breaking the function in
>> two parts: one just submitting the requests and one checking the
>> result and possibly resubmitting the command on error, so we
>> can make the usual path (non-errorpath) smooth and quick, and
>> be called directly when the driver completes a request.
>>
>> That in turn is a prerequisite for proper blk-mq integration
>> with the MMC/SD stack.
>>
>> All that comes later.
>>
>> Linus Walleij (6):
>>   mmc: block: break out mmc_blk_rw_cmd_abort()
>>   mmc: block: break out mmc_blk_rw_start_new()
>>   mmc: block: do not assign mq_rq when aborting command
>>   mmc: block: inline command abortions
>>   mmc: block: introduce new_areq and old_areq
>>   mmc: block: stop passing around pointless return values
>>
>>  drivers/mmc/core/block.c | 108 ++++++++++++++++++++++++++---------------------
>>  drivers/mmc/core/block.h |   2 +-
>>  2 files changed, 60 insertions(+), 50 deletions(-)
>>
>> --
>> 2.9.3
>>
>
> Thanks, applied for next!

Linus,

Seems like this series may have issues. I have looked at boot reports
from kernelci, and particular the reports for
https://kernelci.org/boot/sun7i-a20-bananapi/job/ulfh/ are
interesting.

Apparently, this board has an SD card attached. There have been errors
reported in the log for a while when doing data transfers, although
none of these errors have triggered the kernelci to report a boot
error.

However, I suspect that some of the changes in this series make it
worse. Perhaps because of a changed error handling the mmc block
layer!?

Particular, look at the difference between these [1] boot logs, it
might give you some hints. I have also added Maxime to this thread,
perhaps he can help out with the sunxi mmc driver.

Kind regards
Uffe

[1]
Successful boot - sun7i-a20-bananapi:
https://storage.kernelci.org/ulfh/v4.10-rc5-86-g2ed14a2aac8a/arm-sunxi_defconfig/lab-baylibre-seattle/boot-sun7i-a20-bananapi.txt

Failing boot - sun7i-a20-bananapi:
https://storage.kernelci.org/ulfh/v4.10-rc5-97-ge1defa2da4d3/arm-sunxi_defconfig/lab-baylibre-seattle/boot-sun7i-a20-bananapi.txt

Kind regards
Uffe

Re: [PATCH 0/6] mmc: block: command issue cleanups

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2933 bytes --]

Hi Ulf,

On Fri, Jan 27, 2017 at 08:58:16AM +0100, Ulf Hansson wrote:
> On 26 January 2017 at 09:07, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 24 January 2017 at 11:17, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> The function mmc_blk_issue_rw_rq() is hopelessly convoluted and
> >> need to be refactored to it can be understood by humans.
> >>
> >> In the process I found some weird magic return values passed
> >> around for no good reason.
> >>
> >> Things are more readable after this.
> >>
> >> This work is done towards the goal of breaking the function in
> >> two parts: one just submitting the requests and one checking the
> >> result and possibly resubmitting the command on error, so we
> >> can make the usual path (non-errorpath) smooth and quick, and
> >> be called directly when the driver completes a request.
> >>
> >> That in turn is a prerequisite for proper blk-mq integration
> >> with the MMC/SD stack.
> >>
> >> All that comes later.
> >>
> >> Linus Walleij (6):
> >>   mmc: block: break out mmc_blk_rw_cmd_abort()
> >>   mmc: block: break out mmc_blk_rw_start_new()
> >>   mmc: block: do not assign mq_rq when aborting command
> >>   mmc: block: inline command abortions
> >>   mmc: block: introduce new_areq and old_areq
> >>   mmc: block: stop passing around pointless return values
> >>
> >>  drivers/mmc/core/block.c | 108 ++++++++++++++++++++++++++---------------------
> >>  drivers/mmc/core/block.h |   2 +-
> >>  2 files changed, 60 insertions(+), 50 deletions(-)
> >>
> >> --
> >> 2.9.3
> >>
> >
> > Thanks, applied for next!
> 
> Linus,
> 
> Seems like this series may have issues. I have looked at boot reports
> from kernelci, and particular the reports for
> https://kernelci.org/boot/sun7i-a20-bananapi/job/ulfh/ are
> interesting.
> 
> Apparently, this board has an SD card attached. There have been errors
> reported in the log for a while when doing data transfers, although
> none of these errors have triggered the kernelci to report a boot
> error.
> 
> However, I suspect that some of the changes in this series make it
> worse. Perhaps because of a changed error handling the mmc block
> layer!?
> 
> Particular, look at the difference between these [1] boot logs, it
> might give you some hints. I have also added Maxime to this thread,
> perhaps he can help out with the sunxi mmc driver.

We also had a patch on the pinctrl side in 4.10, whose side effects
would be to disable the pull ups on some boards that have not been
configuring their pins properly (while the previous behaviour was to
keep what the bootloader set).

That board seems to fall in that category.

That behaviour has been reverted, and Linus merged it already, so it
might be entirely unrelated to that serie.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/6] mmc: block: command issue cleanups

[Linus Walleij]

On Fri, Jan 27, 2017 at 8:58 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>>> Linus Walleij (6):
>>>   mmc: block: break out mmc_blk_rw_cmd_abort()
>>>   mmc: block: break out mmc_blk_rw_start_new()
>>>   mmc: block: do not assign mq_rq when aborting command
>>>   mmc: block: inline command abortions
>>>   mmc: block: introduce new_areq and old_areq
>>>   mmc: block: stop passing around pointless return values
(...)
> Seems like this series may have issues. I have looked at boot reports
> from kernelci, and particular the reports for
> https://kernelci.org/boot/sun7i-a20-bananapi/job/ulfh/ are
> interesting.
>
> Apparently, this board has an SD card attached. There have been errors
> reported in the log for a while when doing data transfers, although
> none of these errors have triggered the kernelci to report a boot
> error.

Damned I wish I could be hands-on with this system and bisect
it. It's very helpful with shaky systems really. Sadly the errors are
hard to reproduce :(

The old errors look like so:

[    6.099124] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD SBE !!
[    6.105211] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    6.122394] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    6.665013] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD DTO !!
[    6.671011] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    6.677812] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    7.123727] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD DTO !!
[    7.129692] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    7.136489] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    7.143349] blk_update_request: I/O error, dev mmcblk0, sector 124800
[    7.493691] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD DTO !!
[    7.499651] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    7.506229] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    7.943641] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD DTO !!
[    7.949595] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    7.956222] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    7.963010] blk_update_request: I/O error, dev mmcblk0, sector 124800
[    7.969499] Buffer I/O error on dev mmcblk0p1, logical block 15344,
async page read
[    8.321411] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD DTO !!
[    8.327378] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    8.334018] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    8.763338] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD DTO !!
[    8.769276] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    8.775960] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    8.782750] blk_update_request: I/O error, dev mmcblk0, sector 124928
[    9.125126] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD DTO !!
[    9.131084] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    9.137624] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    9.144445] blk_update_request: I/O error, dev mmcblk0, sector 124928
[    9.150881] Buffer I/O error on dev mmcblk0p2, logical block 0,
async page read

So something was causing errors on the read command.

> However, I suspect that some of the changes in this series make it
> worse. Perhaps because of a changed error handling the mmc block
> layer!?
>
> Particular, look at the difference between these [1] boot logs, it
> might give you some hints. I have also added Maxime to this thread,
> perhaps he can help out with the sunxi mmc driver.

The new errors look like so:

[    6.099171] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD SBE !!
[    6.105259] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    6.127415] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    6.666628] sunxi-mmc 1c0f000.mmc: smc 0 err, cmd 18, RD DTO !!
[    6.672626] sunxi-mmc 1c0f000.mmc: data error, sending stop command
[    6.679420] mmcblk0: timed out sending r/w cmd command, card status 0x900
[    7.503256] sunxi-mmc 1c0f000.mmc: fatal err update clk timeout
[    8.623257] sunxi-mmc 1c0f000.mmc: fatal err update clk timeout

This "fatal err update clk timeout" is new and is coming from the driver.

[    8.630370] mmc0: tried to reset card, got error -5
[    8.635309] blk_update_request: I/O error, dev mmcblk0, sector 124800
[    8.642366] mmcblk0: error -5 sending status command, retrying
[    8.648279] mmcblk0: error -5 sending status command, retrying
[    8.654132] mmcblk0: error -5 sending status command, aborting
[    8.659961] blk_update_request: I/O error, dev mmcblk0, sector 7167872
[    8.667201] mmcblk0: error -5 sending status command, retrying
[    8.673031] mmcblk0: error -5 sending status command, retrying
[    8.678916] mmcblk0: error -5 sending status command, aborting
[    8.684758] blk_update_request: I/O error, dev mmcblk0, sector 124800
[    8.691195] Buffer I/O error on dev mmcblk0p1, logical block 15344,
async page read
[    8.700492] mmcblk0: error -5 sending status command, retrying
[    8.706405] mmcblk0: error -5 sending status command, retrying
[    8.712232] mmcblk0: error -5 sending status command, aborting
[    8.718103] blk_update_request: I/O error, dev mmcblk0, sector 7167872
[    8.724643] Buffer I/O error on dev mmcblk0p2, logical block
880368, async page read
[    8.732403] Unable to handle kernel NULL pointer dereference at
virtual address 00000028
[    8.740501] pgd = c0004000
[    8.743204] [00000028] *pgd=00000000
[    8.746794] Internal error: Oops: 17 [#1] SMP ARM
[    8.751491] Modules linked in:
[    8.754547] CPU: 0 PID: 65 Comm: mmcqd/0 Not tainted
4.10.0-rc5-00097-ge1defa2da4d3 #1
[    8.762450] Hardware name: Allwinner sun7i (A20) Family
[    8.767666] task: ee9b0fc0 task.stack: ee9d8000
[    8.772201] PC is at mmc_blk_rw_rq_prep+0x20/0x39c
[    8.776985] LR is at mmc_blk_issue_rw_rq+0x124/0x370

And now it is crashing at mmc_blk_rw_rq_prep() + 0x20 so I suspect it is one of
these:

struct mmc_blk_request *brq = &mqrq->brq;
struct request *req = mqrq->req;
struct mmc_blk_data *md = mq->blkdata;

I guess the first: mqrq is NULL.

So I suspect the oneliner in commit 0ebd6e72b5ee2592625d5ae567a729345dfe07b6
"mmc: block: do not assign mq_rq when aborting command"

That is not easily reverted so I will sent a RTF patch to restore the behaviour.

Yours,
Linus Walleij

Re: [PATCH 0/6] mmc: block: command issue cleanups

[Linus Walleij]

On Mon, Jan 30, 2017 at 2:05 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jan 27, 2017 at 8:58 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> And now it is crashing at mmc_blk_rw_rq_prep() + 0x20 so I suspect it is one of
> these:
>
> struct mmc_blk_request *brq = &mqrq->brq;
> struct request *req = mqrq->req;
> struct mmc_blk_data *md = mq->blkdata;
>
> I guess the first: mqrq is NULL.
>
> So I suspect the oneliner in commit 0ebd6e72b5ee2592625d5ae567a729345dfe07b6
> "mmc: block: do not assign mq_rq when aborting command"

Bah looking closer at that it just doesn't make any logical sense at all.

I now suspect the NULL check removed in mmc_blk_rw_start_new()
to be behind this, so sent a patch to restore it.

No idea how to test it or if it's the real problem.

I had to rebase my further patches on top too, so if you merge this
I need to resend the next series.

Yours,
Linus Walleij