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: Fri, 3 Feb 2012 14:23:12 +0100 Message-ID: <4F2BDFC0.3010201@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> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from eu1sys200aog104.obsmtp.com ([207.126.144.117]:55260 "EHLO eu1sys200aog104.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756196Ab2BCNXd (ORCPT ); Fri, 3 Feb 2012 08:23:33 -0500 In-Reply-To: <4F2BD895.2030301@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 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. :-) ---------- >>> 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 > ---------------------