* [PATCH] mmc: block: Fix tag condition with packed writes @ 2016-06-10 13:22 Adrian Hunter 2016-06-13 2:42 ` Shawn Lin 2016-06-22 15:23 ` Ulf Hansson 0 siblings, 2 replies; 5+ messages in thread From: Adrian Hunter @ 2016-06-10 13:22 UTC (permalink / raw) To: Ulf Hansson; +Cc: linux-mmc 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 <adrian.hunter@intel.com> --- Hi 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(-) diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index b954516739be..aa5cfaf1fdf0 100644 --- a/drivers/mmc/card/block.c +++ b/drivers/mmc/card/block.c @@ -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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mmc: block: Fix tag condition with packed writes 2016-06-10 13:22 [PATCH] mmc: block: Fix tag condition with packed writes Adrian Hunter @ 2016-06-13 2:42 ` Shawn Lin 2016-06-13 6:26 ` Adrian Hunter 2016-06-22 15:23 ` Ulf Hansson 1 sibling, 1 reply; 5+ messages in thread From: Shawn Lin @ 2016-06-13 2:42 UTC (permalink / raw) To: Adrian Hunter, Ulf Hansson; +Cc: shawn.lin, linux-mmc Hi Adrian, On 2016/6/10 21:22, Adrian Hunter wrote: > 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 <adrian.hunter@intel.com> > --- > > > 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? > > Regards > Adrian > > > drivers/mmc/card/block.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c > index b954516739be..aa5cfaf1fdf0 100644 > --- a/drivers/mmc/card/block.c > +++ b/drivers/mmc/card/block.c > @@ -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) | > -- Best Regards Shawn Lin ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mmc: block: Fix tag condition with packed writes 2016-06-13 2:42 ` Shawn Lin @ 2016-06-13 6:26 ` Adrian Hunter 2016-06-13 7:32 ` Shawn Lin 0 siblings, 1 reply; 5+ messages in thread From: Adrian Hunter @ 2016-06-13 6:26 UTC (permalink / raw) To: Shawn Lin; +Cc: Ulf Hansson, linux-mmc On 13/06/16 05:42, Shawn Lin wrote: > Hi Adrian, > > On 2016/6/10 21:22, Adrian Hunter wrote: >> 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 <adrian.hunter@intel.com> >> --- >> >> >> 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? Looks like that was that fixed by: commit 3a6db10d86902491b759103ee97b2539175dd1dd Author: Yuan, Juntao <juntao.yuan@intel.com> 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 <juntao.yuan@intel.com> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> There is also another fix in the SWCMDQ changes: http://marc.info/?l=linux-mmc&m=146547356109940 > > > >> >> Regards >> Adrian >> >> >> drivers/mmc/card/block.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c >> index b954516739be..aa5cfaf1fdf0 100644 >> --- a/drivers/mmc/card/block.c >> +++ b/drivers/mmc/card/block.c >> @@ -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) | >> > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mmc: block: Fix tag condition with packed writes 2016-06-13 6:26 ` Adrian Hunter @ 2016-06-13 7:32 ` Shawn Lin 0 siblings, 0 replies; 5+ messages in thread From: Shawn Lin @ 2016-06-13 7:32 UTC (permalink / raw) To: Adrian Hunter; +Cc: shawn.lin, Ulf Hansson, linux-mmc On 2016/6/13 14:26, Adrian Hunter wrote: > On 13/06/16 05:42, Shawn Lin wrote: >> Hi Adrian, >> >> On 2016/6/10 21:22, Adrian Hunter wrote: >>> 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 <adrian.hunter@intel.com> >>> --- >>> >>> >>> 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? > > Looks like that was that fixed by: > > commit 3a6db10d86902491b759103ee97b2539175dd1dd > Author: Yuan, Juntao <juntao.yuan@intel.com> > 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 <shawn.lin@rock-chips.com> > 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 <juntao.yuan@intel.com> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > There is also another fix in the SWCMDQ changes: > > http://marc.info/?l=linux-mmc&m=146547356109940 > >> >> >> >>> >>> Regards >>> Adrian >>> >>> >>> drivers/mmc/card/block.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c >>> index b954516739be..aa5cfaf1fdf0 100644 >>> --- a/drivers/mmc/card/block.c >>> +++ b/drivers/mmc/card/block.c >>> @@ -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) | >>> >> >> > > > > -- Best Regards Shawn Lin ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mmc: block: Fix tag condition with packed writes 2016-06-10 13:22 [PATCH] mmc: block: Fix tag condition with packed writes Adrian Hunter 2016-06-13 2:42 ` Shawn Lin @ 2016-06-22 15:23 ` Ulf Hansson 1 sibling, 0 replies; 5+ messages in thread From: Ulf Hansson @ 2016-06-22 15:23 UTC (permalink / raw) To: Adrian Hunter; +Cc: linux-mmc On 10 June 2016 at 15:22, Adrian Hunter <adrian.hunter@intel.com> wrote: > 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 <adrian.hunter@intel.com> Thanks, applied for next! Kind regards Uffe > --- > > > Hi > > 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(-) > > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c > index b954516739be..aa5cfaf1fdf0 100644 > --- a/drivers/mmc/card/block.c > +++ b/drivers/mmc/card/block.c > @@ -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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-22 15:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-10 13:22 [PATCH] mmc: block: Fix tag condition with packed writes Adrian Hunter 2016-06-13 2:42 ` Shawn Lin 2016-06-13 6:26 ` Adrian Hunter 2016-06-13 7:32 ` Shawn Lin 2016-06-22 15:23 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).