[PATCH 00/10] Further MMC core/block cleanups

[PATCH 00/10] Further MMC core/block cleanups

[Linus Walleij]

This is a continuation of the series merged to start cleaning
up the very big mmc_blk_issue_rw_rq() and friends.

Again the goal is to get the code in a shape such that I can
get rid of flusing the pipeline with NULLs and report the
requests as finished directly to the block layer, without
having to wait for an iteration of mmc_blk_issue_req() to
happen before they get postprocessed and finished.

Feel free to apply this partially, or defer it all to after
v4.11-rc1 *NO*RUSH*.

It is just my idead of a cleanup road toward blk MQ
compliance. I think most patches stand on their own but can
be perceived as over-pedantic without that context.

Linus Walleij (10):
  mmc: block: inline the command abort and start new goto:s
  mmc: block: rename rqc and req
  mmc: core: rename mmc_start_req() to *areq()
  mmc: block: refactor mmc_blk_rw_try_restart()
  mmc: block: rename mmc_active to areq
  mmc: queue: turn queue flags into bools
  mmc: block: return errorcode from mmc_sd_num_wr_blocks()
  mmc: block: respect bool returned from blk_end_request()
  mmc: block: return 0 where evident
  mmc: core: start to break apart mmc_start_areq()

 drivers/mmc/core/block.c    | 179 ++++++++++++++++++++++++--------------------
 drivers/mmc/core/core.c     |  79 ++++++++++++-------
 drivers/mmc/core/mmc_test.c |   8 +-
 drivers/mmc/core/queue.c    |  12 +--
 drivers/mmc/core/queue.h    |   7 +-
 include/linux/mmc/core.h    |   2 +-
 6 files changed, 163 insertions(+), 124 deletions(-)

-- 
2.9.3

[PATCH 01/10] mmc: block: inline the command abort and start new goto:s

[Linus Walleij]

The goto statements sprinkled over the mmc_blk_issue_rw_rq()
function has grown over the years and makes the code pretty hard
to read.

Inline the calls such that:

goto cmd_abort; ->
mmc_blk_rw_cmd_abort(card, req);
mmc_blk_rw_start_new(mq, card, rqc);
return;

goto start_new_req; ->
mmc_blk_rw_start_new(mq, card, rqc);
return;

After this it is more clear how we exit the do {} while
loop in this function, and it gets possible to split the
code apart.

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

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 6c8be1a80551..0e23dba62613 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1701,10 +1701,15 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			break;
 		case MMC_BLK_CMD_ERR:
 			ret = mmc_blk_cmd_err(md, card, brq, req, ret);
-			if (mmc_blk_reset(md, card->host, type))
-				goto cmd_abort;
-			if (!ret)
-				goto start_new_req;
+			if (mmc_blk_reset(md, card->host, type)) {
+				mmc_blk_rw_cmd_abort(card, req);
+				mmc_blk_rw_start_new(mq, card, rqc);
+				return;
+			}
+			if (!ret) {
+				mmc_blk_rw_start_new(mq, card, rqc);
+				return;
+			}
 			break;
 		case MMC_BLK_RETRY:
 			retune_retry_done = brq->retune_retry_done;
@@ -1714,15 +1719,20 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 		case MMC_BLK_ABORT:
 			if (!mmc_blk_reset(md, card->host, type))
 				break;
-			goto cmd_abort;
+			mmc_blk_rw_cmd_abort(card, req);
+			mmc_blk_rw_start_new(mq, card, rqc);
+			return;
 		case MMC_BLK_DATA_ERR: {
 			int err;
 
 			err = mmc_blk_reset(md, card->host, type);
 			if (!err)
 				break;
-			if (err == -ENODEV)
-				goto cmd_abort;
+			if (err == -ENODEV) {
+				mmc_blk_rw_cmd_abort(card, req);
+				mmc_blk_rw_start_new(mq, card, rqc);
+				return;
+			}
 			/* Fall through */
 		}
 		case MMC_BLK_ECC_ERR:
@@ -1740,15 +1750,21 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			 */
 			ret = blk_end_request(req, -EIO,
 						brq->data.blksz);
-			if (!ret)
-				goto start_new_req;
+			if (!ret) {
+				mmc_blk_rw_start_new(mq, card, rqc);
+				return;
+			}
 			break;
 		case MMC_BLK_NOMEDIUM:
-			goto cmd_abort;
+			mmc_blk_rw_cmd_abort(card, req);
+			mmc_blk_rw_start_new(mq, card, rqc);
+			return;
 		default:
 			pr_err("%s: Unhandled return value (%d)",
 					req->rq_disk->disk_name, status);
-			goto cmd_abort;
+			mmc_blk_rw_cmd_abort(card, req);
+			mmc_blk_rw_start_new(mq, card, rqc);
+			return;
 		}
 
 		if (ret) {
@@ -1763,14 +1779,6 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			mq_rq->brq.retune_retry_done = retune_retry_done;
 		}
 	} while (ret);
-
-	return;
-
- cmd_abort:
-	mmc_blk_rw_cmd_abort(card, req);
-
- start_new_req:
-	mmc_blk_rw_start_new(mq, card, rqc);
 }
 
 void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
-- 
2.9.3

[PATCH 02/10] mmc: block: rename rqc and req

[Linus Walleij]

In the function mmc_blk_issue_rw_rq() the new request coming in
from the block layer is called "rqc" and the old request that
was potentially just returned back from the asynchronous
mechanism is called "req".

This is really confusing when trying to analyze and understand
the code, it becomes a perceptual nightmare to me. Maybe others
have better parserheads but it is not working for me.

Rename "rqc" to "new_req" and "req" to "old_req" to reflect what
is semantically going on into the syntax.

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

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 0e23dba62613..e1479f114247 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1619,7 +1619,7 @@ static void mmc_blk_rw_start_new(struct mmc_queue *mq, struct mmc_card *card,
 	}
 }
 
