From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ulf Hansson Subject: Re: [RESEND PATCH V3] mmc: core: Detect card removal on I/O error Date: Mon, 6 Feb 2012 10:27:49 +0100 Message-ID: <4F2F9D15.7090000@stericsson.com> References: <1328261593-6112-1-git-send-email-ulf.hansson@stericsson.com> <4F2BC790.3090304@intel.com> <4F2BD00C.8040703@stericsson.com> <4F2BD895.2030301@intel.com> <4F2BDFC0.3010201@stericsson.com> <4F2F971E.6030007@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from eu1sys200aog111.obsmtp.com ([207.126.144.131]:52969 "EHLO eu1sys200aog111.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754075Ab2BFJ2z (ORCPT ); Mon, 6 Feb 2012 04:28:55 -0500 In-Reply-To: <4F2F971E.6030007@intel.com> Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Adrian Hunter Cc: "linux-mmc@vger.kernel.org" , Chris Ball , Per FORLIN , Johan RUDHOLM , Lee Jones Adrian Hunter wrote: > On 03/02/12 15:23, Ulf Hansson wrote: >> Adrian Hunter wrote: >>> On 03/02/12 14:16, Ulf Hansson wrote: >>>> Adrian Hunter wrote: >>>>> On 03/02/12 11:33, Ulf Hansson wrote: >>>>>> To prevent I/O as soon as possible at card removal, a new detect work >>>>>> is re-scheduled without a delay to let a rescan remove the card device >>>>>> as soon a possible. >>>>>> >>>>>> Additionally, MMC_CAP2_DETECT_ON_ERR can now be used to handle "slowly" >>>>>> removed cards that a scheduled detect work did not detect as removed. >>>>>> To prevent further I/O requests for these lingering removed cards, >>>>>> check if card has been removed and >>>>>> then schedule a detect work to properly remove it. >>>>>> >>>>>> Signed-off-by: Ulf Hansson --- >>>>>> >>>>>> Changes in v3: - Check for card is NULL and minor code >>>>>> simplifications. >>>>>> >>>>>> Changes in v2: - Updated according to review comments. - >>>>>> Merging two patches for this feature into one. >>>>>> >>>>>> --- drivers/mmc/core/core.c | 24 +++++++++++++++++++++--- >>>>>> include/linux/mmc/host.h | 1 + 2 files changed, 22 >>>>>> insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index >>>>>> bec0bf2..9645e8c 100644 --- a/drivers/mmc/core/core.c +++ >>>>>> b/drivers/mmc/core/core.c @@ -2077,18 +2077,36 @@ int >>>>>> _mmc_detect_card_removed(struct mmc_host *host) int >>>>>> mmc_detect_card_removed(struct mmc_host *host) { struct >>>>>> mmc_card *card = host->card; + int ret; >>>>>> >>>>>> WARN_ON(!host->claimed); + + if (!card) + return 1; + >>>>>> + ret = mmc_card_removed(card); /* * The card will be >>>>>> considered unchanged unless we have been asked to * detect a >>>>>> change or host requires polling to provide card detection. */ - >>>>>> if (card && !host->detect_change && !(host->caps & >>>>>> MMC_CAP_NEEDS_POLL)) - return mmc_card_removed(card); + >>>>>> if (!host->detect_change && !(host->caps & MMC_CAP_NEEDS_POLL) >>>>>> && + !(host->caps2 & MMC_CAP2_DETECT_ON_ERR)) + >>>>>> return ret; >>>>>> >>>>>> host->detect_change = 0; + if (!ret) { + ret = >>>>>> _mmc_detect_card_removed(host); + if (ret) { >>>>> Please make this >>>>> >>>>> if (ret && (host->caps2 & MMC_CAP2_DETECT_ON_ERR)) >>>>> >>>>> because if rescan is already running this will needlessly queue another >>>>> one. >>>>> >>>> It wont queue another one. It will cancel a work which likely has >>>> been scheduled to be executed within several ms later. Then it will >>>> re-schedule a new one without any delay since there is no need to >>>> wait, when we already know that the card has been removed. >>> It won't cancel a rescan that is already running (waiting to claim >>> the host for example) but it will queue another one. Hence the >>> comment. >>> >> Do you think this is case that we need to bother about? It should be a >> rare case and in worst case we only schedule a second not needed rescan, >> which I believe should not be a problem. >> >> I discussed this with Jaehoon Chung, previously regarding the return value >> of cancel_delayed_work(&host->detect). See below copied comments: >> >> I change accordingly if you like, please let me know. Again. :-) > > Yes please. > Fixed a v4 patch then. Thanks. >> ---------- >> >>>>> if (cancel_delayed_work(&host->detect)) >>>>>>> mmc_detect_change(host, 0); isn't? >>>>> Good comment. That will mean that patch 2 will have to be updated >>>>> as well to something like below. >>>>> >>>>> if (cancel_delayed_work(&host->detect) || (host->caps2 & >>>>> MMC_CAP2_DETECT_ON_ERR)) mmc_detect_change(host, 0); >>>>> >>>>> What do you think? >>>>> >>>>> Could we skip this entirely and leave it as is without checking >>>>> the return value of cancel_delayed_work? That will only mean that >>>>> in some very rare cases (since rescan is clearing the >>>>> detect_change flag) one additional detect work will be triggered >>>>> which shall not cause any problems I believe. But I happily >>>>> change to what you propose if you prefer! >>> Right...maybe..don't cause any problems..i also think. It's rare >>> case. :) Best Regards, Jaehoon Chung >>> >> --------------------- >> > >