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