-static void 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 *new_req)
 {
 	struct mmc_blk_data *md = mq->blkdata;
 	struct mmc_card *card = md->queue.card;
@@ -1627,24 +1627,24 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 	int ret = 1, disable_multi = 0, retry = 0, type, retune_retry_done = 0;
 	enum mmc_blk_status status;
 	struct mmc_queue_req *mq_rq;
-	struct request *req;
+	struct request *old_req;
 	struct mmc_async_req *new_areq;
 	struct mmc_async_req *old_areq;
 
-	if (!rqc && !mq->mqrq_prev->req)
+	if (!new_req && !mq->mqrq_prev->req)
 		return;
 
 	do {
-		if (rqc) {
+		if (new_req) {
 			/*
 			 * When 4KB native sector is enabled, only 8 blocks
 			 * multiple read or write is allowed
 			 */
 			if (mmc_large_sector(card) &&
-				!IS_ALIGNED(blk_rq_sectors(rqc), 8)) {
+				!IS_ALIGNED(blk_rq_sectors(new_req), 8)) {
 				pr_err("%s: Transfer size is not 4KB sector size aligned\n",
-					rqc->rq_disk->disk_name);
-				mmc_blk_rw_cmd_abort(card, rqc);
+					new_req->rq_disk->disk_name);
+				mmc_blk_rw_cmd_abort(card, new_req);
 				return;
 			}
 
@@ -1671,8 +1671,8 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 		 */
 		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;
+		old_req = mq_rq->req;
+		type = rq_data_dir(old_req) == READ ? MMC_BLK_READ : MMC_BLK_WRITE;
 		mmc_queue_bounce_post(mq_rq);
 
 		switch (status) {
@@ -1683,7 +1683,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			 */
 			mmc_blk_reset_success(md, type);
 
-			ret = blk_end_request(req, 0,
+			ret = blk_end_request(old_req, 0,
 					brq->data.bytes_xfered);
 
 			/*
@@ -1693,21 +1693,21 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			 */
 			if (status == MMC_BLK_SUCCESS && ret) {
 				pr_err("%s BUG rq_tot %d d_xfer %d\n",
-				       __func__, blk_rq_bytes(req),
+				       __func__, blk_rq_bytes(old_req),
 				       brq->data.bytes_xfered);
-				mmc_blk_rw_cmd_abort(card, req);
+				mmc_blk_rw_cmd_abort(card, old_req);
 				return;
 			}
 			break;
 		case MMC_BLK_CMD_ERR:
-			ret = mmc_blk_cmd_err(md, card, brq, req, ret);
+			ret = mmc_blk_cmd_err(md, card, brq, old_req, ret);
 			if (mmc_blk_reset(md, card->host, type)) {
-				mmc_blk_rw_cmd_abort(card, req);
-				mmc_blk_rw_start_new(mq, card, rqc);
+				mmc_blk_rw_cmd_abort(card, old_req);
+				mmc_blk_rw_start_new(mq, card, new_req);
 				return;
 			}
 			if (!ret) {
-				mmc_blk_rw_start_new(mq, card, rqc);
+				mmc_blk_rw_start_new(mq, card, new_req);
 				return;
 			}
 			break;
@@ -1719,8 +1719,8 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 		case MMC_BLK_ABORT:
 			if (!mmc_blk_reset(md, card->host, type))
 				break;
-			mmc_blk_rw_cmd_abort(card, req);
-			mmc_blk_rw_start_new(mq, card, rqc);
+			mmc_blk_rw_cmd_abort(card, old_req);
+			mmc_blk_rw_start_new(mq, card, new_req);
 			return;
 		case MMC_BLK_DATA_ERR: {
 			int err;
@@ -1729,8 +1729,8 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			if (!err)
 				break;
 			if (err == -ENODEV) {
-				mmc_blk_rw_cmd_abort(card, req);
-				mmc_blk_rw_start_new(mq, card, rqc);
+				mmc_blk_rw_cmd_abort(card, old_req);
+				mmc_blk_rw_start_new(mq, card, new_req);
 				return;
 			}
 			/* Fall through */
@@ -1739,7 +1739,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			if (brq->data.blocks > 1) {
 				/* Redo read one sector at a time */
 				pr_warn("%s: retrying using single block read\n",
-					req->rq_disk->disk_name);
+					old_req->rq_disk->disk_name);
 				disable_multi = 1;
 				break;
 			}
@@ -1748,22 +1748,22 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			 * time, so we only reach here after trying to
 			 * read a single sector.
 			 */
-			ret = blk_end_request(req, -EIO,
+			ret = blk_end_request(old_req, -EIO,
 						brq->data.blksz);
 			if (!ret) {
-				mmc_blk_rw_start_new(mq, card, rqc);
+				mmc_blk_rw_start_new(mq, card, new_req);
 				return;
 			}
 			break;
 		case MMC_BLK_NOMEDIUM:
-			mmc_blk_rw_cmd_abort(card, req);
-			mmc_blk_rw_start_new(mq, card, rqc);
+			mmc_blk_rw_cmd_abort(card, old_req);
+			mmc_blk_rw_start_new(mq, card, new_req);
 			return;
 		default:
 			pr_err("%s: Unhandled return value (%d)",
-					req->rq_disk->disk_name, status);
-			mmc_blk_rw_cmd_abort(card, req);
-			mmc_blk_rw_start_new(mq, card, rqc);
+					old_req->rq_disk->disk_name, status);
+			mmc_blk_rw_cmd_abort(card, old_req);
+			mmc_blk_rw_start_new(mq, card, new_req);
 			return;
 		}
 
-- 
2.9.3

[PATCH 03/10] mmc: core: rename mmc_start_req() to *areq()

[Linus Walleij]

With the coexisting __mmc_start_request(), mmc_start_request()
and __mmc_start_req() it is a bit confusing that mmc_start_req()
actually does not start a normal request, but an asynchronous
request.

Rename it to mmc_start_areq() to make it explicit what the
function is doing, also fix the kerneldoc for this function
while we're at it.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c    |  8 ++++----
 drivers/mmc/core/core.c     | 14 +++++++-------
 drivers/mmc/core/mmc_test.c |  8 ++++----
 include/linux/mmc/core.h    |  2 +-
 4 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index e1479f114247..3246cd396c77 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1614,8 +1614,8 @@ static void mmc_blk_rw_start_new(struct mmc_queue *mq, struct mmc_card *card,
 		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);
+		mmc_start_areq(card->host,
+			       &mq->mqrq_cur->mmc_active, NULL);
 	}
 }
 
@@ -1653,7 +1653,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 		} else
 			new_areq = NULL;
 
-		old_areq = mmc_start_req(card->host, new_areq, &status);
+		old_areq = mmc_start_areq(card->host, new_areq, &status);
 		if (!old_areq) {
 			/*
 			 * We have just put the first request into the pipeline
@@ -1774,7 +1774,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			 */
 			mmc_blk_rw_rq_prep(mq_rq, card,
 					disable_multi, mq);
-			mmc_start_req(card->host,
+			mmc_start_areq(card->host,
 					&mq_rq->mmc_active, NULL);
 			mq_rq->brq.retune_retry_done = retune_retry_done;
 		}
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 0faa2ee613be..a160c3a7777a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -631,10 +631,10 @@ static void mmc_post_req(struct mmc_host *host, struct mmc_request *mrq,
 }
 
 /**
- *	mmc_start_req - start a non-blocking request
+ *	mmc_start_areq - start an asynchronous request
  *	@host: MMC host to start command
- *	@areq: async request to start
- *	@error: out parameter returns 0 for success, otherwise non zero
+ *	@areq: asynchronous request to start
+ *	@ret_stat: out parameter for status
  *
  *	Start a new MMC custom command request for a host.
  *	If there is on ongoing async request wait for completion
@@ -646,9 +646,9 @@ static void mmc_post_req(struct mmc_host *host, struct mmc_request *mrq,
  *	return the completed request. If there is no ongoing request, NULL
  *	is returned without waiting. NULL is not an error condition.
  */
-struct mmc_async_req *mmc_start_req(struct mmc_host *host,
-				    struct mmc_async_req *areq,
-				    enum mmc_blk_status *ret_stat)
+struct mmc_async_req *mmc_start_areq(struct mmc_host *host,
+				     struct mmc_async_req *areq,
+				     enum mmc_blk_status *ret_stat)
 {
 	enum mmc_blk_status status = MMC_BLK_SUCCESS;
 	int start_err = 0;
@@ -699,7 +699,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
 		*ret_stat = status;
 	return data;
 }
-EXPORT_SYMBOL(mmc_start_req);
+EXPORT_SYMBOL(mmc_start_areq);
 
 /**
  *	mmc_wait_for_req - start a request and wait for completion
diff --git a/drivers/mmc/core/mmc_test.c b/drivers/mmc/core/mmc_test.c
index 83d193c09d98..f99ac3123fd2 100644
--- a/drivers/mmc/core/mmc_test.c
+++ b/drivers/mmc/core/mmc_test.c
@@ -853,7 +853,7 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
 	for (i = 0; i < count; i++) {
 		mmc_test_prepare_mrq(test, cur_areq->mrq, sg, sg_len, dev_addr,
 				     blocks, blksz, write);
-		done_areq = mmc_start_req(test->card->host, cur_areq, &status);
+		done_areq = mmc_start_areq(test->card->host, cur_areq, &status);
 
 		if (status != MMC_BLK_SUCCESS || (!done_areq && i > 0)) {
 			ret = RESULT_FAIL;
@@ -872,7 +872,7 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
 		dev_addr += blocks;
 	}
 
-	done_areq = mmc_start_req(test->card->host, NULL, &status);
+	done_areq = mmc_start_areq(test->card->host, NULL, &status);
 	if (status != MMC_BLK_SUCCESS)
 		ret = RESULT_FAIL;
 
@@ -2402,7 +2402,7 @@ static int mmc_test_ongoing_transfer(struct mmc_test_card *test,
 
 	/* Start ongoing data request */
 	if (use_areq) {
-		mmc_start_req(host, &test_areq.areq, &blkstat);
+		mmc_start_areq(host, &test_areq.areq, &blkstat);
 		if (blkstat != MMC_BLK_SUCCESS) {
 			ret = RESULT_FAIL;
 			goto out_free;
@@ -2440,7 +2440,7 @@ static int mmc_test_ongoing_transfer(struct mmc_test_card *test,
 
 	/* Wait for data request to complete */
 	if (use_areq) {
-		mmc_start_req(host, NULL, &blkstat);
+		mmc_start_areq(host, NULL, &blkstat);
 		if (blkstat != MMC_BLK_SUCCESS)
 			ret = RESULT_FAIL;
 	} else {
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 6dcb339fcd45..a0c63ea28796 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -158,7 +158,7 @@ struct mmc_request {
 struct mmc_card;
 struct mmc_async_req;
 
-struct mmc_async_req *mmc_start_req(struct mmc_host *host,
+struct mmc_async_req *mmc_start_areq(struct mmc_host *host,
 				struct mmc_async_req *areq,
 				enum mmc_blk_status *ret_stat);
 void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq);
-- 
2.9.3

[PATCH 04/10] mmc: block: refactor mmc_blk_rw_try_restart()

[Linus Walleij]

The mmc_blk_rw_start_new() was named after the label inside
mmc_blk_issue_rw_rq() but is really a confusing name for this
function: what it does is to try to restart the latest issued
command on the host and card of the current MMC queue.

So rename it mmc_blk_rw_try_restart() that reflects what it
is doing and at this point also refactore the function to
treat the removed card as an exception and just exit if this
happens and run on in the function if that is not happening.

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

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 3246cd396c77..8216ce3874b9 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1603,20 +1603,27 @@ 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)
+/**
+ * mmc_blk_rw_try_restart() - tries to restart the current async request
+ * @mq: the queue with the card and host to restart
+ * @req: a new request that want to be started after the current one
+ */
+static void mmc_blk_rw_try_restart(struct mmc_queue *mq, struct request *req)
 {
 	if (!req)
 		return;
 
-	if (mmc_card_removed(card)) {
+	/*
+	 * If the card was removed, just cancel everything and return.
+	 */
+	if (mmc_card_removed(mq->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_areq(card->host,
-			       &mq->mqrq_cur->mmc_active, NULL);
+		return;
 	}
+	/* Else proceed and try to restart the current async request */
+	mmc_blk_rw_rq_prep(mq->mqrq_cur, mq->card, 0, mq);
+	mmc_start_areq(mq->card->host, &mq->mqrq_cur->mmc_active, NULL);
 }
 
 static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
@@ -1703,11 +1710,11 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			ret = mmc_blk_cmd_err(md, card, brq, old_req, ret);
 			if (mmc_blk_reset(md, card->host, type)) {
 				mmc_blk_rw_cmd_abort(card, old_req);
-				mmc_blk_rw_start_new(mq, card, new_req);
+				mmc_blk_rw_try_restart(mq, new_req);
 				return;
 			}
 			if (!ret) {
-				mmc_blk_rw_start_new(mq, card, new_req);
+				mmc_blk_rw_try_restart(mq, new_req);
 				return;
 			}
 			break;
@@ -1720,7 +1727,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			if (!mmc_blk_reset(md, card->host, type))
 				break;
 			mmc_blk_rw_cmd_abort(card, old_req);
-			mmc_blk_rw_start_new(mq, card, new_req);
+			mmc_blk_rw_try_restart(mq, new_req);
 			return;
 		case MMC_BLK_DATA_ERR: {
 			int err;
@@ -1730,7 +1737,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 				break;
 			if (err == -ENODEV) {
 				mmc_blk_rw_cmd_abort(card, old_req);
-				mmc_blk_rw_start_new(mq, card, new_req);
+				mmc_blk_rw_try_restart(mq, new_req);
 				return;
 			}
 			/* Fall through */
@@ -1751,19 +1758,19 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			ret = blk_end_request(old_req, -EIO,
 						brq->data.blksz);
 			if (!ret) {
-				mmc_blk_rw_start_new(mq, card, new_req);
+				mmc_blk_rw_try_restart(mq, new_req);
 				return;
 			}
 			break;
 		case MMC_BLK_NOMEDIUM:
 			mmc_blk_rw_cmd_abort(card, old_req);
-			mmc_blk_rw_start_new(mq, card, new_req);
+			mmc_blk_rw_try_restart(mq, new_req);
 			return;
 		default:
 			pr_err("%s: Unhandled return value (%d)",
 					old_req->rq_disk->disk_name, status);
 			mmc_blk_rw_cmd_abort(card, old_req);
-			mmc_blk_rw_start_new(mq, card, new_req);
+			mmc_blk_rw_try_restart(mq, new_req);
 			return;
 		}
 
-- 
2.9.3

[PATCH 05/10] mmc: block: rename mmc_active to areq

[Linus Walleij]

The mmc_active member of struct mmc_queue_req has a very
confusing name: this is certainly not always "active", it is
the asynchronous request associated by the mmc_queue_req
but it is not guaranteed to be "active" in any sense, such
as being running on the host.

Simply rename this member to "areq".

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

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 8216ce3874b9..7dcc27763381 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1302,7 +1302,7 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
 					     struct mmc_async_req *areq)
 {
 	struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req,
-						    mmc_active);
+						    areq);
 	struct mmc_blk_request *brq = &mq_mrq->brq;
 	struct request *req = mq_mrq->req;
 	int need_retune = card->host->need_retune;
@@ -1558,8 +1558,8 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 		brq->data.sg_len = i;
 	}
 
-	mqrq->mmc_active.mrq = &brq->mrq;
-	mqrq->mmc_active.err_check = mmc_blk_err_check;
+	mqrq->areq.mrq = &brq->mrq;
+	mqrq->areq.err_check = mmc_blk_err_check;
 
 	mmc_queue_bounce_pre(mqrq);
 }
@@ -1623,7 +1623,7 @@ static void mmc_blk_rw_try_restart(struct mmc_queue *mq, struct request *req)
 	}
 	/* Else proceed and try to restart the current async request */
 	mmc_blk_rw_rq_prep(mq->mqrq_cur, mq->card, 0, mq);
-	mmc_start_areq(mq->card->host, &mq->mqrq_cur->mmc_active, NULL);
+	mmc_start_areq(mq->card->host, &mq->mqrq_cur->areq, NULL);
 }
 
 static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
@@ -1656,7 +1656,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			}
 
 			mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
-			new_areq = &mq->mqrq_cur->mmc_active;
+			new_areq = &mq->mqrq_cur->areq;
 		} else
 			new_areq = NULL;
 
