From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sujit Reddy Thumma Subject: Re: [PATCH] mmc: core: Kill block requests if card is removed Date: Mon, 14 Nov 2011 09:49:26 +0530 Message-ID: <4EC096CE.7000003@codeaurora.org> References: <1320813119-24383-1-git-send-email-sthumma@codeaurora.org> <4EBA4940.6030808@intel.com> <4EBB4CEA.1050901@codeaurora.org> <4EBB9AE1.2060909@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-arm-msm-owner@vger.kernel.org To: Per Forlin Cc: Adrian Hunter , linux-mmc@vger.kernel.org, linux-arm-msm@vger.kernel.org, cjb@laptop.org List-Id: linux-mmc@vger.kernel.org On 11/10/2011 7:50 PM, Per Forlin wrote: > On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter 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 >>>> 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 >>>>>> --- >>>>> >>>>> >>>>> 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 > } 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? > ------------------------------------- > > >> 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)); + } else { + mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq); + return mmc_start_req(card->host, areq, NULL); + } +} + >> >> 2. How can new requests be prevented from starting once the card >> is gone? >> >> Using BLKPREP_KILL will do that for all but the request that has been >> prepared already (i.e. async case) if the MMC_STATE_CARD_GONE flag is set >> early enough. >> >> Maybe blocking requests at a lower level is not needed. >> >> >>> >>>>