RE: [RESEND PATCH v4] mmc: fix async request mechanism for sequential read scenarios

[Seungwon Jeon <tgih.jun@samsung.com>]

On Wednesday, December 19, 2012, Konstantin Dorfman wrote:
> On 12/17/2012 02:26 PM, Seungwon Jeon wrote:
> > Hi, Konstantin.
> >
> > I added comments more below.
> I've answered below.
> >
> > On Wednesday, December 12, 2012, Seungwon Jeon wrote:
> >> On Monday, December 10, 2012, Konstantin Dorfman wrote:
> >>>   /*
> >>>    * Prepare a MMC request. This just filters out odd stuff.
> >>> @@ -63,11 +62,20 @@ static int mmc_queue_thread(void *d)
> >>>   		set_current_state(TASK_INTERRUPTIBLE);
> >>>   		req = blk_fetch_request(q);
> >>>   		mq->mqrq_cur->req = req;
> >>> +		if (!req && mq->mqrq_prev->req &&
> >>> +			!(mq->mqrq_prev->req->cmd_flags & REQ_FLUSH) &&
> >>> +			!(mq->mqrq_prev->req->cmd_flags & REQ_DISCARD))
> >>> +			mq->card->host->context_info.is_waiting_last_req = true;
> >>> +
> > Unlike normal R/W request,  request for discard/flush will be finished synchronously.
> > That means blk_end_request is called with 1's cycle of 'issue_fn' and request will be freed in block
> layer.
> > Currently 'mqrq_prev->req' is keeping these request and is used above condition.
> > Maybe, same area may be reallocated for normal R/W request after discard/flush is finished.
> > But we still expect discard or flush is saved in 'mq->mqrq_prev->req'.
> > I wonder that 'mq->mqrq_prev->req' indicates the valid area in all case.
> 
> It seems like a bug causing unnecessary call to `issue_fn(mq, req)` even
> when `req` is NULL, because 'mq->mqrq_prev->req' that holds old control
> request is not NULL. It is not directly related to the patch, it could
> be fixed by cleaning 'mq->mqrq_prev->req' (for control requests). I
> think this should be done in separate patch.
Yes, it's different issue. I've already checked it.
But I'm seeing whether cmd_flag of 'mq->mqrq_prev->req' is valid or not after blk_end_request is completed.
As it's mentioned above, special requests are completed at once.
> 
> Also this action (context_info.is_waiting_last_req = true;) could be
> moved into card/block.c: mmc_blk_issue_rq() function just for the case,
> when request is data request. Do you think it will be cleaner?
Wherever it is moved, we need other condition to decide whether last data transfer is ongoing.
'card->host->areq' could be good condition.

> >>> @@ -2086,6 +2186,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
> >>>
> >>>   	mmc_send_if_cond(host, host->ocr_avail);
> >>>
> >>> +	spin_lock_init(&host->context_info.lock);
> >>> +	host->context_info.is_new_req = false;
> >>> +	host->context_info.is_done_rcv = false;
> >>> +	host->context_info.is_waiting_last_req = false;
> >>> +	init_waitqueue_head(&host->context_info.wait);
> >>> +
> >> This path may be retired when mmc_rescan_try_freq is failed.
> >> How about putting these in mmc_add_card(mmc/core/bus.c)
> >> <Quote>
> >>          ret = device_add(&card->dev);
> >>          if (ret)
> >>                  return ret;
> >> -> Here seems good point.
> It is ok to put the initialization just before calling to "device_add",
> because data requests starting to arrive immediately after device_add().
> I've tested this already and will post soon.
Yes, it's reasonable.

Thanks,
Seungwon Jeon
> >>
> >> 	mmc_card_set_present(card);
> >> </Quote>
> >> Thanks,
> >> Seungwon Jeon