@@ -1676,7 +1676,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 		 * An asynchronous request has been completed and we proceed
 		 * to handle the result of it.
 		 */
-		mq_rq =	container_of(old_areq, struct mmc_queue_req, mmc_active);
+		mq_rq =	container_of(old_areq, struct mmc_queue_req, areq);
 		brq = &mq_rq->brq;
 		old_req = mq_rq->req;
 		type = rq_data_dir(old_req) == READ ? MMC_BLK_READ : MMC_BLK_WRITE;
@@ -1782,7 +1782,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			mmc_blk_rw_rq_prep(mq_rq, card,
 					disable_multi, mq);
 			mmc_start_areq(card->host,
-					&mq_rq->mmc_active, NULL);
+					&mq_rq->areq, NULL);
 			mq_rq->brq.retune_retry_done = retune_retry_done;
 		}
 	} while (ret);
diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h
index 0cea02af79d1..e0cd5b1f40ee 100644
--- a/drivers/mmc/core/queue.h
+++ b/drivers/mmc/core/queue.h
@@ -33,7 +33,7 @@ struct mmc_queue_req {
 	char			*bounce_buf;
 	struct scatterlist	*bounce_sg;
 	unsigned int		bounce_sg_len;
-	struct mmc_async_req	mmc_active;
+	struct mmc_async_req	areq;
 };
 
 struct mmc_queue {
-- 
2.9.3

[PATCH 06/10] mmc: queue: turn queue flags into bools

[Linus Walleij]

Instead of masking and setting two bits in the "flags" field
for the mmc_queue, just use two bools named "suspended" and
"new_request".

The masking and setting would likely have race conditions
anyways, it is better to use a simple member like this.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/block.c |  6 +++---
 drivers/mmc/core/queue.c | 12 ++++++------
 drivers/mmc/core/queue.h |  5 ++---
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 7dcc27763381..1ac7fec03c97 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1668,7 +1668,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			 * complete.
 			 */
 			if (status == MMC_BLK_NEW_REQUEST)
-				mq->flags |= MMC_QUEUE_NEW_REQUEST;
+				mq->new_request = true;
 			return;
 		}
 
