On Mon, Nov 14, 2011 at 8:52 AM, Per Forlin <per.lkml@xxxxxxxxx> wrote: > On Mon, Nov 14, 2011 at 5:19 AM, Sujit Reddy Thumma > <sthumma@xxxxxxxxxxxxxx> wrote: >> On 11/10/2011 7:50 PM, Per Forlin wrote: >>> >>> On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter<adrian.hunter@xxxxxxxxx> >>> wrote: >>>> >>>> On 10/11/11 06:02, Sujit Reddy Thumma wrote: >>>>> >>>>> Hi, >>>>>> >>>>>> Hi Adrian, >>>>>> >>>>>> On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter<adrian.hunter@xxxxxxxxx> >>>>>> wrote: >>>>>>> >>>>>>> On 09/11/11 06:31, Sujit Reddy Thumma wrote: >>>>>>>> >>>>>>>> Kill block requests when the host knows that the card is >>>>>>>> removed from the slot and is sure that subsequent requests >>>>>>>> are bound to fail. Do this silently so that the block >>>>>>>> layer doesn't output unnecessary error messages. >>>>>>>> >>>>>>>> This patch implements suggestion from Adrian Hunter, >>>>>>>> http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474 >>>>>>>> >>>>>>>> Signed-off-by: Sujit Reddy Thumma<sthumma@xxxxxxxxxxxxxx> >>>>>>>> --- >>>>>>> >>>>>>> >>>>>>> It is safer to have zero initialisations so I suggest inverting >>>>>>> the meaning of the state bit i.e. >>>>> >>>>> Makes sense. Will take care in V2. >>>>> >>>>>>> >>>>>>> #define MMC_STATE_CARD_GONE (1<<7) /* card removed from >>>>>>> the >>>>>>> slot */ >>>>>>> >>>>>>> >>>>>>> Also it would be nice is a request did not start if the card is >>>>>>> gone. For the non-async case, that is easy: >>>>>>> >>>>>> As far as I understand Sujit's patch it will stop new requests from >>>>>> being issued, blk_fetch_request(q) returns NULL. >>>>> >>>>> Yes, Per you are right. The ongoing requests will fail at the controller >>>>> driver layer and only the new requests coming to MMC queue layer will be >>>>> blocked. >>>>> >>>>> Adrian's suggestion is to stop all the requests reaching host controller >>>>> layer by mmc core. This seems to be a good approach but the problem here >>>>> is >>>>> the host driver should inform mmc core that card is gone. >>>>> >>>>> Adrian, do you agree if we need to change all the host drivers to set >>>>> the >>>>> MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects >>>>> the >>>>> card as removed? >>>> >>>> Typically a card detect interrupt queues a rescan which in turn will have >>>> to wait to claim the host. In the meantime, in the async case, the block >>>> driver will not release the host until the queue is empty. >>> >>> Here is a patch that let async-mmc release host and reclaim it in case of >>> error. >>> When the host is released mmc_rescan will claim the host and run. >>> -------------------------------- >>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c >>> index cf73877..8952e63 100644 >>> --- a/drivers/mmc/card/block.c >>> +++ b/drivers/mmc/card/block.c >>> @@ -1209,6 +1209,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data >>> *md, struct mmc_card *card, >>> return ret; >>> } >>> >>> +/* >>> + * This function should be called to resend a request after failure. >>> + * Prepares and starts the request. >>> + */ >>> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card, >>> + struct mmc_queue *mq, >>> + struct mmc_queue_req >>> *mqrq, >>> + int disable_multi, >>> + struct mmc_async_req >>> *areq) >>> +{ >>> + /* >>> + * Release host after failure in case the host is needed >>> + * by someone else. For instance, if the card is removed the >>> + * worker thread needs to claim the host in order to do >>> mmc_rescan. >>> + */ >>> + mmc_release_host(card->host); >>> + mmc_claim_host(card->host); >>> + >>> + mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq); >>> + return mmc_start_req(card->host, areq, NULL); >>> +} >>> + >>> static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) >>> { >>> struct mmc_blk_data *md = mq->data; >>> @@ -1308,14 +1330,14 @@ static int mmc_blk_issue_rw_rq(struct >>> mmc_queue *mq, struct request *rqc) >>> break; >>> } >>> >>> - if (ret) { >>> + if (ret) >>> /* >>> * In case of a incomplete request >>> * prepare it again and resend. >>> */ >>> - mmc_blk_rw_rq_prep(mq_rq, card, disable_multi, >>> mq); >>> - mmc_start_req(card->host,&mq_rq->mmc_active, >>> NULL); >>> - } >>> + mmc_blk_prep_start(card, mq, mq_rq, disable_multi, >>> + &mq_rq->mmc_active); >>> + >> >> :%s/mmc_blk_prep_start/mmc_blk_resend >> > I'll update. > >>> } while (ret); >>> >>> return 1; >>> @@ -1327,10 +1349,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue >>> *mq, struct request *rqc) >>> spin_unlock_irq(&md->lock); >>> >>> start_new_req: >>> - if (rqc) { >>> - mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq); >>> - mmc_start_req(card->host,&mq->mqrq_cur->mmc_active, NULL); >>> - } >>> + if (rqc) >>> + mmc_blk_prep_start(card, mq, mq->mqrq_cur, 0, >>> + &mq->mqrq_cur->mmc_active); >>> >>> return 0; >>> } >> >> Thanks Per. This looks good. Can we split this into a different patch? >> > Yes I agree. My intention was to treat this as a separate patch. > I'll post it. > >>> ------------------------------------- >>> >>> >>>> The block >>>> driver will see errors and will use a send status command which will fail >>>> so the request will be aborted, but the next request will be started >>>> anyway. >>>> >>>> There are two problems: >>>> >>>> 1. When do we reliably know that the card has really gone? >>>> >>>> At present, the detect method for SD and MMC is just the send status >>>> command, which is what the block driver is doing i.e. if the block >>>> driver sees send status fail, after an errored request, and the >>>> card is removable, then it is very likely the card has gone. >>>> >>>> At present, it is not considered that the host controller driver >>>> knows whether the card is really gone - just that it might be. >>>> >>>> Setting a MMC_STATE_CARD_GONE flag early may be a little complicated. >>>> e.g. mmc_detect_change() flags that the card may be gone. After an >>>> error, if the "card may be gone" flag is set a new bus method could >>>> be called that just does send status. If that fails, then the >>>> MMC_STATE_CARD_GONE flag is set. Any calls to the detect method >>>> must first check the MMC_STATE_CARD_GONE flag so that, once gone, >>>> a card can not come back. >>>> >>>> Maybe you can think of something simpler. >> >> Can we do something like this: >> >> --- a/drivers/mmc/card/block.c >> +++ b/drivers/mmc/card/block.c >> @@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card, >> struct request *req, >> } >> >> /* We couldn't get a response from the card. Give up. */ >> - if (err) >> + if (err) { >> + /* >> + * If the card didn't respond to status command, >> + * it is likely that card is gone. Flag it as removed, >> + * mmc_detect_change() cleans the rest. >> + */ >> + mmc_card_set_card_gone(card); >> return ERR_ABORT; >> + } >> >> /* Flag ECC errors */ >> if ((status & R1_CARD_ECC_FAILED) || >> >> and some additions to Per's patch, changes denoted in (++): >> >> +/* >> + * This function should be called to resend a request after failure. >> + * Prepares and starts the request. >> + */ >> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card, >> + struct mmc_queue *mq, >> + struct mmc_queue_req >> *mqrq, >> + int disable_multi, >> + struct mmc_async_req >> *areq) >> +{ >> + /* >> + * Release host after failure in case the host is needed >> + * by someone else. For instance, if the card is removed the >> + * worker thread needs to claim the host in order to do mmc_rescan. >> + */ >> + mmc_release_host(card->host); >> + mmc_claim_host(card->host); >> ++ if (mmc_card_gone(card)) { >> ++ /* >> ++ * End the pending async request without starting it when card >> ++ * is removed. >> ++ */ >> ++ req->cmd_flags |= REQ_QUIET; >> ++ __blk_end_request(req, -EIO, blk_rq_cur_bytes(req)); > I'll add this to the patch and send it out. I'll wait adding this since mmc_card_gone() is not available yet. Maybe we should send these two patches (aync error release and kill block request) in the same patch-set? 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
