lkml.org 
[lkml]   [2008]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] MMC: Fix race condition in resume/card detect code
    On Mon, 20 Oct 2008 22:41:48 +0300
    Yauhen Kharuzhy <jekhor@gmail.com> wrote:

    > When device wakes up by card change interrupt and MMC_UNSAFE_RESUME is
    > enabled then race condition between mmc_rescan() and
    > mmc_resume()/mmc_sd_resume() appeared.
    >

    Having thought a bit more about this, I'm not sure where the race is.
    mmc_sd_resume() will be called before mmc_detect_change() is.

    There is a race if the drivers call mmc_detect_change() before
    mmc_resume_host() has returned, but that is a driver bug. I won't
    object to adding a safe guard against that, but the commit message
    should reflect that scenario and not something else. There should also
    be some printk() to indicate that the driver is up to no good.

    > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
    > index 044d84e..427f283 100644
    > --- a/drivers/mmc/core/core.c
    > +++ b/drivers/mmc/core/core.c
    > @@ -657,6 +657,9 @@ void mmc_rescan(struct work_struct *work)
    > u32 ocr;
    > int err;
    >
    > + if (host->suspended)
    > + return;
    > +
    > mmc_bus_get(host);
    >
    > if (host->bus_ops == NULL) {

    Was there no way to query the PM layer for this information?

    > @@ -805,6 +810,8 @@ int mmc_resume_host(struct mmc_host *host)
    > */
    > mmc_detect_change(host, 1);
    >
    > + host->suspended = 0;
    > +
    > return 0;
    > }
    >

    You've added a new race here. ;)

    You should set suspended to 0 before calling mmc_detect_change(), not
    after. The point was to protect bus_ops->resume(), nothing else.

    > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
    > index 9c288c9..a584239 100644
    > --- a/include/linux/mmc/host.h
    > +++ b/include/linux/mmc/host.h
    > @@ -139,6 +139,9 @@ struct mmc_host {
    > #ifdef CONFIG_MMC_DEBUG
    > unsigned int removed:1; /* host is being removed */
    > #endif
    > +#ifdef CONFIG_MMC_UNSAFE_RESUME
    > + unsigned int suspended:1;
    > +#endif
    >
    > struct mmc_card *card; /* device attached to this host */
    >

    No #ifdef for this as there are no where this variable is referenced.

    Rgds
    --
    -- Pierre Ossman

    Linux kernel, MMC maintainer http://www.kernel.org
    rdesktop, core developer http://www.rdesktop.org

    WARNING: This correspondence is being monitored by the
    Swedish government. Make sure your server uses encryption
    for SMTP traffic and consider using PGP for end-to-end
    encryption.


    \
     
     \ /
      Last update: 2008-10-26 12:49    [W:0.025 / U:0.336 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site