@@ -1807,7 +1807,7 @@ void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 		goto out;
 	}
 
-	mq->flags &= ~MMC_QUEUE_NEW_REQUEST;
+	mq->new_request = false;
 	if (req && req_op(req) == REQ_OP_DISCARD) {
 		/* complete ongoing async transfer before issuing discard */
 		if (card->host->areq)
@@ -1828,7 +1828,7 @@ void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 	}
 
 out:
-	if ((!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) || req_is_special)
+	if ((!req && !mq->new_request) || req_is_special)
 		/*
 		 * Release host when there are no more requests
 		 * and after special request(discard, flush) is done.
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index 611f5c6d1950..5cb369c2664b 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -86,8 +86,8 @@ static int mmc_queue_thread(void *d)
 			set_current_state(TASK_RUNNING);
 			mmc_blk_issue_rq(mq, req);
 			cond_resched();
-			if (mq->flags & MMC_QUEUE_NEW_REQUEST) {
-				mq->flags &= ~MMC_QUEUE_NEW_REQUEST;
+			if (mq->new_request) {
+				mq->new_request = false;
 				continue; /* fetch again */
 			}
 
@@ -401,8 +401,8 @@ void mmc_queue_suspend(struct mmc_queue *mq)
 	struct request_queue *q = mq->queue;
 	unsigned long flags;
 
