Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()

[Stephen Warren]

On 09/18/2014 12:49 AM, Adrian Hunter wrote:
> On 09/18/2014 08:25 AM, Adrian Hunter wrote:
> > On 09/17/2014 10:57 PM, Stephen Warren wrote:
> > > On 09/17/2014 01:55 PM, Ulf Hansson wrote:
> > > > On 12 September 2014 19:18, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote:
> > > > > From: Stephen Warren <swarren@xxxxxxxxxx>
> > > > >
> > > > > 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().
>
> That comment is wrong.  mmc_gpio_request_cd() has always set up the irq.

Uggh, yes. I did misinterpret your patch again, so that one paragraph is 
just wrong.

Aside from that though, I do think my patch is a step in the correct 
direction. It just needs some thought how to avoid the other issue you 
mentioned - that some drivers rely on calling mmc_gpio_request_cd() 
after the call to mmc_start().

Perhaps the logic should not be to remove mmc_gpio_request_cd()'s call 
to mmc_gpiod_request_cd_irq(), but rather to make it conditional upon 
mmc_start_host() having already been called; I assume that state that 
can easily be checked to determine that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/