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

Message ID 20170201124800.13865-9-linus.walleij@linaro.org
State New
Headers show
Series
  • Further MMC core/block cleanups
Related show

Commit Message

Linus Walleij Feb. 1, 2017, 12:47 p.m.
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(-)

-- 
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 Feb. 3, 2017, 9:52 a.m. | #1
[...]

>

>  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
--
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. 3, 2017, 12:57 p.m. | #2
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
--
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
Arnd Bergmann Feb. 6, 2017, 9:07 a.m. | #3
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
--
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. 6, 2017, 9:54 a.m. | #4
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
--
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. 6, 2017, 10:36 a.m. | #5
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
--
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. 6, 2017, 12:08 p.m. | #6
>> 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
--
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

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)