-	if (!(mq->flags & MMC_QUEUE_SUSPENDED)) {
-		mq->flags |= MMC_QUEUE_SUSPENDED;
+	if (!mq->suspended) {
+		mq->suspended |= true;
 
 		spin_lock_irqsave(q->queue_lock, flags);
 		blk_stop_queue(q);
@@ -421,8 +421,8 @@ void mmc_queue_resume(struct mmc_queue *mq)
 	struct request_queue *q = mq->queue;
 	unsigned long flags;
 
-	if (mq->flags & MMC_QUEUE_SUSPENDED) {
-		mq->flags &= ~MMC_QUEUE_SUSPENDED;
+	if (mq->suspended) {
+		mq->suspended = false;
 
 		up(&mq->thread_sem);
 
diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h
index e0cd5b1f40ee..e298f100101b 100644
--- a/drivers/mmc/core/queue.h
+++ b/drivers/mmc/core/queue.h
@@ -40,9 +40,8 @@ struct mmc_queue {
 	struct mmc_card		*card;
 	struct task_struct	*thread;
 	struct semaphore	thread_sem;
-	unsigned int		flags;
-#define MMC_QUEUE_SUSPENDED	(1 << 0)
-#define MMC_QUEUE_NEW_REQUEST	(1 << 1)
+	bool			new_request;
+	bool			suspended;
 	bool			asleep;
 	struct mmc_blk_data	*blkdata;
 	struct request_queue	*queue;
-- 
2.9.3

[PATCH 07/10] mmc: block: return errorcode from mmc_sd_num_wr_blocks()

[Linus Walleij]

mmc_sd_num_wr_blocks() has an interesting construction that
saves one return argument by casting (u32)-1 as error code
if something goes wrong.

This is however a bit confusing when the normal kernel
pattern is to return an int error code on success.

So instead pass a variable "blocks" that the function can
fill in with the number of successfully transfered blocks
and return an integer as error code.

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

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 1ac7fec03c97..91d506b37024 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -768,7 +768,7 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
 	return 0;
 }
 
-static u32 mmc_sd_num_wr_blocks(struct mmc_card *card)
+static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks)
 {
 	int err;
 	u32 result;
@@ -786,9 +786,9 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card)
 
 	err = mmc_wait_for_cmd(card->host, &cmd, 0);
 	if (err)
-		return (u32)-1;
+		return err;
 	if (!mmc_host_is_spi(card->host) && !(cmd.resp[0] & R1_APP_CMD))
-		return (u32)-1;
+		return err;
 
 	memset(&cmd, 0, sizeof(struct mmc_command));
 
@@ -808,7 +808,7 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card)
 
 	blocks = kmalloc(4, GFP_KERNEL);
 	if (!blocks)
-		return (u32)-1;
+		return -ENOMEM;
 
 	sg_init_one(&sg, blocks, 4);
 
@@ -818,9 +818,11 @@ static u32 mmc_sd_num_wr_blocks(struct mmc_card *card)
 	kfree(blocks);
 
 	if (cmd.error || data.error)
-		result = (u32)-1;
+		return -EIO;
 
-	return result;
+	*written_blocks = result;
+
+	return 0;
 }
 
 static int get_card_status(struct mmc_card *card, u32 *status, int retries)
@@ -1581,9 +1583,10 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
 	 */
 	if (mmc_card_sd(card)) {
 		u32 blocks;
+		int err;
 
-		blocks = mmc_sd_num_wr_blocks(card);
-		if (blocks != (u32)-1) {
+		err = mmc_sd_num_wr_blocks(card, &blocks);
+		if (!err) {
 			ret = blk_end_request(req, 0, blocks << 9);
 		}
 	} else {
-- 
2.9.3

[PATCH 08/10] mmc: block: respect bool returned from blk_end_request()

[Linus Walleij]

The return value from blk_end_request() is a bool but is
treated like an int. This is generally safe, but the variable
also has the opaque name "ret" and gets returned from the
helper function mmc_blk_cmd_err().

- Switch the variable to a bool, applies everywhere.

- Return a bool from mmc_blk_cmd_err() and rename the function
  mmc_blk_rw_cmd_err() to indicate through the namespace that
  this is a helper for mmc_blk_issue_rw_rq().

- Rename the variable from "ret" to "req_pending" inside the
  while() loop inside mmc_blk_issue_rq_rq(), which finally
  makes it very clear what this while loop is waiting for.

- Augment the argument "ret" to mmc_blk_rq_cmd_err() to
  old_req_pending so it becomes evident that this is an
  older state, and it is returned only if we fail to get
  the number of written blocks from an SD card in the
  function mmc_sd_num_wr_blocks().

- Augment the while() loop in mmc_blk_rq_cmd_abort(): it
  is evident now that we know this is a bool variable,
  that the function is just spinning waiting for
  blk_end_request() to return false.

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

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 91d506b37024..92f7772ca56d 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1566,11 +1566,13 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 	mmc_queue_bounce_pre(mqrq);
 }
 
-static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
-			   struct mmc_blk_request *brq, struct request *req,
-			   int ret)
+static bool mmc_blk_rw_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
+			       struct mmc_blk_request *brq, struct request *req,
+			       bool old_req_pending)
 {
 	struct mmc_queue_req *mq_rq;
+	bool req_pending;
+
 	mq_rq = container_of(brq, struct mmc_queue_req, brq);
 
 	/*
@@ -1586,24 +1588,23 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
 		int err;
 
 		err = mmc_sd_num_wr_blocks(card, &blocks);
-		if (!err) {
-			ret = blk_end_request(req, 0, blocks << 9);
-		}
+		if (err)
+			req_pending = old_req_pending;
+		else
+			req_pending = blk_end_request(req, 0, blocks << 9);
 	} else {
-		ret = blk_end_request(req, 0, brq->data.bytes_xfered);
+		req_pending = blk_end_request(req, 0, brq->data.bytes_xfered);
 	}
-	return ret;
+	return req_pending;
 }
 
 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));
+	while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)))
+	{
+	}
 }
 
 /**
@@ -1634,12 +1635,13 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 	struct mmc_blk_data *md = mq->blkdata;
 	struct mmc_card *card = md->queue.card;
 	struct mmc_blk_request *brq;
-	int ret = 1, disable_multi = 0, retry = 0, type, retune_retry_done = 0;
+	int disable_multi = 0, retry = 0, type, retune_retry_done = 0;
 	enum mmc_blk_status status;
 	struct mmc_queue_req *mq_rq;
 	struct request *old_req;
 	struct mmc_async_req *new_areq;
 	struct mmc_async_req *old_areq;
+	bool req_pending = true;
 
 	if (!new_req && !mq->mqrq_prev->req)
 		return;
@@ -1693,15 +1695,14 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			 */
 			mmc_blk_reset_success(md, type);
 
-			ret = blk_end_request(old_req, 0,
-					brq->data.bytes_xfered);
-
+			req_pending = blk_end_request(old_req, 0,
+						      brq->data.bytes_xfered);
 			/*
 			 * If the blk_end_request function returns non-zero even
 			 * though all data has been transferred and no errors
 			 * were returned by the host controller, it's a bug.
 			 */
-			if (status == MMC_BLK_SUCCESS && ret) {
+			if (status == MMC_BLK_SUCCESS && req_pending) {
 				pr_err("%s BUG rq_tot %d d_xfer %d\n",
 				       __func__, blk_rq_bytes(old_req),
 				       brq->data.bytes_xfered);
@@ -1710,13 +1711,13 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			}
 			break;
 		case MMC_BLK_CMD_ERR:
-			ret = mmc_blk_cmd_err(md, card, brq, old_req, ret);
+			req_pending = mmc_blk_rw_cmd_err(md, card, brq, old_req, req_pending);
 			if (mmc_blk_reset(md, card->host, type)) {
 				mmc_blk_rw_cmd_abort(card, old_req);
 				mmc_blk_rw_try_restart(mq, new_req);
 				return;
 			}
-			if (!ret) {
+			if (!req_pending) {
 				mmc_blk_rw_try_restart(mq, new_req);
 				return;
 			}
@@ -1758,9 +1759,9 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			 * time, so we only reach here after trying to
 			 * read a single sector.
 			 */
-			ret = blk_end_request(old_req, -EIO,
-						brq->data.blksz);
-			if (!ret) {
+			req_pending = blk_end_request(old_req, -EIO,
+						      brq->data.blksz);
+			if (!req_pending) {
 				mmc_blk_rw_try_restart(mq, new_req);
 				return;
 			}
@@ -1777,7 +1778,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 			return;
 		}
 
-		if (ret) {
+		if (req_pending) {
 			/*
 			 * In case of a incomplete request
 			 * prepare it again and resend.
@@ -1788,7 +1789,7 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
 					&mq_rq->areq, NULL);
 			mq_rq->brq.retune_retry_done = retune_retry_done;
 		}
-	} while (ret);
+	} while (req_pending);
 }
 
 void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
-- 
2.9.3

[PATCH 09/10] mmc: block: return 0 where evident

[Linus Walleij]

mmc_sd_num_wr_blocks() has this construction:

if (err)
    return err;
if (some_other_stuff_not_touching_err)
    return err;

It follows from logical deduction that we can just return 0
in the latter case.

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

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 92f7772ca56d..c49c90dba839 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -788,7 +788,7 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks)
 	if (err)
 		return err;
 	if (!mmc_host_is_spi(card->host) && !(cmd.resp[0] & R1_APP_CMD))
-		return err;
+		return 0;
 
 	memset(&cmd, 0, sizeof(struct mmc_command));
 
-- 
2.9.3

[PATCH 10/10] mmc: core: start to break apart mmc_start_areq()

[Linus Walleij]

This function is doing to many clever things at the same time under
too many various conditions.

Start to make things clearer by refactoring: break out the
finalization of the previous asynchronous request to its own
function mmc_finalize_areq(). We can get rid of the default
assignment of status and let the call deal with this.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index a160c3a7777a..41b4cd01fccc 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -631,6 +631,37 @@ static void mmc_post_req(struct mmc_host *host, struct mmc_request *mrq,
 }
 
 /**
+ * mmc_finalize_areq() - finalize an asynchronous request
+ * @host: MMC host to finalize any ongoing request on
+ * @ret_stat: the status of the ongoing asynchronous request:
+ *	will return MMC_BLK_SUCCESS if no request was
+ *	going on.
+ */
+static enum mmc_blk_status mmc_finalize_areq(struct mmc_host *host)
+{
+	enum mmc_blk_status status;
+
+	if (!host->areq)
+		return MMC_BLK_SUCCESS;
+
+	status = mmc_wait_for_data_req_done(host, host->areq->mrq);
+	if (status == MMC_BLK_NEW_REQUEST)
+		return status;
+
+	/*
+	 * Check BKOPS urgency for each R1 response
+	 */
+	if (host->card && mmc_card_mmc(host->card) &&
+	    ((mmc_resp_type(host->areq->mrq->cmd) == MMC_RSP_R1) ||
+	     (mmc_resp_type(host->areq->mrq->cmd) == MMC_RSP_R1B)) &&
+	    (host->areq->mrq->cmd->resp[0] & R1_EXCEPTION_EVENT)) {
+		mmc_start_bkops(host->card, true);
+	}
+
+	return status;
+}
+
+/**
  *	mmc_start_areq - start an asynchronous request
  *	@host: MMC host to start command
  *	@areq: asynchronous request to start
@@ -650,7 +681,7 @@ struct mmc_async_req *mmc_start_areq(struct mmc_host *host,
 				     struct mmc_async_req *areq,
 				     enum mmc_blk_status *ret_stat)
 {
-	enum mmc_blk_status status = MMC_BLK_SUCCESS;
+	enum mmc_blk_status status;
 	int start_err = 0;
 	struct mmc_async_req *data = host->areq;
 
@@ -658,35 +689,25 @@ struct mmc_async_req *mmc_start_areq(struct mmc_host *host,
 	if (areq)
 		mmc_pre_req(host, areq->mrq);
 
-	if (host->areq) {
-		status = mmc_wait_for_data_req_done(host, host->areq->mrq);
-		if (status == MMC_BLK_NEW_REQUEST) {
-			if (ret_stat)
-				*ret_stat = status;
-			/*
-			 * The previous request was not completed,
-			 * nothing to return
-			 */
-			return NULL;
-		}
-		/*
-		 * Check BKOPS urgency for each R1 response
-		 */
-		if (host->card && mmc_card_mmc(host->card) &&
-		    ((mmc_resp_type(host->areq->mrq->cmd) == MMC_RSP_R1) ||
-		     (mmc_resp_type(host->areq->mrq->cmd) == MMC_RSP_R1B)) &&
-		    (host->areq->mrq->cmd->resp[0] & R1_EXCEPTION_EVENT)) {
-			mmc_start_bkops(host->card, true);
-		}
+	/* Finalize previous request */
+	status = mmc_finalize_areq(host);
+
+	/* The previous request is still going on... */
+	if (status == MMC_BLK_NEW_REQUEST) {
+		if (ret_stat)
+			*ret_stat = status;
+		return NULL;
 	}
 
+	/* Fine so far, start the new request! */
 	if (status == MMC_BLK_SUCCESS && areq)
 		start_err = __mmc_start_data_req(host, areq->mrq);
 
+	/* Postprocess the old request at this point */
 	if (host->areq)
 		mmc_post_req(host, host->areq->mrq, 0);
 
-	 /* Cancel a prepared request if it was not started. */
+	/* Cancel a prepared request if it was not started. */
 	if ((status != MMC_BLK_SUCCESS || start_err) && areq)
 		mmc_post_req(host, areq->mrq, -EINVAL);
 
-- 
2.9.3

Re: [PATCH 10/10] mmc: core: start to break apart mmc_start_areq()

[Ulf Hansson]

On 1 February 2017 at 13:48, Linus Walleij <linus.walleij@linaro.org> wrote:
> This function is doing to many clever things at the same time under
> too many various conditions.
>
> Start to make things clearer by refactoring: break out the
> finalization of the previous asynchronous request to its own
> function mmc_finalize_areq(). We can get rid of the default
> assignment of status and let the call deal with this.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/core/core.c | 65 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index a160c3a7777a..41b4cd01fccc 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -631,6 +631,37 @@ static void mmc_post_req(struct mmc_host *host, struct mmc_request *mrq,
>  }
>
>  /**
> + * mmc_finalize_areq() - finalize an asynchronous request
> + * @host: MMC host to finalize any ongoing request on
> + * @ret_stat: the status of the ongoing asynchronous request:

This looks like a copy paste error as there is no ret_stat variable.

> + *     will return MMC_BLK_SUCCESS if no request was
> + *     going on.
> + */
> +static enum mmc_blk_status mmc_finalize_areq(struct mmc_host *host)
> +{

[...]

No need for you to re-spin, allow me to take care of the minor
nitpicks before I apply this.

Kind regards
Uffe

Re: [PATCH 08/10] mmc: block: respect bool returned from blk_end_request()

[Ulf Hansson]

[...]

>
>  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));
> +       while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)))
> +       {
> +       }

