From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Subject: Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host() Date: Wed, 17 Sep 2014 13:57:42 -0600 Message-ID: <5419E7B6.7020708@wwwdotorg.org> References: <1410542338-24565-1-git-send-email-swarren@wwwdotorg.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Ulf Hansson Cc: Chris Ball , linux-mmc , "linux-kernel@vger.kernel.org" , Stephen Warren , Russell King , Adrian Hunter , Alexandre Courbot , Linus Walleij List-Id: linux-mmc@vger.kernel.org On 09/17/2014 01:55 PM, Ulf Hansson wrote: > On 12 September 2014 19:18, Stephen Warren wrote: >> From: Stephen Warren >> >> As soon as the CD IRQ is requested, it can trigger, since it's an >> externally controlled event. If it does, delayed_work host->detect will >> be scheduled. >> >> Many host controller probe()s are roughly structured as: >> >> *_probe() { >> host = sdhci_pltfm_init(); >> mmc_of_parse(host->mmc); >> rc = sdhci_add_host(host); >> if (rc) { >> sdhci_pltfm_free(); >> return rc; >> } >> >> In 3.17, CD IRQs can are enabled quite early via *_probe() -> >> mmc_of_parse() -> mmc_gpio_request_cd() -> mmc_gpiod_request_cd_irq(). >> >> Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd() >> rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't >> call mmc_gpiod_request_cd_irq(). However, this issue still exists for >> any other direct users of mmc_gpio_request_cd(). >> >> sdhci_add_host() may fail part way through (e.g. due to deferred >> probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to >> unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is >> coded to assume that if sdhci_add_host() failed, then the delayed_work >> cannot (or should not) have been triggered. >> >> This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when >> kfree(host) is eventually called inside sdhci_pltfm_free(): >> >> WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263 debug_print_object+0x8c/0xb4() >> ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x18 >> >> The object being complained about is host->detect. >> >> There's no need to request the CD IRQ so early; mmc_start_host() already >> requests it, and I *assume* that mmc_start_host() is called somehow for >> all host controllers. For SDHCI hosts at least, the typical call path >> that does this is: *_probe() -> sdhci_add_host() -> mmc_add_host() -> >> mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from >> mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which >> already doesn't call mmc_gpiod_request_cd_irq(). >> >> This solves the problem (eliminates the kernel error message above), >> since it guarantees that the IRQ can't trigger before mmc_start_host() >> is called. >> >> The critical point here is that once sdhci_add_host() calls >> mmc_add_host() -> mmc_start_host(), sdhci_add_host() is coded not to >> fail. In other words, if there's a chance that mmc_start_host() may have >> been called, and CD IRQs triggered, and the delayed_work scheduled, >> sdhci_add_host() won't fail, and so cleanup is no longer via >> sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue) >> but instead must be via sdhci_remove_host(), which calls mmc_remove_host() >> -> mmc_stop_host(), which does free the IRQ and cancel the work queue. >> >> This fixes what I might conclude to be a mistake in commit 740a221ef0e5 >> ("mmc: slot-gpio: Add GPIO descriptor based CD GPIO API"), which added the >> call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added >> incorrectly added a call from mmc_gpio_request_cd() to >> mmc_gpiod_request_cd_irq(). >> >> CC: Russell King >> Cc: Adrian Hunter >> Cc: Alexandre Courbot >> Cc: Linus Walleij >> Signed-off-by: Stephen Warren > > Hi Stephen, > > Thanks for looking into this. It seems like this issue has been > present for quite a while. > I believe your patch should have a stable tag for 3.15+ as well, > unless you object I will add it. Yes, that probably makes sense, thanks.