> @@ -1313,10 +1609,17 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
*mq, struct request *rqc)
> * A block was successfully transferred.
> */
> mmc_blk_reset_success(md, type);
> - spin_lock_irq(&md->lock);
> - ret = __blk_end_request(req, 0,
> +
> + if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> + ret = mmc_blk_end_packed_req(mq, mq_rq);
If a specific request in the packed request consistantly fails, there is
nothing to stop us from sending the same packed request in an endless
loop.
> + break;
Thanks,
Maya Erez
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Maya Erez <[email protected]> wrote:
> > @@ -1313,10 +1609,17 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
> *mq, struct request *rqc)
> > * A block was successfully transferred.
> > */
> > mmc_blk_reset_success(md, type);
> > - spin_lock_irq(&md->lock);
> > - ret = __blk_end_request(req, 0,
> > +
> > + if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> > + ret = mmc_blk_end_packed_req(mq, mq_rq);
> If a specific request in the packed request consistantly fails, there is
> nothing to stop us from sending the same packed request in an endless
> loop.
There is various error case. This patch reused the existing error handling.
What is that case we need to consider?
Best regards,
Seungwon Jeon
> > + break;
>
> Thanks,
> Maya Erez
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Maya Erez <[email protected]> wrote:
>> > @@ -1313,10 +1609,17 @@ static int mmc_blk_issue_rw_rq(struct
>> mmc_queue
>> *mq, struct request *rqc)
>> > * A block was successfully transferred.
>> > */
>> > mmc_blk_reset_success(md, type);
>> > - spin_lock_irq(&md->lock);
>> > - ret = __blk_end_request(req, 0,
>> > +
>> > + if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
>> > + ret = mmc_blk_end_packed_req(mq, mq_rq);
>> If a specific request in the packed request consistantly fails, there is
>> nothing to stop us from sending the same packed request in an endless
>> loop.
> There is various error case. This patch reused the existing error
> handling.
> What is that case we need to consider?
>
> Best regards,
> Seungwon Jeon
This is different from unpacked requests handling since in the packed err
check function you don't always return the error returned from
mmc_blk_err_check. In case the EXT_CSD_PACKED_INDEXED_ERROR is set you
return MMC_BLK_PARTIAL which is handled differently in the
mmc_blk_issue_rw_rd.
In our tests we set to 1 the packed bit in CMD23 arg of the first req (in
the header). As a result, mmc_blk_err_check returned MMC_BLK_CMD_ERR.
However, mmc_blk_packed_err_check returned MMC_BLK_PARTIAL (since the card
indicated the index of the first request as the failed request).
mmc_blk_issue_rw_rd handles MMC_BLK_PARTIAL by sending the packed command
from the failed index and on, but since the packed bit was still set, the
same error occurred and was handled the same way and we ended up with an
endless loop.
I hope my description is clear, let me know if you have further questions.
Thanks,
Maya Erez
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Maya Erez <[email protected]> wrote:
> > Maya Erez <[email protected]> wrote:
> >> > @@ -1313,10 +1609,17 @@ static int mmc_blk_issue_rw_rq(struct
> >> mmc_queue
> >> *mq, struct request *rqc)
> >> > * A block was successfully transferred.
> >> > */
> >> > mmc_blk_reset_success(md, type);
> >> > - spin_lock_irq(&md->lock);
> >> > - ret = __blk_end_request(req, 0,
> >> > +
> >> > + if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> >> > + ret = mmc_blk_end_packed_req(mq, mq_rq);
> >> If a specific request in the packed request consistantly fails, there is
> >> nothing to stop us from sending the same packed request in an endless
> >> loop.
> > There is various error case. This patch reused the existing error
> > handling.
> > What is that case we need to consider?
> >
> > Best regards,
> > Seungwon Jeon
>
> This is different from unpacked requests handling since in the packed err
> check function you don't always return the error returned from
> mmc_blk_err_check. In case the EXT_CSD_PACKED_INDEXED_ERROR is set you
> return MMC_BLK_PARTIAL which is handled differently in the
> mmc_blk_issue_rw_rd.
> In our tests we set to 1 the packed bit in CMD23 arg of the first req (in
> the header). As a result, mmc_blk_err_check returned MMC_BLK_CMD_ERR.
> However, mmc_blk_packed_err_check returned MMC_BLK_PARTIAL (since the card
> indicated the index of the first request as the failed request).
> mmc_blk_issue_rw_rd handles MMC_BLK_PARTIAL by sending the packed command
> from the failed index and on, but since the packed bit was still set, the
> same error occurred and was handled the same way and we ended up with an
> endless loop.
> I hope my description is clear, let me know if you have further questions.
I tested your test case equally.
Even though your test makes the header parameter incorrect artificially
and keeps going with wrong setting repeatedly, we need to assure that
the similar result can be found practically with normal running.
I'll test it heavily and check more.
And if you have more review about this version, please let me know.
Thanks for your review.
Seungwon Jeon.
>
> Thanks,
> Maya Erez
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Maya Erez <[email protected]> wrote:
>> > Maya Erez <[email protected]> wrote:
>> >> > @@ -1313,10 +1609,17 @@ static int mmc_blk_issue_rw_rq(struct
>> >> mmc_queue
>> >> *mq, struct request *rqc)
>> >> > * A block was successfully transferred.
>> >> > */
>> >> > mmc_blk_reset_success(md, type);
>> >> > - spin_lock_irq(&md->lock);
>> >> > - ret = __blk_end_request(req, 0,
>> >> > +
>> >> > + if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
>> >> > + ret = mmc_blk_end_packed_req(mq, mq_rq);
>> >> If a specific request in the packed request consistantly fails, there
>> is
>> >> nothing to stop us from sending the same packed request in an endless
>> >> loop.
>> > There is various error case. This patch reused the existing error
>> > handling.
>> > What is that case we need to consider?
>> >
>> > Best regards,
>> > Seungwon Jeon
>>
>> This is different from unpacked requests handling since in the packed
>> err
>> check function you don't always return the error returned from
>> mmc_blk_err_check. In case the EXT_CSD_PACKED_INDEXED_ERROR is set you
>> return MMC_BLK_PARTIAL which is handled differently in the
>> mmc_blk_issue_rw_rd.
>> In our tests we set to 1 the packed bit in CMD23 arg of the first req
>> (in
>> the header). As a result, mmc_blk_err_check returned MMC_BLK_CMD_ERR.
>> However, mmc_blk_packed_err_check returned MMC_BLK_PARTIAL (since the
>> card
>> indicated the index of the first request as the failed request).
>> mmc_blk_issue_rw_rd handles MMC_BLK_PARTIAL by sending the packed
>> command
>> from the failed index and on, but since the packed bit was still set,
>> the
>> same error occurred and was handled the same way and we ended up with an
>> endless loop.
>> I hope my description is clear, let me know if you have further
>> questions.
> I tested your test case equally.
> Even though your test makes the header parameter incorrect artificially
> and keeps going with wrong setting repeatedly, we need to assure that
> the similar result can be found practically with normal running.
> I'll test it heavily and check more.
> And if you have more review about this version, please let me know.
>
> Thanks for your review.
> Seungwon Jeon.
Our code should be robust enough to deal with any card behavior.
Therefore, I think we need to avoid having endless loops regardless of the
scenario that caused it.
Currently I have no additional comments about this version.
Thanks,
Maya Erez
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum