diff mbox

mmc: block: Fix tag condition with packed writes

Message ID 575ABF08.4010102@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Adrian Hunter June 10, 2016, 1:22 p.m. UTC 
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(-)

Comments

Shawn Lin June 13, 2016, 2:42 a.m. UTC | #1 
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) |
>
Adrian Hunter June 13, 2016, 6:26 a.m. UTC | #2 
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) |
>>
> 
> 

--
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
Shawn Lin June 13, 2016, 7:32 a.m. UTC | #3 
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) |
>>>
>>
>>
>
>
>
>
Ulf Hansson June 22, 2016, 3:23 p.m. UTC | #4 
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
>
--
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
diff mbox

Patch

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) |