mmc: block: Fix tag condition with packed writes

PATCH Applied Adrian Hunter 3 participants 5 messages Jun 10, 2016 – Jun 22, 2016 MMC, SD and SDIO

Share & tools

Adrian Hunter

author

2016-06-10 13:26 UTC #0 lore raw

PATCH 1/1 mmc: block: Fix tag condition with packed writes

Apparently a cut-and-paste error, 'do_data_tag' is using 'brq' for data size even though 'brq' has not been set up. Instead use blk_rq_sectors().

Signed-off-by Adrian Hunter <[email protected]>

---

I don't know if anyone is actually using packed writes, but this is something I noticed.

Regards
Adrian

 drivers/mmc/card/block.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mmc/card/block.c +1 -2

index b954516739be..aa5cfaf1fdf0 100644
@@ -1834,8 +1834,7 @@ static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
 		do_data_tag = (card->ext_csd.data_tag_unit_size) &&
 			(prq->cmd_flags & REQ_META) &&
 			(rq_data_dir(prq) == WRITE) &&
-			((brq->data.blocks * brq->data.blksz) >=
-			 card->ext_csd.data_tag_unit_size);
+			blk_rq_bytes(prq) >= card->ext_csd.data_tag_unit_size;
 		/* Argument of CMD23 */
 		packed_cmd_hdr[(i * 2)] =
 			(do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |

Signature · 1 line

1.9.1

Shawn Lin

2016-06-13 02:42 UTC #1 lore raw

Hi Adrian,

Quoted 11 lines · Adrian Hunter

Apparently a cut-and-paste error, 'do_data_tag' is using 'brq' for data size even though 'brq' has not been set up. Instead use blk_rq_sectors().
Signed-off-by Adrian Hunter <[email protected]>
---
Hi
I don't know if anyone is actually using packed writes, but this is something I noticed.

I think if brq has not been set up, the we could meet another problem of checking the case of whether 4KB native sector is enabled. When fetching blk req from the queue, mmc_blk_issue_rw_rq already use brq there which is the same from your case, namely mq->mqrq_cur->brq, right?

Quoted 23 lines

Regards
Adrian

 drivers/mmc/card/block.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mmc/card/block.c +1 -2

index b954516739be..aa5cfaf1fdf0 100644
@@ -1834,8 +1834,7 @@ static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
 		do_data_tag = (card->ext_csd.data_tag_unit_size) &&
 			(prq->cmd_flags & REQ_META) &&
 			(rq_data_dir(prq) == WRITE) &&
-			((brq->data.blocks * brq->data.blksz) >=
-			 card->ext_csd.data_tag_unit_size);
+			blk_rq_bytes(prq) >= card->ext_csd.data_tag_unit_size;
 		/* Argument of CMD23 */
 		packed_cmd_hdr[(i * 2)] =
 			(do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |

Signature · 2 lines

Best Regards
Shawn Lin

Adrian Hunter

author

2016-06-13 06:30 UTC #2 lore raw

Quoted 16 lines · Shawn Lin

Hi Adrian,
Quoted 11 lines · Adrian Hunter
Apparently a cut-and-paste error, 'do_data_tag' is using 'brq' for data size even though 'brq' has not been set up. Instead use blk_rq_sectors().
Signed-off-by Adrian Hunter <[email protected]>
---
Hi
I don't know if anyone is actually using packed writes, but this is something I noticed.

Quoted 5 lines

I think if brq has not been set up, the we could meet another problem of checking the case of whether 4KB native sector is enabled. When fetching blk req from the queue, mmc_blk_issue_rw_rq already use brq there which is the same from your case, namely mq->mqrq_cur->brq, right?

Looks like that was that fixed by:

commit 3a6db10d86902491b759103ee97b2539175dd1dd

Author: Yuan, Juntao <[email protected]>
Date:   Fri May 13 07:59:24 2016 +0000

mmc: block: correct 4KB alignment check

In sectors alignment check, brq->data.blocks means sectors of the previous mqrq since data.blocks for mqrq_cur hasn't been updated yet. data.blocks will be updated later in mmc_blk_packed_hdr_wrq_prep or mmc_blk_rw_rq_prep.

static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, ......
......
struct mmc_blk_request *brq = &mq->mqrq_cur->brq;

Signed-off-by Yuan Juntao <[email protected]>

Signed-off-by Ulf Hansson <[email protected]>

There is also another fix in the SWCMDQ changes:

	http://marc.info/?l=linux-mmc&m=146547356109940

Quoted 29 lines

Quoted 27 lines

Regards
Adrian

 drivers/mmc/card/block.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mmc/card/block.c +1 -2

index b954516739be..aa5cfaf1fdf0 100644
@@ -1834,8 +1834,7 @@ static void mmc_blk_packed_hdr_wrq_prep(struct
 mmc_queue_req *mqrq,
         do_data_tag = (card->ext_csd.data_tag_unit_size) &&
             (prq->cmd_flags & REQ_META) &&
             (rq_data_dir(prq) == WRITE) &&
-            ((brq->data.blocks * brq->data.blksz) >=
-             card->ext_csd.data_tag_unit_size);
+            blk_rq_bytes(prq) >= card->ext_csd.data_tag_unit_size;
         /* Argument of CMD23 */
         packed_cmd_hdr[(i * 2)] =

             (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |

Shawn Lin

Reviewed-by

2016-06-13 07:33 UTC #3 lore raw

Quoted 23 lines · Adrian Hunter

Quoted 16 lines · Shawn Lin
Hi Adrian,
Quoted 11 lines · Adrian Hunter
Apparently a cut-and-paste error, 'do_data_tag' is using 'brq' for data size even though 'brq' has not been set up. Instead use blk_rq_sectors().
Signed-off-by Adrian Hunter <[email protected]>
---
Hi
I don't know if anyone is actually using packed writes, but this is something I noticed.
Quoted 5 lines
I think if brq has not been set up, the we could meet another problem of checking the case of whether 4KB native sector is enabled. When fetching blk req from the queue, mmc_blk_issue_rw_rq already use brq there which is the same from your case, namely mq->mqrq_cur->brq, right?

Quoted 6 lines

Looks like that was that fixed by:
commit 3a6db10d86902491b759103ee97b2539175dd1dd
Author: Yuan, Juntao <[email protected]>
Date:   Fri May 13 07:59:24 2016 +0000

Ahh.. yes, I see it now from Ulf's next.

But I'm wondering about why not cast the mqrq_cur->brq to NULL or memset it to be zero if it's expired after the completion. And a WARN_ON will prevent the brq from being used by mistake any more.

Anyway, you patch of course fix the problem. :)

Reviewed-by Shawn Lin <[email protected]>

Quoted 51 lines

mmc: block: correct 4KB alignment check
In sectors alignment check, brq->data.blocks means sectors of the previous mqrq since data.blocks for mqrq_cur hasn't been updated yet. data.blocks will be updated later in mmc_blk_packed_hdr_wrq_prep or mmc_blk_rw_rq_prep.
static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, ......
......
struct mmc_blk_request *brq = &mq->mqrq_cur->brq;
Signed-off-by Yuan Juntao <[email protected]>
Signed-off-by Ulf Hansson <[email protected]>
There is also another fix in the SWCMDQ changes:
	http://marc.info/?l=linux-mmc&m=146547356109940
Quoted 29 lines
Quoted 27 lines
Regards
Adrian
 drivers/mmc/card/block.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/mmc/card/block.c +1 -2
index b954516739be..aa5cfaf1fdf0 100644
@@ -1834,8 +1834,7 @@ static void mmc_blk_packed_hdr_wrq_prep(struct
 mmc_queue_req *mqrq,
         do_data_tag = (card->ext_csd.data_tag_unit_size) &&
             (prq->cmd_flags & REQ_META) &&
             (rq_data_dir(prq) == WRITE) &&
-            ((brq->data.blocks * brq->data.blksz) >=
-             card->ext_csd.data_tag_unit_size);
+            blk_rq_bytes(prq) >= card->ext_csd.data_tag_unit_size;
         /* Argument of CMD23 */
         packed_cmd_hdr[(i * 2)] =
             (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |

Signature · 2 lines

Best Regards
Shawn Lin

Ulf Hansson

Applied

2016-06-22 15:23 UTC #4 lore raw

In reply to Adrian Hunter

Quoted 4 lines · Adrian Hunter

Apparently a cut-and-paste error, 'do_data_tag' is using 'brq' for data size even though 'brq' has not been set up. Instead use blk_rq_sectors().
Signed-off-by Adrian Hunter <[email protected]>

Thanks, applied for next!

Kind regards
Uffe

Quoted 32 lines

---

I don't know if anyone is actually using packed writes, but this is something I noticed.

Regards
Adrian

 drivers/mmc/card/block.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mmc/card/block.c +1 -3

index b954516739be..aa5cfaf1fdf0 100644
@@ -1834,8 +1834,7 @@ static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
                do_data_tag = (card->ext_csd.data_tag_unit_size) &&
                        (prq->cmd_flags & REQ_META) &&
                        (rq_data_dir(prq) == WRITE) &&
-                       ((brq->data.blocks * brq->data.blksz) >=
-                        card->ext_csd.data_tag_unit_size);
+                       blk_rq_bytes(prq) >= card->ext_csd.data_tag_unit_size;
                /* Argument of CMD23 */
                packed_cmd_hdr[(i * 2)] =
                        (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |
--

1.9.1