These brackets isn't needed, so am going to change this before applying.

while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)));

[...]

Kind regards
Uffe

Re: [PATCH 00/10] Further MMC core/block cleanups

[Ulf Hansson]

On 1 February 2017 at 13:47, Linus Walleij <linus.walleij@linaro.org> wrote:
> This is a continuation of the series merged to start cleaning
> up the very big mmc_blk_issue_rw_rq() and friends.
>
> Again the goal is to get the code in a shape such that I can
> get rid of flusing the pipeline with NULLs and report the
> requests as finished directly to the block layer, without
> having to wait for an iteration of mmc_blk_issue_req() to
> happen before they get postprocessed and finished.
>
> Feel free to apply this partially, or defer it all to after
> v4.11-rc1 *NO*RUSH*.
>
> It is just my idead of a cleanup road toward blk MQ
> compliance. I think most patches stand on their own but can
> be perceived as over-pedantic without that context.

I like it as is! Please continue! :-)

Each step puts me in a better position of maintaining the code.

>
> Linus Walleij (10):
>   mmc: block: inline the command abort and start new goto:s
>   mmc: block: rename rqc and req
>   mmc: core: rename mmc_start_req() to *areq()
>   mmc: block: refactor mmc_blk_rw_try_restart()
>   mmc: block: rename mmc_active to areq
>   mmc: queue: turn queue flags into bools
>   mmc: block: return errorcode from mmc_sd_num_wr_blocks()
>   mmc: block: respect bool returned from blk_end_request()
>   mmc: block: return 0 where evident
>   mmc: core: start to break apart mmc_start_areq()
>
>  drivers/mmc/core/block.c    | 179 ++++++++++++++++++++++++--------------------
>  drivers/mmc/core/core.c     |  79 ++++++++++++-------
>  drivers/mmc/core/mmc_test.c |   8 +-
>  drivers/mmc/core/queue.c    |  12 +--
>  drivers/mmc/core/queue.h    |   7 +-
>  include/linux/mmc/core.h    |   2 +-
>  6 files changed, 163 insertions(+), 124 deletions(-)
>
> --
> 2.9.3
>

