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); + } 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; } ------------------------------------- > 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. > > 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. > > >> >>> >>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>>> index 5278ffb..91d7721 100644 >>>> --- a/drivers/mmc/core/core.c >>>> +++ b/drivers/mmc/core/core.c >>>> @@ -259,7 +259,7 @@ static void mmc_wait_for_req_done(struct mmc_host *host, >>>> wait_for_completion(&mrq->completion); >>>> >>>> cmd = mrq->cmd; >>>> - if (!cmd->error || !cmd->retries) >>>> + if (!cmd->error || !cmd->retries || >>>> mmc_card_gone(host->card)) >>> host->card will be NULL >> >> Yes, the host->card will be NULL for some requests. Will take care of it. >> >>> static void mmc_remove(struct mmc_host *host) >>> { >>> BUG_ON(!host); >>> BUG_ON(!host->card); >>> >>> mmc_remove_card(host->card); >>> host->card = NULL; >>> } >>> card is not freed until later. >>> >>>> break; >>>> >>>> pr_debug("%s: req failed (CMD%u): %d, retrying...\n", >>>> @@ -374,6 +374,10 @@ EXPORT_SYMBOL(mmc_start_req); >>>> */ >>>> void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq) >>>> { >>>> + if (mmc_card_gone(host->card)) { >>>> + mrq->cmd->error = -ENODEV; >>>> + return; >>>> + } >> >> Looks good. Just for clarification, some host controller drivers return >> -ENOMEDIUM in host->ops->request() when they see the card is gone. Do you >> think we can remove this now from host controller drivers? > > I see include/linux/mmc/core.h has specified the use of -ENOMEDIUM so that > should be used. > >> >> >>>> __mmc_start_req(host, mrq); >>>> mmc_wait_for_req_done(host, mrq); >>>> } >>>> >>>> >>>> >>>> The async case is harder... >>>> >>> I can help out with the non-async code if we go for your proposal. >> >> I will check the possibilities. >> >>> >>>> >>>>> drivers/mmc/card/queue.c | 5 +++++ >>>>> drivers/mmc/core/bus.c | 2 ++ >>>>> include/linux/mmc/card.h | 3 +++ >>>>> 3 files changed, 10 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c >>>>> index dcad59c..f8a3298 100644 >>>>> --- a/drivers/mmc/card/queue.c >>>>> +++ b/drivers/mmc/card/queue.c >>>>> @@ -29,6 +29,8 @@ >>>>> */ >>>>> static int mmc_prep_request(struct request_queue *q, struct request *req) >>>>> { >>>>> + struct mmc_queue *mq = q->queuedata; >>>>> + >>>>> /* >>>>> * We only like normal block requests and discards. >>>>> */ >>>>> @@ -37,6 +39,9 @@ static int mmc_prep_request(struct request_queue *q, >>>>> struct request *req) >>>>> return BLKPREP_KILL; >>>>> } >>>>> >>>>> + if (mq&& mq->card&& !mmc_card_inserted(mq->card)) >>>>> + return BLKPREP_KILL; >>>>> + >>>>> req->cmd_flags |= REQ_DONTPREP; >>>>> >>>>> return BLKPREP_OK; >>>>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c >>>>> index 46b6e84..ea3be5d 100644 >>>>> --- a/drivers/mmc/core/bus.c >>>>> +++ b/drivers/mmc/core/bus.c >>>>> @@ -308,6 +308,7 @@ int mmc_add_card(struct mmc_card *card) >>>>> mmc_card_ddr_mode(card) ? "DDR " : "", >>>>> type, card->rca); >>>>> } >>>>> + mmc_card_set_inserted(card); >>>>> >>>>> #ifdef CONFIG_DEBUG_FS >>>>> mmc_add_card_debugfs(card); >>>>> @@ -340,6 +341,7 @@ void mmc_remove_card(struct mmc_card *card) >>>>> pr_info("%s: card %04x removed\n", >>>>> mmc_hostname(card->host), card->rca); >>>>> } >>>>> + card->state&= ~MMC_STATE_INSERTED; >>>>> device_del(&card->dev); >>>>> } >>>>> >>>>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h >>>>> index b7a8cb1..be4125a 100644 >>>>> --- a/include/linux/mmc/card.h >>>>> +++ b/include/linux/mmc/card.h >>>>> @@ -209,6 +209,7 @@ struct mmc_card { >>>>> #define MMC_STATE_HIGHSPEED_DDR (1<<4) /* card is in >>>>> high speed mode */ >>>>> #define MMC_STATE_ULTRAHIGHSPEED (1<<5) /* card is in >>>>> ultra high speed mode */ >>>>> #define MMC_CARD_SDXC (1<<6) /* card is SDXC */ >>>>> +#define MMC_STATE_INSERTED (1<<7) /* card present in the >>>>> slot */ >>>>> unsigned int quirks; /* card quirks */ >>>>> #define MMC_QUIRK_LENIENT_FN0 (1<<0) /* allow SDIO FN0 >>>>> writes outside of the VS CCCR range */ >>>>> #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1) /* use func->cur_blksize */ >>>>> @@ -362,6 +363,7 @@ static inline void __maybe_unused >>>>> remove_quirk(struct mmc_card *card, int data) >>>>> #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO) >>>>> >>>>> #define mmc_card_present(c) ((c)->state& MMC_STATE_PRESENT) >>>>> +#define mmc_card_inserted(c) ((c)->state& MMC_STATE_INSERTED) >>>>> #define mmc_card_readonly(c) ((c)->state& MMC_STATE_READONLY) >>>>> #define mmc_card_highspeed(c) ((c)->state& MMC_STATE_HIGHSPEED) >>>>> #define mmc_card_blockaddr(c) ((c)->state& MMC_STATE_BLOCKADDR) >>>>> @@ -370,6 +372,7 @@ static inline void __maybe_unused >>>>> remove_quirk(struct mmc_card *card, int data) >>>>> #define mmc_card_ext_capacity(c) ((c)->state& MMC_CARD_SDXC) >>>>> >>>>> #define mmc_card_set_present(c) ((c)->state |= MMC_STATE_PRESENT) >>>>> +#define mmc_card_set_inserted(c) ((c)->state |= MMC_STATE_INSERTED) >>>>> #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY) >>>>> #define mmc_card_set_highspeed(c) ((c)->state |= MMC_STATE_HIGHSPEED) >>>>> #define mmc_card_set_blockaddr(c) ((c)->state |= MMC_STATE_BLOCKADDR) >>>> >> >> > > -- 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
