On 21 June 2011 08:40, Per Forlin <per.forlin@xxxxxxxxxx> wrote:
> On 20 June 2011 17:17, Kishore Kadiyala <kishorek.kadiyala@xxxxxxxxx> wrote:
>> On Mon, Jun 20, 2011 at 2:47 AM, Per Forlin <per.forlin@xxxxxxxxxx> wrote:
>>> Change mmc_blk_issue_rw_rq() to become asynchronous.
>>> The execution flow looks like this:
>>> The mmc-queue calls issue_rw_rq(), which sends the request
>>> to the host and returns back to the mmc-queue. The mmc-queue calls
>>> issue_rw_rq() again with a new request. This new request is prepared,
>>> in isuue_rw_rq(), then it waits for the active request to complete before
>>> pushing it to the host. When to mmc-queue is empty it will call
>>> isuue_rw_rq() with req=NULL to finish off the active request
>>> without starting a new request.
>>>
>>> Signed-off-by: Per Forlin <per.forlin@xxxxxxxxxx>
>>> ---
>>> drivers/mmc/card/block.c | 121 +++++++++++++++++++++++++++++++++-------------
>>> drivers/mmc/card/queue.c | 17 +++++--
>>> drivers/mmc/card/queue.h | 1 +
>>> 3 files changed, 101 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>> index 6a84a75..66db77a 100644
>>> --- a/drivers/mmc/card/block.c
>>> +++ b/drivers/mmc/card/block.c
>>> @@ -108,6 +108,7 @@ static DEFINE_MUTEX(open_lock);
>>>
>>> enum mmc_blk_status {
>>> MMC_BLK_SUCCESS = 0,
>>> + MMC_BLK_PARTIAL,
>>> MMC_BLK_RETRY,
>>> MMC_BLK_DATA_ERR,
>>> MMC_BLK_CMD_ERR,
>>> @@ -668,14 +669,16 @@ static inline void mmc_apply_rel_rw(struct mmc_blk_request *brq,
>>> }
>>> }
>>>
>>> -static enum mmc_blk_status mmc_blk_err_check(struct mmc_blk_request *brq,
>>> - struct request *req,
>>> - struct mmc_card *card,
>>> - struct mmc_blk_data *md)
>>> +static int mmc_blk_err_check(struct mmc_card *card,
>>> + struct mmc_async_req *areq)
>>> {
>>> struct mmc_command cmd;
>>> u32 status = 0;
>>> enum mmc_blk_status ret = MMC_BLK_SUCCESS;
>>> + struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req,
>>> + mmc_active);
>>> + struct mmc_blk_request *brq = &mq_mrq->brq;
>>> + struct request *req = mq_mrq->req;
>>>
>>> /*
>>> * Check for errors here, but don't jump to cmd_err
>>> @@ -770,7 +773,11 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_blk_request *brq,
>>> else
>>> ret = MMC_BLK_DATA_ERR;
>>> }
>>> -out:
>>> +
>>> + if (ret == MMC_BLK_SUCCESS &&
>>> + blk_rq_bytes(req) != brq->data.bytes_xfered)
>>> + ret = MMC_BLK_PARTIAL;
>>> + out:
>>> return ret;
>>> }
>>>
>>> @@ -901,27 +908,59 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>>> brq->data.sg_len = i;
>>> }
>>>
>>> + mqrq->mmc_active.mrq = &brq->mrq;
>>> + mqrq->mmc_active.err_check = mmc_blk_err_check;
>>> +
>>> mmc_queue_bounce_pre(mqrq);
>>> }
>>>
>>> -static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
>>> +static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>>> {
>>> struct mmc_blk_data *md = mq->data;
>>> struct mmc_card *card = md->queue.card;
>>> - struct mmc_blk_request *brq = &mq->mqrq_cur->brq;
>>> - int ret = 1, disable_multi = 0;
>>> + struct mmc_blk_request *brq;
>>> + int ret = 1;
>>> + int disable_multi = 0;
>>> enum mmc_blk_status status;
>>> + struct mmc_queue_req *mq_rq;
>>> + struct request *req;
>>> + struct mmc_async_req *areq;
>>> +
>>> + if (!rqc && !mq->mqrq_prev->req)
>>> + goto out;
>>>
>>> do {
>>> - mmc_blk_rw_rq_prep(mq->mqrq_cur, card, disable_multi, mq);
>>> - mmc_wait_for_req(card->host, &brq->mrq);
>>> + if (rqc) {
>>> + mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
>>> + areq = &mq->mqrq_cur->mmc_active;
>>> + } else
>>> + areq = NULL;
>>> + areq = mmc_start_req(card->host, areq, (int *) &status);
>>
>> I think 'status' is used uninitialized.
> status is an out parameter. From that perspective it is always initialised.
> I should update the doc description of mmc_start_req to clarify this.
>
>> With this struct mmc_async_req *mmc_start_req in your first patch
>> if (error)
>> *error = err;
>> return data;
>> condition which always passes.
>>
>> You can have
>> enum mmc_blk_status status = MMC_BLK_SUCCESS;
In core.c there is no access to the type "enum mmc_blk_status status",
this type is block.c specific.
The intention is to make this function available for SDIO as well.
"int err = 0;" is set at the top of mmc_start_req(). Default err condition is 0.
What do you think about the following changes?
* @areq: async request to start
- * @error: non zero in case of error
+ * @error: out parameter returns 0 for success, otherwise non zero
*
* Start a new MMC custom command request for a host.
* If there is on ongoing async request wait for completion
@@ -334,9 +334,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
mmc_post_req(host, areq->mrq, -EINVAL);
host->areq = NULL;
- if (error)
- *error = err;
- return data;
+ goto out;
}
}
@@ -347,6 +345,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
mmc_post_req(host, host->areq->mrq, 0);
host->areq = areq;
+ out:
if (error)
*error = err;
return data;
>>
>> struct mmc_async_req *mmc_start_req {
>> err = host->areq->err_check(host->card, host->areq);
>> if (err) {
>> ...
>> ...
>> *error = err;
>> }
>>
>> no need to update * error here in success case
>> return data
>> }
> I agree, makes the code more clear.
>
> Thanks,
> Per
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