I have reviewed this and it looks good to me. As we likely can expect
getting at least an rc7 from Linus, I think we still have time.

So, I have queued this up for next! Thanks!

Kind regards
Uffe

Re: [PATCH 08/10] mmc: block: respect bool returned from blk_end_request()

[Linus Walleij]

On Fri, Feb 3, 2017 at 10:52 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> [...]
>>  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));
>> +       while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)))
>> +       {
>> +       }
>
> These brackets isn't needed, so am going to change this before applying.
>
> while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)));

Aha you know your C syntax better than me. Looks like something that
checkpatch should catch actually, but I ran this through checkpatch and
it didn't complain.

Yours,
Linus Walleij

Re: [PATCH 08/10] mmc: block: respect bool returned from blk_end_request()

[Arnd Bergmann]

On Fri, Feb 3, 2017 at 1:57 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>         if (mmc_card_removed(card))
>>>                 req->rq_flags |= RQF_QUIET;
>>> -       while (ret)
>>> -               ret = blk_end_request(req, -EIO,
>>> -                                     blk_rq_cur_bytes(req));
>>> +       while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)))
>>> +       {
>>> +       }
>>
>> These brackets isn't needed, so am going to change this before applying.
>>
>> while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)));
>
> Aha you know your C syntax better than me. Looks like something that
> checkpatch should catch actually, but I ran this through checkpatch and
> it didn't complain.

IIRC gcc-7 will warn about Ulf's version, as it is a bit misleading and can
lead to subtle bugs like

   int ret = 0;
   while (!ret);
        ret = do_something();

which would pass a casual review but is actually an endless loop.

    Arnd

Re: [PATCH 08/10] mmc: block: respect bool returned from blk_end_request()

[Ulf Hansson]

On 6 February 2017 at 10:07, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Feb 3, 2017 at 1:57 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>         if (mmc_card_removed(card))
>>>>                 req->rq_flags |= RQF_QUIET;
>>>> -       while (ret)
>>>> -               ret = blk_end_request(req, -EIO,
>>>> -                                     blk_rq_cur_bytes(req));
>>>> +       while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)))
>>>> +       {
>>>> +       }
>>>
>>> These brackets isn't needed, so am going to change this before applying.
>>>
>>> while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)));
>>
>> Aha you know your C syntax better than me. Looks like something that
>> checkpatch should catch actually, but I ran this through checkpatch and
>> it didn't complain.
>
> IIRC gcc-7 will warn about Ulf's version, as it is a bit misleading and can
> lead to subtle bugs like
>
>    int ret = 0;
>    while (!ret);
>         ret = do_something();
>
> which would pass a casual review but is actually an endless loop.
>
>     Arnd

Okay, seems reasonable. However mine version is already being used at
some places in the kernel.

Linus version, was actually complained by checkpatch. Perhaps me an
Linus ran different versions of check patch.

So, then which version is preferred here?

Perhaps something like this is the best?

while (1) {
     ret = do_something();
     if (!ret)
         break;
}

Br
Uffe

Re: [PATCH 08/10] mmc: block: respect bool returned from blk_end_request()

[Linus Walleij]

On Mon, Feb 6, 2017 at 10:54 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 6 February 2017 at 10:07, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Feb 3, 2017 at 1:57 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>         if (mmc_card_removed(card))
>>>>>                 req->rq_flags |= RQF_QUIET;
>>>>> -       while (ret)
>>>>> -               ret = blk_end_request(req, -EIO,
>>>>> -                                     blk_rq_cur_bytes(req));
>>>>> +       while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)))
>>>>> +       {
>>>>> +       }
>>>>
>>>> These brackets isn't needed, so am going to change this before applying.
>>>>
>>>> while (blk_end_request(req, -EIO, blk_rq_cur_bytes(req)));
>>>
>>> Aha you know your C syntax better than me. Looks like something that
>>> checkpatch should catch actually, but I ran this through checkpatch and
>>> it didn't complain.
>>
>> IIRC gcc-7 will warn about Ulf's version, as it is a bit misleading and can
>> lead to subtle bugs like
>>
>>    int ret = 0;
>>    while (!ret);
>>         ret = do_something();
>>
>> which would pass a casual review but is actually an endless loop.
>>
>>     Arnd
>
> Okay, seems reasonable. However mine version is already being used at
> some places in the kernel.
>
> Linus version, was actually complained by checkpatch. Perhaps me an
> Linus ran different versions of check patch.

I might have missed it. It complained when I had:

while (cond)
{
};

about the semicolon in the end... maybe there was another complaint
too.

> So, then which version is preferred here?
>
> Perhaps something like this is the best?
>
> while (1) {
>      ret = do_something();
>      if (!ret)
>          break;
> }

Actually in this case the whole while() thing looks a bit bogus:
it is just hammering the block layer to end the request.

I guess since there is no way to return any unfinished status
upward.

True, I preserved that strange semantic, but what should we
really do? Timeout and complain in dmesg?

It just looks dangerous.

Anyways let's address any issues with follow-up patches...

Yours,
Linus Walleij

Re: [PATCH 08/10] mmc: block: respect bool returned from blk_end_request()

[Ulf Hansson]

>> Perhaps something like this is the best?
>>
>> while (1) {
>>      ret = do_something();
>>      if (!ret)
>>          break;
>> }
>
> Actually in this case the whole while() thing looks a bit bogus:
> it is just hammering the block layer to end the request.
>
> I guess since there is no way to return any unfinished status
> upward.
>
> True, I preserved that strange semantic, but what should we
> really do? Timeout and complain in dmesg?
>
> It just looks dangerous.
>
> Anyways let's address any issues with follow-up patches...

Agreed!

Kind regards
Uffe